Page MenuHomePhabricator

CSS sanitizer refuses TemplateStyles variable assignment to border-color but does permit background-color
Open, Needs TriagePublicFeature

Description

Steps to replicate the issue

What happens?:

  • Sanitizer refuses publishing, complaining about such lines.
  • Sanitizer will accept assignment to various other properties.
  • Sanitizer does accept background-color: var(--border-color-progressive,#3366cc);
  • Sanitizer does accept color: var(--color-progressive--hover,#447ff5);

What should have happened instead?:

  • Sanitizer should accept border-color: var();
  • There is no higher risk of border-color rather than background-color

Details

Related Changes in Gerrit:

Event Timeline

You can work around by setting border-top-color, border-bottom-color, etc independently.

border: 1px solid var( --border-color-base, #a2a9b1 ); also works as advised in T368637#9942128.

I don't get why the security issue of variable concatenation made it impossible to also set border-color: var(--border-color-base) without any additional concatenation. This, to me, seems like an easy thing to allow right now without having the perfect to be an enemy of the good.

border: 1px solid var( --border-color-base, #a2a9b1 ); also works as advised in T368637#9942128.

If the components are split in various declaractions, e.g. a common 1px and other like padding as common ancestor, later various particular colours, e.g background + border + foreground appearance, there is no shorthand border: available.

Izno changed the subtype of this task from "Bug Report" to "Feature Request".Oct 28 2024, 3:07 AM
Izno moved this task from Backlog to External (css-sanitizer) on the TemplateStyles board.

For now this is intended behavior, see T361934#9692764

Could someone explain why the sanitizer e.g. cannot accept a border-color rule with only a single var(...) as its value?

Change #1195373 had a related patch set uploaded (by SD0001; author: SD0001):

[css-sanitizer@master] Allow CSS custom properties in border-color

https://gerrit.wikimedia.org/r/1195373

At the very least, it doesn't make sense to allow border: var(--foo) but not allow border-color

The reason is that border: var(--foo) var(--bar) is invalid, while border-color: var(--foo) var(--bar) is valid. See T361934#9692764.

The reason is that border: var(--foo) var(--bar) is invalid, while border-color: var(--foo) var(--bar) is valid. See T361934#9692764.

I'm confused here. border: var(--foo) var(--bar) is fully valid. Obviously they can't both be colours, but e.g. if --foo: 1px; --bar: solid it works. And you can combine multiple identifiers into one token like --foo: red; --bar: solid 1px.

To be honest, I feel like I don't really understand our threat model around variables. Are we worried about someone putting in a non-sanitized value for some property (In case the CSS spec adds something evil in future)? In such a case it doesn't make sense to allow any variables anywhere. Are we worried about short hand properties being extended to include other things in them? In that case we should not allow them in any short hand properties at all.

Are we worried that someone could create a sequence of var() calls that when everything is substituted and computed eventually get reparsed to make an identifier that wasn't present in any of the original variables (e.g. if --u: u, would var(--u)rl('http://example') somehow make a valid url() function? I first of all don't think that is possible with CSS variables if browsers are correctly implementing the spec (You can glue together tokens, but you can't retokenize. a url ident token is not the same thing as a u token followed by an r token followed by an l token), but even if it were, it still seems like there are two possible approaches to the problem - either a) not allow variables anywhere because we don't want the risk of url() anywhere, or b) don't allow them in any css property that could take a scary value like url() but allow them in properties that don't take any scary values. It seems like we are doing neither of those things.


Edit: i misunderstood before, the idea is not that multiple var() are invalid in border in general in css, but that template styles only allows one var(). I still think if this is something we are worried about, then it would also be reasonable to be worried about having some directly written prefix followed by a var(). I'm suspicious that if this threat was real, what we are doing currently would not be sufficient.

The reason is that border: var(--foo) var(--bar) is invalid

Are you sure? ;) Seems pretty valid to me. This creates a 2px solid border in Chrome:

--b-width: 2px;
--b-style: solid;
border: var(--b-width) var(--b-style);

[Edit conflict: didn't see Bawolff's comment before writing this.]

Also if this is a threat we are worried about, seems like we should also be worried about:

<div style="--foo: ur;--bar: l('https://evil.com')"><div style="background:var(--foo) var(--bar)"></div></div>

Or even

<div style="--foo: ur;--bar: l('https://evil.com')"><div style="--baz:var(--foo) var(--bar)"></div></div>

And then template styles that sets background: var(--baz) in template styles.

So, the current situation is that, in actual fact, for years, the following has been working fine:

<div style="border-color: var(--border-color-content-added, #C7D0F8);"> ... </div>

but when we tidy up the code and transfer that inline style to TemplateStyles, the system barfs on it (giving you no pointers for workarounds or any useful information whatsoever).

The very few of us who are persistent enough to report the bug (and have it closed as a duplicate) then eventually find out that styles.css will accept the following workaround:

border-top-color: var(--border-color-content-added, #C7D0F8);
border-right-color: var(--border-color-content-added, #C7D0F8);
border-bottom-color: var(--border-color-content-added, #C7D0F8);
border-left-color: var(--border-color-content-added, #C7D0F8);

Even if passing a var() to background were somehow a security problem, this blockage is clearly not a fix at all, as you can easily do the prohibited thing in inline styles.

So please be so kind to stop interfering with the work of unpaid volunteers to clean up inline CSS in templates and elsewhere.

Only background: is dangerous since it might bear an image URL to malicous.

The following would do no harm if an URL is attributed:

  • border:
  • border-color:
  • border-width:
  • background-color:
  • etc.

What could be refused is an assignment by

Here is an exhaustive list of vulnerable attributes, taken from https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/url_function:

The url() function can be included as a value for background, background-image, border, border-image, border-image-source, content, cursor, filter, list-style, list-style-image, mask, mask-image, offset-path, clip-path, src as part of a @font-face block, and @counter-style/symbol

Not sure why they include border, though, since it's shorthand for border-width, border-style, and border-color, none of which accept url(). Perhaps a mistake?

However, keep in mind that these attributes are only actually risky if CSS variables can be added together to create a token not in any one individually, which most people here seem skeptical of. @cscott Can you clarify your position on this? Is this token-merging indeed possible? Or is there some other risk we're all missing?