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 upAdd "referrer" to fetch request used in navigation #22890
Comments
|
I'm not sure I understand what's being proposed here. The constellation is in charge of starting fetch requests for document navigations based on the script thread sending an InitiateNavigateRequest message, so it's not clear what we should be doing differently. |
|
I'm not completely sure myself, and it appeared to me that although there is perhaps a fetch request somewhere deep inside the navigation of a document, we could expose it more clearly in the script code. For example, I don't think we have a way to do Same with 14.3 of https://html.spec.whatwg.org/#window-open-steps Those two examples I think would be relevant to comply with Step 7 of https://fetch.spec.whatwg.org/#concept-main-fetch So the answer would be "expose the fetch request(or at least more of its configuration) for the navigation to script"? That's actually much simpler than what I sketched above... |
|
Right, we should expose any required configuration as part of the InitiateNavigateRequest message, and make sure that configuration is passed along any intermediate messages. |
|
Removed the "interesting-project", since this turns out to be a much smaller change than I thought, doesn't mean it's not interesting though! |
|
@highfive assign me |
|
Hey @miller-time! Thanks for your interest in working on this issue. It's now assigned to you! |
|
@gterzian (or maybe @jdm) any pointers to help me get started on this? I'm reading the methods that were mentioned above, but it's not entirely clear to me what needs to be changed. It looks like the numbered steps (in comments) for window.open seem to be pretty outdated, so I could start there. |
|
@miller-time Thanks for working on this one! Re the numbering, we've actually just merged #23011 which already improved the situation, and I suggest you rebase off the last master as a starting point(and note that the links to the code above in this issue will link to a older version of the codebase). The idea is that we want to add https://fetch.spec.whatwg.org/#concept-request-referrer to the various places in the code where we navigate using Currently I think the first step would be to add an argument representing Then you could take it further by taking a look at the various places where You might want to focus on a few low hanging fruits first, and perhaps open issues for things that could be best done separately. To see the various places where Note that this is the "fetch" spec, which is separate from the "html" spec, and fetch is mostly implemented in https://github.com/servo/servo/tree/master/components/net |
|
@gterzian thanks for the additional tips! I opened a draft PR, I hope that's okay... thought it might be helpful to see where I'm going.
I looked at InitiateNavigateRequest, per advice of @jdm, and saw that in constellation we handle that message, which passes a req_init (RequestInit). In Request.from_init we do the following: req.referrer = if let Some(url) = init.referrer_url {
Referrer::ReferrerUrl(url)
} else {
Referrer::NoReferrer
};So I think that will need to be changed as part of my efforts. I skimmed the definition of window.load_url and came across a MainThreadScriptMsg::Navigate message that is sent, which passes a LoadData struct. I might be able to add referrer to that. I tried to follow that message's usage, but I got to a scheduler and wasn't sure what happens next. I was hoping that the MainThreadScriptMsg::Navigate message results in a ScriptMsg::InitiateNavigateRequest message being sent eventually. If so, then this all ties together there. There are a lot of other uses of RequestInit, and I feel like I'm still missing... a lot.
That's good to know! I'll definitely do that next. |
|
Ok, sounds like you're going in the right direction. The servo/components/script/script_thread.rs Line 1743 in 7a67443 and handled as part of servo/components/script/script_thread.rs Line 3198 in 7a67443 There is actually another roundtrip happening before the servo/components/constellation/constellation.rs Line 2205 in 7a67443 It is approved via a message from the embedder received at servo/components/constellation/constellation.rs Line 1064 in 7a67443 which will result in So there is actually quite a bit of back and forth between the initial The question is whether the |
|
Thanks @gterzian I updated my PR - got it to build, cleaned it up. Tests pass and I haven't touched them yet... Is that good or bad? I've been browsing the fetch spec...
I can only find one occurrence of "about:client": servo/components/script/dom/request.rs Line 554 in 25aa650 Side note: should the branch
servo/components/net/fetch/methods.rs Line 142 in 7a67443 Per the spec it looks like I could do some work there.
servo/components/net/http_loader.rs Line 774 in 25aa650 It looks like referrer is being handled correctly, though the numbering in comments seems slightly out-of-date per the spec. Should I try to update that?
servo/components/net/http_loader.rs Line 1359 in 25aa650 It looks like we're correctly copying the referrer from the request.
I found several TODO comments when I added an servo/components/constellation/constellation.rs Line 1951 in 25aa650 servo/components/constellation/constellation.rs Line 2033 in 25aa650 servo/components/constellation/constellation.rs Line 2095 in 25aa650 I could use some guidance in case those should have logic for determining referrer... (on my branch, I have it set to One last question... Should existing references to |
|
Thanks for all the research! Note that
The spec seems to indeed point in that direction. Good catch! I think the servo/components/net_traits/request.rs Line 65 in 25aa650 It is used in the fetch code, as well as in the DOM request. Currently it looks like the servo/components/net_traits/request.rs Line 315 in 25aa650
Yes you might want to try that and see what other parts of the code are using it and it if it can be changes relatively easily. It would seem to make sense to just do everything with the referrer.
Yes that's a good idea. You could try to separate it from the referrer work either by commit or by PR. I think that just adding a separate commit to the PR should be fine. For the websocket I think you will have to look at servo/components/script/dom/websocket.rs Line 145 in 25aa650 Note that the spec seems to say that the referrer should be Re the TODO's in the constellation that you've found:
I think we should expect some tests to unexpectedly go from |
|
Yep, sorry I thought you had seen it auto linked by GH |
|
Updates
done!
done (I think) The last thing I wanted to do before requesting a review of the PR is:
I still haven't updated any tests... or seen any test failures... |
|
By the way, figuring out exactly how the steps have changed in the spec for |
|
Hmm in looking at main_fetch again: servo/components/net/fetch/methods.rs Line 185 in e98db92 ...I'm not sure if it makes sense to change anything in this PR. |
|
Thanks for the work! Just looking at fetch/method, I saw this FIXME at servo/components/net/fetch/methods.rs Line 194 in fe07d2c which points to this issue #14507 it's interesting because it would appear that in the case of I'm still looking at the overall PR and haven't had a chance to review everything, and I can't suggest exactly what to do re the above. I'll keep looking into over the next couple of days, and perhaps you can too when you have a chance? Re the tests, it might actually be that there are no tests yet for this specifically, we'll have to look into it a bit more as well... |
nice! that issue links to the exact same part of the spec that I mentioned in a previous comment. I hadn't paid close enough attention to that comment and noticed that there was a separate issue to handle that. I'll start to poke around in the tests, but I'm actually not familiar with them at all... Any pointers for where to look? |
|
I tried running |
|
Re the tests, I think we can find relevant ones in https://github.com/servo/servo/tree/ab5a57fadf649ede43ae3f478be6df9bfa69893a/tests/wpt/web-platform-tests/referrer-policy Re running the tests, when you run the whole suite locally you usually get a lot of timeout errors, so it's best to target a specific folder, or a single test. I'll run the suite on your PR via a dedicated command, to give us an indication if anything changed or not. Note that even then there can be quite a few unrelated failures occurring intermittently. |
Add referrer to navigation fetch request <!-- Please describe your changes on the following line: --> Implement step 13 of [following hyperlinks](https://html.spec.whatwg.org/#following-hyperlinks-2) and step 14.3 of [window open](https://html.spec.whatwg.org/#window-open-steps), as well as other referrer- and fetch-related updates. --- <!-- 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 - [x] These changes fix #22890 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/23090) <!-- Reviewable:end -->
|
@gterzian thanks! I'm starting to drill into these wpt tests. I think I found one unexpected passing test already. I found a failing test, but when I checked out master and ran:
...it failed there too. Just to make sure I'm on the right track... I assume I should at least run |
|
So locally if you run a single test, you'll see the result, but not whether it's unexpected or not. However when you run say So for example the one that you found that is failing on master is probably expected to fail(you'll know if you run the entire folder). From the CI run it looks like some tests went from It might also tie in with what's going on in main fetch, since now servo/components/net/fetch/methods.rs Line 193 in fe07d2c |
|
Running a single test does tell you whether it's unexpected or not. It just may not be clear if it's passing or not from the output, but that's separate from expected/unexpected. |
|
I tried changing the default from
Note: I switched to master again and ran Sorry I haven't made much progress on this, I'll keep digging. Should I ping anyone on the pull request? To get any additional feedback, etc. Or do I need to finish up these wpt tests before the review? Just curious (this is only my second one). |
|
Yep so it's the failures that are part of It is indeed better to try to first find out what is causing the unexpected test failures, before asking for a review. I can suggest picking a test that currently unexpectedly fails with Even though a lot of tests are failing unexpectedly, I suspect they all fail for the same reason and it could be a fairly easy fix that will become obvious once you've printed out the various state of the It might also be a slightly more complicated fix involving plugging your work in with the iframe navigations(a lot of tests seem to involve an iframe) and ensuring the new |
|
It may be easier to write your own manual tests first, using a script like this to print out the received referrer. This should be easier to determine if the problem lies in providing an accurate referrer, or if the problem lies in the code that acts on the requested referrer policy. |
|
So it turns out that having a bunch of small commits ended up being very helpful. By running the tests after each commit I discovered that ebb2ddf introduced all of the failures. I mishandled a lot of uses of LoadData that had a referrer_url set. I added a |
Add referrer to navigation fetch request <!-- Please describe your changes on the following line: --> Implement step 13 of [following hyperlinks](https://html.spec.whatwg.org/#following-hyperlinks-2) and step 14.3 of [window open](https://html.spec.whatwg.org/#window-open-steps), as well as other referrer- and fetch-related updates. --- <!-- 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 - [x] These changes fix #22890 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/23090) <!-- Reviewable:end -->
|
I've been looking into those last 3 failing tests. When I changed this line the referrer changed from In the test
it appears that servo/components/script/dom/location.rs Line 171 in 7726453 I think I have 2 options to fix this test:
I've poked a bit in the spec for location:
It's not clear to me how we would add a referrer in All tests should pass now. |
Add referrer to navigation fetch request <!-- Please describe your changes on the following line: --> Implement step 13 of [following hyperlinks](https://html.spec.whatwg.org/#following-hyperlinks-2) and step 14.3 of [window open](https://html.spec.whatwg.org/#window-open-steps), as well as other referrer- and fetch-related updates. --- <!-- 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 - [x] These changes fix #22890 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/23090) <!-- Reviewable:end -->
Add referrer to navigation fetch request <!-- Please describe your changes on the following line: --> Implement step 13 of [following hyperlinks](https://html.spec.whatwg.org/#following-hyperlinks-2) and step 14.3 of [window open](https://html.spec.whatwg.org/#window-open-steps), as well as other referrer- and fetch-related updates. --- <!-- 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 - [x] These changes fix #22890 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/23090) <!-- Reviewable:end -->
Add referrer to navigation fetch request <!-- Please describe your changes on the following line: --> Implement step 13 of [following hyperlinks](https://html.spec.whatwg.org/#following-hyperlinks-2) and step 14.3 of [window open](https://html.spec.whatwg.org/#window-open-steps), as well as other referrer- and fetch-related updates. --- <!-- 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 - [x] These changes fix #22890 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/23090) <!-- Reviewable:end -->
Add referrer to navigation fetch request <!-- Please describe your changes on the following line: --> Implement step 13 of [following hyperlinks](https://html.spec.whatwg.org/#following-hyperlinks-2) and step 14.3 of [window open](https://html.spec.whatwg.org/#window-open-steps), as well as other referrer- and fetch-related updates. --- <!-- 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 - [x] These changes fix #22890 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/23090) <!-- Reviewable:end -->
Add referrer to navigation fetch request <!-- Please describe your changes on the following line: --> Implement step 13 of [following hyperlinks](https://html.spec.whatwg.org/#following-hyperlinks-2) and step 14.3 of [window open](https://html.spec.whatwg.org/#window-open-steps), as well as other referrer- and fetch-related updates. --- <!-- 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 - [x] These changes fix #22890 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/23090) <!-- Reviewable:end -->
Add referrer to navigation fetch request <!-- Please describe your changes on the following line: --> Implement step 13 of [following hyperlinks](https://html.spec.whatwg.org/#following-hyperlinks-2) and step 14.3 of [window open](https://html.spec.whatwg.org/#window-open-steps), as well as other referrer- and fetch-related updates. --- <!-- 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 - [x] These changes fix #22890 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/23090) <!-- Reviewable:end -->
Add referrer to navigation fetch request <!-- Please describe your changes on the following line: --> Implement step 13 of [following hyperlinks](https://html.spec.whatwg.org/#following-hyperlinks-2) and step 14.3 of [window open](https://html.spec.whatwg.org/#window-open-steps), as well as other referrer- and fetch-related updates. --- <!-- 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 - [x] These changes fix #22890 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/23090) <!-- Reviewable:end -->
Add referrer to navigation fetch request <!-- Please describe your changes on the following line: --> Implement step 13 of [following hyperlinks](https://html.spec.whatwg.org/#following-hyperlinks-2) and step 14.3 of [window open](https://html.spec.whatwg.org/#window-open-steps), as well as other referrer- and fetch-related updates. --- <!-- 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 - [x] These changes fix #22890 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/23090) <!-- Reviewable:end -->

It seems that the spec is moving towards integration of navigation with Fetch, as exemplified by the aptly named process a navigate fetch and process a navigate response.
For example:
Our implementation of follow-hyperlink, window.open and navigate all use window.load_url, as entry point, which sends a message to the script-thread handled in
handle_navigate, which results in "parallel" steps being processes in the constellation. This "load url" algorithm is, as currently implemented, not integrated with our implementation of fetch, which lives somewhere around https://github.com/servo/servo/tree/65370f17c98225f7e71c72ea2e0cb2d0a81487f3/components/net/fetch.There is already some plumbing to integrate the constellation with fetch, in the form of https://github.com/servo/servo/blob/master/components/constellation/network_listener.rs, which might be re-used to deal with "load_url" as well...