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).
Created attachment 142705 [details] Patch
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()?
Created attachment 142719 [details] Patch
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.
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?
Created attachment 142786 [details] Patch
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. :)
Comment on attachment 142786 [details] Patch Attachment 142786 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12724583
Comment on attachment 142786 [details] Patch Attachment 142786 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12722920
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.
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
Alright. I'll rebase this sometime after http://webk.it/86943 lands and hand it back to you for CQing. Thanks!
Created attachment 143299 [details] Patch
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>?
Created attachment 143493 [details] Patch
(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.
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
Created attachment 144047 [details] Patch
looks good, thank you
Created attachment 144051 [details] Patch
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.
Comment on attachment 144051 [details] Patch Looking decent. I'll cq? in the hopes that it passes the remaining bots happily.
Comment on attachment 144051 [details] Patch jochen is happy. The bots are happy. I'm happy. Let's rock and roll.
Comment on attachment 144051 [details] Patch Clearing flags on attachment: 144051 Committed r118585: <http://trac.webkit.org/changeset/118585>
All reviewed patches have been landed. Closing bug.
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.
Yes. There's a bug on file to that effect. I suspect it's the same issue on all the JSC ports.
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
(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.
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.
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?
We should probably add a COMPILE_ASSERT comparing the size of Element with StyledElement so we don't make this mistake again.
(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.
Created attachment 146325 [details] Patch
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?
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.
Comment on attachment 146325 [details] Patch Clearing flags on attachment: 146325 Committed r119776: <http://trac.webkit.org/changeset/119776>
Reopening to attach new patch.
Created attachment 146487 [details] Patch
(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?
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) ?
(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).