1223378 - Improve service worker register() origin checks

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1221365 it was pointed out we should be checking the document's principal for trustworthiness, not the document URI.  This matches the spec which talks about verifying the origin, not the URI itself.

In addition, now that the potential trustworthy origin check permits things like file:// and wss:// URIs, we should explicitly check for http:// and https://.

Comment 1

[Jonathan Watt [:jwatt]]

Doesn't bug 1162772 and bug 1220687 cover this?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonathan Watt [:jwatt] from comment #1)
> Doesn't bug 1162772 and bug 1220687 cover this?

Fixing those bugs will probably do more than I am planning to do here.  Its not clear to me what the final outcome of those bugs will be, though.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Tighten service worker register() principal checks. r=baku

This tightens some of the checks in service worker register() to use principals instead of the document URI.

Also, it checks the scheme explicitly as described in this spec issue:

https://github.com/slightlyoff/ServiceWorker/issues/780

Try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6afcdf62bf97

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8685462 [details] [diff] [review]
Tighten service worker register() principal checks. r=baku

Review of attachment 8685462 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +1503,5 @@
> +  // is a good block against accidentally allowing blob: script URIs which
> +  // might inherit the origin.
> +  bool isHttp = false;
> +  bool isHttps = false;
> +  aScriptURI->SchemeIs("http", &isHttp);

should you check the return value?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini (:baku) from comment #4)
> ::: dom/workers/ServiceWorkerManager.cpp
> > +  bool isHttp = false;
> > +  bool isHttps = false;
> > +  aScriptURI->SchemeIs("http", &isHttp);
> 
> should you check the return value?

If SchemeIs() returns failure, then isHttp will still be false.  So I just check the boolean value as the end result is the same.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Oh, I don't need the nsContentUtils::IsSystemPrincipal() conditional since the previous code already checks IsChromeDoc().  I'll turn that into an assert.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=324502e6833d

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1223378_interdiff_001.patch

The app: protocol rains on my parade again.  We need to allow it on non-release builds.

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

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89c1060df73f

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89c1060df73f

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8685462 [details] [diff] [review]
Tighten service worker register() principal checks. r=baku

Approval Request Comment
[Feature/regressing bug #]: service workers
[User impact if declined]: We are trying to ship service workers in 44 if possible. Making sure register() has the right restrictions is important to that release.
[Describe test coverage new/current, TreeHerder]: Existing mochitests/wpt tests.
[Risks and why]: Minimal.  Only affects service workers.
[String/UUID change made/needed]: None.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8685462 [details] [diff] [review]
Tighten service worker register() principal checks. r=baku

Service Workers is planned for FF44, the sooner these changes make it to Aurora44, the sooner we can get end-user feedback and potential issues.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/46d8b9f21820

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/46d8b9f21820