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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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 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?
Attachment #8852763 - Flags: review?(bugs)
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.
Attachment #8852764 - Flags: review?(bugs)
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?
Attachment #8852763 - Flags: review?(bugs) → review-
Attachment #8852764 - Flags: review?(bugs) → review+
(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.
Flags: needinfo?(bugs)
(too late right now here, but perhaps Chris is online)
Where is the load failing?  DOM APIs like location.href does set the triggeringPrincipal as expected
Location::SetURI
Flags: needinfo?(bugs)
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().
Or maybe not.  I can do `location.replace(crossOriginURL)` in devtools and it works.
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.  :(
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.
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.
Attachment #8852763 - Attachment is obsolete: true
Attachment #8856627 - Flags: review?(bugs)
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
Attachment #8852764 - Attachment is obsolete: true
Attachment #8856629 - Flags: review?(bugs)
Isn't TypeError a bit surprising for this case.
/me reads the spec.
Ah, url parsing uses TypeError too. Still a bit surprising, but at least consistent.
TypeError is spec'd in step 6.9 here:

https://w3c.github.io/ServiceWorker/#client-navigate
Attachment #8856627 - Flags: review?(bugs) → review+
Attachment #8856629 - Flags: review?(bugs) → review+

Comment 18

2 years ago
Pushed by bkelly@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2606285d497d
https://hg.mozilla.org/mozilla-central/rev/3f5856b402d6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.