Implement meta referrer policy delivery

[rebstar6]

Thank you for contributing to Servo! Please replace each [ ] by [X] when the step is complete, and replace __ with appropriate data:

• ./mach build -d does not report any errors
• ./mach test-tidy --faster does not report any errors
• These changes are part of #10311 (github issue number if applicable).

Either:

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

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @jgraham: tests/wpt/include.ini
• @KiChjang: components/net/http_loader.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/webidls/Document.webidl, components/script/dom/htmlmetaelement.rs, components/script/dom/document.rs, components/script/dom/htmliframeelement.rs

[rebstar6]

Notes:

• Is there a was to have tidy ignore the new tests in /tests/wpt/mozilla/tests/mozilla/referrer-policy? I copied directly from the WPT tests, but tidy is very unhappy with them. Other than those, tidy is passing.
• I have left default referrer policy at "no-referrer" rather than "None" (which translates to no-ref-when-downgrade). I think this is probably safer until we have implemented the other delivery policies (header, etc), but I can change that easily.

[jdm]

You can add the referrer-policy directory to the list of ignored directories in python/tidy/servo_tidy/tidy.py.

[highfive]

New code was committed to pull request.

[rebstar6]

Tidy still isnt working on my local machine - did I add in the right place?

[jdm]

Looks like there's only one error left, which is actually a valid one - we should remove the console.log from referrer-policy-test-case.js.

[nox]

@jdm Are you reviewing this? Should I assign you?

[jdm]

I can review this, but I am not at the moment.

[highfive]

New code was committed to pull request.

[bors-servo]

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

[rebstar6]

fyi: './mach test-tidy --faster' does not work, './mach test-tidy' does. Not sure if that's just a local issue or some ignore getting skipped in --faster

[jdm]

What's the output of --faster?

[bors-servo]

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

[nox]

I've started reviewing this. Could you start rebasing and squashing things together so that further reviews are simpler? Thanks in advance.

-S-awaiting-review +S-needs-code-changes +S-needs-squash

Previously, bors-servo wrote…

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

---

Reviewed 54 of 64 files at r1, 31 of 31 files at r2, 6 of 6 files at r3, 1 of 1 files at r4, 12 of 12 files at r5, 61 of 61 files at r6, 15 of 15 files at r7, 2 of 2 files at r8, 3 of 3 files at r9, 2 of 2 files at r10, 34 of 34 files at r11, 1 of 1 files at r12, 1 of 1 files at r13, 1 of 1 files at r14, 2 of 2 files at r15, 117 of 127 files at r16, 31 of 31 files at r17, 6 of 6 files at r18, 1 of 1 files at r19, 12 of 12 files at r20, 61 of 61 files at r21, 15 of 15 files at r22, 2 of 2 files at r23, 3 of 3 files at r24, 2 of 2 files at r25, 34 of 34 files at r26, 1 of 1 files at r27, 1 of 1 files at r28, 1 of 1 files at r29, 2 of 2 files at r30.
Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

---

components/net/http_loader.rs, line 570 [r30] (raw file):

                              auth_cache: &Arc<RwLock<AuthCache>>,
                              load_data: &LoadData,
                              referrer_url: &mut Option<Url>) {

Can we return an Option<Url> instead of taking &mut Option<Url>?

---

components/script/dom/document.rs, line 1823 [r1] (raw file):

        }
    }
    pub fn set_meta_referrer(&self, content: &str) {

Make content an Option<ReferrerPolicy> and rename it policy.`

---

components/script/dom/document.rs, line 1823 [r1] (raw file):

        }
    }
    pub fn set_meta_referrer(&self, content: &str) {

Rename the method set_referrer_policy.

---

components/script/dom/document.rs, line 2788 [r1] (raw file):

/// https://w3c.github.io/webappsec-referrer-policy/#determine-policy-for-token
fn determine_tokens_policy(token: String) -> Option<ReferrerPolicy> {

Make token an &str.

---

components/script/dom/document.rs, line 2788 [r1] (raw file):

/// https://w3c.github.io/webappsec-referrer-policy/#determine-policy-for-token
fn determine_tokens_policy(token: String) -> Option<ReferrerPolicy> {

Rename function to determine_policy_for_token, tokens sound plural when it shouldn't.

---

components/script/dom/document.rs, line 2789 [r1] (raw file):

/// https://w3c.github.io/webappsec-referrer-policy/#determine-policy-for-token
fn determine_tokens_policy(token: String) -> Option<ReferrerPolicy> {
    let lower = token.trim().to_lowercase();

No need to trim here, you already do in caller in HTMLMetaElement as the spec dictates.

---

components/script/dom/document.rs, line 2793 [r1] (raw file):
It seems you forgot step 4:

If token is an ASCII case-insensitive match for the string "same-origin", return "same-origin".

---

components/script/dom/document.rs, line 2790 [r2] (raw file):

fn determine_tokens_policy(token: String) -> Option<ReferrerPolicy> {
    let lower = token.trim().to_lowercase();
    println!("POLICY STR {:?}", lower);

Remove before merging.

---

components/script/dom/document.rs, line 1840 [r17] (raw file):

        }
    }
    pub fn set_meta_referrer(&self, content: &str) {

Nit: newline before method.

---

components/script/dom/htmlmetaelement.rs, line 98 [r1] (raw file):
The spec says:

If any meta elements are inserted into the document or removed from the document, or existing meta elements have their name or content attributes changed, user agents must run the following algorithm: (...).

Indeed, the document's referrer policy should only change if the first <meta> referrer policy element is altered or removed.

---

components/script/dom/htmlmetaelement.rs, line 101 [r1] (raw file):
The spec says:

The element is a child of a head element

It isn't limited to <meta> elements in the document's <head> element.

Don't forget to check that the element is in a document (first condition) when you will fix this.

---

components/script/dom/htmlmetaelement.rs, line 101 [r1] (raw file):

        let doc = document_from_node(self);
        if let Some(head) = doc.GetHead() {
            if head.upcast::<Node>().is_parent_of(self.upcast::<Node>()){

Nit: missing space before brace.

---

components/script/dom/htmlmetaelement.rs, line 107 [r1] (raw file):

                    if !content.is_empty() {
                        doc.set_meta_referrer(content.trim());
                    }

You check for emptiness before trimming. This is why I suggest making the document method take a ReferrerPolicy directly.

---

components/script/dom/xmlhttprequest.rs, line 159 [r5] (raw file):

    fn new_inherited(global: GlobalRef) -> XMLHttpRequest {
        //TODO - will this panic (outside of the scope of the ref policy tests)?
        let current_doc = global.as_window().Document();

This can panic because XMLHttpRequest is also exposed to workers. Workers have their own stuff for referrer policy.

https://html.spec.whatwg.org/multipage/#concept-workerglobalscope-referrer-policy

---

tests/wpt/mozilla/tests/mozilla/referrer-policy/generic/subresource/subresource.py, line 27 [r13] (raw file):

        destination_netloc = get_swapped_origin_netloc(parsed.netloc)

    query = filter(lambda x: x.startswith('id='), parsed.query.split('&'))

This should be removed before merging, right?

---

tests/wpt/web-platform-tests/referrer-policy/generic/referrer-policy-test-case.js, line 114 [r1] (raw file):

          //               t._expectedReferrerUrl,
          //               "Reported Referrer URL is '" +
          //               t._scenario.referrer_url + "'.");

Remove this before merging.

---

Comments from Reviewable

[rebstar6]

@jdm: --faster does not seem to recognize the update in python/tidy/servo_tidy/tidy.py - it gives me all the formatting errors in the new tests at tests/wpt/mozilla/tests/mozilla/referrer-policy (folder isn't ignored)

[rebstar6]

Review status: 4 of 153 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

---

components/net/http_loader.rs, line 570 [r30] (raw file):

Previously, nox (Anthony Ramine) wrote…

Can we return an Option<Url> instead of taking &mut Option<Url>?

the modify_request_headers method handles multiple headers, not just Referer. Seems odd to me to have it return the updated referrer_url, but if you guys decide that's best I can change it

---

components/script/dom/document.rs, line 1823 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

Rename the method set_referrer_policy.

Done.

---

components/script/dom/document.rs, line 1823 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

Make content an Option<ReferrerPolicy> and rename it policy.`

Done.

---

components/script/dom/document.rs, line 2788 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

Make token an &str.

Done.

---

components/script/dom/document.rs, line 2788 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

Rename function to determine_policy_for_token, tokens sound plural when it shouldn't.

Done.

---

components/script/dom/document.rs, line 2789 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

No need to trim here, you already do in caller in HTMLMetaElement as the spec dictates.

Done.

---

components/script/dom/document.rs, line 2793 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

It seems you forgot step 4:

If token is an ASCII case-insensitive match for the string "same-origin", return "same-origin".

Oy, this spec is a moving target...that's a new one. Our tests right now don't have the "same-origin" policy, and "origin" is "origin-only" (as it was formerly).

@jdm, thoughts??

---

components/script/dom/document.rs, line 2790 [r2] (raw file):

Previously, nox (Anthony Ramine) wrote…

Remove before merging.

Done.

---

components/script/dom/document.rs, line 1840 [r17] (raw file):

Previously, nox (Anthony Ramine) wrote…

Nit: newline before method.

Done.

---

components/script/dom/htmlmetaelement.rs, line 101 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

The spec says:

The element is a child of a head element

It isn't limited to <meta> elements in the document's <head> element.

Don't forget to check that the element is in a document (first condition) when you will fix this.

"It isn't limited to elements in the document's element." -- what do you mean?

---

components/script/dom/htmlmetaelement.rs, line 107 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

You check for emptiness before trimming. This is why I suggest making the document method take a ReferrerPolicy directly.

Done.

---

Comments from Reviewable

[rebstar6]

@jdm @nox - re: the comment above about "same-origin" and the spec changing: should we change sources over to https://www.w3.org/TR/referrer-policy/ (vs https://w3c.github.io/webappsec-referrer-policy/)?

[rebstar6]

Xml tests no longer work after addressing merge conflicts - I'll look into it.

[nox]

Great work rebasing and squashing this, must not have been easy.

-S-awaiting-review -S-needs-squash +S-needs-code-changes

Previously, rebstar6 (Rebecca) wrote…

Xml tests no longer work after addressing merge conflicts - I'll look into it.

---

Reviewed 4 of 31 files at r2, 5 of 15 files at r7, 4 of 31 files at r17, 5 of 15 files at r22, 151 of 151 files at r31, 84 of 84 files at r32, 62 of 62 files at r33, 149 of 149 files at r34, 84 of 84 files at r35, 62 of 62 files at r36.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

components/net/http_loader.rs, line 570 [r30] (raw file):

Previously, rebstar6 (Rebecca) wrote…

the modify_request_headers method handles multiple headers, not just Referer. Seems odd to me to have it return the updated referrer_url, but if you guys decide that's best I can change it

Fair enough, let's keep it that way for now.

---

components/script/dom/htmlmetaelement.rs, line 101 [r1] (raw file):

Previously, rebstar6 (Rebecca) wrote…

"It isn't limited to elements in the document's element." -- what do you mean?

@rebstar6 Did you actually see just that as a comment? If yes this sounds like we found a Reviewable bug (Cc @pkaminski):

So what I said is that all meta elements with a name=referrer which parent are a head should be taken into consideration, not just the ones found in the document's head element.

---

components/script/dom/htmlmetaelement.rs, line 107 [r1] (raw file):
You still don't trim at the right moment:

let content = content.value().trim();
if !content.is_empty() {
    doc.set_referrer_policy(determine_policy_for_token(content));
}

See what the spec says:

• Let value be the result of stripping leading and trailing whitespace from the value of element's content attribute.
• If value is not the empty string, then: (...)

---

Comments from Reviewable

[pkaminski]

components/script/dom/htmlmetaelement.rs, line 101 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

@rebstar6 Did you actually see just that as a comment? If yes this sounds like we found a Reviewable bug (Cc @pkaminski):

So what I said is that all meta elements with a name=referrer which parent are a head should be taken into consideration, not just the ones found in the document's head element.

Uh-oh, that could actually be a security concern. @rebstar6, where did you see the comment with the missing tags? In Reviewable, on GitHub, in email, ...? If you could capture a screenshot that would be great. Thanks.

---

Comments from Reviewable

[rebstar6]

components/script/dom/htmlmetaelement.rs, line 101 [r1] (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

Uh-oh, that could actually be a security concern. @rebstar6, where did you see the comment with the missing tags? In Reviewable, on GitHub, in email, ...? If you could capture a screenshot that would be great. Thanks.

Ahh, weird, I now see what you see. I was thinking it was a copy/paste error perhaps, but the tags stay when I copy it.

Not sure what happened, but no worries, my screenshot is the same as above.

---

Comments from Reviewable

[highfive]

New code was committed to pull request.

[rebstar6]

Latest change addresses the xml tests failing - reviewable comments haven't yet been addressed (feel free to remove S-awaiting-review)

[bors-servo]

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

[jdm]

Note, these changes need to be reverted or they'll go upstream.

[rebstar6]

ill rebase again - i think that should address it

[rebstar6]

Review status: 6 of 154 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

components/script/dom/htmlmetaelement.rs, line 98 [r1] (raw file):

Previously, somebody wrote…

The spec says:

If any meta elements are inserted into the document or removed from the document, or existing meta elements have their name or content attributes changed, user agents must run the following algorithm: (...).

Indeed, the document's referrer policy should only change if the first <meta> referrer policy element is altered or removed.

The 'altered or removed' case is more confusing. I was thinking of checking if doc.get_referrer_policy is None before setting it here, though that would stop it from ever changing.

Is there any straightforward way to know if the current htmlmetaelement is the first referrer-policy element?

---

components/script/dom/htmlmetaelement.rs, line 101 [r1] (raw file):

Previously, rebstar6 (Rebecca) wrote…

Ahh, weird, I now see what you see. I was thinking it was a copy/paste error perhaps, but the tags stay when I copy it.

Not sure what happened, but no worries, my screenshot is the same as above.

Also, to the initial comment, what is the difference between meta elements "whose parent are a head" and meta elements "found in the document's head element"?

---

components/script/dom/htmlmetaelement.rs, line 107 [r1] (raw file):

Previously, somebody wrote…

You still don't trim at the right moment:

let content = content.value().trim();
if !content.is_empty() {
    doc.set_referrer_policy(determine_policy_for_token(content));
}

See what the spec says:

• Let value be the result of stripping leading and trailing whitespace from the value of element's content attribute.
• If value is not the empty string, then: (...)

Got it! Done.

---

components/script/dom/xmlhttprequest.rs, line 159 [r5] (raw file):

Previously, somebody wrote…

This can panic because XMLHttpRequest is also exposed to workers. Workers have their own stuff for referrer policy.

https://html.spec.whatwg.org/multipage/#concept-workerglobalscope-referrer-policy

For right now they don't though - I've just been implementing in Document.

I need some way for the document referrer policy to be passed along to this XMLHttpRequest type. What I have now works according to the tests, but given what you're saying, I'm worried it could panic if a worker is calling XMLHttpRequest::new.

In theory, if a worker calls this, it should set referrer_url and referrer_policy to None. Do you know if there is a good way to address this?

---

tests/wpt/web-platform-tests/referrer-policy/generic/referrer-policy-test-case.js, line 114 [r1] (raw file):

Previously, somebody wrote…

Remove this before merging.

Done. I thought this would fail. I was wrong.

---

Comments from Reviewable

[rebstar6]

I don't like this double let, but "let content = content.value().trim();' throws "borrowed value does not live long enough."

Better way?

[nox]

Reviewed 4 of 31 files at r2, 5 of 15 files at r7, 4 of 31 files at r17, 5 of 15 files at r22, 148 of 148 files at r37, 84 of 84 files at r38, 62 of 62 files at r39, 152 of 152 files at r40, 1 of 1 files at r41, 83 of 83 files at r42, 62 of 62 files at r43.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

components/script/dom/document.rs, line 2789 [r1] (raw file):

Previously, rebstar6 (Rebecca) wrote…

Done.

This is still here it seems.

---

components/script/dom/htmlmetaelement.rs, line 98 [r1] (raw file):

Previously, rebstar6 (Rebecca) wrote…

The 'altered or removed' case is more confusing. I was thinking of checking if doc.get_referrer_policy is None before setting it here, though that would stop it from ever changing.

Is there any straightforward way to know if the current htmlmetaelement is the first referrer-policy element?

Let's call a `meta` element which satisfies the conditions in step 1 and with a policy different than the empty string a valid candidate.

Let's call the first valid candidate that is in the document the referrer element of the document.

• Whenever a valid candidate is inserted into the document, or an existing meta element is mutated such as it becomes a valid candidate:
  • if that new valid candidate is after the referrer element of the document, nothing changes.
  • if it is before the referrer element, it becomes the new referrer element.
• Whenever a valid candidate is removed from the document, or an existing valid candidate is mutated such as it is not one anymore:
  • if it is not the referrer element of the document, nothing changes.
  • if it is the referrer element, the next valid candidate in the document becomes the new referrer element, if any.

When I say "before" and "after", I mean in tree order. If we get the spec to change to say "in the head element document" in the fourth document, things get simpler to cleverly avoid the tree order comparison.

Does this make sense @jdm?

---

components/script/dom/htmlmetaelement.rs, line 101 [r1] (raw file):

Previously, rebstar6 (Rebecca) wrote…

Also, to the initial comment, what is the difference between meta elements "whose parent are a head" and meta elements "found in the document's head element"?

See end of comment above.

---

components/script/dom/htmlmetaelement.rs, line 107 [r41] (raw file):

Previously, rebstar6 (Rebecca) wrote…

I don't like this double let, but "let content = content.value().trim();' throws "borrowed value does not live long enough."

Better way?

Such is life. :)

---

Comments from Reviewable

[jdm]

That does seem reasonable.

[nox]

@jdm @rebstar6 I wonder, should we recreate the PR after the next reviewing iteration? Reviewable is a bit unusable with all the revisions. :)

[rebstar6]

Up to you guys!
Also, all set with THE head element (vs any head element) - whatwg/html#1311

[nox]

Great work! Please recreate the PR, I would rather review it from scratch now that things settled than use a very slow Reviewable UI. :)

[KiChjang]

Superseded by #11422.