Inline style bits are very unclear

[bzbarsky]

They don't seem to match browser behavior, for one thing: as far as I can tell, the spec requires the style attribute to be parsed, but not applied. How that's supposed to interact with .style manipulation is a mystery.

Furthermore, there's some confusion about cloning. Spec says a clone should not have its style attr applied, as far as I can tell, but at least some browsers do apply it, and there are web platform tests testing that it gets applied; see web-platform-tests/wpt#5614

Please get this sorted out and bugs filed on browsers as needed....

// cc @mikewest

[bzbarsky]

@mikewest Deciding on the right behavior here is actually blocking some work we're trying to do soonish in Gecko, so it would be good to have some clarity on the spec here...

[mikewest]

Digging through commit logs, we originally made this change in WebKit bug 112270 to address a bug jQuery filed. Their expectation was that something like the following code would result in two green squares, even in the presence of a CSP that blocked inline style (style-src 'self' or similar).

var d = document.createElement('div');
d.style.background = "green";
d.style.height = "100px";
d.style.width = "100px";
d.style.margin = "1em";
document.body.appendChild(d);

document.body.appendChild(d.cloneNode());

Based on the tiny experiment at https://jsbin.com/tusohe, it looks like both Firefox and WebKit/Blink have this behavior today, and I think it makes sense to keep its broad strokes (as the alternative would presumably break some set of developers who rely on the existing behavior, along the lines of the jQuery ticket above).

I think you're correct, though, that the specs don't make sense together. In WebKit/Blink, we end up doing something that I think equates to adding a flag to the "append" call in step 2.2.2 of https://dom.spec.whatwg.org/#concept-node-clone, noting that the operation is the result of cloning, and then thread that through to a hook that's called on elements when their style attribute changes.

If you squint, that hook more or less maps to "the attribute's value must be parsed when the attribute is added or has its value changed" in https://html.spec.whatwg.org/multipage/dom.html#the-style-attribute.

Does that seem like a reasonable direction to explore?

as far as I can tell, the spec requires the style attribute to be parsed, but not applied.

The distinction between "parsing" and "applying" in https://html.spec.whatwg.org/multipage/dom.html#the-style-attribute seems like one without a difference, as it looks like "parsing" turns into actual style applied to the element. The intent of the CSP bits there is to prevent the content of the style attribute from being used to style its element, but it's not really clear to me where "styling" happens. I'd appreciate advice about how to phrase this in a way that makes more sense.

/cc @annevk @tabatkins who might have opinions.

[annevk]

If you manipulate the style IDL attribute that results in changes to the content attribute per all the standards. (This is not uniformly implemented that way unfortunately.) Content attributes are cloned and I don't think we want to add exceptions there. So that should all continue to work without CSP getting in the way.

It seems that getting information out of the style IDL attribute is also fine. The main problem is post-parsing (although, when does fetching happens? Ah right, not well defined...): layout. That should be prevented (as well as fetching, whenever that happens) by CSP.

I don't know how to best define that, since despite asking for a long time, the CSS WG is very resistant to defining clear imperative entry points that they then also use themselves. I suppose we could try again.

[bzbarsky]

So a few things, from my point of view:

1. Ignoring what the specs might say right now, "parsing" and "applying" are conceptually separate operations for inline style. You can, conceptually, produce a CSSStyleDeclaration from the string (or from explicit DOM manipulation), but then not actually cascade that CSSStyleDeclaration if CSP says to not do so. In particular, given <div id="x" style="color: green"> and a CSP forbidding inline style, you could have x.style.color evaluate to "" if the attr is not parsed or to "green" if it is, and in either case you could conceivably have the div not actually be green. So the real question here is which model we want CSP to be using, which partially depends on what threat models it's trying to address.

2. Modifying the style IDL attribute via its various accessors does two things in practice: (a) modify the CSSStyleDeclaration (which per above may or may not be used for styling) and (b) modify the content attribute. The spec around this is slightly broken, because it doesn't define exactly what "set the content attribute" means and also says that setting the content attribute is supposed to update the declaration; I'm not sure whether it's observable whether this happens if the content attribute is set by the CSSOM manipulation, but I expect it very much is in cases when base URIs are mutated and that what the spec says doesn't match UAs.

3. The way all this is actually implemented in Gecko is that internally the value of a style attribute consists of two things, either one of which may be present or absent. One is the CSSStyleDeclaration object; the other is a string. setAttribute and the HTML parser set the string and create a CSSStyleDeclaration parsed from the string. Mutating the CSSStyleDeclaration changes it in-place and makes the string be absent. getAttribute uses the string if there is one, and serializes the CSSStyleDeclaration otherwise. So conceptually you could say that mutating the CSSStyleDeclaration immediately updates the string to its serialization but does not do so via the normal "set the attribute value" codepath; the lazy serialization is just an optimization. Finally, cloning in Gecko, for the specific case of the "style" attribute, clones the (CSSStyleDeclaration, string) pair directly instead of serializing and reparsing. This was added at some point as a performance optimization. That's why the web platform test in its current incarnation passes: we never go through the parsing step, and the parsing step is what currently makes the CSP check.

So some plausible approaches to speccing this stuff, assuming we want to preserve the "CSP doesn't apply to .style mutations" and "CSP doesn't apply to cloning" behavior:

• We could have CSP prevent parsing of the attr value into the CSSStyleDeclaration. Direct mutations to the CSSStyleDeclaration would still work. Cloning would be defined to clone the CSSStyleDeclaration as part of the cloning steps, in addition to cloning the attr value. We should think about how this should work in the importNode case, which can have different base URIs, or indeed other situations in which the base URI at clone time doesn't match the base URI at initial parse time. Some testing around how browsers handle this right now would probably be useful.

• We could have CSP prevent parsing of the attr value into the CSSStyleDeclaration. Direct mutations to the CSSStyleDeclaration would still work. Cloning would be defined to have a cloning step that checks whether the thing being cloned has a nonempty CSSStyleDeclaration and if so parses the style attribute on the clone into a CSSStyleDeclaration no matter what CSP says. I think this is black-box identical to the other option, except in the face of dynamic base URI changes...

• We could define some sort of threading things down through attribute setting as described above for Chrome. I think this would give us the same black-box behavior as the "reparse if nonempty decl" option and be much more complicated to spec.

• We could allow parsing no matter what but have CSP block application of style if it came from parsing an attr value; in that case we would have to think a bit about how to define cloning so things that were created via CSSOM on the element being cloned do not count as "came from an attr value" on the clone.

I do think we should clarify the CSSOM end of this stuff too, as we do this.. @zcorpan

[annevk]

Oh great, so Gecko wouldn't do mutation observer callbacks either if you modify the style IDL attribute... That clearly violates DOM invariants :/

[bzbarsky]

so Gecko wouldn't do mutation observer callbacks either if you modify the style IDL attribute

Why not? We do them. That is, when the CSSStyleDeclaration is modified we make it look from the point of view of everything outside like right after the mutation we're serializing it and then setting/parsing the resulting string, without actually doing that serialization and parsing. We fire DOMAttrModified, mutation observers, etc, as normal.

The internal state of the CSSStyleDeclaration doesn't come from parsing, though, which is observable via base URIs and the CSP interactions.

[mikewest]

I'll spend more time on this in the morning, but some quick thoughts now:

So the real question here is which model we want CSP to be using, which partially depends on what threat models it's trying to address.

The underlying goal of 'unsafe-inline' processing for style-src is to allow developers to mitigate the risk of direct style injection into an attribute or <style> block. I don't think the same justification applies to the style IDL attribute, so we allow CSSOM manipulations. That bit seems reasonably easy to justify.

By the same token, we don't have a particular need to prevent developers from cloning nodes with styling that they've poked at via CSSOM (though we do want to ensure that cloning an element with an inline style attribute doesn't inadvertently apply the style, which makes things complicated).

Of the options you present, the first two seem most straightforward to define with the specs that we have. I'm not convinced that threading some creation state through attribute setting would be "much" more complicated, though. That might be worth exploring (if only because it's what Blink is doing at the moment, and it looks like we're using that state in a few other places for reasons that I'm not sure I can justify by pointing to any spec (e.g. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLFormControlElement.cpp?rcl=ae0e0ee4a1b482fc86a7edcd371179c9cfd75d87&l=146, https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp?rcl=36b8552bced16b02fb080f98738087678accf273&l=184, and https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLElement.cpp?rcl=36b8552bced16b02fb080f98738087678accf273&l=430).

The fact that this is going to turn out to be complicated to spec means that we might just be doing the wrong thing, though. Maybe I should add some metrics to Chrome to measure how often we actually clone nodes with inline styles in documents whose policy would prevent the style from applying if we just accepted the currently-specced behavior for node cloning...

[annevk]

The internal state of the CSSStyleDeclaration doesn't come from parsing, though, which is observable via base URIs and the CSP interactions.

I guess that could be defined. Setting the style content attribute ends up observing an internal flag. When the flag is set, the CSS is not (re)parsed. When you manipulate through CSSStyleDeclaration it sets the flag, sets the attribute, unsets the flag. That should preserve everything without violating DOM invariants.

CSP would affect whether the it takes part in the cascade and whether fetches happen.

It seems cloning in that case would be fine, as CSP would continue to control the dangerous bits.

[bzbarsky]

So here is a question: if an element with an inline style attribute in a document that has a CSP that allows inline style is imported or adopted into a document that has a CSP that does NOT allow them, what should happen?

[bzbarsky]

I filed w3c/csswg-drafts#1559 on the CSSOM issue from #212 (comment) item 2.

[annevk]

The adoption (import is clone) case should probably fail as well.