Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upcorrectly send secure cookies after hsts url match #9780
Conversation
highfive
commented
Feb 27, 2016
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon. |
|
Thank you for finding and solving this! I've filed #9783 to ensure we're not making this mistake in other places, too. |
|
@bors-servo: r+ |
|
|
correctly send secure cookies after hsts url match Fix for #8100, where sites in the hsts list were not recieving secure cookies if the site was originally loading using a plain http url. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9780) <!-- Reviewable:end -->
|
|
We should look into why this test now fails with this change. |
Changed hostname rewrite to happen inside obtain response after any htst changes. Removed url from load leaving just doc_url to avoid confusion
|
As discussed on IRC, the test was failing due to the hostname being modified by the HOST file entry in the test runner. This meant the cookies were incorrectly failing to match the host in the updated request cookies method, and not being sent. Tried updating the code to move hostname changes inside obtain_response and remove the duplicate urls. Ran this against the unit tests and WPT tests, I get some failures and timeouts on WPT but I get the same failures on master. I tried to test github site itself but I can't seem to press the final login button as the browser won't react to it. |
| @@ -632,7 +632,7 @@ pub fn obtain_response<A>(request_factory: &HttpRequestFactory<R=A>, | |||
| if let Some(pipeline_id) = *pipeline_id { | |||
| send_request_to_devtools( | |||
| devtools_chan.clone(), request_id.clone().into(), | |||
| url.clone(), method.clone(), request_headers.clone(), | |||
| connection_url.clone(), method.clone(), request_headers.clone(), | |||
This comment has been minimized.
This comment has been minimized.
|
This looks great, with one small change necessary! |
| let cookie = Cookie::new_wrapped( | ||
| cookie_pair, | ||
| &cookie_url, | ||
| CookieSource::NonHTTP |
This comment has been minimized.
This comment has been minimized.
|
@bors-servo: r+ |
|
|
correctly send secure cookies after hsts url match Fixes #8100, where sites in the hsts list were not recieving secure cookies if the site was originally loading using a plain http url. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9780) <!-- Reviewable:end -->
|
|
bobthekingofegypt commentedFeb 27, 2016
Fixes #8100, where sites in the hsts list were not recieving secure
cookies if the site was originally loading using a plain http url.