Closed Bug 1203359 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: marco, Assigned: ehsan)

References

Details

Attachments

(4 files)

A service worker should be able to fetch/cache resources coming from other origins, even if they don't have CORS headers.
They can if you use a "no-cors" RequestMode.  Can I see the code you are using?
Flags: needinfo?(mar.castelluccio)
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).
event.request.mode will be "no-cors" if the network request being intercepted supports no-cors.

What type of network request are you intercepting?
Attached file index.html
Flags: needinfo?(mar.castelluccio)
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
Flags: needinfo?(mar.castelluccio)
It works with dom.serviceWorkers.interception.opaque.enabled set to true. Is there a bug for enabling it by default?
Flags: needinfo?(mar.castelluccio)
We can use this bug for that.  We're not quite ready to do it yet, though.
Summary: Allow event.respondWith for off-origin resources, even with no CORS headers → Enable dom.serviceWorkers.interception.opaque.enabled by default once its safe to do so
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
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.
Attachment #8678504 - Flags: review?(bkelly)
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.
Attachment #8678504 - Flags: review?(bkelly)
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.
Flags: needinfo?(bkelly)
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?
Flags: needinfo?(bkelly)
(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. :/
Attached patch toggle the prefSplinter Review
I still think this is the wrong thing to do, but in the interest to move ahead, here is the alternative patch.
Attachment #8679083 - Flags: review?(bkelly)
Comment on attachment 8679083 [details] [diff] [review]
toggle the pref

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

Thanks.
Attachment #8679083 - Flags: review?(bkelly) → review+
https://hg.mozilla.org/mozilla-central/rev/9d49a2527b07
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.