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

Unexpected response when requesting with multiple comma separated origins and wildcard AllowedOrigins #131

Open
latonz opened this issue Aug 3, 2022 · 6 comments

Comments

@latonz
Copy link

latonz commented Aug 3, 2022

If an origin https://*.example.com is allowed and a client sends a request with a header Origin: https://github.com,https://test.example.com the wildcard match is performed against the comma separated origin header value and succeeds.
This leads to an unexpected and according to the specification invalid response header Access-Control-Allow-Origin: https://github.com,https://test.example.com. Also it was never intended that https://github.com should be allowed.

As far as I know current Browser implementations never send comma separated values in origin headers (and never multiple headers) and I'm not even sure if the specification would allow this. However, I think it would make sense to validate the origin header's format and reject requests with an unexpected format (not sure on how to do this backward compatible?).

@rs
Copy link
Owner

rs commented Aug 3, 2022

We can validate origin header yes. I don't see how this will pose backward compatible issue if we cover all formats browsers may send.

@latonz
Copy link
Author

latonz commented Aug 3, 2022

I think the hard part is to cover all formats a browser may send. I don't think the change itself is breaking, but the risk of introducing a breaking change for some edge cases not correctly covered by the validation should be considered. Therefore it may be a good option to add a flag to disable the validation.

Would you accept a PR? Do you have any expectations on how this validation should be implemented?

@rs
Copy link
Owner

rs commented Aug 3, 2022

Sure for PR. Expectation no dep, no regex, doesn't break :)

@jub0bs
Copy link
Contributor

jub0bs commented Oct 29, 2023

@rs Out of curiosity, why do you insist on not relying on regexps? I do agree with you, but I'd like to understand your reasons.

@rs
Copy link
Owner

rs commented Oct 29, 2023

It's slow and unnecessary

@jub0bs
Copy link
Contributor

jub0bs commented Apr 18, 2024

@latonz

I'm not even sure if the specification would allow this.

I'm a bit late to the party but,

  1. because "Origin" is a so-called forbidden request-header name, no client running in a Fetch-compliant browser can include such a header in its requests; only the browser itself can;
  2. Fetch-compliant browsers either omit to include an Origin header or include one that contains exactly one (no less, no more) origin value.

Therefore, because no request issued from a Fetch-compliant browser can include the following header,

Origin: https://github.com,https://test.example.com

allowing https://*.example.com in your configuration shouldn't be cause for concern (at least if your server indeed trusts all subdomains of example.com).

However, I do believe that the range of origin patterns supported by rs/cors is too broad and easy to abuse; for instance, rs/cors supports https://* as an origin pattern, but it arguably shouldn't, because such a pattern is dangerously overpermissive.

I think it would make sense to validate the origin header's format and reject requests with an unexpected format.

FWIW, some CORS libraries do parse or validate the value of the Origin header. Mine parses it because it needs to determine its constituents (scheme, host, port) to figure out whether the origin is allowed; the performance penalty for doing so is minimal. rs/cors too could parse/validate the Origin header.

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

No branches or pull requests

3 participants