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
Mike West
2012-06-19 00:24:26 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. (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. :) Created attachment 148286 [details]
Patch
(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(). This looks right to me, but you might want to check with japhet before landing. (By the way, we should do this for all the kinds of loads, not just images.) (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 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 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
Multiple patches would be great. (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 (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... > 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 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). +fishd in case he avoiding firing the error event here on purpose. (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. :) 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 (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. :) Created attachment 152790 [details]
Patch
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? (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. Created attachment 159272 [details]
Picking this back up.
(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 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 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
Created attachment 159284 [details]
Fixing whitespace.
Comment on attachment 159284 [details] Fixing whitespace. Clearing flags on attachment: 159284 Committed r126194: <http://trac.webkit.org/changeset/126194> All reviewed patches have been landed. Closing bug. (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. |