- Notifications
You must be signed in to change notification settings - Fork20.6k
Manipulation: Avoid concatenating strings in buildFragment#4724
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
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.
Thanks for the explanation! LGTM and saves space so double good.
876c924
toda74354
Comparekoto commentedJun 2, 2020
Looks great, thanks Michał! |
Concatenating HTML strings in buildFragment is a possible security risk as itcreates an opportunity of escaping the concatenated wrapper. It also makes itimpossible to support secure HTML wrappers like[trusted types](https://web.dev/trusted-types/). It's safer to create wrapperelements using `document.createElement` & `appendChild`.The previous way was needed in jQuery <4 because IE <10 doesn't accept tableparts set via `innerHTML`, even if the element which contents are set isa proper table element, e.g.:```jstr.innerHTML = "<td></td>";```The whole structure needs to be passed in one HTML string. jQuery 4 dropssupport for IE <11 so this is no longer an issue; in older version we'd have toduplicate the code paths.IE <10 needed to have `<option>` elements wrapped in`<select multiple="multiple">` but we no longer need that on master which makesthe `document.createElement` way shorter as we don't have to call`setAttribute`.jQuery 1.x sometimes needed to have more than one element in the wrapper thatwould precede parts wrapping HTML input so descending needed to use `lastChild`.Since all wrappers are single-element now, we can use `firstChild` whichcompresses better as it's used in other places in the code as well.All these improvements, apart from making logic more secure, decrease thegzipped size by 55 bytes.Refjquerygh-4409Refangular/angular.js#17028
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
@gibson042 Nice catch with the Please have another look. |
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.
LGTM!
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
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
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
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
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
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
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, includinggh-4642 andgh-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.Fixesgh-4409Closesgh-4927Refgh-4642Refgh-4724
Uh oh!
There was an error while loading.Please reload this page.
Summary
Concatenating HTML strings in buildFragment is a possible security risk as it
creates an opportunity of escaping the concatenated wrapper. It also makes it
impossible to support secure HTML wrappers like
trusted types. It's safer to create wrapper
elements using
document.createElement
&appendChild
.The previous way was needed in jQuery <4 because IE <10 doesn't accept table
parts set via
innerHTML
, even if the element which contents are set isa proper table element, e.g.:
The whole structure needs to be passed in one HTML string. jQuery 4 drops
support for IE <11 so this is no longer an issue; in older version we'd have to
duplicate the code paths.
IE <10 needed to have
<option>
elements wrapped in<select multiple="multiple">
but we no longer need that on master which makesthe
document.createElement
way shorter as we don't have to callsetAttribute
.jQuery 1.x sometimes needed to have more than one element in the wrapper that
would precede parts wrapping HTML input so descending needed to use
lastChild
.Since all wrappers are single-element now, we can use
firstChild
whichcompresses better as it's used in other places in the code as well.
All these improvements, apart from making logic more secure, decrease the
gzipped size by 55 bytes.
Refgh-4409
Refangular/angular.js#17028
Checklist
If needed, a docs issue/PR was created athttps://github.com/jquery/api.jquery.com