Update basic auth cache to key off of origin instead of url #13281

gilbertw1 · 2016-09-15T18:08:16Z

This pull request's primary purpose is to store basic auth credentials based on the url origin instead of the entire url. This fixes an issue where servo continuously prompts the user for credentials any time a basic auth secured resource is requested even though the user has already entered auth credentials for a different resource from the same origin.

The test associated with this PR hides image redirects behind a python handler that requires basic authentication. The reference page loads two images by directly specifying the image to load, while the test page loads the two images using the basic auth redirect handler with only the first image tag providing auth credentials.

I'd like to point a few specific items for review:

url::Origin does not derive Hash, so I am using ascii_serialization as the cache key. This seems like a stable enough representation.
I've updated the http loader to store credentials not only on Success responses, but Redirect responses as well. I stumbled on this because nginx was redirecting 'test' -> 'test/' in my testing, and other browsers were storing the credentials on the redirect response vs. prompting for credentials a second time.
In the test I'm using a timeout to load the second image (without authentication), otherwise the order that the images were loaded was unpredictable.

./mach build -d does not report any errors
./mach test-tidy does not report any errors related to these changes
These changes fix #12095 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-09-15T18:08:22Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Ms2ger (or someone else) soon.

highfive · 2016-09-15T18:08:22Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/fetch/methods.rs, components/net/resource_thread.rs, components/net/http_loader.rs

jdm

This looks really great! There's only one change we can make to ensure that the new test is robust against timing changes.

jdm · 2016-09-15T19:26:46Z

tests/wpt/web-platform-tests/http/basic-auth-cache-test.html

+  <script type="text/javascript">
+    var authImg = '<img src="http://testuser:testpass@' + window.location.host + '/http/resources/securedimage.py">';
+    document.getElementById('auth').innerHTML = authImg;
+    setTimeout(function() {


Rather than a timeout (which could cause intermittent failures if the first image finishes loading before the timeout fires), we should:

add class="reftest-wait" to the <html> element

add an onload handler to the first image, which starts the second image load

add an onload handler to the second load which removes the reftest-wait class

Thanks for the feedback! I've just incorporated the test changes in a new commit.

bors-servo · 2016-09-16T00:53:40Z

☔ The latest upstream changes (presumably #13280) made this pull request unmergeable. Please resolve the merge conflicts.


        updated basic auth cache to key off of url origin


        store basic auth in cache on redirect response


        added test that verifies proper basic auth caching behavior

gilbertw1 · 2016-09-16T12:47:15Z

@jdm: Could you please advise on the MANIFEST.json file? Are the test entries in the correct location, should I update it in some way so that the CI build will pass? Thanks!

jdm · 2016-09-16T13:59:51Z

If you run ./mach update-manifest it should do the right thing.

gilbertw1 · 2016-09-16T15:08:59Z

Thanks! Problem fixed.

jdm · 2016-09-16T15:30:08Z

tests/wpt/web-platform-tests/http/basic-auth-cache-test.html

@@ -1,15 +1,26 @@
 <!doctype html>
 <meta charset="utf-8">
 <link rel="match" href="basic-auth-cache-test-ref.html">


Oh, these elements should be children of the <html> element.

ah right, fixed.


        removed race condition possibility from auth cache test & fixed up te…

…st errors

jdm · 2016-09-16T16:05:50Z

@bors-servo: r+
Thanks!

bors-servo · 2016-09-16T16:05:50Z

📌 Commit 715682c has been approved by jdm

bors-servo · 2016-09-16T16:05:53Z

⌛ Testing commit 715682c with merge e5b9987...


        Auto merge of #13281 - gilbertw1:basic-auth-cache-clean, r=jdm

Update basic auth cache to key off of origin instead of url This pull request's primary purpose is to store basic auth credentials based on the url origin instead of the entire url. This fixes an issue where servo continuously prompts the user for credentials any time a basic auth secured resource is requested even though the user has already entered auth credentials for a different resource from the same origin. The test associated with this PR hides image redirects behind a python handler that requires basic authentication. The reference page loads two images by directly specifying the image to load, while the test page loads the two images using the basic auth redirect handler with only the first image tag providing auth credentials. I'd like to point a few specific items for review: * url::Origin does not derive ```Hash```, so I am using ```ascii_serialization``` as the cache key. This seems like a stable enough representation. * I've updated the http loader to store credentials not only on Success responses, but Redirect responses as well. I stumbled on this because nginx was redirecting 'test' -> 'test/' in my testing, and other browsers were storing the credentials on the redirect response vs. prompting for credentials a second time. * In the test I'm using a timeout to load the second image (without authentication), otherwise the order that the images were loaded was unpredictable. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors related to these changes - [x] These changes fix #12095 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13281)

bors-servo · 2016-09-16T17:04:14Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

gilbertw1 · 2016-09-16T17:06:55Z

No problem, I plan to continue contributing in the future!

highfive assigned Ms2ger Sep 15, 2016

highfive added the S-awaiting-review label Sep 15, 2016

jdm requested changes Sep 15, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 15, 2016

jdm assigned jdm and unassigned Ms2ger Sep 15, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 15, 2016

highfive added the S-needs-rebase label Sep 16, 2016

gilbertw1 added 3 commits Sep 14, 2016

updated basic auth cache to key off of url origin

82e45a4

store basic auth in cache on redirect response

0bcd04d

added test that verifies proper basic auth caching behavior

712b1d5

gilbertw1 force-pushed the gilbertw1:basic-auth-cache-clean branch from 09ab326 to 54c7c33 Sep 16, 2016

gilbertw1 force-pushed the gilbertw1:basic-auth-cache-clean branch from 54c7c33 to 43dae3d Sep 16, 2016

jdm requested changes Sep 16, 2016

View changes

jdm added S-needs-code-changes and removed S-needs-rebase S-awaiting-review labels Sep 16, 2016

removed race condition possibility from auth cache test & fixed up te…

Loading status checks…

715682c

…st errors

gilbertw1 force-pushed the gilbertw1:basic-auth-cache-clean branch from 43dae3d to 715682c Sep 16, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 16, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 16, 2016

jdm approved these changes Sep 16, 2016

View changes

bors-servo merged commit 715682c into servo:master Sep 16, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 16, 2016

gilbertw1 deleted the gilbertw1:basic-auth-cache-clean branch Sep 16, 2016

servo / servo

Update basic auth cache to key off of origin instead of url #13281

Update basic auth cache to key off of origin instead of url #13281

gilbertw1 commented Sep 15, 2016 •

edited by larsbergstrom

highfive commented Sep 15, 2016

highfive commented Sep 15, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

bors-servo commented Sep 16, 2016

gilbertw1 commented Sep 16, 2016

jdm commented Sep 16, 2016

gilbertw1 commented Sep 16, 2016

This comment has been minimized.

This comment has been minimized.

jdm commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

gilbertw1 commented Sep 16, 2016

servo / servo

Join GitHub today

Update basic auth cache to key off of origin instead of url #13281

Update basic auth cache to key off of origin instead of url #13281

Conversation

gilbertw1 commented Sep 15, 2016 • edited by larsbergstrom

highfive commented Sep 15, 2016

highfive commented Sep 15, 2016

jdm left a comment

This comment has been minimized.

jdm Sep 15, 2016

This comment has been minimized.

gilbertw1 Sep 15, 2016

bors-servo commented Sep 16, 2016

gilbertw1 commented Sep 16, 2016

jdm commented Sep 16, 2016

gilbertw1 commented Sep 16, 2016

This comment has been minimized.

jdm Sep 16, 2016

This comment has been minimized.

gilbertw1 Sep 16, 2016

jdm commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

gilbertw1 commented Sep 16, 2016

gilbertw1 commented Sep 15, 2016 •

edited by larsbergstrom