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

in_http: allow empty Origin header requests to pass CORS checks #4866

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dlackty
Copy link
Contributor

@dlackty dlackty commented Mar 16, 2025

Which issue(s) this PR fixes:

What this PR does / why we need it:
Some requests, such as those made by apps, certain automated scripts, or older browsers, may not include an Origin header. Previously, such requests were blocked by the CORS check, even though they may not necessarily be cross-origin.

For CORS, the server is responsible for reporting the allowed origins. The web browser is responsible for enforcing that requests are only sent from allowed domains. So this change updates the CORS handling logic to allow requests with an empty Origin header to pass, ensuring compatibility with legitimate non-browser clients while maintaining security.

Docs Changes:

Release Note:

Some requests, such as those made by apps, certain automated scripts, or older browsers, may not include an Origin header. Previously, such requests were blocked by the CORS check, even though they may not necessarily be cross-origin.

This change updates the CORS handling logic to allow requests with an empty Origin header to pass, ensuring compatibility with legitimate non-browser clients while maintaining security.

Signed-off-by: Richard Lee <14349+dlackty@users.noreply.github.com>
@daipom daipom self-assigned this Mar 17, 2025
@daipom
Copy link
Contributor

daipom commented Mar 17, 2025

@dlackty Thanks for this PR!
I will review this soon.

@daipom
Copy link
Contributor

daipom commented Mar 18, 2025

For CORS, the server is responsible for reporting the allowed origins. The web browser is responsible for enforcing that requests are only sent from allowed domains.

I see.
I am not familiar with CORS, but it certainly seems to be true.
CORS does not protect the server.
The browser makes the decision on whether to allow the communication.

Given this CORS specification, this in_http's specification, which returns 403 depending on cors_allow_origins, would not necessarily be required.
So, it would be possible to relax the condition as this PR.

On the other hand, it would also be possible that in_http intentionally checks the condition strictly, although it is not a CORS specification.
If there is a possibility that some users are intentionally using the current strict feature, it may be better not to change the default behavior in order to keep compatibility.

I would like to hear opinions on this point.

Note:

@daipom daipom added this to the v1.19.0 milestone Mar 18, 2025
Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least, there would be no problem to relax the condition like this PR.
(In the first place, I don't think it is necessary to return 403.)

I will wait a while, and if there are no objections, I will merge this.
Thanks!

@dlackty
Copy link
Contributor Author

dlackty commented Mar 18, 2025

Suggestion: To avoid breaking compatibility while still allowing requests with an empty Origin header, we could consider allowing cors_allow_origins to include nil as a valid value.

For example:

cors_allow_origins [nil, "example.com", "another-domain.com"]

This way:
• Existing strict behavior remains unchanged by default.
• Users who explicitly want to allow empty Origin headers can opt in by adding nil.
• This keeps the logic flexible without affecting current users who rely on the strict enforcement.

Would love to hear your thoughts on this approach! 🚀

@daipom
Copy link
Contributor

daipom commented Mar 19, 2025

I see! Thanks!
So, we have a workaround. We can use this workaround for the current version.

Even if we have a workaround, I see no problem with this change.

If anyone has a different opinion, please let me know.
I will wait a while, and if there are no objections, I will merge this for v1.19.0.

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

2 participants