Manipulation: Avoid concatenating strings in buildFragment #4724
Conversation
This file contains hidden or 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 was referenced May 31, 2020
Closed
mgol
force-pushed
the
secure-buildFragment
branch
from
b9588a7 to
8a24861
Compare
dmethvin
approved these changes
Jun 1, 2020
mgol
force-pushed
the
secure-buildFragment
branch
2 times, most recently
from
876c924 to
da74354
Compare
timmywil
approved these changes
Jun 1, 2020
|
Looks great, thanks Michał! |
mgol
force-pushed
the
secure-buildFragment
branch
from
da74354 to
872affd
Compare
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](https://web.dev/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 is a proper table element, e.g.: ```js tr.innerHTML = "<td></td>"; ``` 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 makes the `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 that would precede parts wrapping HTML input so descending needed to use `lastChild`. Since all wrappers are single-element now, we can use `firstChild` which compresses 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. Ref jquerygh-4409 Ref angular/angular.js#17028
mgol
force-pushed
the
secure-buildFragment
branch
from
872affd to
bb726fc
Compare
gibson042
reviewed
Jun 8, 2020
Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
|
@gibson042 Nice catch with the Please have another look. |
4 tasks
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Sep 13, 2021
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-for` Content Security Policy directive. This commit builds on previous work needed for trusted types support, including jquerygh-4642 and jquerygh-4724. One restriction is that while any TrustedHTML wrapper should work as input for jQuery methods like `.html()` or `.append()`, for passing directly to the `jQuery` factory the string must start with `<` and end with `>`; no trailing or 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 the API being Blink-only, at least for now. The ban on passing strings to `innerHTML` means support tests relying on such assignments 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's a need to re-add such a test, we'll need an escape hatch to skip them for apps needing CSP-enforced TrustedHTML. See https://web.dev/trusted-types/ for more information about TrustedHTML. Fixes jquerygh-4409 Ref jquerygh-4642 Ref jquerygh-4724
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Sep 14, 2021
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-for` Content Security Policy directive. This commit builds on previous work needed for trusted types support, including jquerygh-4642 and jquerygh-4724. One restriction is that while any TrustedHTML wrapper should work as input for jQuery methods like `.html()` or `.append()`, for passing directly to the `jQuery` factory the string must start with `<` and end with `>`; no trailing or 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 the API being Blink-only, at least for now. The ban on passing strings to `innerHTML` means support tests relying on such assignments 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's a need to re-add such a test, we'll need an escape hatch to skip them for apps needing CSP-enforced TrustedHTML. See https://web.dev/trusted-types/ for more information about TrustedHTML. Fixes jquerygh-4409 Ref jquerygh-4642 Ref jquerygh-4724
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Sep 17, 2021
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-for` Content Security Policy directive. This commit builds on previous work needed for trusted types support, including jquerygh-4642 and jquerygh-4724. One restriction is that while any TrustedHTML wrapper should work as input for jQuery methods like `.html()` or `.append()`, for passing directly to the `jQuery` factory the string must start with `<` and end with `>`; no trailing or 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 the API being Blink-only, at least for now. The ban on passing strings to `innerHTML` means support tests relying on such assignments 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's a need to re-add such a test, we'll need an escape hatch to skip them for apps needing CSP-enforced TrustedHTML. See https://web.dev/trusted-types/ for more information about TrustedHTML. Fixes jquerygh-4409 Ref jquerygh-4642 Ref jquerygh-4724
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Sep 23, 2021
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-for` Content Security Policy directive. This commit builds on previous work needed for trusted types support, including jquerygh-4642 and jquerygh-4724. One restriction is that while any TrustedHTML wrapper should work as input for jQuery methods like `.html()` or `.append()`, for passing directly to the `jQuery` factory the string must start with `<` and end with `>`; no trailing or 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 the API being Blink-only, at least for now. The ban on passing strings to `innerHTML` means support tests relying on such assignments 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's a need to re-add such a test, we'll need an escape hatch to skip them for apps needing CSP-enforced TrustedHTML. See https://web.dev/trusted-types/ for more information about TrustedHTML. Fixes jquerygh-4409 Ref jquerygh-4642 Ref jquerygh-4724
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Sep 29, 2021
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-for` Content Security Policy directive. This commit builds on previous work needed for trusted types support, including jquerygh-4642 and jquerygh-4724. One restriction is that while any TrustedHTML wrapper should work as input for jQuery methods like `.html()` or `.append()`, for passing directly to the `jQuery` factory the string must start with `<` and end with `>`; no trailing or 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 the API being Blink-only, at least for now. The ban on passing strings to `innerHTML` means support tests relying on such assignments 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's a need to re-add such a test, we'll need an escape hatch to skip them for apps needing CSP-enforced TrustedHTML. See https://web.dev/trusted-types/ for more information about TrustedHTML. Fixes jquerygh-4409 Ref jquerygh-4642 Ref jquerygh-4724
mgol
added a commit
to mgol/jquery
that referenced
this pull request
Sep 29, 2021
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-for` Content Security Policy directive. This commit builds on previous work needed for trusted types support, including jquerygh-4642 and jquerygh-4724. One restriction is that while any TrustedHTML wrapper should work as input for jQuery methods like `.html()` or `.append()`, for passing directly to the `jQuery` factory the string must start with `<` and end with `>`; no trailing or 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 the API being Blink-only, at least for now. The ban on passing strings to `innerHTML` means support tests relying on such assignments 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's a need to re-add such a test, we'll need an escape hatch to skip them for apps needing CSP-enforced TrustedHTML. See https://web.dev/trusted-types/ for more information about TrustedHTML. Fixes jquerygh-4409 Ref jquerygh-4642 Ref jquerygh-4724
mgol
added a commit
that referenced
this pull request
Sep 30, 2021
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-for` Content Security Policy directive. This commit builds on previous work needed for trusted types support, including gh-4642 and gh-4724. One restriction is that while any TrustedHTML wrapper should work as input for jQuery methods like `.html()` or `.append()`, for passing directly to the `jQuery` factory the string must start with `<` and end with `>`; no trailing or 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 the API being Blink-only, at least for now. The ban on passing strings to `innerHTML` means support tests relying on such assignments 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's a need to re-add such a test, we'll need an escape hatch to skip them for apps needing CSP-enforced TrustedHTML. See https://web.dev/trusted-types/ for more information about TrustedHTML. Fixes gh-4409 Closes gh-4927 Ref gh-4642 Ref gh-4724
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.
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.createElementway 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
firstChildwhichcompresses 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.
Ref gh-4409
Ref angular/angular.js#17028
Checklist
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com