Skip to content

/ servo

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

Jump to bottom

Authentication request for all page resources behind nginx auth_basic #12095

Closed
rmfitzpatrick opened this issue Jul 1, 2016 · 20 comments
Closed

Authentication request for all page resources behind nginx auth_basic #12095

rmfitzpatrick opened this issue Jul 1, 2016 · 20 comments
Labels
A-network C-assigned E-easy

Comments

@rmfitzpatrick
Copy link

@rmfitzpatrick rmfitzpatrick commented Jul 1, 2016

When accessing a personal page set up w/ basic nginx authentication, the username and password prompts were displayed for all referenced site contents. This resulted in a barrage of prompts that don't stop despite providing the correct credentials for any given one.

It's also a little distracting to have separate prompts for username and password but understandably this isn't in scope for the issue at hand.

Servo 0.0.1
OSX 10.11.5 (15F34)
@notriddle notriddle added the A-network label Jul 1, 2016
@aneeshusa
Copy link
Member

@aneeshusa aneeshusa commented Jul 1, 2016

How did you get the Servo version number? Is that the full output of ./servo --version? The version number should have a git revision associated with it; if it doesn't we've got a bug somewhere.
@rmfitzpatrick
Copy link
Author

@rmfitzpatrick rmfitzpatrick commented Jul 1, 2016

Sorry I just used the nightly build (haven't tried building myself) and went with what the UI told me.

Servo 0.0.1-4f81384
@aneeshusa
Copy link
Member

@aneeshusa aneeshusa commented Jul 1, 2016

OK, version number is all good them; just wanted to confirm our nightlies were getting the git revision embedded.
@jdm
Copy link
Member

@jdm jdm commented Jul 1, 2016 •
edited

Our http auth cache is keyed on the precise URL; we might need to change that to the origin instead.
@mpaolini
Copy link

@mpaolini mpaolini commented Jul 19, 2016

same issue here with Servo 0.0.1-589c6ee
@jdm
Copy link
Member

@jdm jdm commented Jul 19, 2016

Let's change AuthCache.entries to be a hashmap using Origin keys instead of Url. We can obtain an origin value by calling the origin method of Url.

Code: components/net/resource_thread.rs
Test: We should write a reference test that contains two images that reference a python handler - the first one should pass a username and password as part of the URL, and the second one should not. The handler should return a valid image response body if the correct Authorization HTTP header is present, and a 401 response if not. The reference file should contain two instances of the valid image.
@jdm jdm added the E-easy label Jul 19, 2016
@highfive
Copy link

@highfive highfive commented Jul 19, 2016

Please make a comment here if you intend to work on this issue. Thank you!
@marinintim
Copy link

@marinintim marinintim commented Jul 20, 2016

I'd like to try to tackle this issue
@jdm
Copy link
Member

@jdm jdm commented Jul 20, 2016

@marinintim Please do! Ask questions if anything is unclear.
@jdm jdm added the C-assigned label Jul 20, 2016
@marinintim
Copy link

@marinintim marinintim commented Jul 23, 2016

I'm trying to tackle test infrastructure.

@jdm, should I put new tests under tests/wpt/web-platform-tests/http/basic_auth.html?
@jdm
Copy link
Member

@jdm jdm commented Jul 24, 2016

Sure!
@marinintim
Copy link

@marinintim marinintim commented Aug 3, 2016

Sorry, didn't have the time to actually do something yet about this issue, and compiling is not an easy challenge on my underpowered machine. I hope to tackle it somewhere around this weekend.
@jdm
Copy link
Member

@jdm jdm commented Aug 3, 2016

Thanks for letting us know!
@jdm
Copy link
Member

@jdm jdm commented Aug 22, 2016

@marinintim Any progress, or should someone else take a shot at this?
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Sep 11, 2016

Unassigning due to inactivity. This issue is open for grabs!
@KiChjang KiChjang removed the C-assigned label Sep 11, 2016
@gilbertw1
Copy link
Contributor

@gilbertw1 gilbertw1 commented Sep 14, 2016

Seems pretty straight forward, I'd like to tackle this issue and get my feet wet with this code base.
@jdm
Copy link
Member

@jdm jdm commented Sep 14, 2016

@gilbertw1 Please do! Ask questions if anything is unclear.
@jdm jdm added the C-assigned label Sep 14, 2016
@gilbertw1
Copy link
Contributor

@gilbertw1 gilbertw1 commented Sep 14, 2016

@jdm: I've got this a fix for this issue completed, and I've tested + verified it against a remove server I setup running nginx. However, I am having a tough time figuring out where to get started on creating the reference test and the corresponding python handler. Could you point me to an existing test that uses a python handler (along with the handler location)?

Thanks!
@jdm
Copy link
Member

@jdm jdm commented Sep 14, 2016

The python handlers are invoked by any URL that references them, like in this test. That corresponds to this handler.
@gilbertw1 gilbertw1 mentioned this issue Sep 15, 2016
Merged
4 of 5 tasks complete
@gilbertw1
Copy link
Contributor

@gilbertw1 gilbertw1 commented Sep 15, 2016

Just submitted a PR for this issue, may have err'ed a little on the side of over explaining. Thanks for your help!
bors-servo added a commit that referenced this issue Sep 16, 2016
@bors-servo
Auto merge of #13281 - gilbertw1:basic-auth-cache-clean, r=jdm
Loading status checks…
e5b9987 
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13281)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-network C-assigned E-easy
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Update basic auth cache to key off of origin instead of url
9 participants
@jdm @marinintim @gilbertw1 @mpaolini @notriddle @aneeshusa @KiChjang @highfive @rmfitzpatrick
You can’t perform that action at this time.