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.

Sign up for GitHub

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

Throw instead of rendering error on security issue #2070

Merged
merged 3 commits into from
Aug 28, 2021
Merged

Throw instead of rendering error on security issue #2070

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2737dd1
Throw instead of rendering error on security issue
rictic Aug 20, 2021
55ec3ed
Reflow and clarify errors.
rictic Aug 28, 2021
5728589
Add changeset
rictic Aug 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions packages/lit-html/src/lit-html.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,13 +1080,20 @@ class ChildPart implements Disconnectable {
) {
const parentNodeName = this._$startNode.parentNode?.nodeName;
if (parentNodeName === 'STYLE' || parentNodeName === 'SCRIPT') {
this._insert(
new Text(
'/* lit-html will not write ' +
'TemplateResults to scripts and styles */'
)
);
return;
let message = 'Forbidden';
if (DEV_MODE) {
if (parentNodeName === 'STYLE') {
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`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you beak this line up to be < 80 cols?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

} else {
message =
`Lit does not support binding inside script nodes. ` +
Copy link
Collaborator

Choose a reason for hiding this comment

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

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...

Copy link
Collaborator Author

@rictic rictic Aug 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

`This is a security risk, as it could allow arbitrary code execution.`;
}
}
throw new Error(message);
}
}
this._$committedValue = this._insert(value);
Expand Down