-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Access-Control-Allow-Method #133
Comments
I am not hundred percent sure but perhaps the header “Access-Control-Allow-Methods” also need to be included in the “Access-Control-Allow-Headers” in order to get and parse that value? |
Could be worth a try. Indeed. Will try to patch the MediaMTX sources accordingly |
….although I think it’s better to go with Ice-Lite, since these PATCHes and Ice Restarts add another level of complexity for server and clients |
Hmm. No. Even with this response for the preflight the result is null:
|
Sorry, I realized it probably need to be in |
|
Yes, that works now. Great. But my change is a bit clumsy, since I enabled all for both headers:
Would you know a "less generic" setup? |
That would be to explicitly add Access-Control-Allow-Methods to the list in both Expose and Allow headers directive |
No, doesn't work like so. I guess only "*" works |
Also some are sceptical, why this is necessary at all. However, w/o these "" here and there your code doesn't correctly determine the state of the allow header. I also don't have an explanation, why these "" are required, but I find it too generic and possibly problematic for security reasons. |
So ctx.Writer.Header().Set("Access-Control-Allow-Headers", "Authorization, Content-Type, If-Match, Access-Control-Allow-Methods") and ctx.Writer.Header().Set("Access-Control-Expose-Headers", "Authorization, Content-Type, If-Match, Access-Control-Allow-Methods, Link") does not work? |
Exactly |
Ok, what is the recommended way to determine in browser space what methods are available / allowed without actually trying the method? |
Good question... .:) |
Let me know if I can help with something |
This is what we do in the WHIP endpoint side in one of our server implementations this.server.register(require("fastify-cors"), { |
Anything on the server side saying why the PATCH failed? Or is it not even receiving it? |
No, because it's a local browser issue |
Aha, etag of course. CORS is fun… |
OK, fixed it now. Hit all the places where MediaMTX messes with CORS headers :) Works. |
So etag needs to be in both allow and expose. Looking in the preflight to patch request that one is missing |
Thanks for your patience |
No problem! Happy to be able to assist |
Hope you don’t mind me closing this issue now. Let us know if you find something that should be corrected or if you have any further questions |
Sure. Sorry, I should have done that. Have a nice weekend. |
I'm after a problem with your way to determine, if PATCH is allowed or not.
If I interpret this line in the SDK's index.ts correctly you are firing an OPTIONS request to the server.
and later on you are trying to determine, if PATCH is allowed.
I'm wondering, why
config.headers.get("access-control-allow-methods")
return null in my case. Is it due to the camel-case? I think MediaMTX clearly states, it would support PATCH...The text was updated successfully, but these errors were encountered: