Bug 93865 - Break on inline scripts blocked by CSP.
Summary: Break on inline scripts blocked by CSP.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords: WebExposed
Depends on:
Blocks: 93197 96867
  Show dependency treegraph
 
Reported: 2012-08-13 10:45 PDT by Mike West
Modified: 2012-09-15 22:17 PDT (History)
6 users (show)

See Also:


Attachments
Patch (26.00 KB, patch)
2012-08-14 02:45 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (31.94 KB, patch)
2012-08-16 04:45 PDT, Mike West
no flags Details | Formatted Diff | Diff
resetting tests. (32.29 KB, patch)
2012-08-16 04:49 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (33.40 KB, patch)
2012-08-16 05:12 PDT, Mike West
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (551.60 KB, application/zip)
2012-08-16 06:02 PDT, WebKit Review Bot
no flags Details
Let's try this. (34.20 KB, patch)
2012-08-16 07:07 PDT, Mike West
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (608.23 KB, application/zip)
2012-08-16 07:53 PDT, WebKit Review Bot
no flags Details
Patch (31.12 KB, patch)
2012-08-20 05:54 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (31.10 KB, patch)
2012-08-20 07:58 PDT, Mike West
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (387.56 KB, application/zip)
2012-08-20 08:46 PDT, WebKit Review Bot
no flags Details
Patch (30.77 KB, patch)
2012-09-15 12:03 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2012-08-13 10:45:54 PDT
Right now, blocking inline scripts with CSP results in an unhelpful "line 1". We can do better. pfeldman suggests:

- Introduce WebCore::InspectorInstrumentation::scriptBlockedByCSP() and call it from where appropriate
- Wire it to InspectorDebuggerAgent::scriptBlockedByCSP()
- Latter should call InspectorDebuggerAgent::breakProgram() in case "Break on exceptions" is engaged. It will look like it stopped on exception (with JS stack trace and such).
- If there are call sites in V8, we would need to go through the bindings
Comment 1 Mike West 2012-08-14 01:47:11 PDT
Hey Pavel, I'll have a patch for you in just a bit. Thanks for the detailed description. :)

Would you prefer that the tests for this feature live with the other debugger tests, or with the other CSP tests?
Comment 2 Adam Barth 2012-08-14 02:01:55 PDT
> Would you prefer that the tests for this feature live with the other debugger tests, or with the other CSP tests?

I'd guess with the other debugger test since there's a bunch of infrastructure along with those tests.
Comment 3 Mike West 2012-08-14 02:05:45 PDT
(In reply to comment #2)
> > Would you prefer that the tests for this feature live with the other debugger tests, or with the other CSP tests?
> 
> I'd guess with the other debugger test since there's a bunch of infrastructure along with those tests.

Thanks! How are you possibly awake? :)

I'll throw it in LayoutTests/inspector/debugger for now; can always move it later if it becomes unwieldy.
Comment 4 Mike West 2012-08-14 02:45:15 PDT
Created attachment 158271 [details]
Patch
Comment 5 Pavel Feldman 2012-08-14 05:59:33 PDT
Comment on attachment 158271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158271&action=review

Overall looks good.

> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:582
> +        breakProgram(InspectorFrontend::Debugger::Reason::Exception, desc.release());

Reason::Exception should have exception in the aux data, while you only pass plain text (see ScopeChainSidebarPane.js:83). I think you should create a new reason "CSPViolation" and pass 0 as the aux data string (it does not add much info). You would need to change ScriptsPanel around line 288 to format it properly as well.

> Source/WebCore/inspector/InspectorDebuggerAgent.h:117
> +    virtual void scriptExecutionBlockedByCSP();

While you are here, could you move schedulePauseOnNextStatement, cancelPauseOnNextStatement, breakProgram to above this line as well? They are a part of the instrumentation API, but squeezed into the middle of the protocol methods.

> Source/WebCore/page/ContentSecurityPolicy.cpp:598
> +    bool checkInlineAndReportViolation(CSPDirective*, const String& consoleMessage, const String& contextURL, const WTF::OrdinalNumber& contextLine, bool isInlineScript = true) const;

checkInline receiving isInlineScript confuses me. Leaving this up to abarth@.
Comment 6 Mike West 2012-08-14 08:02:03 PDT
Comment on attachment 158271 [details]
Patch

Thanks Pavel! I'll probably have a new patch up for you tomorrow.

View in context: https://bugs.webkit.org/attachment.cgi?id=158271&action=review

>> Source/WebCore/inspector/InspectorDebuggerAgent.cpp:582
>> +        breakProgram(InspectorFrontend::Debugger::Reason::Exception, desc.release());
> 
> Reason::Exception should have exception in the aux data, while you only pass plain text (see ScopeChainSidebarPane.js:83). I think you should create a new reason "CSPViolation" and pass 0 as the aux data string (it does not add much info). You would need to change ScriptsPanel around line 288 to format it properly as well.

How about passing in the directive text as the auxData string, which would enable a more informative status message: "Paused on a script blocked by Content Security Policy directive 'XXX'"?

>> Source/WebCore/page/ContentSecurityPolicy.cpp:598
>> +    bool checkInlineAndReportViolation(CSPDirective*, const String& consoleMessage, const String& contextURL, const WTF::OrdinalNumber& contextLine, bool isInlineScript = true) const;
> 
> checkInline receiving isInlineScript confuses me. Leaving this up to abarth@.

This was poorly named. Changing it to 'isScript'. We use 'checkInlineAndReportViolation' for both inline script and inline style; I need a mechanism to distinguish between the two, and it seemed cleaner to do it with (yet another) parameter rather than adding the inspector notification to each of the higher-level methods ('CSPDirectiveList::allowInlineScript', for instance).
Comment 7 Pavel Feldman 2012-08-14 08:20:22 PDT
> How about passing in the directive text as the auxData string, which would enable a more informative status message: "Paused on a script blocked by Content Security Policy directive 'XXX'"?

Sounds good! You've seen the place you can format it in the front-end, right?

> This was poorly named. Changing it to 'isScript'. We use 'checkInlineAndReportViolation' for both inline script and inline style; I need a mechanism to distinguish between the two, and it seemed cleaner to do it with (yet another) parameter rather than adding the inspector notification to each of the higher-level methods ('CSPDirectiveList::allowInlineScript', for instance).

Sounds good.
Comment 8 Mike West 2012-08-16 04:45:31 PDT
Created attachment 158773 [details]
Patch
Comment 9 Mike West 2012-08-16 04:47:32 PDT
(In reply to comment #8)
> Created an attachment (id=158773) [details]
> Patch

This should address your feedback, Pavel. I'm pretty sure I added the new CSPViolation to all the right places, but I'd appreciate you double-checking that. :)

WDYT?
Comment 10 Mike West 2012-08-16 04:49:14 PDT
Created attachment 158774 [details]
resetting tests.
Comment 11 Pavel Feldman 2012-08-16 05:00:00 PDT
Comment on attachment 158774 [details]
resetting tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=158774&action=review

> Source/WebCore/inspector/Inspector.json:2817
> +                    { "name": "reason", "type": "string", "enum": [ "XHR", "DOM", "EventListener", "exception", "CSPViolation", "other" ], "description": "Pause reason." },

Oh, so that string was a part of the enum. Now we are violating the protocol a tiny bit. I'll think of removing this enum declration.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:306
> +            this.sidebarPanes.callstack.setStatus(WebInspector.UIString("Paused on a script blocked due to Content Security Policy directive: \"%s\".", details.auxData.directiveText));

Please add this to the English.lproj/localizedStrings.js and you are good to go.
Comment 12 Mike West 2012-08-16 05:12:22 PDT
Created attachment 158781 [details]
Patch
Comment 13 WebKit Review Bot 2012-08-16 06:02:43 PDT
Comment on attachment 158781 [details]
Patch

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

New failing tests:
inspector/debugger/debugger-pause-on-blocked-script-url.html
inspector/debugger/debugger-pause-on-blocked-script-injection.html
inspector/debugger/debugger-pause-on-blocked-event-handler.html
Comment 14 WebKit Review Bot 2012-08-16 06:02:47 PDT
Created attachment 158791 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 15 Mike West 2012-08-16 06:25:35 PDT
These tests pass for me locally (chromium port, linux). The only difference I see between "resetting tests." patch (which passed) and the "Patch" test (which failed) is the localized text.

Did I do something wrong adding it to localizedStrings.js? Does it need to be registered somewhere?
Comment 16 Pavel Feldman 2012-08-16 06:39:26 PDT
(In reply to comment #15)
> These tests pass for me locally (chromium port, linux). The only difference I see between "resetting tests." patch (which passed) and the "Patch" test (which failed) is the localized text.
> 
> Did I do something wrong adding it to localizedStrings.js? Does it need to be registered somewhere?

It looks more like your tests are flaky - entire line is missing, not just the localized piece.
Comment 17 Mike West 2012-08-16 06:55:09 PDT
True. In that case, I'm a bit stuck. I'm running them in a loop locally, and they seem stable (no failures yet, about halfway through running them 100 times each).

Do you know of any case in which setting a sniffer on `setStatus` would flake? 

I'll rework the tests so that `setStatus` callback has to be called in order to finish... Perhaps they're just getting called after the test has exited due to interesting async behaviors?
Comment 18 Pavel Feldman 2012-08-16 07:06:55 PDT
(In reply to comment #17)
> True. In that case, I'm a bit stuck. I'm running them in a loop locally, and they seem stable (no failures yet, about halfway through running them 100 times each).
> 
> Do you know of any case in which setting a sniffer on `setStatus` would flake? 
> 
> I'll rework the tests so that `setStatus` callback has to be called in order to finish... Perhaps they're just getting called after the test has exited due to interesting async behaviors?

Yeah, it is a layered async cake as you probably noticed...
Comment 19 Mike West 2012-08-16 07:07:52 PDT
Created attachment 158809 [details]
Let's try this.
Comment 20 Mike West 2012-08-16 07:29:35 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > True. In that case, I'm a bit stuck. I'm running them in a loop locally, and they seem stable (no failures yet, about halfway through running them 100 times each).
> > 
> > Do you know of any case in which setting a sniffer on `setStatus` would flake? 
> > 
> > I'll rework the tests so that `setStatus` callback has to be called in order to finish... Perhaps they're just getting called after the test has exited due to interesting async behaviors?
> 
> Yeah, it is a layered async cake as you probably noticed...

Hrm. The current patch ensures that `setStatus` must be called in order to complete.

Since it's timing out, I think something else is going wrong... it's not just an async issue. I'm sure there's something I'm doing wrong, but I don't really know where to start looking to figure out what the bot is doing. :(
Comment 21 Pavel Feldman 2012-08-16 07:36:25 PDT
> Since it's timing out, I think something else is going wrong... it's not just an async issue. I'm sure there's something I'm doing wrong, but I don't really know where to start looking to figure out what the bot is doing. :(

I wfh today, so am only hacking the front-end. I can apply it tomorrow and see what is wrong.
Comment 22 Mike West 2012-08-16 07:37:34 PDT
(In reply to comment #21)
> > Since it's timing out, I think something else is going wrong... it's not just an async issue. I'm sure there's something I'm doing wrong, but I don't really know where to start looking to figure out what the bot is doing. :(
> 
> I wfh today, so am only hacking the front-end. I can apply it tomorrow and see what is wrong.

No worries, and no rush. No one's waiting on this but me. :)

Thanks!
Comment 23 WebKit Review Bot 2012-08-16 07:53:49 PDT
Comment on attachment 158809 [details]
Let's try this.

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

New failing tests:
inspector/debugger/debugger-pause-on-blocked-script-url.html
inspector/debugger/debugger-pause-on-blocked-script-injection.html
inspector/debugger/debugger-pause-on-blocked-event-handler.html
Comment 24 WebKit Review Bot 2012-08-16 07:53:53 PDT
Created attachment 158820 [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 25 Pavel Feldman 2012-08-20 05:54:24 PDT
Created attachment 159404 [details]
Patch
Comment 26 WebKit Review Bot 2012-08-20 07:34:16 PDT
Comment on attachment 159404 [details]
Patch

Rejecting attachment 159404 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
50>At revision 134927.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13538681
Comment 27 Mike West 2012-08-20 07:58:27 PDT
Created attachment 159429 [details]
Patch
Comment 28 Mike West 2012-08-20 07:59:46 PDT
(In reply to comment #26)
> (From update of attachment 159404 [details])
> Rejecting attachment 159404 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> 50>At revision 134927.
> 
> ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> 
> ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> Updating webkit projects from gyp files...
> 
> Full output: http://queues.webkit.org/results/13538681

"Merge conflict during commit: Conflict at '/trunk/LayoutTests/ChangeLog' at /usr/lib/git-core/git-svn line 570" Hilarious.

Rebased onto an even tippier ToT. Want to try again? :)
Comment 29 WebKit Review Bot 2012-08-20 08:46:42 PDT
Comment on attachment 159429 [details]
Patch

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

New failing tests:
inspector/debugger/debugger-pause-on-blocked-script-url.html
inspector/debugger/debugger-pause-on-blocked-script-injection.html
inspector/debugger/debugger-pause-on-blocked-event-handler.html
Comment 30 WebKit Review Bot 2012-08-20 08:46:46 PDT
Created attachment 159440 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 31 Mike West 2012-08-20 08:49:56 PDT
(In reply to comment #29)
> (From update of attachment 159429 [details])
> Attachment 159429 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13543372
> 
> New failing tests:
> inspector/debugger/debugger-pause-on-blocked-script-url.html
> inspector/debugger/debugger-pause-on-blocked-script-injection.html
> inspector/debugger/debugger-pause-on-blocked-event-handler.html

I didn't touch anything other than the ChangeLog. :(

Bah. I'll look at the diff between the diffs in a few minutes.
Comment 32 Pavel Feldman 2012-08-20 08:59:17 PDT
Heh. I've seen it somewhere :) So it did not work. It sounds like setStatus is not consistently called. I'll need to investigate :(
Comment 33 Mike West 2012-09-15 07:03:20 PDT
(In reply to comment #32)
> Heh. I've seen it somewhere :) So it did not work. It sounds like setStatus is not consistently called. I'll need to investigate :(

I haven't had time to dig into this flake, and it's certainly not high-prio enough for you to spend time on it, Pavel.

Would you mind if I just dropped the setStatus check in favor of a FIXME comment in the test, and landed the core feature without it? Users will still get a nice experience if "break on exceptions" is on. If that sounds alright to you, I'll rebase the patch onto trunk.
Comment 34 Pavel Feldman 2012-09-15 08:15:48 PDT
Sure, lets do that. Please file a bug on that and mention it in the FIXME.
Comment 35 Mike West 2012-09-15 12:03:15 PDT
Created attachment 164293 [details]
Patch
Comment 36 Mike West 2012-09-15 12:08:13 PDT
(In reply to comment #35)
> Created an attachment (id=164293) [details]
> Patch

Mostly a clean rebase. Let's see what the bots say.
Comment 37 Mike West 2012-09-15 21:46:08 PDT
Comment on attachment 164293 [details]
Patch

Bots are happy. CQ?
Comment 38 WebKit Review Bot 2012-09-15 22:17:13 PDT
Comment on attachment 164293 [details]
Patch

Clearing flags on attachment: 164293

Committed r128703: <http://trac.webkit.org/changeset/128703>
Comment 39 WebKit Review Bot 2012-09-15 22:17:19 PDT
All reviewed patches have been landed.  Closing bug.