Bug 86848 – Inline script and style blocked by Content Security Policy should provide more detailed console errors.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

Currently, inline script and style blocked by content security policy logs something like the following error to the console:

    Refused to execute inline script because it violates the following Content Security Policy directive: DIRECTIVE.

This would be improved by providing developers with the error's context. Adding a line number to the console message would enable the developer to jump directly to the error so they could see the issue clearly, rather than grepping through the DOM tree manually (or giving up entirely).

[Mike West]

Created attachment 142705 [details]
Patch

[Mike West]

This is a hopefully-not-too-ugly first pass at adding line numbers into the console messages. I'm not incredibly familiar with the innards of the document parser or script controller, so I'd appreciate some constructive feedback. :)

Comment on attachment 142705 [details]
Patch

Can you add a test that creates a script element and inserts it into the document, something like

var el = document.createElement('script');
el.innerText = 'alert(1)';
document.body.appendChild(el);


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

> Source/WebCore/page/ContentSecurityPolicy.cpp:526
> +    bool checkInlineAndReportViolation(CSPDirective*, const String& consoleMessage, const String& url, int lineNumber) const;

maybe use sourceUrl and sourceLineNumber so it's clear what kind of url this is and that the both belong together?

> Source/WebCore/page/ContentSecurityPolicy.cpp:694
> +    return checkEvalAndReportViolation(operativeDirective(m_scriptSrc.get()), consoleMessage, KURL(), 0);

String() instead of KURL()?

[Mike West]

Created attachment 142719 [details]
Patch

[Mike West]

Thanks Jochen, good idea to add injection tests.

1. I'm not actually sure what the correct blocking behavior should be: the `script` elements don't actually get injected (I think?) so the line numbers point to the script block that contained the script that injected the script (which is confusing just to type). The better behavior would be to flag that at the `appendChild` or `document.write` call, but I'm not sure at all how I would get that context piped through.

2. I went with `context{URL,LineNumber}` instead of `source*`. `source` is already being used in this context to talk about the resources themselves that are being blocked.

(In reply to comment #5)
> Thanks Jochen, good idea to add injection tests.

Now that you added new tests, you could update the WebCore/ChangeLog entry to reflect that

> 
> 1. I'm not actually sure what the correct blocking behavior should be: the `script` elements don't actually get injected (I think?) so the line numbers point to the script block that contained the script that injected the script (which is confusing just to type). The better behavior would be to flag that at the `appendChild` or `document.write` call, but I'm not sure at all how I would get that context piped through.

The script elements are actually being inserted into the document. I guess you could get the correct line number at ScriptElement::insertedAt and ::childrenChanged, however, there are already FIXME comments about the need to get the "real" line number


> 
> 2. I went with `context{URL,LineNumber}` instead of `source*`. `source` is already being used in this context to talk about the resources themselves that are being blocked.

[Adam Barth]

Comment on attachment 142719 [details]
Patch

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

> Source/WebCore/bindings/js/JSLazyEventListener.cpp:83
> +    ScriptController* script = document->frame()->script();

Any particular reason you moved this line?

> Source/WebCore/dom/ScriptElement.cpp:75
> +    if (m_element->document() && m_element->document()->scriptableDocumentParser())

m_element->document() is never null.  The only way a node can have a null document is if it's a DocumentType node.

> Source/WebCore/dom/ScriptElement.h:96
> +    int m_startLineNumber;

Can you use an ordinal number (or whatever the class name is) so we don't forget if this is a one-based or zero-based line number?

[Mike West]

Created attachment 142786 [details]
Patch

[Mike West]

Thanks for the feedback, Adam. The new patch should have taken care of the items you noted.

I haven't had much success at fixing the FIXME'd `ScriptElement::insertedAt` and `ScriptElement::childrenChanged`. I'll keep digging around, but I'd appreciate suggestions. :)

[Early Warning System Bot]

Comment on attachment 142786 [details]
Patch

Attachment 142786 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12724583

[Early Warning System Bot]

Comment on attachment 142786 [details]
Patch

Attachment 142786 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12722920

[Mike West]

Created attachment 142792 [details]
Patch

(In reply to comment #9)
> Thanks for the feedback, Adam. The new patch should have taken care of the items you noted.
> 
> I haven't had much success at fixing the FIXME'd `ScriptElement::insertedAt` and `ScriptElement::childrenChanged`. I'll keep digging around, but I'd appreciate suggestions. :)

I think you can't assign a useful line number to injected script tags. If you did, what if it's inserted before another tag that was originally in the file, would you update the line numbers in there?

Instead, I think the line number should be left undefined in these cases.

[Adam Barth]

Comment on attachment 142792 [details]
Patch

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

> Source/WebCore/page/ContentSecurityPolicy.h:38
> +namespace WTF {
> +
> +class OrdinalNumber;
> +
> +}

No need for blank lines around class OrdinalNumber

[Mike West]

Alright. I'll rebase this sometime after http://webk.it/86943 lands and hand it back to you for CQing.

Thanks!

[Mike West]

Created attachment 143299 [details]
Patch

[Mike West]

Comment on attachment 143299 [details]
Patch

Rebased onto trunk. Please take another look, if you don't mind, Adam.

Comment on attachment 143299 [details]
Patch

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

> LayoutTests/http/tests/security/contentSecurityPolicy/injected-inline-script-blocked-expected.txt:1
> +CONSOLE MESSAGE: line 8: Refused to execute inline script because it violates the following Content Security Policy directive: "script-src http://127.0.0.1:*".

I think it's a bit confusing that it points at line 8 because clearly that's not an inline script
 What about only storing the line number if the element was parser inserted?

Can you add a similar test for <style>?

[Mike West]

Created attachment 143493 [details]
Patch

[Mike West]

(In reply to comment #18)
> I think it's a bit confusing that it points at line 8 because clearly that's not an inline script
>  What about only storing the line number if the element was parser inserted?

I've added a check to ScriptElement::ScriptElement, and it already existed in StyleElement::StyleElement. The `document.write` call still ends up with a line number, however. Do you know if there's a good mechanism for detecting that insertion and clearing out the line number?
 
> Can you add a similar test for <style>?

Good idea. Added.

[Mike West]

Friendly ping. Are you happy with the current behavior, Jochen? If so, I'll cq?. If not, I'm happy to make changes. :)

Comment on attachment 143493 [details]
Patch

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

> Source/WebCore/dom/ScriptElement.cpp:76
> +    if (parserInserted && m_element->document()->scriptableDocumentParser())

I think && !m_element->document()->isInDocumentWrite() should do the trick

> Source/WebCore/page/ContentSecurityPolicy.h:35
> +

no emtpy lines here

> Source/WebCore/page/ContentSecurityPolicy.h:37
> +

or here

[Mike West]

Created attachment 144047 [details]
Patch

looks good, thank you

[Mike West]

Created attachment 144051 [details]
Patch

[Mike West]

Failed to apply; needed a rebase.

The github webkit mirror seems a bit out of date, however, which might make the new patch fail as well. Let's see how it goes.

[Mike West]

Comment on attachment 144051 [details]
Patch

Looking decent. I'll cq? in the hopes that it passes the remaining bots happily.

[Adam Barth]

Comment on attachment 144051 [details]
Patch

jochen is happy.  The bots are happy.  I'm happy.  Let's rock and roll.

[WebKit Review Bot]

Comment on attachment 144051 [details]
Patch

Clearing flags on attachment: 144051

Committed r118585: <http://trac.webkit.org/changeset/118585>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

This patch appears to have caused two regressions on EFL port:
  http/tests/security/contentSecurityPolicy/inline-script-blocked-javascript-url.html = TEXT
  http/tests/security/contentSecurityPolicy/javascript-url-blocked.html = TEXT

The diffs look like:
-CONSOLE MESSAGE: line 1: Refused to execute JavaScript URL because it violates the following Content Security Policy directive: "script-src 'none'".
+CONSOLE MESSAGE: Refused to execute JavaScript URL because it violates the following Content Security Policy directive: "script-src 'none'".

basically, the line numbers are not printed for those tests.

It appears GTK and Mac have platform-specific expectations (without the line numbers) for those 2 tests.

[Adam Barth]

Yes.  There's a bug on file to that effect.  I suspect it's the same issue on all the JSC ports.

[János Badics]

platform/qt/http/tests/security/contentSecurityPolicy/inline-script-blocked-javascript-url.html and
platform/qt/http/tests/security/contentSecurityPolicy/javascript-url-blocked-expected.txt
had the same problem as Christophe Dumez mentioned. I have added Qt platform specific expectations. Committed in http://trac.webkit.org/changeset/118738

[Chris Dumez]

(In reply to comment #33)
> platform/qt/http/tests/security/contentSecurityPolicy/inline-script-blocked-javascript-url.html and
> platform/qt/http/tests/security/contentSecurityPolicy/javascript-url-blocked-expected.txt
> had the same problem as Christophe Dumez mentioned. I have added Qt platform specific expectations. Committed in http://trac.webkit.org/changeset/118738

I proposed a fix for it in Bug 87558. I will update my patch so that it removes Qt expectations as well now that you pushed those.

[Andreas Kling]

This patch increased the size of every element by 4 bytes (the WTF::OrdinalNumber added to StyledElement.)

StyledElement is an extremely high-volume WebCore object, and we shouldn't be adding members to it unless absolutely necessary. Since this patch is only about adding more verbose debug output, it's an unacceptable regression in object size.

[Adam Barth]

Comment on attachment 144051 [details]
Patch

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

> Source/WebCore/dom/StyledElement.h:97
> +    WTF::OrdinalNumber m_startLineNumber;

Ah, you mean this member.  I missed the distinction between StyledElement and StyleElement.

Mike, would you be willing to remove this member?

[Adam Barth]

We should probably add a COMPILE_ASSERT comparing the size of Element with StyledElement so we don't make this mistake again.

[Mike West]

(In reply to comment #36)
> (From update of attachment 144051 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144051&action=review
> 
> > Source/WebCore/dom/StyledElement.h:97
> > +    WTF::OrdinalNumber m_startLineNumber;
> 
> Ah, you mean this member.  I missed the distinction between StyledElement and StyleElement.
> 
> Mike, would you be willing to remove this member?

Taking a look at this now.

[Mike West]

Created attachment 146325 [details]
Patch

[Mike West]

It looks like it's possible to get the same effect without the property (since it's already hanging off Document in a way that we can access when we need it). Would you mind taking a look?

[Adam Barth]

Comment on attachment 146325 [details]
Patch

Thanks.  Would you be willing to add a COMPILE_ASSERT in a followup patch?  If you grep around, you should be able to find some similar COMPILE_ASSERTs for other objects.

[WebKit Review Bot]

Comment on attachment 146325 [details]
Patch

Clearing flags on attachment: 146325

Committed r119776: <http://trac.webkit.org/changeset/119776>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mike West]

Reopening to attach new patch.

[Mike West]

Created attachment 146487 [details]
Patch

[Mike West]

(In reply to comment #41)
> (From update of attachment 146325 [details])
> Thanks.  Would you be willing to add a COMPILE_ASSERT in a followup patch?  If you grep around, you should be able to find some similar COMPILE_ASSERTs for other objects.

Sure thing. Patch attached.

I didn't see any other asserts directly comparing to other classes; most seemed to create a separate "SizeOfXXX" class for the comparison. I'm not sure that's stylistically correct here, since there aren't any new properties on StyledElement, but I've done it for consistency.

WDYT?

[Adam Barth]

Comment on attachment 146487 [details]
Patch

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

In the future, please use new bugs for new patches.  Bugs are cheap.  :)

> Source/WebCore/dom/StyledElement.cpp:53
> +COMPILE_ASSERT(sizeof(StyledElement) == sizeof(SameSizeAsStyledElement), styledelement_should_remain_small);

Why not just use sizeof(Element) ?

[Mike West]

(In reply to comment #47)
> (From update of attachment 146487 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146487&action=review
> 
> In the future, please use new bugs for new patches.  Bugs are cheap.  :)

Moving to http://webk.it/88627 and sizeof(Element).