Bug 112270

Created attachment 193045 [details] test case proposal

Created attachment 193047 [details] test case proposal (expected results)

I was taking a look at the problem and I ended up doing the layout test. May be useful for someone else that is interested in this issue too.

(In reply to comment #3) > I was taking a look at the problem and I ended up doing the layout test. May be useful for someone else that is interested in this issue too. I'd encourage you to continue! Clone node ends up writing all the attributes from the initial element to the new element in Element::cloneAttributesFromElement (http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.cpp#L2848). A little ways down the chain, this ends up triggering StyledElement::styleAttributeChanged where the CSP check is performed. One option for a solution might be to pipe a bit of state down by adding an enum, perhaps `ChangedDirectly`/`ChangedViaClone`, and bypassing the check there in the latter case.

Thanks for the encouraging note :) Here goes my thoughts: I believe that the full walk through is: (Element::)cloneNode() `- (Element::)cloneAttributesFromElement() `- (StyledElement::)attributeChanged() `- (StyledElement::)styleAttributeChanged() `- CSP checks goes here. I am thinking that may not be a good idea to change (Element::)attributeChanged signature due the fact that it is virtual, implemented by everyone which extends Element. Maybe more modification that we really want. Instead I am thinking in create a m_styleAttributesAreBeenCloned (or so) state which could be set to true when cloneAttributesFromElement function is called, and before it returns we can set it to false. While true the CSP check can be ignored (at StyledElement::styleAttributeChanged). I need to double check if there is any race condition but apparently the cloneNode just returns after this process been done. I will work on a patch as soon as I arrive home.

Mike West, can you make this bug block jQuery's meta ticket so I can track progress? Thanks!

Created attachment 193479 [details] Patch

Comment on attachment 193479 [details] Patch This doesn't seem like the right approach.

Comment on attachment 193479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193479&action=review > Source/WebCore/dom/Element.cpp:2934 > unsigned bitfield; > + unsigned secondBitfield; Overflowing to a second bit field seems bad too.

Comment on attachment 193479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193479&action=review Thanks for the patch. I think Adam's correct to deny the review, however. Comments inline. > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Please add a bit more detail about what changed and why. > Source/WebCore/dom/Element.h:99 > + mutable unsigned m_attributeBeenCloned : 1; Adding another attribute to Element is expensive; it would be best to keep the object as small as possible, given the number that will be created during the process of parsing a document. I would suggest reconsidering the signature of attributeChanged, perhaps something like 'attributeChanged(const QualifiedName&, const AtomicString&, ReasonForAttributeChange reason = AttributeSetDirectly);', and passing in AttributeSetFromClone (or something similar) in the one callsite that matters. You're correct to say that it will adjust each of Element, StyledElement, and SVGElement. That is, however, much more acceptable than adding attributes to Element. > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:6 > +if (window.testRunner) { Nit: No braces are necessary for single-line blocks. > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:19 > + var div2 = div1.cloneNode(true); I'd suggest doing a bit more verification in JS. You should be able to assert that the attribute exists on both elements, that it has the same value, and that that value is applied to the element's computedStyle. Take a look at LayoutTests/fast/frames/seamless/seamless-body-margin.html for an example of how such a test might look.

Hey guys, thank you for the review. I am working on the signature approach and improving the test.

Created attachment 193825 [details] Patch

Comment on attachment 193825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193825&action=review Thanks for updating the patch! A few comments and questions inline. > Source/WebCore/ChangeLog:14 > + (WebCore::Element::attributeChanged): Please add a bit of detail here about what changed in the signature, and why. It doesn't have to be paragraphs, just a quick note that you added an enum and what it means. > Source/WebCore/dom/Element.h:376 > + enum ReasonForAttributeChange { AttributeSetDirectly = 0, AttributeSetFromClone }; Style nit: please break this enum out into separate lines, similar to http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/ScriptControllerBase.h#L35. > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects-expected.txt:1 > +CONSOLE MESSAGE: Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self'". Note that 'style-src' was not explicitly set, so 'default-src' is used as a fallback. Hrm. Where is this violation coming from? > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:4 > + <meta http-equiv="Content-Security-Policy" content="script-src 'self' 'unsafe-eval' 'unsafe-inline'; default-src 'self';"> You can simplify the console warnings by setting "style-src 'self'" rather than 'default-src'. > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:80 > + <div id="ops" style="background: rgb(238, 130, 238)"> Thank you for adding this check; I hadn't thought about this case in particular, so I'm glad you did. It seems a bit fragile, though, relying as it does on the fact that we never call setInlineStyleFromString. That said, I can't think of cases off the top of my head in which that would go wrong...

Comment on attachment 193825 [details] Patch Attachment 193825 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17184407

Comment on attachment 193825 [details] Patch Attachment 193825 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17253017

Comment on attachment 193825 [details] Patch Thanks for updating your patch. This new version is much better. Generally speaking, we prefer explicit data flows (like in your updated patch) to implicit data flows (like in your previous patch). I probably should have said that explicitly before. ;)

Comment on attachment 193825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193825&action=review > Source/WebCore/dom/Element.h:379 > - virtual void attributeChanged(const QualifiedName&, const AtomicString&); > + virtual void attributeChanged(const QualifiedName&, const AtomicString&, ReasonForAttributeChange = AttributeSetDirectly); I'm surprised there aren't more overrides of this function that need to be changed. Did you grep the codebase completely? Ideally these sorts of errors would show up in tests, but our test coverage isn't perfect.

Comment on attachment 193825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193825&action=review > Source/WebCore/dom/Element.cpp:847 > -void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue) > +void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue, ReasonForAttributeChange reason) Your patch is triggering a build failure on the apple-mac port: /Volumes/Data/EWS/WebKit/Source/WebCore/dom/Element.cpp:847:114: error: unused parameter 'reason' [-Werror,-Wunused-parameter] void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue, ReasonForAttributeChange reason) The solution to this problem is to remove the parameter name. That tells -Wunused-parameter that you know the parameter is unused.

Comment on attachment 193825 [details] Patch Attachment 193825 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17203307

Created attachment 193916 [details] Patch

(In reply to comment #13) > Thanks for updating the patch! A few comments and questions inline. Thank you for the review :) > > > Source/WebCore/ChangeLog:14 > > + (WebCore::Element::attributeChanged): > > Please add a bit of detail here about what changed in the signature, and why. It doesn't have to be paragraphs, just a quick note that you added an enum and what it means. Done. > > Source/WebCore/dom/Element.h:376 > > + enum ReasonForAttributeChange { AttributeSetDirectly = 0, AttributeSetFromClone }; > > Style nit: please break this enum out into separate lines, similar to http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/ScriptControllerBase.h#L35. Done. > > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects-expected.txt:1 > > +CONSOLE MESSAGE: Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self'". Note that 'style-src' was not explicitly set, so 'default-src' is used as a fallback. > > Hrm. Where is this violation coming from? > The first comes from: "../../resources/js-test-pre.js" script, the second is expected, from: '<div id="ops" style="background: rgb(238, 130, 238)'. > > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:4 > > + <meta http-equiv="Content-Security-Policy" content="script-src 'self' 'unsafe-eval' 'unsafe-inline'; default-src 'self';"> > > You can simplify the console warnings by setting "style-src 'self'" rather than 'default-src'. Done. Thanks ;)

(In reply to comment #17) > I'm surprised there aren't more overrides of this function that need to be changed. Did you grep the codebase completely? Ideally these sorts of errors would show up in tests, but our test coverage isn't perfect. Me too, at first sight I was expecting a lot of overrides, that is why I suggested to other approach. After you guys comments, I figured out that they are not so many. (zimmerle@omar)-(~/core/webkit)$ grep "attributeChanged(const QualifiedName&, const AtomicString&)" -Ri Source/* | wc -l 0

(In reply to comment #18) > (From update of attachment 193825 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193825&action=review > > > Source/WebCore/dom/Element.cpp:847 > > -void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue) > > +void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue, ReasonForAttributeChange reason) > > Your patch is triggering a build failure on the apple-mac port: > > /Volumes/Data/EWS/WebKit/Source/WebCore/dom/Element.cpp:847:114: error: unused parameter 'reason' [-Werror,-Wunused-parameter] > void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue, ReasonForAttributeChange reason) > > The solution to this problem is to remove the parameter name. That tells -Wunused-parameter that you know the parameter is unused. Done. Thanks :)

Comment on attachment 193916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193916&action=review Thanks for taking another pass. I'll unofficially r+,cq- this for the typo noted below. Hopefully Adam can take an official pass in the morning. > Source/WebCore/ChangeLog:9 > + style is allowed otherwise it relays on default permission mechanism. Nit: There's a typo in 'relies'. > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects-expected.txt:1 > +CONSOLE MESSAGE: Refused to apply inline style because it violates the following Content Security Policy directive: "style-src 'self'". This still bothers me, but it's not this patch's fault. :) I assume it's coming from 'js-test-pre.js' insertStyleSheet call. If so, it should have a line number. I'll poke at that in a separate bug.

Comment on attachment 193916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193916&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:4 > + <meta http-equiv="Content-Security-Policy" content="script-src 'self' 'unsafe-eval' 'unsafe-inline'; style-src 'self';"> One more thing: I think this will be even more clear if you just drop the 'script-src' directive entirely. It doesn't add anything to the test; you're only testing the behavior of 'style-src'.

Comment on attachment 193916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193916&action=review > Source/WebCore/dom/Element.h:379 > + enum ReasonForAttributeChange { > + AttributeSetDirectly, > + AttributeSetFromClone > + }; That doesnt sounds like a "reason" to me. More like who set it. Did you find similarly names things in WebCore, if not, take a look around ByClone? > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects-expected.txt:38 > +ops. > +ops. > +ops. ? :)

Created attachment 194139 [details] Patch

(In reply to comment #24) > > Nit: There's a typo in 'relies'. > Fixed.

(In reply to comment #25) > > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:4 > > + <meta http-equiv="Content-Security-Policy" content="script-src 'self' 'unsafe-eval' 'unsafe-inline'; style-src 'self';"> > > One more thing: I think this will be even more clear if you just drop the 'script-src' directive entirely. It doesn't add anything to the test; you're only testing the behavior of 'style-src'. Done.

(In reply to comment #26) > (From update of attachment 193916 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193916&action=review > > > Source/WebCore/dom/Element.h:379 > > + enum ReasonForAttributeChange { > > + AttributeSetDirectly, > > + AttributeSetFromClone > > + }; > > That doesnt sounds like a "reason" to me. More like who set it. Did you find similarly names things in WebCore, if not, take a look around > After talk to Kenneth, We agreed to change it to: enum AttributeModifyReason { ModifiedDirectly, ModifiedByCloning };

Kenneth, I'd appreciate it if you could take some time to officially review this since Adam is out? Thanks!

Comment on attachment 194139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194139&action=review > LayoutTests/ChangeLog:9 > + * http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects-expected.txt: Added. > + * http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html: Added. These files don't appear to be in the patch. Assuming they do what they say, this patch LGTM.

Created attachment 196324 [details] Patch

Comment on attachment 196324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196324&action=review > Source/WebCore/dom/Element.cpp:850 > +void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue, AttributeModifyReason) My apologies, I missed this in the last version of the patch: "AttributeModifyReason" sounds very strange. Please change it to "AttributeModificationReason" so that it's a reasonable noun, and I'll CQ the patch for you. Thanks!

Created attachment 196334 [details] Patch

Comment on attachment 196334 [details] Patch Thanks! Bots are happy, CQing.

Comment on attachment 196334 [details] Patch Clearing flags on attachment: 196334 Committed r147558: <http://trac.webkit.org/changeset/147558>

All reviewed patches have been landed. Closing bug.