Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent adding Link headers for CORS preflight requets #3265

Merged
merged 1 commit into from Nov 15, 2019

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Nov 15, 2019

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? yes/no
Tickets fixes #3262
License MIT
Doc PR n/a

Preflight requests cannot be accessed in JS, therefore settings the Link headers for Hydra and Mercure is useless for these requests Not setting them prevent the issues with invalid URLs being generated. This an alternative fix to #3262.

@teohhanhui
Copy link
Contributor

I think this approach is risky. During my exploration from many years ago, I came upon this conclusion: nelmio/NelmioCorsBundle#54 (comment)

@teohhanhui
Copy link
Contributor

Really, NelmioCorsBundle is breaking all OPTIONS requests, it should be fixed there...

@dunglas
Copy link
Member Author

dunglas commented Nov 15, 2019

But is there a better short term solution as NelmioCors won't change soon? It doesn't hurt for these 2 specific headers to not be generated for preflight requests (actually, it will even help reducing servers' load). It's not a general solution, but for Mercure and Hydra it's good enough IMO.

@teohhanhui
Copy link
Contributor

teohhanhui commented Nov 15, 2019

There are consequences for caching. We must add Vary: Access-Control-Request-Method now, which is really telling us that we're going in the wrong direction...

@teohhanhui
Copy link
Contributor

teohhanhui commented Nov 15, 2019

For a short term solution, I'd rather we set the correct values in the RequestContext as suggested in nelmio/NelmioCorsBundle#125 (comment) (NelmioCorsBundle breaks it, we un-break it), instead of varying the response based on this header.

@dunglas
Copy link
Member Author

dunglas commented Nov 15, 2019

I don't want to introduce such complexity (a new listener that we'll have to maintain BC for) just to fix this issue with a 3rd party library. And NelmioCors is already returning a different response for Preflight requests and non-preflight requests, so the potential cache issue already exists.

My proposal is to:

  • "temporarily" merge this as a quick fix
  • remove those checks if/when NelmioCors is fixed, or if Symfony includes native CORS support (it would be the best option IMHO)

@teohhanhui teohhanhui merged commit d4f5d44 into api-platform:2.5 Nov 15, 2019
@teohhanhui
Copy link
Contributor

Thanks @dunglas! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants