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

Implement referrer policy delivery via noreferrer link relation #12493

Merged
merged 2 commits into from Sep 21, 2016

Conversation

Projects
None yet
6 participants
@TheKK
Contributor

TheKK commented Jul 18, 2016

According to https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-delivery, there's <a>, <link> and <area> could apply this delivery method. This PR contains changes for <a> and <link> but not <area>, since HTMLAreaElement is barely implemented.

We should file another issue for it.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11862
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Jul 18, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @KiChjang (or someone else) soon.

highfive commented Jul 18, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @KiChjang (or someone else) soon.

@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Jul 18, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/htmlanchorelement.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/htmlscriptelement.rs, components/script/dom/document.rs, components/script/dom/window.rs, components/script/dom/htmllinkelement.rs, components/script/dom/location.rs, components/script/document_loader.rs

highfive commented Jul 18, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/htmlanchorelement.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/htmlscriptelement.rs, components/script/dom/document.rs, components/script/dom/window.rs, components/script/dom/htmllinkelement.rs, components/script/dom/location.rs, components/script/document_loader.rs
@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Jul 18, 2016

Member

I don't believe the fallback should occur in that method, since it doesn't match https://w3c.github.io/webappsec-referrer-policy/#determine-policy-for-token .

Member

jdm commented Jul 18, 2016

I don't believe the fallback should occur in that method, since it doesn't match https://w3c.github.io/webappsec-referrer-policy/#determine-policy-for-token .

@TheKK

This comment has been minimized.

Show comment
Hide comment
@TheKK

TheKK Jul 18, 2016

Contributor

Oops. I misunderstand this with the empty string referrer policy. fixed.

Contributor

TheKK commented Jul 18, 2016

Oops. I misunderstand this with the empty string referrer policy. fixed.

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jul 18, 2016

Contributor

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

Contributor

bors-servo commented Jul 18, 2016

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

@TheKK

This comment has been minimized.

Show comment
Hide comment
@TheKK

TheKK Jul 19, 2016

Contributor

Hmm. Seems like 1a242d8 change the default referrer policy of Document and change part of my test expectations. I understand it's safer to set default as no-referrer for now, but it makes some of my test expectations from pass to failed and vice versa.

That's quite subtle since those passed tests are actually wrong. What do you think about this?

Contributor

TheKK commented Jul 19, 2016

Hmm. Seems like 1a242d8 change the default referrer policy of Document and change part of my test expectations. I understand it's safer to set default as no-referrer for now, but it makes some of my test expectations from pass to failed and vice versa.

That's quite subtle since those passed tests are actually wrong. What do you think about this?

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Jul 19, 2016

Member

I think now that we implement multiple ways of delivering referrer policies (meta, HTTP header, and attributes) we can switch the default to no-referrer-when-downgrade without concern.

Member

jdm commented Jul 19, 2016

I think now that we implement multiple ways of delivering referrer policies (meta, HTTP header, and attributes) we can switch the default to no-referrer-when-downgrade without concern.

@KiChjang

This comment has been minimized.

Show comment
Hide comment
@KiChjang
Member

KiChjang commented Jul 21, 2016

r? @jdm

@highfive highfive assigned jdm and unassigned KiChjang Jul 21, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jul 21, 2016

Contributor

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

Contributor

bors-servo commented Jul 21, 2016

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

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Jul 28, 2016

Member

I tried to use reviewable, but either it or Firefox choked. I'll leave a comment when I'm done reviewing the diffs on github.

Member

jdm commented Jul 28, 2016

I tried to use reviewable, but either it or Firefox choked. I'll leave a comment when I'm done reviewing the diffs on github.

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Jul 28, 2016

Member

Review complete! Thanks for tackling this, and for writing the additional tests!

Member

jdm commented Jul 28, 2016

Review complete! Thanks for tackling this, and for writing the additional tests!

@TheKK

This comment has been minimized.

Show comment
Hide comment
@TheKK

TheKK Sep 20, 2016

Contributor

Added!

Contributor

TheKK commented Sep 20, 2016

Added!

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm
Member

jdm commented Sep 20, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 20, 2016

Contributor

📌 Commit a7639d9 has been approved by jdm

Contributor

bors-servo commented Sep 20, 2016

📌 Commit a7639d9 has been approved by jdm

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 20, 2016

Contributor

⌛️ Testing commit a7639d9 with merge 2945449...

Contributor

bors-servo commented Sep 20, 2016

⌛️ Testing commit a7639d9 with merge 2945449...

bors-servo added a commit that referenced this pull request Sep 20, 2016

Auto merge of #12493 - TheKK:referrer_policy_dliver_via_rel, r=jdm
Implement referrer policy delivery via noreferrer link relation

According to https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-delivery, there's `<a>`, `<link>` and `<area>` could apply this delivery method. This PR contains changes for `<a>` and `<link>` **but** not `<area>`, since HTMLAreaElement is barely implemented.

We should file another issue for it.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11862
- [X] There are tests for these changes

<!-- 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/12493)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 20, 2016

Contributor

💔 Test failed - mac-rel-wpt

Contributor

bors-servo commented Sep 20, 2016

💔 Test failed - mac-rel-wpt

@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Sep 20, 2016

  ▶ TIMEOUT [expected OK] /webgl/conformance-1.0.3/conformance/textures/origin-clean-conformance.html

highfive commented Sep 20, 2016

  ▶ TIMEOUT [expected OK] /webgl/conformance-1.0.3/conformance/textures/origin-clean-conformance.html
@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm
Member

jdm commented Sep 20, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 20, 2016

Contributor

⌛️ Testing commit a7639d9 with merge 8a78e75...

Contributor

bors-servo commented Sep 20, 2016

⌛️ Testing commit a7639d9 with merge 8a78e75...

bors-servo added a commit that referenced this pull request Sep 20, 2016

Auto merge of #12493 - TheKK:referrer_policy_dliver_via_rel, r=jdm
Implement referrer policy delivery via noreferrer link relation

According to https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-delivery, there's `<a>`, `<link>` and `<area>` could apply this delivery method. This PR contains changes for `<a>` and `<link>` **but** not `<area>`, since HTMLAreaElement is barely implemented.

We should file another issue for it.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11862
- [X] There are tests for these changes

<!-- 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/12493)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo
Contributor

bors-servo commented Sep 21, 2016

@bors-servo bors-servo merged commit a7639d9 into servo:master Sep 21, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 21, 2016

Member

Woooo! Thanks for sticking with this, @TheKK!

Member

jdm commented Sep 21, 2016

Woooo! Thanks for sticking with this, @TheKK!

@TheKK TheKK deleted the TheKK:referrer_policy_dliver_via_rel branch Sep 21, 2016

@TheKK TheKK restored the TheKK:referrer_policy_dliver_via_rel branch Sep 21, 2016

@TheKK TheKK deleted the TheKK:referrer_policy_dliver_via_rel branch Sep 22, 2016

bors-servo added a commit that referenced this pull request Mar 31, 2017

Auto merge of #13713 - TheKK:fix_a_tag_referrer_policy_test, r=TheKK
Fix referrer policy tests for a-tag

This RP tries to fix referrer policy test for <a> which was introduced in #12493 (sorry for my silly mistake). But the fact that Servo lakes of some functionalities make these tests a little tricky to do.

The desired solution for tests for <a> is to:
1. create a document which is running test harness
2. append an `<iframe>` to its parent document and give it a name
3. append and `<a>` to its parent document and set its `target` as `<iframe>`'s name
4. each referrer policy attribute (eg. HTTP header, <meta>) would contribute to `<a>` directly
5. we call `click()` on `<a>` and when the test was done, we call `postMessage()` inside `<iframe>` to notify its parent document

And target feature for `<a>` and cross origin `postMessage()` is still on its way. My solution is:
1. create a document which is running test harness
2. append an `<iframe>` to its parent document
3. append and `<a>` into `<iframe>`
4. we call `click()` on `<a>` and `<iframe>` navigate to `<a>`'s href

Current solution does not work for some cases:
- HTTP header, it only apply to test harness html document but `<a>` inside `<iframe>`
- cross origin detection, we navigate `<iframe>` rather than its parent document, which make test expectation wrong

One workaround in my mind is to load our test harness html document **inside** `<iframe>` under sandbox, so the test won't run again and we get `<meta>` and HTTP header as we expect. But this would break some consistency in `common.js` and make thing more complex.

---

Sorry for the long description. But I'd like to hear more thought before I actually make things dirty, and find the most proper solution for this.

<!-- 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/13713)

<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Mar 31, 2017

Auto merge of #13713 - TheKK:fix_a_tag_referrer_policy_test, r=jdm
Fix referrer policy tests for a-tag

This RP tries to fix referrer policy test for <a> which was introduced in #12493 (sorry for my silly mistake). But the fact that Servo lakes of some functionalities make these tests a little tricky to do.

The desired solution for tests for <a> is to:
1. create a document which is running test harness
2. append an `<iframe>` to its parent document and give it a name
3. append and `<a>` to its parent document and set its `target` as `<iframe>`'s name
4. each referrer policy attribute (eg. HTTP header, <meta>) would contribute to `<a>` directly
5. we call `click()` on `<a>` and when the test was done, we call `postMessage()` inside `<iframe>` to notify its parent document

And target feature for `<a>` and cross origin `postMessage()` is still on its way. My solution is:
1. create a document which is running test harness
2. append an `<iframe>` to its parent document
3. append and `<a>` into `<iframe>`
4. we call `click()` on `<a>` and `<iframe>` navigate to `<a>`'s href

Current solution does not work for some cases:
- HTTP header, it only apply to test harness html document but `<a>` inside `<iframe>`
- cross origin detection, we navigate `<iframe>` rather than its parent document, which make test expectation wrong

One workaround in my mind is to load our test harness html document **inside** `<iframe>` under sandbox, so the test won't run again and we get `<meta>` and HTTP header as we expect. But this would break some consistency in `common.js` and make thing more complex.

---

Sorry for the long description. But I'd like to hear more thought before I actually make things dirty, and find the most proper solution for this.

<!-- 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/13713)

<!-- Reviewable:end -->

bors-servo added a commit that referenced this pull request Mar 31, 2017

Auto merge of #13713 - TheKK:fix_a_tag_referrer_policy_test, r=jdm
Fix referrer policy tests for a-tag

This RP tries to fix referrer policy test for <a> which was introduced in #12493 (sorry for my silly mistake). But the fact that Servo lakes of some functionalities make these tests a little tricky to do.

The desired solution for tests for <a> is to:
1. create a document which is running test harness
2. append an `<iframe>` to its parent document and give it a name
3. append and `<a>` to its parent document and set its `target` as `<iframe>`'s name
4. each referrer policy attribute (eg. HTTP header, <meta>) would contribute to `<a>` directly
5. we call `click()` on `<a>` and when the test was done, we call `postMessage()` inside `<iframe>` to notify its parent document

And target feature for `<a>` and cross origin `postMessage()` is still on its way. My solution is:
1. create a document which is running test harness
2. append an `<iframe>` to its parent document
3. append and `<a>` into `<iframe>`
4. we call `click()` on `<a>` and `<iframe>` navigate to `<a>`'s href

Current solution does not work for some cases:
- HTTP header, it only apply to test harness html document but `<a>` inside `<iframe>`
- cross origin detection, we navigate `<iframe>` rather than its parent document, which make test expectation wrong

One workaround in my mind is to load our test harness html document **inside** `<iframe>` under sandbox, so the test won't run again and we get `<meta>` and HTTP header as we expect. But this would break some consistency in `common.js` and make thing more complex.

---

Sorry for the long description. But I'd like to hear more thought before I actually make things dirty, and find the most proper solution for this.

<!-- 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/13713)

<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment