- Notifications
You must be signed in to change notification settings - Fork20.6k
Core:Manipulation: Add basic TrustedHTML support#4927
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Travis fails due to#4926 which is unrelated. |
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| } ), | ||
| inputs = [ | ||
| [ "<div></div>", "<div class='test'></div>", [ "div" ] ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does it make sense to also add a test forTrustedHTMLs that wrap over a non-obvious HTML, e.g. just some text? From what I can tell, different branches are used then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, good point. That will only apply to the second array items as the first ones are passed directly tojQuery which also supports selectors - and since for TrustedHTML we cannot carve out a part of an input string to later parse, I opted to restrict support for TrustedHTML-wrapped strings to ones that are start with< and end with> - i.e. what was already going through the fast path skipping the regex match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So, I assume thatTrustedHTML wrappingsomething will just get stringified somewhere and be treated as a selector? That's OK, I don't think there would be expectations for jQuery to do anything else. I think it just makes sense to have an explicit test for that behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@koto It would actually just create a jQuery wrapper with a single element being this TrustedHTML instance, i.e. it would treat it like any other non-DOM object. I just didn't handle it in any special way and let it go where it goes naturally in the current flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To be clear, this limitation only applies to (pseudo-code)jQuery(TrustedHTML('foo'));jQuery(document.body).append(TrustedHTML('foo')) should append the textfoo just as without wrapping in TrustedHTML; I'll add a test for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, I forgot abouthttps://api.jquery.com/jQuery/#jQuery-object behavior :) Yeah, that's fine, thanks for clarifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added one test case for text content. I haven't added one for the object wrapper as I'm not sure if we want this to be a part of the contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sounds good!
koto commentedSep 10, 2021
Looks great! |
mgol commentedSep 13, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
PR updated, we're down from +38 to +30 bytes. |
linux-foundation-easyclabot commentedSep 14, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
This ensures HTML wrapped in TrustedHTML can be used as an input to jQuerymanipulation methods in a way that doesn't violate the`require-trusted-types-for` Content Security Policy directive.This commit builds on previous work needed for trusted types support, includingjquerygh-4642 andjquerygh-4724.One restriction is that while any TrustedHTML wrapper should work as inputfor jQuery methods like `.html()` or `.append()`, for passing directly to the`jQuery` factory the string must start with `<` and end with `>`; no trailingor leading whitespaces are allowed. This is necessary as we cannot parse outa part of the input for further construction; that would violate the CSP rule -and that's what's done to HTML input not matching these constraints.No trusted types API is used explicitly in source; the majority of the work isensuring we don't pass the input converted to string to APIs that wouldeventually assign it to `innerHTML`. This extra cautiousness is caused by theAPI being Blink-only, at least for now.The ban on passing strings to `innerHTML` means support tests relying on suchassignments are impossible. We don't currently have such tests on the `main`branch but we used to have many of them in the 3.x & older lines. If there'sa need to re-add such a test, we'll need an escape hatch to skip them for appsneeding CSP-enforced TrustedHTML.Seehttps://web.dev/trusted-types/ for more information about TrustedHTML.Fixesjquerygh-4409Refjquerygh-4642Refjquerygh-4724
@timmywil@koto I also added one small change in the tests (in the fixup commit) that makes the tests run even in browsers with no TrustedHTML support - there, it uses a simple object wrapper with a |
Uh oh!
There was an error while loading.Please reload this page.
Summary
This ensures HTML wrapped in TrustedHTML can be used as an input to jQuery
manipulation methods in a way that doesn't violate the
require-trusted-types-forContent Security Policy directive.This commit builds on previous work needed for trusted types support, including
gh-4642 andgh-4724.
One restriction is that while any TrustedHTML wrapper should work as input
for jQuery methods like
.html()or.append(), for passing directly to thejQueryfactory the string must start with<and end with>; no trailingor leading whitespaces are allowed. This is necessary as we cannot parse out
a part of the input for further construction; that would violate the CSP rule -
and that's what's done to HTML input not matching these constraints.
No trusted types API is used explicitly in source; the majority of the work is
ensuring we don't pass the input converted to string to APIs that would
eventually assign it to
innerHTML. This extra cautiousness is caused by theAPI being Blink-only, at least for now.
The ban on passing strings to
innerHTMLmeans support tests relying on suchassignments are impossible. We don't currently have such tests on the
mainbranch but we used to have many of them in the 3.x & older lines. If there's
a need to re-add such a test, we'll need an escape hatch to skip them for apps
needing CSP-enforced TrustedHTML.
Seehttps://web.dev/trusted-types/ for more information about TrustedHTML.
Fixesgh-4409
Refgh-4642
Refgh-4724
+30 bytes
Checklist