move service worker secure context checks to a webidl [Func] annotation

RESOLVED FIXED in Firefox 63

Status

()

P2
normal
RESOLVED FIXED
8 months ago
3 months ago

People

(Reporter: bkelly, Assigned: ytausky)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(6 attachments)

Currently we do some manual checking for secure context in ServiceWorkerContainer::Register().  We should really move this to a webidl [Func] annotation so that the ServiceWorkerContainer is hidden instead of rejecting promises.

Note, we will need this before we can do bug 1131324 because the current code is very window-specific.

Updated

8 months ago
Priority: -- → P3
(Assignee)

Updated

4 months ago
Assignee: nobody → ytausky

Updated

4 months ago
Priority: P3 → P2
(Assignee)

Comment 1

4 months ago
Created attachment 8998628 [details]
Bug 1455078: Implement [SecureContext] for ServiceWorkerContainer

The spec mandates that ServiceWorkerContainer only be visible in a secure context,
yet currently it is (almost) always visible, but rejects calls to register() in
non-secure contexts. This commit moves the context check to a [Func] function, thus
implementing the behavior exactly as specified.

This commit uses the same mechanism used by [SecureContext] bindings instead of the
current ad hoc implementation.
(Assignee)

Updated

4 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 months ago
Created attachment 8999902 [details]
Bug 1455078: Refactor test to run in code in page

This test runs code in the frame script, which has system privileges. Even
though it trips the security tests as it is, it's not clear whether that
should be the case or not. By running the code in the page itself, the
expected behavior is governed by the service workers spec and is thus well
defined.
Comment on attachment 8999902 [details]
Bug 1455078: Refactor test to run in code in page

Andrew Sutherland [:asuth] has approved the revision.
Attachment #8999902 - Flags: review+
(Assignee)

Comment 4

4 months ago
Created attachment 9001552 [details]
Bug 1455078: Test exposure of navigator.serviceWorker to data: iframes

Documents whose creation URL is a data: URL are not considered secure contexts,
therefore they should not have access to navigator.serviceWorker. Another WPT test,
local-url-inherit-controller.https.html, currently assumes the this interface is
available and uses it to test that data: frames do not inherit the parent's
controller; this is wrong, but before we can remove that test, we'd better test
explicitly that such frames cannot access the service worker interface and
therefore cannot have a controller.
Comment on attachment 8999902 [details]
Bug 1455078: Refactor test to run in code in page

Alexandre Poirot [:ochameau] has approved the revision.
Attachment #8999902 - Flags: review+
(Assignee)

Updated

4 months ago
Depends on: 1483505

Comment 6

4 months ago
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/autoland/rev/3c2f920f1a2d
Refactor test to run in code in page r=ochameau,asuth

Comment 7

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3c2f920f1a2d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Assignee)

Comment 8

4 months ago
Sorry for the confusion, but that was only one of several patches required to resolve this bug. Is there a way to mark a patch as partial?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

4 months ago
Status: REOPENED → ASSIGNED
(Assignee)

Updated

4 months ago
Keywords: leave-open
(Assignee)

Comment 9

4 months ago
Created attachment 9001911 [details]
Bug 1455078: Enable service worker testing in test

This preference must be enabled since the test creates an insecure context, in
which navigator.serviceWorker is undefined.
(Assignee)

Comment 10

4 months ago
Created attachment 9001912 [details]
Bug 1455078: Remove non-conformant ServiceWorker test case

This test case would fail on a conforming implementation of the spec, since
navigator.serviceWorker is undefined in frames originating from data: URLs,
thus navigator.serviceWorker.controller doesn't exist.
(Assignee)

Comment 11

4 months ago
Created attachment 9001914 [details]
Bug 1455078: Make ServiceWorker test check interface availability with lowered privileges

The test as it is currently written checks whether navigator.serviceWorker
exists with system privileges. By using eval(), this commit makes it
perform the check with the content's privileges.
Comment on attachment 8998628 [details]
Bug 1455078: Implement [SecureContext] for ServiceWorkerContainer

Andrew Sutherland [:asuth] has approved the revision.
Attachment #8998628 - Flags: review+
Comment on attachment 9001914 [details]
Bug 1455078: Make ServiceWorker test check interface availability with lowered privileges

Andrew Sutherland [:asuth] has approved the revision.
Attachment #9001914 - Flags: review+
Comment on attachment 9001911 [details]
Bug 1455078: Enable service worker testing in test

Andrew Sutherland [:asuth] has approved the revision.
Attachment #9001911 - Flags: review+
Comment on attachment 9001912 [details]
Bug 1455078: Remove non-conformant ServiceWorker test case

Andrew Sutherland [:asuth] has approved the revision.
Attachment #9001912 - Flags: review+
Comment on attachment 9001552 [details]
Bug 1455078: Test exposure of navigator.serviceWorker to data: iframes

Andrew Sutherland [:asuth] has approved the revision.
Attachment #9001552 - Flags: review+

Comment 17

4 months ago
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/autoland/rev/5e8edebf7500
Test exposure of navigator.serviceWorker to data: iframes r=asuth

Comment 18

4 months ago
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/autoland/rev/6fc9afb46004
Remove non-conformant ServiceWorker test case r=asuth

Comment 19

4 months ago
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/autoland/rev/3f29d9aef185
Enable service worker testing in test r=asuth

Comment 20

4 months ago
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/autoland/rev/9e44b02e94ca
Make ServiceWorker test check interface availability with lowered privileges r=asuth

Comment 21

4 months ago
Pushed by bugmail@asutherland.org:
https://hg.mozilla.org/integration/autoland/rev/7472e52f6d12
Implement [SecureContext] for ServiceWorkerContainer r=asuth
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12623 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged
(Assignee)

Comment 26

4 months ago
All the patches landed, this bug can be closed now.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago4 months ago
Resolution: --- → FIXED
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.