Re: [csswg-drafts] [css-values-4] Switch advanced attr() to being var()-like (#4482)

> To make this easier to handle, I think this part: [omitted] should be changed to use a familiar calc()-like expression instead.

I don't like this approach; either `* 1px` is a special-cased token sequence that just indicates the same thing that "px" does in the current spec, or a full calculation is possible, which requires some complex new parsing (to allow for an ident at the beginning, which isn't part of calc()) and hooking into the "math function" part of the spec. That's a lot of complexity to avoid either a keyword or just using attr() inside a calc(). It's also unclear how this would mesh with the non-numeric attr() cases.

> Yes, the type checking on parse seemed an unnecessary step, as the value is going to be validated again anywhere when you evaluate the property.

While not technically necessary, I think it's useful as a way to trigger the fallback value. This functionality isn't present in var(), but I think that attributes are more likely to accidentally contain trash (most particularly, the empty string or whitespace, instead of being deleted) than custom properties are, and being able to trigger your default value in those cases seems useful.

> Although adding resolution seems harmless enough.

Yes, I'm adding the same types that Typed OM is aware of: resolution and flex are new additions.

> not so sure about the attr-in-attr thing, that means you need to do cycle detection on that too, and that's one of the most expensive parts of var().

Currently the spec requires the attribute value to be a simple literal; `attr(foo length)` would fail for `foo="calc(1px + 2px)"`, as it's looking for a dimension token. I probably want to relax that, but I do plan on explicitly disallowing attr(), var(), and env() in the attribute value; their presence will make the value illegal so it'll use the fallback instead. This avoids all the dependency-tracking issues, without losing anything significant I think; writing `foo="attr(bar px)"` seems like a weird thing to do.

It also keeps the information flow to a minimum; you'll only get precisely the attribute you requested injected into your stylesheet, not an unexpected attribute or a random custom property.

(I'm excluding env() because it's an another arbitrary reference, especially once author-defined env() gets nailed down.)  (I'll also put a note in the spec about excluding future reference functionality, to remind us to amend this spec if we add more stuff in the future. For example, I think I'll want to exclude custom functions once they exist.)

> attr() opens the door to make any attribute potentially an image load,

The threat surface is just that y'all sanitize URLs proactively, yeah? And this would allow URLs to get fed into the system that y'all haven't pre-examined.

It doesn't look like this allows any attacks in XSS terms or anything; an attribute can only cause an image load if the author purposely put an attr(foo url) in an image-loading property, which seems pretty explicit about their intent. And with attr() being disallowed from the attribute's value, it can't shift the attack surface to an unexpected attribute either.

Is this right? How bad is the sanitization issue?

> Is there any reason not to make all attr() values valid at parse time?

Yeah, after discussion on the call, this is my plan. It's technically a behavior change for existing attr() uses, but a minor one; if an author is currently writing something like `content: "foo"; content: attr(foo) 10px;` (accidentally relying on the second declaration being invalid and instead using the first), then after this change it would instead accept the second declaration and just be iacvt. That seems like a pretty insignificant worry to me, tho. (In particular, if they're currently writing an invalid 'content' by itself, with no preceding 'content' to fall back to or 'content' from another lower-specificity rule to cascade with, then iacvt is identical behavior to parsing failure in user-observable respects.)

> The interesting case here is where you disallow styles, but end up injecting styles anyway due to an attr() function.

Can you elaborate on this, @emilio?

-- 
GitHub Notification of comment by tabatkins
Please view or discuss this issue at https://github.com/w3c/csswg-drafts/issues/4482#issuecomment-555770806 using your GitHub account

Received on Tuesday, 19 November 2019 23:55:49 UTC