Closed Bug 1223378 Opened 4 years ago Closed 4 years ago

Improve service worker register() origin checks

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- wontfix
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(2 files)

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://.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Doesn't bug 1162772 and bug 1220687 cover this?
(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.
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
Attachment #8685462 - Flags: review?(amarchesini)
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?
Attachment #8685462 - Flags: review?(amarchesini) → review+
(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.
Oh, I don't need the nsContentUtils::IsSystemPrincipal() conditional since the previous code already checks IsChromeDoc().  I'll turn that into an assert.
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
Attachment #8685494 - Flags: review?(amarchesini)
Attachment #8685494 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/89c1060df73f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.
Attachment #8685462 - Flags: approval-mozilla-aurora?
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.
Attachment #8685462 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.