Bug 80024 - Web Inspector: [Styles] [CRASH] Handle rule addition and inline style editing failure due to Content-Security-Policy in the page
Summary: Web Inspector: [Styles] [CRASH] Handle rule addition and inline style editing...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-01 06:38 PST by Alexander Pavlov (apavlov)
Modified: 2012-03-05 02:45 PST (History)
13 users (show)

See Also:


Attachments
Patch (4.88 KB, patch)
2012-03-01 06:56 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (11.08 KB, patch)
2012-03-01 08:23 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Fixed CSP violation reporting for inline styles (13.76 KB, patch)
2012-03-02 06:28 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Comments addressed (12.63 KB, patch)
2012-03-02 08:33 PST, Alexander Pavlov (apavlov)
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2012-03-01 06:38:59 PST
Patch to follow.
Comment 1 Alexander Pavlov (apavlov) 2012-03-01 06:56:50 PST
Created attachment 129699 [details]
Patch
Comment 2 Pavel Feldman 2012-03-01 07:07:45 PST
Comment on attachment 129699 [details]
Patch

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

> Source/WebCore/inspector/InspectorCSSAgent.cpp:905
> +    // BUG80024: We may have failed to add an actual StyleSheet into the document due to CSP getting in our way.

CSP should not block native code from adding rule. Or you should build a way to override CSP.
Comment 3 Alexander Pavlov (apavlov) 2012-03-01 08:23:12 PST
Created attachment 129707 [details]
Patch
Comment 4 WebKit Review Bot 2012-03-01 12:21:30 PST
Comment on attachment 129707 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/inline-style-blocked.html
Comment 5 Pavel Feldman 2012-03-02 01:29:55 PST
Comment on attachment 129707 [details]
Patch

r- for regressing CSP
Comment 6 Alexander Pavlov (apavlov) 2012-03-02 06:28:07 PST
Created attachment 129893 [details]
[PATCH] Fixed CSP violation reporting for inline styles
Comment 7 Pavel Feldman 2012-03-02 07:55:56 PST
Comment on attachment 129893 [details]
[PATCH] Fixed CSP violation reporting for inline styles

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

> Source/WebCore/dom/Document.cpp:2579
> +void Document::setAllowInlineStyle(bool value)

You should override content security policy in ContentSecurityPolicy.cpp.

> Source/WebCore/dom/StyledElement.cpp:-101
> -        else if (document()->contentSecurityPolicy()->allowInlineStyle())

Please do not change call sites.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:900
> +        bool inlineStyleEnabled = document->allowInlineStyle(false);

Should be:
ContentSecurityPolicy* policy = document->contentSecurityPolicy();
policy->setOverrideAllowInlineStyle(true);
targetNode->...
policy->setOverrideAllowInlineStyle(false);

> Source/WebCore/page/ContentSecurityPolicy.cpp:525
> +    CSPDirective* styleSrcDirective = operativeDirective(m_styleSrc.get());

drop this

> LayoutTests/inspector/styles/add-new-rule-inline-style-csp.html:13
> +    InspectorTest.selectNodeAndWaitForStyles("inspected", step1);

These are not 3 steps, these are 3 tests.
Comment 8 Alexander Pavlov (apavlov) 2012-03-02 08:33:41 PST
Created attachment 129903 [details]
[PATCH] Comments addressed
Comment 9 Adam Barth 2012-03-02 09:10:07 PST
Comment on attachment 129903 [details]
[PATCH] Comments addressed

The changes to ContentSecurityPolicy.cpp/h look fine.
Comment 10 Pavel Feldman 2012-03-02 09:11:03 PST
Comment on attachment 129903 [details]
[PATCH] Comments addressed

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

> Source/WebCore/inspector/InspectorCSSAgent.cpp:745
> +        *errorString = "Failed to find or create inspector stylesheet, probably due to Content-Security-Policy";

This is no longer the case.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1324
> +    ContentSecurityPolicy* policy = m_element->ownerDocument() ? m_element->ownerDocument()->contentSecurityPolicy() : 0;

ownerDocument never returns NULL for an element. Also, I would recommend using raii idiom for this case.

> Source/WebCore/page/ContentSecurityPolicy.h:73
> +    void setOverrideAllowInlineStyle(bool);

Do we need a better name for this method? setOverrideAllowInlineStyleForInspector ?
Comment 11 Adam Barth 2012-03-02 09:21:31 PST
> Do we need a better name for this method? setOverrideAllowInlineStyleForInspector ?

setOverrideAllowInlineStyle ?
Comment 12 Alexander Pavlov (apavlov) 2012-03-02 09:27:40 PST
(In reply to comment #11)
> > Do we need a better name for this method? setOverrideAllowInlineStyleForInspector ?
> 
> setOverrideAllowInlineStyle ?

Will revert to this name when landing.
Comment 13 Alexander Pavlov (apavlov) 2012-03-05 02:45:12 PST
Committed r109730: <http://trac.webkit.org/changeset/109730>