1351935 - Client.navigate() should resolve null and not reject for cross-origin URLs

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently client.navigate() is written to reject on cross-origin URLs.  This is checked in the WPT test added in bug 1218148:

http://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/client-navigate-worker.js#46

This is not correct, though.  The client.navigate() steps say that cross-origin URLs should resolve with null, but it does not say it should reject:

https://w3c.github.io/ServiceWorker/#client-navigate

In addition, chrome expects cross-origin client.navigate() to resolve null:

https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/serviceworker/windowclient-navigate.html?l=51

We should fix our impl and our test.

I ran into this while re-implementating Client.navigate() in bug 1293277.  I'd like to fix this first before landing that mega patch, though.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make Client.navigate() support cross-origin loads. r=smaug

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Fix client.navigate() WPT test to support cross-origin navigations. r=smaug

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8155e9801a432961d7faf473fd6995f42e244319

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8852763 [details] [diff] [review]
P1 Make Client.navigate() support cross-origin loads. r=smaug

According to the spec the Client.navigate() method should permit cross-origin URLs.  We just have to be careful to resolve null and not a WindowClient in these cases.

https://w3c.github.io/ServiceWorker/#client-navigate

Our Client.navigate() implementation, however, rejects on cross-origin URLs.  This patch attempts to fix it.

First, I removed the CheckMayLoad() which was the explicit same-origin check here.

I found, however, that I also had to change the LoadInfo passed to nsDocShell::LoadURI() in a couple ways:

1) I had to remove the triggering principal.
2) I had to remove the referrer.

Without these changes nsDocShell::LoadURI would return a failure.

The referrer change seems correct as discussed in this spec issue:

https://github.com/w3c/ServiceWorker/issues/1086

I am less sure of the triggering principal change.  Should I instead be using a different set of flags or something with nsDocShell?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8852764 [details] [diff] [review]
P2 Fix client.navigate() WPT test to support cross-origin navigations. r=smaug

Fix our WPT test to expect cross-origin client.navigate() to resolve null instead of reject.  This fails on current m-c and passes when P1 is applied.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8852763 [details] [diff] [review]
P1 Make Client.navigate() support cross-origin loads. r=smaug

This doesn't look right. Why null triggering principal?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] (review queue closed. Please ask reviews from other DOM peers) from comment #5)
> This doesn't look right. Why null triggering principal?

Because its the only way I could get nsDocShell::LoadURI() to work with a cross-origin URI here.

I'm looking for help, though.  What is the correct way to use nsDocShell::LoadURI() to potentially navigate a docshell to a cross-origin URL?  Should I set triggering principal and some other flag?

Any help is appreciated.  Thanks.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(too late right now here, but perhaps Chris is online)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Where is the load failing?  DOM APIs like location.href does set the triggeringPrincipal as expected
Location::SetURI

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Thanks!

Looking at it I'm guessing its because I'm trying to use "replace" logic.  That doesn't make a lot of sense cross-origin.  I also don't think its right for Client.navigate().

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Or maybe not.  I can do `location.replace(crossOriginURL)` in devtools and it works.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So just a few notes:

1)  You absolutely must set trigggeringPrincipal.  It's used for security checks that MUST be done properly.

2)  You absolutely must not be calling any Location methods from random C++ without script on the stack.  That means the Navigate call from a runnable's Run must NOT touch Location in any way.

3)  The triggeringPrincipal and whatnot sort of correspond to the spec's concept of "source browsing context", except that concept is known to be broken; a longstanding HTML spec issue.  :(

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Thanks Boris.  I think the bit I was misunderstanding was that using an http:// URL in this test would result in a mixed-content iframe violation.  I'll expand the test to explicitly check for this case in addition to the cross-origin secure URL case.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make Client.navigate() support cross-origin loads. r=smaug

As per the previous patch this:

1. Remove the explicit referer.
2. Removes the explicit same-origin check.

It also does this now:

3. Includes the triggering principal.
4. Changes loadStopContentAndReplace to just loadStopContent.  We shouldn't be hiding the navigation from session history.
5. Ensures that errors throw TypeError as required in the spec.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Fix client.navigate() WPT test to support cross-origin navigations. r=smaug

Updated the test from the last r+.  New additions:

1. Includes a test case for navigation to an insecure URL that triggers the mixed-content block.
2. Update some assert_throws() statements to verify cross-origin access throws SecurityError.
3. Remove iframes when tests complete to make the test cases work more consistently.  Keeping iframes alive can prevent the next service worker install.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65841742b967b5ce2b1fc0e0cbedd7f36dc8d59c

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Isn't TypeError a bit surprising for this case.
/me reads the spec.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Ah, url parsing uses TypeError too. Still a bit surprising, but at least consistent.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

TypeError is spec'd in step 6.9 here:

https://w3c.github.io/ServiceWorker/#client-navigate

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2606285d497d
P1 Make Client.navigate() support cross-origin loads. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5856b402d6
P2 Fix client.navigate() WPT test to support cross-origin navigations. r=smaug

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2606285d497d
https://hg.mozilla.org/mozilla-central/rev/3f5856b402d6