1203359 - Enable dom.serviceWorkers.interception.opaque.enabled by default once its safe to do so

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Marco Castelluccio [:marco]]

A service worker should be able to fetch/cache resources coming from other origins, even if they don't have CORS headers.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

They can if you use a "no-cors" RequestMode.  Can I see the code you are using?

Comment 2

[Marco Castelluccio [:marco]]

I'm not using the no-cors option:
self.addEventListener("fetch", function(event) {
  event.respondWith(fetch(event.request));
});

I'll try using the option shortly.
We should print some warning to avoid headaches.
Several tools to automatically generate/handle service workers are not taking this into consideration, but I think it could be a pretty common issue (for example when loading images from other origins).

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

event.request.mode will be "no-cors" if the network request being intercepted supports no-cors.

What type of network request are you intercepting?

Comment 4

[Marco Castelluccio [:marco]]

Attachment: index.html

Comment 5

[Marco Castelluccio [:marco]]

Attachment: service-worker.js

This prints:
"http://localhost:8000/ - same-origin"
"https://walter.trakt.us/images/shows/000/094/630/posters/thumb/6ac53bd38a.jpg - no-cors"

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Oh, I forgot we have a preference blocking opaque responses.  Can you try setting this to true in about:config?

  dom.serviceWorkers.interception.opaque.enabled

Comment 7

[Marco Castelluccio [:marco]]

It works with dom.serviceWorkers.interception.opaque.enabled set to true. Is there a bug for enabling it by default?

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

We can use this bug for that.  We're not quite ready to do it yet, though.

Comment 9

[(no longer active)]

Attachment: Enable interception of opaque responses through service workers

I'm removing the pref completely since we can't ship an implementation without this, and if we find more issues from now on, we would probably want to disable interception completely while we fix it, so keeping this pref is not very valuable.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

I personally think we should keep the pref for now.  Opaque interception is the most likely part to reveal sec issues in the future.  We should keep a way tithout turning SW completely off.  Once SW rides to betabwe canremove the pref on trunk.

Comment 11

[(no longer active)]

We have dom.serviceWorkers.interception.enabled for that, right?

My point was that I don't think there is any case where we would want dom.serviceWorkers.interception.enabled to be true but dom.serviceWorkers.interception.opaque.enabled to be false.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

If we found a problem in aurora 44, why wouldn't we want to go back to our current state of opaque disabled while the rest of interception is enabled?

The security team recently lamented to me that we have removed these kinds of prefs too soon in the past.  What is the harm in waiting to remove the pref until we decide interception is ready to ride to beta?

Comment 13

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #12)
> If we found a problem in aurora 44, why wouldn't we want to go back to our
> current state of opaque disabled while the rest of interception is enabled?

Because that will mean most real apps using SW will be broken if we did that.  Basically _I_ think that after we move to Aurora, any patch that would turn off dom.serviceWorkers.interception.opaque.enabled while leaving interception on should be r-'ed.

> The security team recently lamented to me that we have removed these kinds
> of prefs too soon in the past.  What is the harm in waiting to remove the
> pref until we decide interception is ready to ride to beta?

On the broad topic of prefs that disable parts of a Web facing feature in the name of security, we should never use them in practice.  Not sure what they were complaining about more specifically.

Back to this issue, one measurable harm in not taking this patch is having one string that our localizers need to worry about but our users will never see in the real world.

But no point in arguing over this...  I'm more interested in getting this patch off my queue for now. :/

Comment 14

[(no longer active)]

Attachment: toggle the pref

I still think this is the wrong thing to do, but in the interest to move ahead, here is the alternative patch.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8679083 [details] [diff] [review]
toggle the pref

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

Thanks.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d49a2527b07

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d49a2527b07