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

Should AccessControlAllowOrigin::Value’s value really be a parsed Url? #526

Closed
SimonSapin opened this issue May 12, 2015 · 11 comments
Closed
Labels
A-headers Area: headers.

Comments

@SimonSapin
Copy link
Contributor

In https://fetch.spec.whatwg.org/#cors-check , the result of "parsing" a Access-Control-Allow-Origin seems to be a string, not an URL. See servo/servo#6020.

@pyfisch
Copy link
Contributor

pyfisch commented May 12, 2015

Well, like all the specs are not as strongly typed as hyper is, so I think this difference is normal and hyper should provide the parsed URL. But is it a real problem, that the URL gets parsed and you compare two URLs and not two strings? Are there cases where this could be a real problem?

@SimonSapin
Copy link
Contributor Author

I don’t know how real the problem is, but it’s not the same behavior. URL parsing does some normalization along the way, so comparing parsed URLs is not the same as comparing strings. And CORS is a security/privacy feature, so I’d rather not take chances.

@SimonSapin
Copy link
Contributor Author

@Ms2ger, do you have an opinion on this?

@Ms2ger
Copy link

Ms2ger commented May 13, 2015

CC @velmont; I assume he's written tests that would fail with parsed URLs, if there's a difference.

@annevk
Copy link

annevk commented May 14, 2015

CORS needs to do string comparison. You can probably do something with utf-8 percent-encoding vs punycode to show the difference.

@seanmonstar seanmonstar added servo A-headers Area: headers. labels May 17, 2015
@seanmonstar
Copy link
Member

So, is it only in some cases it should be a String, or in all?

@pyfisch
Copy link
Contributor

pyfisch commented May 17, 2015

We could use Strings everywhere in hyper for URLs. At many places the spec uses relative URLs we can not parse with rust-url, so it is inconsistent but writing a custom URL parser is work and will make using hyper together with rust-url based projects harder.

@odinho
Copy link

odinho commented May 17, 2015

@Ms2ger: Good to see you assume the best of people :P There's some I'm quite sure would fail if you throw them through URL normalization. But there is for sure lots of cases that's not tested as I'm not awesome enough to think of'em.

Here's the ones that exist: http://w3c-test.org/cors/origin.htm

(Aside: Blink does everything correct except the \0's which possibly it strips in the "strip whitespace" step. Maybe I did wrong there as I see Gecko agrees.)

@pyfisch
Copy link
Contributor

pyfisch commented Jul 4, 2015

@velmont \0 is not allowed in header values in general so I don't think it should be an allowed origin.

@Ms2ger
Copy link

Ms2ger commented Jul 4, 2015

Regardless of whether it's allowed, what should browsers do if they encounter it anyway?

@seanmonstar
Copy link
Member

Well, according to that test, is should be considered illegal. The test expects xhr.send() to throw an error.

Fail    Disallow origin: \0http://w3c-test.org
assert_throws: send function "function () { client.send() }" did not throw

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

No branches or pull requests

6 participants