Bug 89440

Summary: Blocking a resource via Content Security Policy should trigger an Error event.
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, japhet, jochen, mkwst, ossy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 94677    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
Picking this back up.
none
Archive of layout-test-results from gce-cr-linux-05
none
Fixing whitespace. none

Description Mike West 2012-06-19 00:24:26 PDT
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.
Comment 1 Adam Barth 2012-06-19 00:27:48 PDT
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.
Comment 2 Mike West 2012-06-19 00:42:48 PDT
(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. :)
Comment 3 Mike West 2012-06-19 01:47:08 PDT
Created attachment 148286 [details]
Patch
Comment 4 Mike West 2012-06-19 01:49:43 PDT
(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().
Comment 5 Adam Barth 2012-06-19 01:59:43 PDT
This looks right to me, but you might want to check with japhet before landing.
Comment 6 Adam Barth 2012-06-19 02:00:27 PDT
(By the way, we should do this for all the kinds of loads, not just images.)
Comment 7 Mike West 2012-06-19 02:13:32 PDT
(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. :)
Comment 8 WebKit Review Bot 2012-06-19 02:18:56 PDT
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
Comment 9 WebKit Review Bot 2012-06-19 02:19:00 PDT
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
Comment 10 Adam Barth 2012-06-19 13:44:54 PDT
Multiple patches would be great.
Comment 11 Mike West 2012-06-21 08:14:01 PDT
(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.
Comment 12 jochen 2012-06-21 08:59:38 PDT
(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
Comment 13 Mike West 2012-06-21 11:45:25 PDT
(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...
Comment 14 Adam Barth 2012-06-21 13:11:12 PDT
> 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?
Comment 15 jochen 2012-06-21 15:24:04 PDT
(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)
Comment 16 jochen 2012-06-21 15:24:42 PDT
(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
Comment 17 Adam Barth 2012-06-21 15:27:31 PDT
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).
Comment 18 Adam Barth 2012-06-21 15:28:07 PDT
+fishd in case he avoiding firing the error event here on purpose.
Comment 19 Mike West 2012-07-15 05:16:09 PDT
(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. :)
Comment 20 Adam Barth 2012-07-15 10:13:22 PDT
I think it's fine to fire the error event in that case.
Comment 21 jochen 2012-07-15 10:40:53 PDT
(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
Comment 22 jochen 2012-07-16 06:59:30 PDT
Talked with Darin offline, he said that he doesn't remember any reason not to send the error event
Comment 23 Mike West 2012-07-17 10:47:57 PDT
(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. :)
Comment 24 Mike West 2012-07-17 11:15:27 PDT
Created attachment 152790 [details]
Patch
Comment 25 Mike West 2012-07-17 11:16:32 PDT
japhet, would you mind taking a look at this patch?

Thanks!
Comment 26 jochen 2012-07-17 11:19:02 PDT
will now a broken image icon show up when you block the load via web permission client? What happens for CSPs?
Comment 27 Mike West 2012-07-17 11:40:06 PDT
(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.
Comment 28 Mike West 2012-08-18 15:16:58 PDT
Created attachment 159272 [details]
Picking this back up.
Comment 29 Mike West 2012-08-18 15:22:17 PDT
(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. :)
Comment 30 WebKit Review Bot 2012-08-18 17:01:53 PDT
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
Comment 31 WebKit Review Bot 2012-08-18 17:01:58 PDT
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
Comment 32 Mike West 2012-08-19 00:42:54 PDT
Created attachment 159284 [details]
Fixing whitespace.
Comment 33 WebKit Review Bot 2012-08-21 15:22:37 PDT
Comment on attachment 159284 [details]
Fixing whitespace.

Clearing flags on attachment: 159284

Committed r126194: <http://trac.webkit.org/changeset/126194>
Comment 34 WebKit Review Bot 2012-08-21 15:22:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Csaba Osztrogonác 2012-08-22 01:35:01 PDT
(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.