Add "origin" and "same-origin" referrer policies, replacing "origin-only". #11978

aravind-pg · 2016-07-01T02:43:54Z

Adds the "origin" and "same-origin" referrer policy options, with the former replacing "origin-only".

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

This change is

highfive · 2016-07-01T02:44:00Z

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

highfive · 2016-07-01T02:44:00Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/document.rs, components/net/http_loader.rs

aravind-pg · 2016-07-01T03:18:46Z

cc @jdm

aravind-pg · 2016-07-05T03:58:27Z

Updated with wpt test updates. Commits kept separate to keep things manageable to review -- it's basically only the first that's important.

For the record, this is what I did in the second commit: update spec.src.json to add "origin" and "same-origin" in place of "origin-only", generate the new test files, run tests (in release mode), and update expectations. The third commit is almost the same but for tests/wpt/mozilla/tests/mozilla/referrer-policy, and along with the the "origin" and "same-origin" updates it also removes all but iframe-tag tests (see #11384 for context).

r? @jdm -- since you've been following along on the issue.

bors-servo · 2016-07-05T06:04:16Z

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

jdm · 2016-07-05T15:41:30Z

I'm going to merge #12265 first, since that includes updates to the referrer-policy tests that are duplicated here. We should be able to remove the second commit in this PR, I believe.

aravind-pg · 2016-07-05T17:34:19Z

Sure, sounds good. But won't I still need to update test expectations for the new tests? It's only this PR that adds the new functionality, and in that PR all the new tests will probably be marked as failing, right?

jdm · 2016-07-05T18:35:54Z

That's correct.

bors-servo · 2016-07-05T22:06:36Z

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

aravind-pg · 2016-07-07T06:20:31Z

All right! Updated and rebased, should be good to go now.

rebstar6 · 2016-07-07T14:39:20Z

Hey, just checking in - looking at your branch:
this file: https://github.com/aravind-pg/servo/blob/new-referrer-pols/tests/wpt/mozilla/tests/mozilla/referrer-policy/spec.src.json
should be the same as this file: https://github.com/aravind-pg/servo/blob/new-referrer-pols/tests/wpt/web-platform-tests/referrer-policy/spec.src.json
except that the 1st only has iframe-tag as a subresource. There are other changes (see unset-referrer-policy, delivery methods, etc) that I never copied over, and you don't seem to either. I'd suggest just copying the spec.src.json file from wpt/web-platform-tests into wpt/mozilla as the starting point, making the changes to be iframe-only, and regenerating from there.

aravind-pg · 2016-07-07T15:06:03Z

Ah yeah so I did consider doing that, but then told myself that they must be different for a reason, especially considering what @jdm said on the issue about why they existed at all:

The tests in the mozilla folder are modifications of the original tests from referrer-policy, because the originals rely on features that Servo doesn't support yet.

If I do as you say and make the two spec.src.json files essentially the same (except one only has iframe-tag), then the generated tests and expectations in the two folders won't be different at all -- one will just be a strict subset of the other. Is that really what we want? Maybe it is, but I'm kinda confused about it all.

rebstar6 · 2016-07-07T15:20:06Z

Unless something's changed, that is what you want. Basically, the tests in wpt/web-platform-tests are pulled directly from https://github.com/w3c/web-platform-tests. Other browsers use them, so we dont want to make servo-specific changes within those (just updating expectations for what servo doesnt yet support).

The issue is that the way that the w3c referrer policy tests implement iframe tags uses some other functionality that servo doesnt support. Because of that, the iframe tests within web-platform-tests that should pass based on the referrer policy implementation will fail (timeout I think). To get around that, the mozilla tests are the same test files, but the actual mechanism iframe uses is different (ie some of the stuff in the generic folder around how these tests are run)

aravind-pg · 2016-07-07T15:34:07Z

I see, interesting, that makes things a bit clearer -- it's how the tests are run (i.e. the generic folder) that's different.

But then there's a small issue: in the mozilla folder, I can't really get away with not updating parts of the generic folder too. This is because the spec.src.json in that folder has some outdated things (such as "http-csp" instead of "http-rp") which, if updated (say to "http-rp"), generate.py wouldn't understand. So I need to s/http-csp/http-rp in generate.py as well. But that's just a specific example and a specific piecemeal change. I think there must be other such updates to things in the generic folder, so that they keep pace with the updates to spec.src.json (EDIT: to be concrete, I see e.g. changes to generic/common.js, generic/referrer-policy-test-case.js, changes to things in generic/template/, a new generic/subresource-test/ folder, etc. -- not sure which of these are changes that we want and which we don't). How do I go about updating things without affecting the things that are actually supposed to be different?

rebstar6 · 2016-07-07T16:07:58Z

Good call - I think the only changes I made outside of spec.src.json are in this PR:
web-platform-tests/wpt#3118
There are actually minimal changes outside of that one file. Generally anything outside of implementing how iframe works is fine (so adding/changing delivery options or policies is pretty safe).

jdm · 2016-07-07T16:18:14Z

In an ideal world, I think we would have a script that would copy everything from the upstream tests then apply a set of patches on top of that.

jdm · 2016-07-07T16:44:19Z

The only files that should have been changed in the modified tests are document.html.template, common.js, and spec.src.json.

jdm · 2016-07-07T16:46:18Z

Oh, and the addition of stash.py.

aravind-pg · 2016-07-07T21:26:03Z

Alright then, I'll update only the files you mentioned as well as make the s/http-csp/http-rp change to generate.py (as in web-platform-tests/wpt#3118). I can't find any stash.py in referrer-policy/ though. This means I'm not updating the following:

referrer-policy-test-case.js
test.debug.html.template, test.release.html.template
generic/subresource/subresource.py
the entire new directory of generic/subresource-test/

Lemme know if this sounds good.

Also, I wonder if you could describe what that required set of patches would be? Would be good to know.

jdm · 2016-07-07T21:45:34Z

stash.py was an addition to the mozilla referrer-policy tests. It is not present in the originals. Via the following commands, the current required patches are (not including any changes to spec.src.json):

diff -U4 tests/wpt/web-platform-tests/referrer-policy/generic/template/document.html.template tests/wpt/mozilla/tests/mozilla/referrer-policy/generic/template/document.html.template
diff -U4 tests/wpt/web-platform-tests/referrer-policy/generic/common.js tests/wpt/mozilla/tests/mozilla/referrer-policy/generic/common.js
diff -NU4 tests/wpt/web-platform-tests/referrer-policy/generic/subresource/stash.py tests/wpt/mozilla/tests/mozilla/referrer-policy/generic/subresource/stash.py

--- tests/wpt/web-platform-tests/referrer-policy/generic/template/document.html.template    2016-05-12 10:46:08.000000000 -0400
+++ tests/wpt/mozilla/tests/mozilla/referrer-policy/generic/template/document.html.template 2016-05-27 11:47:23.000000000 -0400
@@ -4,13 +4,29 @@
     <title>This page reports back it's request details to the parent frame</title>
   </head>
   <body>
     <script>
+      function getQueryVariable(variable)
+      {
+             var query = window.location.search.substring(1);
+             var vars = query.split("&");
+             for (var i=0;i<vars.length;i++) {
+                     var pair = vars[i].split("=");
+                     if(pair[0] == variable){return pair[1];}
+             }
+             return(false);
+      }
+
+      var referrer = 'referrer' in document ? document.referrer : undefined;
+
       var result = {
         location: document.location.toString(),
-        referrer: document.referrer.length > 0 ? document.referrer : undefined,
+        referrer: referrer,
         headers: %(headers)s
       };
-      parent.postMessage(result, "*");
+      var xhr = new XMLHttpRequest();
+      xhr.open('POST', 'stash.py?id=' + getQueryVariable("id"), false);
+      xhr.send(JSON.stringify(result));
+
     </script>
   </body>
 </html>
--- tests/wpt/web-platform-tests/referrer-policy/generic/common.js  2016-05-24 10:09:46.000000000 -0400
+++ tests/wpt/mozilla/tests/mozilla/referrer-policy/generic/common.js   2016-05-27 11:47:23.000000000 -0400
@@ -99,17 +99,30 @@
   }
 }

 function queryIframe(url, callback, referrer_policy) {
-  var iframe = appendIframeToBody(url, referrer_policy);
-  var listener = function(event) {
-    if (event.source != iframe.contentWindow)
-      return;
-
-    callback(event.data, url);
-    window.removeEventListener("message", listener);
+  var x = document.createElement('script');
+  x.src = '/common/utils.js';
+  x.onerror = function() { console.log('whoops') };
+  x.onload = function() { doQuery() };
+  document.getElementsByTagName("head")[0].appendChild(x);
+
+  function doQuery() {
+    var id = token();
+    var iframe = appendIframeToBody(url + "&id=" + id, referrer_policy);
+    iframe.addEventListener("load", function listener() {
+      var xhr = new XMLHttpRequest();
+      xhr.open('GET', '/_mozilla/mozilla/referrer-policy/generic/subresource/stash.py?id=' + id, false);
+      xhr.onreadystatechange = function(e) {
+        if (this.readyState == 4 && this.status == 200) {
+          var server_data = JSON.parse(this.responseText);
+          callback(server_data, url);
+        }
+      };
+      xhr.send();
+      iframe.removeEventListener("load", listener);
+    });
   }
-  window.addEventListener("message", listener);
 }

 function queryImage(url, callback, referrer_policy) {
   decodeImage(url, function(server_data) {
--- tests/wpt/web-platform-tests/referrer-policy/generic/subresource/stash.py   1969-12-31 19:00:00.000000000 -0500
+++ tests/wpt/mozilla/tests/mozilla/referrer-policy/generic/subresource/stash.py    2016-05-27 11:47:23.000000000 -0400
@@ -0,0 +1,6 @@
+def main(request, response):
+    print request.GET['id']
+    if request.method == 'POST':
+        request.server.stash.put(request.GET["id"], request.body)
+        return ''
+    return request.server.stash.take(request.GET["id"])


        net: Add "origin" and "same-origin" referrer policies, replacing "ori…

…gin-only".


        test: Update tests/wpt/mozilla/.../referrer-policy/

Only has changes to spec.src.json, generate.py etc., no actual tests generated yet.


        test: Remove all but iframe tests from wpt/.../mozilla/referrer-polic…

…y/spec.src.json Again, essentially only updates spec.src.json without actually generating any tests.


        test: Update wpt/mozilla/.../referrer-policy generated tests and expe…

…ctations

aravind-pg · 2016-07-08T07:10:36Z

Alright whew, just push a bunch of new commits, this time separated more usefully. I note that in my comment above when I said "I'll only update the files you mentioned" really I should've said "I'll take care to preserve differences in the files you mentioned and update everything else".

In any case, I looked through all the diffs carefully to double-check and am pretty confident I've made the required updates while preserving all necessary differences. All that is in the second commit -- basically it updates just spec.src.json and generate.py (everything else was either unchanged or had changes that needed to be preserved). In the third commit I just update spec.src.json to remove all but iframe tests. And finally the fourth is the purely mechanical one of generating the new tests plus expectations. The last three should probably be squashed before merging.

@rebstar6 would be great if you could sanity-check the second and third commits too :)

jdm · 2016-07-12T19:56:47Z

I like leaving the commits separate like this. Thanks for doing this!
@bors-servo: r+

bors-servo · 2016-07-12T19:56:47Z

📌 Commit 79cc2f7 has been approved by jdm

bors-servo · 2016-07-12T20:44:34Z

⌛ Testing commit 79cc2f7 with merge 37dbb50...


        Auto merge of #11978 - aravind-pg:new-referrer-pols, r=jdm

Add "origin" and "same-origin" referrer policies, replacing "origin-only".  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11384 - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11978)

bors-servo · 2016-07-12T21:47:13Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

aravind-pg · 2016-07-12T21:59:18Z

Hooray 😄 Thanks for all the help, @jdm!

highfive assigned mbrubeck Jul 1, 2016

highfive added the S-awaiting-review label Jul 1, 2016

aravind-pg force-pushed the aravind-pg:new-referrer-pols branch from 6553813 to 41cdd3d Jul 5, 2016

highfive assigned jdm and unassigned mbrubeck Jul 5, 2016

highfive added the S-needs-rebase label Jul 5, 2016

aravind-pg force-pushed the aravind-pg:new-referrer-pols branch from f89e915 to 818f9b4 Jul 5, 2016

jdm added S-blocked-on-external and removed S-needs-rebase labels Jul 5, 2016

highfive added the S-needs-rebase label Jul 5, 2016

aravind-pg force-pushed the aravind-pg:new-referrer-pols branch 2 times, most recently from b3fd1b4 to aadaef0 Jul 7, 2016

jdm removed S-blocked-on-external S-needs-rebase labels Jul 7, 2016

aravind-pg added 4 commits Jul 1, 2016

net: Add "origin" and "same-origin" referrer policies, replacing "ori…

eeccb75

…gin-only".

test: Update tests/wpt/mozilla/.../referrer-policy/

fd50eb1

Only has changes to spec.src.json, generate.py etc., no actual tests generated yet.

test: Remove all but iframe tests from wpt/.../mozilla/referrer-polic…

a6808cb

…y/spec.src.json Again, essentially only updates spec.src.json without actually generating any tests.

test: Update wpt/mozilla/.../referrer-policy generated tests and expe…

Loading status checks…

79cc2f7

…ctations

aravind-pg force-pushed the aravind-pg:new-referrer-pols branch from aadaef0 to 79cc2f7 Jul 8, 2016

aravind-pg mentioned this pull request Jul 12, 2016

`referrer_url` argument to `modify_request_headers` is unused #12398

Closed

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 12, 2016

bors-servo merged commit 79cc2f7 into servo:master Jul 12, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 12, 2016

aravind-pg deleted the aravind-pg:new-referrer-pols branch Jul 13, 2016

servo / servo

Add "origin" and "same-origin" referrer policies, replacing "origin-only". #11978

Add "origin" and "same-origin" referrer policies, replacing "origin-only". #11978

aravind-pg commented Jul 1, 2016 •

edited by larsbergstrom

highfive commented Jul 1, 2016

highfive commented Jul 1, 2016

aravind-pg commented Jul 1, 2016

aravind-pg commented Jul 5, 2016

bors-servo commented Jul 5, 2016

jdm commented Jul 5, 2016

aravind-pg commented Jul 5, 2016

jdm commented Jul 5, 2016

bors-servo commented Jul 5, 2016

aravind-pg commented Jul 7, 2016

rebstar6 commented Jul 7, 2016

aravind-pg commented Jul 7, 2016

rebstar6 commented Jul 7, 2016

aravind-pg commented Jul 7, 2016 •

edited

rebstar6 commented Jul 7, 2016

jdm commented Jul 7, 2016

jdm commented Jul 7, 2016

jdm commented Jul 7, 2016

aravind-pg commented Jul 7, 2016

jdm commented Jul 7, 2016 •

edited

aravind-pg commented Jul 8, 2016 •

edited

jdm commented Jul 12, 2016

bors-servo commented Jul 12, 2016

bors-servo commented Jul 12, 2016

bors-servo commented Jul 12, 2016

aravind-pg commented Jul 12, 2016

servo / servo

Join GitHub today

Add "origin" and "same-origin" referrer policies, replacing "origin-only". #11978

Add "origin" and "same-origin" referrer policies, replacing "origin-only". #11978

Conversation

aravind-pg commented Jul 1, 2016 • edited by larsbergstrom

Adds the "origin" and "same-origin" referrer policy options, with the former replacing "origin-only".

highfive commented Jul 1, 2016

highfive commented Jul 1, 2016

aravind-pg commented Jul 1, 2016

aravind-pg commented Jul 5, 2016

bors-servo commented Jul 5, 2016

jdm commented Jul 5, 2016

aravind-pg commented Jul 5, 2016

jdm commented Jul 5, 2016

bors-servo commented Jul 5, 2016

aravind-pg commented Jul 7, 2016

rebstar6 commented Jul 7, 2016

aravind-pg commented Jul 7, 2016

rebstar6 commented Jul 7, 2016

aravind-pg commented Jul 7, 2016 • edited

rebstar6 commented Jul 7, 2016

jdm commented Jul 7, 2016

jdm commented Jul 7, 2016

jdm commented Jul 7, 2016

aravind-pg commented Jul 7, 2016

jdm commented Jul 7, 2016 • edited

aravind-pg commented Jul 8, 2016 • edited

jdm commented Jul 12, 2016

bors-servo commented Jul 12, 2016

bors-servo commented Jul 12, 2016

bors-servo commented Jul 12, 2016

aravind-pg commented Jul 12, 2016

aravind-pg commented Jul 1, 2016 •

edited by larsbergstrom

aravind-pg commented Jul 7, 2016 •

edited

jdm commented Jul 7, 2016 •

edited

aravind-pg commented Jul 8, 2016 •

edited