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

Should all 3 script IDL setters change the associated script text value identically #517

Open
lukewarlow opened this issue May 20, 2024 · 11 comments
Labels

Comments

@lukewarlow
Copy link
Member

A good point came up during code review of an associated webkit patch that the .innerText setter steps and the .textContent/.text setter steps are different, and presumably could result in a different child contents value? If this is the case the current spec (and Chromium implementation) could lead to some script execution errors despite being set via a trusted object and sanctioned IDL attribute?

Question 1: Are these actually identical in behaviour?

Question 2: If they're not should the spec and implementationed be updated to account for the differences?

@annevk
Copy link
Member

annevk commented May 21, 2024

@lukewarlow
Copy link
Member Author

cc @otherdaniel and @koto curious what your thoughts on this are. I suspect that people might be using this in the wild? Question is can we remove it as a valid IDL sink or do we just document in the spec that it's an oddity and might not always work as expected?

@koto
Copy link
Member

koto commented May 21, 2024

[setting script content via innerText] could lead to some script execution errors despite being set via a trusted object

Isn't this the case even without Trusted Types? The spec doesn't provide any indication that the value will be functional for any given sink, only that the value has passed through a policy so there's no DOM XSS risk with assigning it.

Can we just not support that?

We definitely need to cover the DOM XSS sinks with Trusted Types, and script contents being set via innerText is used commonly in applications. TT consciously tries to work with the existing sinks, as opposed to e.g. proposing new ones and disabling old ones.

@lukewarlow
Copy link
Member Author

[setting script content via innerText] could lead to some script execution errors despite being set via a trusted object

Isn't this the case even without Trusted Types? The spec doesn't provide any indication that the value will be functional for any given sink, only that the value has passed through a policy so there's no DOM XSS risk with assigning it.

Specifically I mean trusted types related script execution errors. Because the value may mismatch in which case TT will reject it, which could be unexpected.

Sounds like we should either update the implementations and spec to set the value after it's been set so that it matches. Or just to add a note that explains there may be a mismatch?

@annevk
Copy link
Member

annevk commented May 21, 2024

Commonly? Can you point to an example? Given what it does with newlines I'd expect all kinds of brokenness.

@koto
Copy link
Member

koto commented May 21, 2024

Specifically I mean trusted types related script execution errors. Because the value may mismatch in which case TT will reject it, which could be unexpected.

I might be missing some context. Do you mean that a string would be passed to innerText on a script? Applications do use innerText with script elements intentionally to execute code, which is the only case in which TT would trigger? Can you give an example of what you mean if I misunderstood?

Commonly? Can you point to an example? Given what it does with newlines I'd expect all kinds of brokenness.

That's not easily shareable, but I see it (HTMLElement.innerText TT violations) right now in current violation reports, ~200K distinct violations in last 14 days. The scripts are all one liners. It seems to be just yet another variant of injecting code into applications that some developers use for no particular reason.

@annevk
Copy link
Member

annevk commented May 21, 2024

Ah, if the scripts are all on a single line they won't run into problems, but I don't think enshrining that makes sense for a standard.

@koto
Copy link
Member

koto commented May 21, 2024

Again, I don't think the TT claim that the values reaching the sinks are semantically correct, only that they were checked before reaching the sinks.

@annevk
Copy link
Member

annevk commented May 21, 2024

Sure, but we also store the given value in some internal slot, no? It seems that would mismatch with the value we end up using, which seems bad.

@lukewarlow
Copy link
Member Author

lukewarlow commented May 21, 2024

Again, I don't think the TT claim that the values reaching the sinks are semantically correct, only that they were checked before reaching the sinks.

Yes, but TT also claims that if they've been checked they'll attempt to execute. Which afaict isn't guarunteed for multiline text with innerText. Because we set the internal slot before the processing, and as such the prepareScript function will throw (or at least call the default policy).

data:text/html,<meta http-equiv="Content-Security-Policy" content="require-trusted-types-for 'script'; trusted-types default;"><body><script>const script = document.createElement('script'); script.innerText = TrustedScript.fromLiteral`console.log('line 1');\nconsole.log('line 2');`; document.body.appendChild(script);</script>

If you paste the above data URL into Chrome (with experimental flag for the fromLiteral to work) then it will error rather than execute. That's not an error from .innerText's spec, that's an error from the Trusted Types spec. The fromLiteral isn't relevant it's just the easiest way to write a quick example like this.

@lukewarlow
Copy link
Member Author

This will be fixed by #533 as it avoids the weirdness with innerTexts newline handling (now uses a flag not a duplication of value)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants