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

Fix module script descendant referrer tests to match spec PR #22038

Merged
merged 1 commit into from Mar 11, 2020

Conversation

domfarolino
Copy link
Member

This PR fixes module script descendent referrer tests that I originally added in https://crrev.com/c/1809205. The tests I added match the spec, and we were going to change Chromium's implementation accordingly, however after more discussion in w3c/webappsec-referrer-policy#123, we're aiming to change the spec to be more sane.

This PR makes the module script descendant scripts tests assert that a descendant script's referrer string is used in determining if the request is same-origin or not. Currently the spec computes same-origin-ness by comparing request's currently URL and request's origin (which can be its client's origin). It does not consider the referrer string, which may be different than its client's origin, via module scripts. Consider:

  • A.com/page.html fetches top-level module script B.com/x.js
  • B.com/x.js fetches a descendant script A.com/x.js

In this case, the request for A.com/x.js's referrer string is B.com/x.js, but it's client's origin is A.com. The point of the HTML Standard setting the descendant's referrer string to its parent's URL is to make the parent behave as the referrer in this case, but the Referrer Policy spec didn't respect this. w3c/webappsec-referrer-policy#123 changes the Referrer Policy spec accordingly, and this PR considers the above request to be cross-origin (the descendant request is cross-origin relative its parent).

@annevk
Copy link
Member

annevk commented Mar 3, 2020

  1. Are you saying that the fetch due to B.com/x.js (to A.com/x.js) should have referrer B.com, not B.com/x.js?
  2. What happens if A.com/page requests B.com/redirect and that goes to A.com/js?

@domfarolino
Copy link
Member Author

Are you saying that the fetch due to B.com/x.js (to A.com/x.js) should have referrer B.com, not B.com/x.js?

I think the request to A.com/x.js should have a "referrer string" of B.com/x.js. But since this request is cross-origin (relative to the referrer string), the Referer header might end up as B.com if the referrer policy is something like origin-when-cross-origin.

What happens if A.com/page requests B.com/redirect and that goes to A.com/js?

Not sure I understand. Does A.com/js do anything interesting? Is it the same as A.com/x.js?

@annevk
Copy link
Member

annevk commented Mar 4, 2020

Sure, it could be equivalent to /x.js. Sorry for adding confusion.

@domfarolino
Copy link
Member Author

domfarolino commented Mar 10, 2020

My understanding is that the following would happen:
cross origin redirect to same origin

Here the redirect doesn't change anything: the final request to A.com/x.js is the same with/without the redirect (no generated Referer, due to the referrer-string-origin request-url-origin not matching).

A case where the redirect would change something is when some request in the chain is cross-origin solely due to a redirect. Like below:
cross-origin redirect

Here there is no Referer when requesting a.com/final.js, whereas if the redirect wasn't there, that request would have a full Referer. Let me know if I didn't capture the scenario you were thinking of though. We can add redirect tests too, but it might be simpler to handle them outside of this PR. Any thoughts?

@annevk
Copy link
Member

annevk commented Mar 10, 2020

Thanks @domfarolino. I think that looks okay.

@annevk annevk closed this Mar 10, 2020
@annevk annevk reopened this Mar 10, 2020
@domfarolino domfarolino merged commit 324142d into master Mar 11, 2020
@domfarolino domfarolino deleted the domfarolino/module-script-descendant-referrer branch March 11, 2020 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants