-
Notifications
You must be signed in to change notification settings - Fork 317
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
Consider allowing string values in CSSResults? #451
Comments
This would be a potential security vulnerability. |
What do you suggest for this use case? It seems like it should be a common one.. wanting to use the meta url in paths. |
This should work as long as you use
|
Since rootPath will likely be generated from a variable, its not possible to create it through a |
^ exactly this @sorvell its coming from FYI you can work around it with |
Note: here is how I'm using the import { DomModule } from '@polymer/polymer/lib/elements/dom-module.js';
import { stylesFromTemplate } from '@polymer/polymer/lib/utils/style-gather.js';
import { CSSResult } from 'lit-element';
export const getStyleModule = (id, cb) => {
const template = DomModule.import(id, 'template');
let cssText =
template &&
stylesFromTemplate(template)
.map(style => style.textContent)
.join(' ');
// callback to process cssText
// use case: rewrite tag name for slotted children:
// example: vaadin-item -> lit-item
if (cb) {
cssText = cb(cssText);
}
return new CSSResult(cssText);
}; The result is added to the styles like this: static get styles() {
return [...super.styles, getStyleModule('lumo-button')];
} See https://github.com/web-padawan/lit-components/tree/master/packages/polymer-style-module I really hope the export for |
Hm. When running in secure mode, we could add a check that forbade strings but allowed TrustedResourceUrls. I'm not sure how you'd build a TrustedResourceUrl out of |
Why is this an issue with |
Interpolated strings in |
This could simply be a case where you need to use styles inline with the template for now until we figure out how to feed I hear there's work on trusted types in Chrome, so there may be a longer-term path forward there. |
In LitElement you define your styles via a static getter. This does not allow you to use element's instance properties which contain dynamic data which often comes from the untrusted user input. You compose you final styles once on a class level. I don't see where you will get untrusted user input there, you will likely always compose your static styles from other ES modules and just static values originating from your source code. So I think the problem is exaggerated a lot. It certainly exists theoretically, but how often? I think the solution is unbalanced: it fixes the theoretical issue while breaking a very frequent use case. |
I also would like to use css variables that are reusable in many contexts, not being coupled to CSSResult per se. For instance: I want to be able to define my (grid) breakpoints as a single source of truth throughout my app, and reuse them in both css and js: Or, more generically speaking: I might want to be able to combine my globally defined variables in other contexts (for instance combine it with other technology stacks) that do not rely on CSSResult. It would be nice if I could do that without wrapping all my vars in css``. Also, in some situations with a lot of small nested css strings (for instance when creating functions comparable to Sass Mixins) it feels a bit cumbersome to wrap every small string in Alternative to CSSResult const myBreakpoint = 400;
const myStyles = css`
@media (max-width: ${css([myBreakpoint])}px) {
...
}
` This would also mean that usage of const myBreakpoint = somethingPotentiallyMaliciousProvidedByUserInput; Taking this into account, it would be the responsibility of the developer to make sure there would be no security issues, not the Sanitization? if (supportsAdoptingStyleSheets) {
this._styleSheet = new CSSStyleSheet();
this._styleSheet.replaceSync(this.cssText);
} https://github.com/Polymer/lit-element/blob/master/src/lib/css-tag.ts#L30 |
In CSS, the risk is mostly privacy violations. There's always injecting a background URL that's a transparent image that phones home to a server controlled by the attacker. The other risk is stealing data from the page including from form inputs. Ideally, URLs in CSS would all be specified as SafeURLs and coerced to a known format like url("...").
Ok, so you already trust the origin Do JS bundlers rewrite |
I do. I don't remember saying I don't, so not sure I quite follow you here...
It gets rewritten to a path relative to the output bundle afaik, with static assets output alongside it (as well as other chunks). |
Sorry for the confusion. I was asking to see if we're reasoning along the same lines.
Thanks for explaining. |
@mikesamuel @rictic Would it be feasible to allow strings if we fed the bindings through the sanitizer with a fake |
Seems reasonable to me. I'm not sure what values are allowed as style text. Is there a TrustedStyleContent type? |
See #471 (comment). |
* Adds `unsafeCss` for composing values into `css` 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. * Address review feedback * add docs * refine error message
@justinfagnani Seems reasonable. Which sanitizer are we talking about though? |
Currently
css
only acceptsCSSResult
values (othercss
calls) which means we can no longer do something like this:This is a common pattern I have throughout my project to handle usages of
import.meta
so assets can have absolute paths.If you can suggest a better way to handle passing in a path, fair enough, but otherwise it would be handy to be able to interpolate string values here.
The text was updated successfully, but these errors were encountered: