Reconsider usage of Numbers in CSSResult (based on real life painful use cases)

[tlouisse]

Description

Sorry to bother you again with the CSSResult variables issue, but I was hoping there is some room left for improvement of Developer Experience, after seeing the remark of @mikesamuel: #451 (comment)

We appreciate your considerations with regard to security, but we currently are experiencing some drawbacks as far as Developer Experience is concerned.
To illustrate this, see the following example (very verbose and thus harder to read and maintain):

import { css } from 'lit-element';
import { unsafeCss } from 'lit-element/lib/css-tag.js';
import { small, large, containerPadding } from '../variables/breakpoints.js';
// This would become quite unreadable:
export const gridCssModule = css`  
  .grid-container {
    margin: auto;
    max-width: ${unsafeCss(Number(small.cssText.slice(0,-2)) - (Number(containerPadding.cssText.slice(0,-2) * 2)))}px;
  }
  @media (min-width: ${small}) {
    .grid-container {
      max-width: ${unsafeCss(Number(large.cssText.slice(0,-2)) - Number((containerPadding.cssText.slice(0,-2)) * 2))}px;
    }
  }
`;

https://stackblitz.com/edit/css-variables-for-computations?file=style%2Fmodules%2Fgrid-css-module_numbersNotAllowed.js

My apologies for annoying you with a question that was already asked before, but we just wanted to show you some of our 'real life implications'.
So the derived question then would be: can Numbers on their own be a security issue?

If not, we could rewrite our example above to:

import { css } from 'lit-element';
import { small, large, containerPadding } from '../variables/breakpoints.js';
export const gridCssModule = css`  
  .grid-container {
    margin: auto;
    max-width: ${small - (containerPadding * 2)}px;
  }
  @media (min-width: ${small}px) {
    .grid-container {
      max-width: ${large - (containerPadding * 2)}px;
    }
  }
`;

https://stackblitz.com/edit/css-variables-for-computations?file=style%2Fmodules%2Fgrid-css-module_numbersAllowed.js

Live Demo

See above

[sorvell]

It may be possible to make this change, but we'll need to do a careful security review first. We're trying to be especially careful around security issues in CSS since it's particularly difficult to sanitize CSS.

However, I want to make sure I understand the examples given above. The intention of unsafeCss is to allow you to use any value. To be clear, the function is named this way so that it's clear that you may be incurring some risk.

So, you should be able to do something like this:

max-width: ${unsafeCss(small - containerPadding * 2)}px;

While this is slightly more verbose, it's definitely not as cumbersome as the example given.

[tlouisse]

That indeed would mitigate the pain.

But our aim mainly is to have a set of global variables that should be considered safe by default and can be applied without giving the impression they should be used with care. So I was hoping unsafeCss would be left for exceptional cases like import.meta.url.

Defining string values with css and numbers without would hugely improve readabilty for computations, but unsafeCss would then also be required for 'plain' usages(@media (min-width: ${unsafeCss(small)}px)).

However definitely the best option for now. Thanks for the suggestion and hopefully the security review will turn out positively for us. 



[oswee]

My simple custom Context Menu element use-case would be:

static get styles() {
	return css`
		:host {
			position: absolute;
			top: ${this.clientX}px;
			left: ${this.clientY}px;
                        // or ${unsafeCss(this.clientY)}
		}
	`;
}

where clientY is of { type: Number/String }
Currently i moved that part into render() { ... <style></style>}

[mikesamuel]

So the derived question then would be: can Numbers on their own be a security issue?

@tlouisse A minor one.

ISE would prefer to just allow numbers through rather than get developers in the habit of sprinkling unsafeCss directives throughout their code.

Primary risk of numbers

Stringifying a number can produce ("NaN", "Infinity", "-Infinity") or a string with chars in [0-9.e\-]. Those keyword values are not substrings of sensitive keyword values. Those other characters have not been instrumental in injection attacks elsewhere.

There is a risk of leaks with numbers combined with URLs:

1. Use a number in a margin-top or other directive to push content below the fold when prior content exceeds a certain length.
2. Use a URL in a background-image as a beacon
3. The backgroud URL is only fetched when the content takes a certain number of lines on common screen sizes.

Severity of attacker-controlled numbers

1. This is a low bandwidth leak about the content.
2. It's sufficient to guard URLs in CSS and can be further mitigated by the image-src Content-Security-Policy directive.

---

IMO the greater risk comes from a developer in the habit of routinely using unsafeCss allowing an input like } crafted[selector] { ... to reach unsafeCss.

[tlouisse]

Thanks a lot for the clarification

[daKmoR]

@mikesamuel imho the idea is not to just allow numbers to pass through but to safely type check them and only allow if they are truely valid numbers.

The check could be included here: https://github.com/Polymer/lit-element/blob/master/src/lib/css-tag.ts#L64

strawman proposal

const textFromCSSResult = (value: CSSResult) => {
  if (value instanceof CSSResult) {
    return value.cssText;
  } else if (typeof value === 'number' && isFinite(value) && value === value) {
    // check for being a number but not (+/-) infinity or NaN
    // (NaN is the only value in javascript which is not equal to itself)
    // we use isFinite and value === value to stay compatible with IE11
    return value;
  } else {
    throw new Error(
        `Value passed to 'css' function must be a 'css' function result: ${
            value}. Use 'unsafeCSS' to pass non-literal values, but
            take care to ensure page security.`);
  }
};

With such a check "valid" numbers should be save to use everywhere right? (also in the examples you posted if I am not mistaken - pls correct me if I am wrong)

get developers in the habit of sprinkling unsafeCss directives throughout their code.

I can could not agree more... once developers are in that habit they will not stop 🙈

[LarsDenBakker]

If there is no security risks with allowing Infinity and NaN then I'd go for just a typeof check.

[daKmoR]

friendly reminder 🤗

[mikesamuel]

@daKmoR I did thumbs up your summary and proposal from last week, but now that I think about it, that's unlikely to trigger a Github notification. Sorry if that got lost in the noise.

I +1 your proposal in #488 (comment)

I'll defer to the maintainers on whether the place you cite is the best place to make the change, but I don't see anything wrong.

Re #488 (comment) I see no security risk with allowing Infinity or NaN, so don't take a position on whether they should be passed/blocked.

[daKmoR]

friendly reminder on this... any progress or anything we can help with?

[daKmoR]

hey quick question if someone creates a merge request to allow for numbers as outlined here #488 (comment)

how high would be the chance for that merge request to be merged?

[sorvell]

A PR for this would be welcome. Pending review I think the objections have been addressed so this seems fine. Thanks!

[daKmoR]

So I took the code from #488 (comment) and put it into a Pull Request.

• Added test
• Added types
• Added info to changelog
• I could not come up with any valid use case where Infinity or NaN would make sense in css so it's not allowed for now (could be easily changed later if need be)