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

Add configuration flag to allow to opt-out from skipping CORS when same as origin #178

Merged
merged 3 commits into from Feb 15, 2023

Conversation

nocive
Copy link
Contributor

@nocive nocive commented Sep 21, 2022

I would like to propose introducing a flag that allows the bundle user to opt-out of the default "skip if same as origin" logic done in CorsListener (see https://github.com/nelmio/NelmioCorsBundle/blob/master/EventListener/CorsListener.php#L67).

As far as I understand allowing this would not violate the CORS spec.

For us this is a valid usage scenario, for reasons which are a bit convoluted to explain, but in short we have a reverse proxy setup where the symfony backend is not aware of all the domains it can be routed through. In this scenario, we are in fact using an Origin header which matches (even though the external facing domain does not) the same scheme & http host the symfony backend "sees".

With this proposed change nothing would change for current users, while allowing the flexibility for those that want to opt-out to do so without having to override the whole onKernelRequest method from the original listener.

Any feedback or help refining this is welcomed.

@nocive
Copy link
Contributor Author

nocive commented Oct 26, 2022

cc @Seldaek

README.md Show resolved Hide resolved
@gndk
Copy link

gndk commented Jan 15, 2023

We have a similar usecase and would appreciate this configuration option. I also saw random JS errors in our Sentry related to missing CORS headers, even for same-origin requests, until I forced them. Seems like some browsers (mostly webview-related in my case) treat this more strictly than others. Currently overriding the onKernelRequest method for this purpose.

@nocive I'd suggest adding tests as this is a new feature. This would likely also make it a higher probability to get this merged.

@Seldaek Seldaek merged commit 18ac0c1 into nelmio:master Feb 15, 2023
@Seldaek
Copy link
Member

Seldaek commented Feb 15, 2023

Thanks, looks reasonable and opt-in anyway.

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

3 participants