1534681 - Use ReferrerInfo class in document and make referrer in iframe to be compliant with spec.

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Comment 2

[Thomas Nguyen (:tnguyen)]

https://www.w3.org/TR/referrer-policy/#determine-requests-referrer
While document is an iframe srcdoc document, let document be document’s browsing context’s browsing context container’s node document.
And another
https://www.w3.org/TR/referrer-policy/#referrer-policy-delivery-nested

Comment 3

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1534681 Use ReferrerInfo class in document

Also, in many place, we use document uri as referrer. It is not right
for the case srdoc iframe. We should use the last non-srdoc parent
document's uri

Comment 4

[Thomas Nguyen (:tnguyen)]

:annek, do we have time to take a look (or talk) at this a bit? I really expect to get referrer and referrer policy from the same source (inheritance iframe)

https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-delivery-nested [1]
And https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer step 3-3 [2]

iframe srcdoc seems be in good shape, but how's about iframe with blob's url and another type (data:url and javascript:). Should we add a bit more to [2], and do the same way as iframe srcdoc?

Comment 5

[Thomas Nguyen (:tnguyen)]

https://phabricator.services.mozilla.com/D30191#change-73Rvei7Gl9nx.
That is blob url wpt test, I think request's referrer should treat blob the same as srcdoc.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Thomas Nguyen from comment #4)

:annek, do we have time to take a look (or talk) at this a bit? I really expect to get referrer and referrer policy from the same source (inheritance iframe)

@Anne, in particular, could you take a look at ShouldInheritReferrerInfo(), where we explicitly list:
about:srcdoc, blob:, data:,

In addition, with the patch we would only inherit for iframe loads but never for top-level window (problem is somehow mitigated in that we don't allow top-level data: URIs anymore). But still, would it make more sense to move the inheritance coputations from document into docshell, that way we could also cover all kinds of top-level inheritance. Thanks!

Comment 7

[Anne (:annevk)]

There's a couple things here:

1. We need to inherit (or rather, copy-on-navigate, see below) into iframe, but also top-level, otherwise window.open() or equivalent would be an escape.
2. Ideally we use a single abstraction for this pattern that is shared across base URLs, CSP list, referrer policy, HTTPS state, and any other state we might add to document that needs to be copied into documents that cannot set such state themselves.
3. Ideally this state is copied at the moment the new context is being navigated (i.e., not when it is created), to have as few cross-process roundtrips as possible and give web developers a somewhat reasonable and clear model to write applications against.
4. document.open() should also copy.
5. Specifications around this are all over the map and don't have 1, 2, or 3, or at least not consistently across all affected features. It will require some work to get things in shape there, but I don't think that should block us from pushing our desired model.
6. We should also reach out to Chrome and Safari about this and see if they have any disagreements. (In the context of Feature Policy Google seemed in agreement at least.)

bz, given that you've raised a number of these problems over the years and also fixed a fair number, any issues we might run into with the above?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Anne (:annevk) from comment #7)

There's a couple things here:
2. Ideally we use a single abstraction for this pattern that is shared across base URLs, CSP list, referrer policy, HTTPS state, and any other state we might add to document that needs to be copied into documents that cannot set such state themselves.

Some of the infrastructure is already there, let me provide some background:

Within Bug 965637 we also manually copy the CSP over for about:blank and also document.open(). I agree that we should introduce a new container which sets all of base URL, CSP, referrer and what not for the newly created browsing context.

For the front-end code we recently added the loadURIOptions dictionary (see Bug 1513241) which provides all of that information and passes it through to the docshell. We verified this works correctly for the various ways to open a new browsing context. Within the docshell we can then create that new container based on whether the URI to be loaded actually needs to inherit.

I think what I am about to say is, I agree to bullet (3) that we can create that new abstraction within docshell, which is the context being navigated.

Comment 9

[Boris Zbarsky [:bzbarsky]]

A few things:

1. See some existing discussion on this in bug 1265961 (which should get marked duplicate of this, or vice versa, either way?). Note the comments in there about places that get referrers from principals, which is wrong too.

2. For the specific case of srcdoc, iframe is the only case we need to worry about.

3. For blob: and data: we would need to handle window.open()... What do we do right now for those? What do other browsers do? What does the spec currently say, if anything? Note that I don't think we should block fixing the srcdoc case on this. So a plausible sequence of events is to add a "not the document URI or the principal" source of referrer information, make sure we use it consistently for referrers, make it work right for srcdoc, then think about how to set that source of information for the blob:/data: cases.

4. The spec for about:srcdoc referrer is broken, now that I look at it. Consider the case of a srcdoc inside a srcdoc inside another document...

5. In general, the plan from comment 7 sounds good to me, but I have one concern. It's observably different from the current attempt at https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer in cases when the document URL changes. In particular, what happens if a document adds a srcdoc iframe, say, then does a pushState on itself that changes the URL? Should that srcdoc use the pre-pushState URL for its subresource loads or the post-pushState one? What about the document that's doing the pushState itself? Presumably for compat with existing implementations that should be the post-pushState url?

Comment 10

[Thomas Nguyen (:tnguyen)]

Thanks bz, we will not block fixing srcdoc (the original plan is only fixing srcdoc in this bug).
My wpt tests result that Firefox and Chrome will send null referrer for every load from iframe created by blob and data:url (referrer header is stripped URL and only accept ftp, http, https). Unfortunately, window.open does not mention about how to handle referrer and referrer policy.

I probably will go back to the original patch, and leave data: and blob url in a follow-up

1. See some existing discussion on this in bug 1265961 (which should get marked duplicate of this, or vice versa, either way?). Note the comments in there about places that get referrers from principals, which is wrong too.
2. In general, the plan from comment 7 sounds good to me, but I have one concern. It's observably different from the current attempt at https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer in cases when the document URL changes. In particular, what happens if a document adds a srcdoc iframe, say, then does a pushState on itself that changes the URL? Should that srcdoc use the pre-pushState URL for its subresource loads or the post-pushState one? What about the document that's doing the pushState itself? Presumably for compat with existing implementations that should be the post-pushState url?

That's an interesting point, I understand that getting referrer from principal seems not to be the right thing, I will notice your concern when fixing the problem.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5df2e6d8478
Use ReferrerInfo class in document r=ckerschb,baku,Gijs

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b055270d5a52
Use ReferrerInfo class in document: update code after merge conflict. a=merge-conflict CLOSED TREE

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b5df2e6d8478
https://hg.mozilla.org/mozilla-central/rev/b055270d5a52

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18208 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged