Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds unsafeCss for composing values into css #474

Merged
merged 3 commits into from
Jan 24, 2019
Merged

Adds unsafeCss for composing values into css #474

merged 3 commits into from
Jan 24, 2019

Conversation

sorvell
Copy link
Member

@sorvell sorvell commented Jan 23, 2019

Fixes #451 and #471. Non-literal values can now be included in styling with css by using the unsafeCss function. This is named "unsafe" so users know this they must use this carefully to avoid security issues.

Fixes #451 and #471. Non-literal values can now be included in styling with `css` by using the `unsafeCss` function. This is named "unsafe" so users know this they must use this carefully to avoid security issues.
@kenchris
Copy link
Contributor

Some documentation about what common security issues to be aware of, would be a nice addition

export const unsafeCss = (value: any) => {
return new CSSResult(String(value), constructionToken);
};

const textFromCSSResult = (value: CSSResult) => {
if (value instanceof CSSResult) {
return value.cssText;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider mentioning unsafeCss in the error message here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -37,6 +44,10 @@ export class CSSResult {
}
}

export const unsafeCss = (value: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer unknown to any here (and generally).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -37,6 +44,10 @@ export class CSSResult {
}
}

export const unsafeCss = (value: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation? Maybe:

/**
 * Wrap a value for interpolation in a css tagged template literal.
 *
 * This is unsafe because untrusted CSS text can be used to phone home
 * or exfiltrate data to an attacker controlled site. Take care to only use
 * this with trusted input.
 */

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks.

* add docs
* refine error message
Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@dfreedm dfreedm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dfreedm
Copy link
Member

dfreedm commented Jan 24, 2019

Some documentation about what common security issues to be aware of, would be a nice addition

@kenchris: Good point. Some documentation was added to the function, but I've also filed lit/lit.dev#868 to add some further detail and examples.

@dfreedm dfreedm merged commit e620444 into master Jan 24, 2019
@dfreedm dfreedm deleted the unsafe-css branch January 24, 2019 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider allowing string values in CSSResults?
5 participants