Do not expose ServiceWorkerContainer to WebExtension principals. (Registering service worker from background javascript in Firefox Add-On fails)
Categories
(Core :: DOM: Service Workers, enhancement, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox103 | --- | fixed |
People
(Reporter: ultimaustin, Assigned: rpl)
References
Details
(Whiteboard: [addons-jira])
Attachments
(1 file)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0.1 Safari/605.1.15
Steps to reproduce:
The issue was originally recorded in stackoverflow here:
https://stackoverflow.com/questions/58690449/registering-service-worker-from-background-javascript-in-firefox-add-on-fails?noredirect=1#comment103697630_58690449
I have a browser extension that is working perfectly in chrome. when I ported it across to firefox, the following snippet of code, which runs in the extensions background thread, throws an exception.
if ('serviceWorker' in navigator) {
navigator.serviceWorker.register('/firebase-messaging-sw.js',{
updateViaCache: 'none'
})
.then(function(registration) {
console.log('Registration successful, scope is:', registration.scope);
}).catch(function(err) {
console.log('Service worker registration failed, error:', err);
});
}
I have tried every combination of change to the manifest.json that I can think of and the manifest currently looks like this:
"permissions": ["storage", "contextMenus", "tabs", "notifications", "://localhost/"],
"content_security_policy": "script-src 'self' '<MY-SHA256>' https://www.gstatic.com https://storage.googleapis.com; object-src 'self'",
"background": {
"page": "background.html",
"persistent": true
},
"content_scripts": [
{
"matches": ["<all_urls>"],
"js": ["/assets/js/browser-polyfill.min.js","/load.js"]
}],
"web_accessible_resources" : ["*.html",
"https://sitecontent.loopworks.com/scripts/fa-5-11-2/js/all.js",
"https://www.gstatic.com/firebasejs/6.5.0/firebase.js",
"https://www.gstatic.com/firebasejs/6.5.0/firebase-messaging.js"
]
This is the content of the backgound page:
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />
<script src="/assets/js/firebase.js"></script>
<script src="/assets/js/firebase-messaging.js"></script>
<script src="/assets/js/browser-polyfill.min.js"></script>
<script src="/background.js"></script>
</head>
<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
</body>
</html>
Actual results:
The error is as follows: DOMException: "The operation is insecure."
I am experiencing this issue in Firefox 70.0.1 and so I believe that these existing answers around cookie expiration settings out of date (??): https://stackoverflow.com/questions/49539306/firefox-service-worker-securityerror-domexception-the-operation-is-insecure
Expected results:
I expect that the service worker registration to succeed. I can't see why this is causing a security exception. If Service workers are not supported from extensions then I don't think that the initial if condition...
if ('serviceWorker' in navigator)
... should evaluate to true
|
||
Comment 1•5 years ago
|
||
Bugbug thinks this bug should belong to this component, but please revert this change in case of error.
|
||
Comment 2•5 years ago
|
||
(In reply to ultimaustin from comment #0)
I expect that the service worker registration to succeed. I can't see why this is causing a security exception. If Service workers are not supported from extensions then I don't think that the initial if condition...
if ('serviceWorker' in navigator)
... should evaluate to true
This is a good point. ServiceWorkers are not supported in WebExtensions at this time, although the WebExtensions team is moving towards that. We should not be exposing the ServiceWorkerContainer to webextension principals, but the webextension principals automatically pass storage checks.
We should add special-casing logic in https://searchfox.org/mozilla-central/rev/8b7aa8af652f87d39349067a5bc9c0256bf6dedc/dom/serviceworkers/ServiceWorkerContainer.cpp#86
|
||
Updated•2 years ago
|
|
|
||
Comment 3•2 years ago
|
||
Redirect a needinfo that is pending on an inactive user to the triage owner.
:jstutte, since the bug has high priority and recent activity, could you have a look please?
For more information, please visit auto_nag documentation.
|
Assignee | |
Comment 5•2 years ago
|
||
I'm happy to take care of this, by special casing the logic pointed by Andrew in comment 2 (by also checking if StaticPrefs::extensions_serviceWorkerRegister_allowed() when the subject principal is an extension principal, similarly to the check that prevents ServiceWorkerContainer::Register from completing successfully if the extensions.serviceWorkerRegister.allowed is set to false, which is currently the default on all channels).
We may also consider this a bug that fits into the WebExtensions bugzilla component, but I'm not moving it just yet, in case it would be preferred to keep it filed under the "DOM: Service Workers" bguzilla components from a "DOM Workers and Storage" team perspective.
|
||
Comment 6•2 years ago
•
|
||
(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #5)
I'm happy to take care of this
Thanks!
We may also consider this a bug that fits into the
WebExtensionsbugzilla component, but I'm not moving it just yet, in case it would be preferred to keep it filed under the "DOM: Service Workers" bugzilla components from a "DOM Workers and Storage" team perspective.
I think it's fine to move it, if that helps to keep it on your radar. Still :asuth should probably help with reviews here, of course.
|
|
Assignee | |
Comment 7•2 years ago
|
||
|
||
Updated•2 years ago
|
|
|
Assignee | |
Comment 8•2 years ago
|
||
(In reply to Jens Stutte [:jstutte] from comment #6)
(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #5)
I'm happy to take care of this
Thanks!
We may also consider this a bug that fits into the
WebExtensionsbugzilla component, but I'm not moving it just yet, in case it would be preferred to keep it filed under the "DOM: Service Workers" bugzilla components from a "DOM Workers and Storage" team perspective.I think it's fine to move it, if that helps to keep it on your radar. Still :asuth should probably help with reviews here, of course.
+1 absolutely!
Independently if filed in the WebExtensions bugzilla components or not, I would absolutely never land a patch with changes applied to "DOM workers"-related internals without an explicit review and sign off fro a "DOM: Workers and Storage" module peer (same with other module peers if the patch also need changes elsewhere in the tree).
I've just attached a patch with a small set of changes to ServiceWorkerContainer.cpp (basically what described in comment 5) and a trivial tweak to the pre-existing test case (added as part of Bug 1609920, along with the previous changes to ServiceWorkerContainer::Register which provided the current behavior of throwing NS_ERROR_DOM_SECURITY_ERROR when service worker registration is disabled per "extensions.serviceWorkerRegister.allowed" pref).
|
|
Assignee | |
Updated•2 years ago
|
|
||
Updated•2 years ago
|
|
|
||
Comment 9•2 years ago
|
||
(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #8)
Independently if filed in the WebExtensions bugzilla components or not, I would absolutely never land a patch with changes applied to "DOM workers"-related internals without an explicit review and sign off fro a "DOM: Workers and Storage" module peer (same with other module peers if the patch also need changes elsewhere in the tree).
I had no doubt about your role here, it was more to say "Andrew should and will find time for this" ;-)
|
|
Assignee | |
Comment 10•2 years ago
|
||
|
||
Comment 11•2 years ago
|
||
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/3a2907ad88e8 Only show ServiceWorkerContainer to WebExtension principals if extensions.serviceWorkerRegister.allowed is true. r=asuth
|
||
Comment 12•2 years ago
|
||
| bugherder | ||
Description
•