Bug 93865 – Break on inline scripts blocked by CSP.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

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

[Mike West]

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?

[Adam Barth]

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

[Mike West]

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

[Mike West]

Created attachment 158271 [details]
Patch

[Pavel Feldman]

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

[Mike West]

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

[Pavel Feldman]

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

[Mike West]

Created attachment 158773 [details]
Patch

[Mike West]

(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?

[Mike West]

Created attachment 158774 [details]
resetting tests.

[Pavel Feldman]

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.

[Mike West]

Created attachment 158781 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Mike West]

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?

[Pavel Feldman]

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

[Mike West]

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?

[Pavel Feldman]

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

[Mike West]

Created attachment 158809 [details]
Let's try this.

[Mike West]

(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. :(

[Pavel Feldman]

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

[Mike West]

(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!

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Pavel Feldman]

Created attachment 159404 [details]
Patch

[WebKit Review Bot]

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

[Mike West]

Created attachment 159429 [details]
Patch

[Mike West]

(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? :)

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Mike West]

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

[Pavel Feldman]

Heh. I've seen it somewhere :) So it did not work. It sounds like setStatus is not consistently called. I'll need to investigate :(

[Mike West]

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

[Pavel Feldman]

Sure, lets do that. Please file a bug on that and mention it in the FIXME.

[Mike West]

Created attachment 164293 [details]
Patch

[Mike West]

(In reply to comment #35)
> Created an attachment (id=164293) [details]
> Patch

Mostly a clean rebase. Let's see what the bots say.

[Mike West]

Comment on attachment 164293 [details]
Patch

Bots are happy. CQ?

[WebKit Review Bot]

Comment on attachment 164293 [details]
Patch

Clearing flags on attachment: 164293

Committed r128703: <http://trac.webkit.org/changeset/128703>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.