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:

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

Mike West

Reported 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.

Attachments
Patch (8.37 KB, patch) 2012-06-19 01:47 PDT, Mike West	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-03 (471.97 KB, application/zip) 2012-06-19 02:19 PDT, WebKit Review Bot	no flags	Details
Patch (10.27 KB, patch) 2012-07-17 11:15 PDT, Mike West	no flags	Details Formatted Diff Diff
Picking this back up. (10.39 KB, patch) 2012-08-18 15:16 PDT, Mike West	no flags	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-05 (364.92 KB, application/zip) 2012-08-18 17:01 PDT, WebKit Review Bot	no flags	Details
Fixing whitespace. (10.39 KB, patch) 2012-08-19 00:42 PDT, Mike West	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 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.

Mike West

Comment 2 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. :)

Mike West

Comment 3 2012-06-19 01:47:08 PDT

Created attachment 148286 [details] Patch

Mike West

Comment 4 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().

Adam Barth

Comment 5 2012-06-19 01:59:43 PDT

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

Adam Barth

Comment 6 2012-06-19 02:00:27 PDT

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

Mike West

Comment 7 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. :)

WebKit Review Bot

Comment 8 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

WebKit Review Bot

Comment 9 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

Adam Barth

Comment 10 2012-06-19 13:44:54 PDT

Multiple patches would be great.

Mike West

Comment 11 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.

jochen

Comment 12 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

Mike West

Comment 13 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...

Adam Barth

Comment 14 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?

jochen

Comment 15 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)

jochen

Comment 16 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

Adam Barth

Comment 17 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).

Adam Barth

Comment 18 2012-06-21 15:28:07 PDT

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

Mike West

Comment 19 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. :)

Adam Barth

Comment 20 2012-07-15 10:13:22 PDT

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

jochen

Comment 21 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

jochen

Comment 22 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

Mike West

Comment 23 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. :)

Mike West

Comment 24 2012-07-17 11:15:27 PDT

Created attachment 152790 [details] Patch

Mike West

Comment 25 2012-07-17 11:16:32 PDT

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

jochen

Comment 26 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?

Mike West

Comment 27 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.

Mike West

Comment 28 2012-08-18 15:16:58 PDT

Created attachment 159272 [details] Picking this back up.

Mike West

Comment 29 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. :)

WebKit Review Bot

Comment 30 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

WebKit Review Bot

Comment 31 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

Mike West

Comment 32 2012-08-19 00:42:54 PDT

Created attachment 159284 [details] Fixing whitespace.

WebKit Review Bot

Comment 33 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>

WebKit Review Bot

Comment 34 2012-08-21 15:22:43 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 35 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.

Note You need to log in before you can comment on or make changes to this bug.