Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
mgol merged 3 commits intojquery:masterfrommgol:secure-buildFragment
Jun 10, 2020

Conversation

mgol
Copy link
Member

@mgolmgol commentedMay 31, 2020
edited
Loading

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 usingdocument.createElement &appendChild.

The previous way was needed in jQuery <4 because IE <10 doesn't accept table
parts set viainnerHTML, even if the element which contents are set is
a proper table element, e.g.:

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
thedocument.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 uselastChild.
Since all wrappers are single-element now, we can usefirstChild 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.

Refgh-4409
Refangular/angular.js#17028

Checklist

@mgolmgol added this to the4.0.0 milestoneMay 31, 2020
@mgolmgol requested a review fromtimmywilMay 31, 2020 21:57
@mgolmgol self-assigned thisMay 31, 2020
@mgolmgolforce-pushed thesecure-buildFragment branch fromb9588a7 to8a24861CompareMay 31, 2020 21:59
Copy link
Member

@dmethvindmethvin left a 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.

@mgolmgolforce-pushed thesecure-buildFragment branch 2 times, most recently from876c924 toda74354CompareJune 1, 2020 16:25
@mgolmgol requested a review fromgibson042June 1, 2020 16:32
@koto
Copy link

koto commentedJun 2, 2020

Looks great, thanks Michał!

@mgolmgolforce-pushed thesecure-buildFragment branch fromda74354 to872affdCompareJune 5, 2020 17:00
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
@mgolmgolforce-pushed thesecure-buildFragment branch from872affd tobb726fcCompareJune 6, 2020 20:52
@mgol
Copy link
MemberAuthor

mgol commentedJun 8, 2020

@gibson042 Nice catch with thedocument -> context switch, somehow our unit tests don't check for it. This is now at -58 bytes so we saved an additional 3, nice!

Please have another look.

Copy link
Member

@gibson042gibson042 left a comment

Choose a reason for hiding this comment

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

LGTM!

@mgolmgol merged commit9c98e4e intojquery:masterJun 10, 2020
@mgolmgol deleted the secure-buildFragment branchJune 10, 2020 14:13
mgol added a commit to mgol/jquery that referenced this pull requestSep 13, 2021
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
mgol added a commit to mgol/jquery that referenced this pull requestSep 14, 2021
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
mgol added a commit to mgol/jquery that referenced this pull requestSep 17, 2021
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
mgol added a commit to mgol/jquery that referenced this pull requestSep 23, 2021
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
mgol added a commit to mgol/jquery that referenced this pull requestSep 29, 2021
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
mgol added a commit to mgol/jquery that referenced this pull requestSep 29, 2021
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
mgol added a commit that referenced this pull requestSep 30, 2021
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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@dmethvindmethvindmethvin approved these changes

@timmywiltimmywiltimmywil approved these changes

@gibson042gibson042gibson042 approved these changes

Assignees

@mgolmgol

Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.

5 participants
@mgol@koto@dmethvin@timmywil@gibson042

[8]ページ先頭

©2009-2025 Movatter.jp