Throw instead of rendering error on security issue #2070
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is easier for users to understand what's going on and where the problem is. Upstreaming cl/369366746
🦋 Changeset detectedLatest commit: 5728589 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultslit-element-list
render
update
update-reflect
lit-html-kitchen-sink
render
update
nop-update
lit-html-repeat
render
update
lit-html-template-heavy
render
update
reactive-element-list
render
update
update-reflect
|
justinfagnani
approved these changes
Aug 28, 2021
| `Consider instead using css\`...\` literals to compose styles, and accomplishing dynamicism by mutating the DOM rather than styles`; | ||
| } else { | ||
| message = | ||
| `Lit does not support binding inside script nodes. ` + |
There was a problem hiding this comment.
This is fine as an existing restriction, but I'd like to dig into this more with the ISE team.
First, in client-side rendering <script> tags in Lit templates will not execute (in SSR they can, but we could make sure they don't).
Then <script> tags can be used for other purposes than executable JS, mainly because they're raw text elements and can contain unescaped "<" characters. The Playground Elements use <script> tags for file content. I've seen syntax highlighters that do the same. I could see cases where you want those to be dynamic...
There was a problem hiding this comment.
Yeah, we could add some additional checks here, e.g. that there's a type field of a non-javascript type.
It is the case that even if the type is a javascript type, script tags rendered like this don't typically run, but are we certain that there isn't a way to make them run? e.g. render into an unattached document fragment, then attach that? Something something inert template and clone into document?
There was a problem hiding this comment.
I'm not 100% sure if cloning drops the script-created bit, but I'd like to be! Seems like good tests to eventually add.
packages/lit-html/src/lit-html.ts
Outdated
| message = | ||
| `Lit does not support binding inside style nodes. ` + | ||
| `This is a security risk, as style injection attacks can exfiltrate data and spoof UIs. ` + | ||
| `Consider instead using css\`...\` literals to compose styles, and accomplishing dynamicism by mutating the DOM rather than styles`; |
There was a problem hiding this comment.
Can you beak this line up to be < 80 cols?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is easier for users to understand what's going on and where the problem is. Upstreaming cl/369366746