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

How to test "no-referrer-when-downgrade" in a web platform test #1094

Closed
mfalken opened this issue Mar 27, 2017 · 18 comments
Closed

How to test "no-referrer-when-downgrade" in a web platform test #1094

mfalken opened this issue Mar 27, 2017 · 18 comments

Comments

@mfalken
Copy link
Member

mfalken commented Mar 27, 2017

fetch-event.https.html has a test for the referrer policy, but Chrome won't pass it because it blocks mixed content fetches by default.

The test performs a fetch to an http URL from an https iframe. It expects the service worker to receive a fetch event whose request has a referrerPolicy of "no-referrer-when-downgrade".

However in Chrome, the fetch() will reject, with an accompanying Mixed Content warning in the console.

We could get around this by adding Chrome-specific configuration to the web platform test:

if (window.testRunner)
  testRunner.overridePreference('WebKitAllowRunningInsecureContent', true);

But this seems unfortunate. Questions:

  • Does the Mixed Content spec allow Chrome to do this?
  • If so, is there a better way to test behavior in the web platform test without adding Chrome-specific code?

cc/ @yutakahirano

@mfalken
Copy link
Member Author

mfalken commented Mar 27, 2017

@jakearchibald @mikewest

@mikewest
Copy link
Member

We have a number of tests in WPT for referrer policy that verify various policies by loading images with wide-open CORS headers that encode the referrer value in their pixels. We read the pixels via <canvas> and decode the value. But that relies on lax handling of mixed image content. That's going to be difficult to do from inside a service worker.

That said, I'm not sure I understand what you're trying to test. Since we expect fetch() to a non-secure endpoint to be blocked, shouldn't the test make that assertion instead?

@wanderview
Copy link
Member

I think maybe we added this test? We use a pref to disable the mixed content blocking in order to be able to test the referrer policy here:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/service-workers/service-worker/fetch-event.https.html.ini#3

Note, however, the pref is stored outside of the test file itself. Do you have a similar mechanism?

@wanderview
Copy link
Member

Looks like @ehsan added these tests cases in:

https://bugzilla.mozilla.org/show_bug.cgi?id=1251872

@mfalken
Copy link
Member Author

mfalken commented Mar 27, 2017

I see, I assumed Firefox was testing its default behavior.

I think we might have a similar mechanism. But regardless, I tend to agree with @mikewest, if the browsers' default behavior is to block the fetch(), and that is what the spec is recommending, it seems we should test that instead. And then a smaller, separate test can be added for what happens when mixed content is allowed. WDYT?

@wanderview
Copy link
Member

On my mobile now, so can't easily check the code, but I thought this was already broken out as a separate test case.

Also, aren't there other tests for mixed content already in WPT.

In the end I don't have a strong preference here. We could also move this to a Mozilla specific test in our tree.

@mfalken
Copy link
Member Author

mfalken commented Mar 27, 2017

Thanks. You're right, I'd be surprised if fetch() to mixed content is not in an existing WPT test.

As far as I can tell, this (fetch-event.https.html) is a massive test file so it'll make sense to break it up anyway.

@mfalken
Copy link
Member Author

mfalken commented Mar 27, 2017

I've also learned we indeed have a external configuration mechanism via testharnessreporter.js:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources/testharnessreport.js

@mikewest
Copy link
Member

Also, aren't there other tests for mixed content already in WPT.

Yes. https://github.com/w3c/web-platform-tests/tree/master/mixed-content. Also Referrer Policy: https://github.com/w3c/web-platform-tests/tree/master/referrer-policy/ (though I'm pretty sure neither of those have substantial coverage of requests made from within service workers).

I've also learned we indeed have a external configuration mechanism via testharnessreporter.js:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources/testharnessreport.js

The default implementation of that file in WPT is empty. We don't use it to configure non-default settings, nor to expose APIs that tests can/should rely upon. It's certainly worth talking about adding such a thing (and I think Web Bluetooth was interested in doing so for testing APIs?), but that's a larger question.

@annevk
Copy link
Member

annevk commented Mar 29, 2017

Can't we test no-referrer-when-downgrade through <img> and use the stash feature of wptserve? I agree that we shouldn't test it through fetch() (other than testing that it rejects).

@jakearchibald
Copy link
Contributor

I build a little test-case https://cdn.rawgit.com/jakearchibald/0e3e6ae11fa354ca6ba1183efebc002d/raw/bf7a76470ba48b33d95c5a862997d7ab99769774/

Currently both Firefox and Chrome fail to fetch in the event.respondWith(fetch(event.request)) case, so neither implement https://w3c.github.io/webappsec-mixed-content/#is-passthrough.

Our intention was to have event.respondWith(fetch(event.request)) behave as much as possible like no-serviceworker, and to allow the caching of mixed content resources to make it possible to build a podcast player.

I'd like to hear from Apple & MS. If there's no interest in this passthrough stuff, we could give up & remove it.

@wanderview
Copy link
Member

Currently both Firefox and Chrome fail to fetch in the event.respondWith(fetch(event.request)) case, so neither implement https://w3c.github.io/webappsec-mixed-content/#is-passthrough.

We want to fix it in firefox. I have some work-in-progress that will move us closer to being able to fix it.

@annevk
Copy link
Member

annevk commented Mar 30, 2017

If we got away with not adding it, shall we just not fix it? It's much better for security if you can't downgrade at all.

@jakearchibald
Copy link
Contributor

F2F "green lock good, that's what we've been training people" - even if we support this, users are going to get scary MIX warning. We should drop this capability from the SW spec and MIX. We can bring it back if people really hate it, but we haven't heard much so far.

Apologies to @mikewest who made this work in MIX.

@jakearchibald
Copy link
Contributor

We should check that "upgrade insecure requests" happens before service worker.

@mikewest
Copy link
Member

mikewest commented Apr 4, 2017

Easy, I'm happy to just drop it if it turns out that we don't need it. The lack of implementation is a good indication in that direction. We even marked it as "at-risk" in the CR, so there's no process impact.

/cc @hillbrad @wseltzer @dveditz

mikewest added a commit to w3c/webappsec-mixed-content that referenced this issue Apr 4, 2017
No one implemented it, and the Service Worker folks decided in their most
recent F2F that they didn't need it anymore[1].

[1]: w3c/ServiceWorker#1094
@jakearchibald
Copy link
Contributor

@mattto I believe the OP is redundant now, but please reopen if I'm wrong.

@annevk
Copy link
Member

annevk commented Apr 5, 2017

Pretty happy with this. @briansmith probably appreciates this too to the extent he's still following along. Thanks for filing the test issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants