Bug 89440 – Blocking a resource via Content Security Policy should trigger an Error event.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

http/tests/security/contentSecurityPolicy/register-bypassing-scheme.html should be generating two additional "PASS" messages, but the `onerror` handler isn't being triggered.

See https://bugs.webkit.org/show_bug.cgi?id=89373 for context.

[Adam Barth]

In theory, we should treat CSP blocks just like empty 400 responses.  When we block things for CORS, I think we correctly generate error events nowadays, so we might want to model what we do for CSP off of that.

[Mike West]

(In reply to comment #1)
> In theory, we should treat CSP blocks just like empty 400 responses.  When we block things for CORS, I think we correctly generate error events nowadays, so we might want to model what we do for CSP off of that.

Thanks for tracking down the CORS patch (https://bugs.webkit.org/show_bug.cgi?id=81998). Looks like it should be fairly straightforward to hook into the same sort of logic for CSP violations.

If no one else picks this up, I'll take a look at it sometime after I/O. :)

[Mike West]

Created attachment 148286 [details]
Patch

[Mike West]

(In reply to comment #2)
> If no one else picks this up, I'll take a look at it sometime after I/O. :)

Eh. As it turns out, I'm far too busy procrastinating to work on I/O right now. WebKit it is!

The attached patch works for the limited test cases I've put together, but I need to dig through CachedResourceLoader a bit more to determine where else I need to test. I don't think CSP is the only thing that can return a null image in ImageLoader::updateFromElement().

[Adam Barth]

This looks right to me, but you might want to check with japhet before landing.

[Adam Barth]

(By the way, we should do this for all the kinds of loads, not just images.)

[Mike West]

(In reply to comment #6)
> (By the way, we should do this for all the kinds of loads, not just images.)

We should indeed. I started with images since you pointed me exactly at the right file, but you're entirely correct: errors should be triggered for all sorts of resources.

I assumed that multiple patches would be preferred, but I'm happy to bundle up all the resource types I can track down in a single patch if you like. Whatever suits you. There's no real rush to get this in, since it's been this way for quite some time. :)

[WebKit Review Bot]

Comment on attachment 148286 [details]
Patch

Attachment 148286 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12970970

New failing tests:
platform/chromium/permissionclient/image-permissions.html

[WebKit Review Bot]

Created attachment 148291 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

Multiple patches would be great.

[Mike West]

(In reply to comment #8)
> (From update of attachment 148286 [details])
> Attachment 148286 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12970970
> 
> New failing tests:
> platform/chromium/permissionclient/image-permissions.html

Adam, Jochen (adding you because you worked on this test): Is the test's behavior correct? Should the `onerror` event not fire? I'm not at all familiar with the permissionclient work, so I'm not sure if I need to adjust the test expectations or adjust the behavior.

(In reply to comment #11)
> (In reply to comment #8)
> > (From update of attachment 148286 [details] [details])
> > Attachment 148286 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/12970970
> > 
> > New failing tests:
> > platform/chromium/permissionclient/image-permissions.html
> 
> Adam, Jochen (adding you because you worked on this test): Is the test's behavior correct? Should the `onerror` event not fire? I'm not at all familiar with the permissionclient work, so I'm not sure if I need to adjust the test expectations or adjust the behavior.

Yes, if images are disabled by means of the permission client, no error event should be fired

[Mike West]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #8)
> > > (From update of attachment 148286 [details] [details] [details])
> > > Attachment 148286 [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > Output: http://queues.webkit.org/results/12970970
> > > 
> > > New failing tests:
> > > platform/chromium/permissionclient/image-permissions.html
> > 
> > Adam, Jochen (adding you because you worked on this test): Is the test's behavior correct? Should the `onerror` event not fire? I'm not at all familiar with the permissionclient work, so I'm not sure if I need to adjust the test expectations or adjust the behavior.
> 
> Yes, if images are disabled by means of the permission client, no error event should be fired

Ok, thanks! Am I correct in thinking that http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/loader/cache/CachedResourceLoader.cpp&exact_package=chromium&q=allowImage%20file:third_party/webkit&type=cs&l=368 is the permission client check?

If so, do you have any good ideas about how to decide whether to generate errors?  Checking for `!newImage` can't distinguish between the two types, and I don't think it's a good idea to extract the logic from CachedResourceLoader...

[Adam Barth]

> Yes, if images are disabled by means of the permission client, no error event should be fired

Why is that?  Does the load event fire instead?

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #8)
> > > > (From update of attachment 148286 [details] [details] [details] [details])
> > > > Attachment 148286 [details] [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > > Output: http://queues.webkit.org/results/12970970
> > > > 
> > > > New failing tests:
> > > > platform/chromium/permissionclient/image-permissions.html
> > > 
> > > Adam, Jochen (adding you because you worked on this test): Is the test's behavior correct? Should the `onerror` event not fire? I'm not at all familiar with the permissionclient work, so I'm not sure if I need to adjust the test expectations or adjust the behavior.
> > 
> > Yes, if images are disabled by means of the permission client, no error event should be fired
> 
> Ok, thanks! Am I correct in thinking that http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/loader/cache/CachedResourceLoader.cpp&exact_package=chromium&q=allowImage%20file:third_party/webkit&type=cs&l=368 is the permission client check?
> 

right

> If so, do you have any good ideas about how to decide whether to generate errors?  Checking for `!newImage` can't distinguish between the two types, and I don't think it's a good idea to extract the logic from CachedResourceLoader...

what about the way Adam suggested in comment 1? (i.e. checking the CSP in notifyFinished)

(In reply to comment #14)
> > Yes, if images are disabled by means of the permission client, no error event should be fired
> 
> Why is that?  Does the load event fire instead?

I don't know :)

the load event doesn't fire either

Darin wrote the code, you reviewed it. I just added a test

[Adam Barth]

IMHO, it's better to fire the error event in that case too.  We should treat these permission failures similarly to network errors (i.e., the image failed to load -> fire the error event).

[Adam Barth]

+fishd in case he avoiding firing the error event here on purpose.

[Mike West]

(In reply to comment #18)
> +fishd in case he avoiding firing the error event here on purpose.

Ping. :)

I'm happy to do whatever here, but I need some help deciding what that is. :)

[Adam Barth]

I think it's fine to fire the error event in that case.

(In reply to comment #20)
> I think it's fine to fire the error event in that case.

IIRC the important bit was that the broken image icon is not displayed if the image was blocked by content settings

Talked with Darin offline, he said that he doesn't remember any reason not to send the error event

[Mike West]

(In reply to comment #22)
> Talked with Darin offline, he said that he doesn't remember any reason not to send the error event

Thanks for following up, Jochen. :)

[Mike West]

Created attachment 152790 [details]
Patch

[Mike West]

japhet, would you mind taking a look at this patch?

Thanks!

will now a broken image icon show up when you block the load via web permission client? What happens for CSPs?

[Mike West]

(In reply to comment #26)
> will now a broken image icon show up when you block the load via web permission client? What happens for CSPs?

Ah, apologies, I apparently misunderstood Darin's email. I thought he was saying the whole error behavior wasn't intentional, so I haven't tested the behavior with regard to a broken image icon for content settings. I'll remove the review flag until I've done so.

[Mike West]

Created attachment 159272 [details]
Picking this back up.

[Mike West]

(In reply to comment #26)
> will now a broken image icon show up when you block the load via web permission client? What happens for CSPs?

I've finally gotten around to compiling this patch into Chromium; so far as I can tell, the visual behavior of blocked images is unchanged. No broken image shows up, either for content settings blockage, or content security policy blockage.

Would you take a look at this when you get a chance, Jochen? Adam r+'d it way back in June, but given the discussion and the time lapse, I'd prefer to clear the flag and go through another cycle. :)

[WebKit Review Bot]

Comment on attachment 159272 [details]
Picking this back up.

Attachment 159272 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13520845

New failing tests:
platform/chromium/permissionclient/image-permissions.html

[WebKit Review Bot]

Created attachment 159277 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Mike West]

Created attachment 159284 [details]
Fixing whitespace.

[WebKit Review Bot]

Comment on attachment 159284 [details]
Fixing whitespace.

Clearing flags on attachment: 159284

Committed r126194: <http://trac.webkit.org/changeset/126194>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #33)
> (From update of attachment 159284 [details])
> Clearing flags on attachment: 159284
> 
> Committed r126194: <http://trac.webkit.org/changeset/126194>

It caused regression on Qt and on GTK. Could you check it, please?
https://bugs.webkit.org/show_bug.cgi?id=94677 is the new bug report.