1353685 - Should ServiceWorker call SetBlockedRequest

Status: NEW

(Core :: DOM: Security, enhancement, P3)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

In Bug 1267075 we have a patch to prevent calling SetBlockedRequest from ServiceWorker.
In this bug we could re-think how ServiceWorker should handle image blocking.

see https://bugzilla.mozilla.org/show_bug.cgi?id=1267075#c38 from bz

"Maybe what we should do is simply set a boolean member while calling into imagelib to indicate that we're in that call.  When that boolean is not set, ignore calls to SetBlockedRequest.  And when that boolean _is_ set, ignore or throw for calls to LoadImage, since it means some extension is being evil and reentering us.

Then file a followup for how we want to handle async stuff like service workers."

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I think this is a perf thing?

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #1)
> I think this is a perf thing?
not exactly, ServiceWorker will call CSP check in http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#254

Now in Bug 1267075 we will call SetBlockedRequest if CSP check failed, that means now SetBlockedRequest could be triggered by ServiceWorker, which might have problems if we didn't think it carefully.
For example I met some problem related to this in https://bugzilla.mozilla.org/show_bug.cgi?id=1267075#c10.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I guess I have no idea what SetBlockedRequest is or what it means.  Without that description and the impact on end user UI/script its hard for me to answer this.

Can you summarize what the impact is from bug 1267075?  What will be changing for service worker?

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

See comment 1 for the context.

In Bug 1267075 we've decided to call SetBlockedRequest if NS_CheckContentLoadPolicy failed.(Part 1 patch), however SetBlockedRequest is originally designed to be called synchrously from image loader code, now it could be called from anywhere calling Content Polciy check, for example from ServiceWorker.

And in Bug 1267075 Part 4 I added a work-around to prevent SetBlockedRequest is called by SW, so in this bug we need to think about how to make calling SetBlockedRequest correctly from ServiceWorker.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

So what happens from the users perspective if an image is blocked by NS_CheckContentLoadPolicy() via a service worker that is different from if its blocked in the image loader?  I get that we're not calling this method, but I don't understand what it does in terms of user observable behavior.

Comment 6

[Boris Zbarsky [:bzbarsky]]

SetBlockedRequest sets image state that is then used to implement the :-moz-suppressed, :-moz-broken, :-moz-user-disabled, etc pseudo-classes.

That is, it communicates to the image _why_ it was blocked.

We then render it differently based on the reason it was blocked (not rendering at all, rendering with a little placeholder icon, etc).

Comment 7

[Boris Zbarsky [:bzbarsky]]

So in a sane world, this sort of information would be stored in the error Response and then imagelib could surface it to the DOM image code or something.  Unfortunately, Necko has no concept of Response right now, and our content policy checks are a slightly unholy mix of sync and async checks, with the sync case definitely not having a useful concept of Response because we just fail out from AsyncOpen2...

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

If SetBlockedRequest or the information representing it was set on the nsIChannel, then we could propagate it through the InternalResponse properly.