8000 Attributes: Don't stringify values as it breaks Trusted Types · Issue #4948 · jquery/jquery · GitHub
[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

Attributes: Don't stringify values as it breaks Trusted Types #4948

Closed
mgol opened this issue Oct 7, 2021 · 7 comments · Fixed by #4949
Closed

Attributes: Don't stringify values as it breaks Trusted Types #4948

mgol opened this issue Oct 7, 2021 · 7 comments · Fixed by #4949
Assignees
@mgol
Labels
Attributes Has Pull Request
Milestone
4.0.0

Comments

@mgol
Copy link
Member
mgol commented Oct 7, 2021 •
edited
Loading

Originally posted by @koto in #4409 (comment)

I noticed one violation still in jQuery.attr, where the value of the attribute is stringified before calling setAttribute:

const policy = trustedTypes.createPolicy('a', {createScriptURL: s=>s})
jQuery(aScript, policy.createScriptURL('foo'));

Uncaught TypeError: Failed to execute 'setAttribute' on 'Element': This document requires 'TrustedScriptURL' assignment.
    at attr (jquery.js:6997)
    at access (jquery.js:3467)
    at jQuery.fn.init.attr (jquery.js:6955)
    at <anonymous>:1:6

There is a workaround via .attr hooks, but it might be worth addressing nonetheless.

It seems like this was introduced in ff75767, I suspect to workaround an IE <= 9 bug, which incorrectly stringified objects passed to setAttribute.

I'm not sure what the most elegant solution would be here, I guess it depends on whether jQuery 4 aims to support IE9. If not, it's safe not to stringify values (browser API would). If yes, then there's only a less-than-ideal option of testing for the bug? IIRC this would be a good test:

with (document.createElement('div')) {
  setAttribute('title', {toString:()=>''}); 
  getAttribute('title') === ''
}
@mgol mgol mentioned this issue Oct 7, 2021
Closed
@mgol
Copy link
Member Author
mgol commented Oct 7, 2021

jQuery 4.x will only support IE 11 so if this was just to please IE <=9 we should be good to remove the manual stringification.

@mgol mgol added this to the 4.0.0 milestone Oct 7, 2021
@mgol mgol self-assigned this Oct 7, 2021
@mgol mgol added the Attributes label Oct 7, 2021
mgol added a commit to mgol/jquery that referenced this issue Oct 7, 2021
@mgol
Attributes: Don't stringify attributes in the setter
5965d0b 
Stringifying attributes in the setter was needed for IE <=9 but it breaks
trusted types enforcement when setting a script `src` attribute.

Fixes jquerygh-4948
@mgol mgol mentioned this issue Oct 7, 2021
Merged
3 tasks
@mgol
Copy link
Member Author
mgol commented Oct 7, 2021

A draft PR: #4949. Setting the attribute now works but the script will still be blocked. This is because in domManip scripts are not inserted directly but instead first disabled and then their src attributes are read and inserted in fresh scripts.

There's probably not much we can do when the scripts are deep inside of the inserted HTML string - natively scripts would not fire then but jQuery does execute them which will not work here. However, we could at least make .append(scriptElem) work by forking the code path and treating such top-level scripts independently.

I think we could land #4949 as-is, just without the part where it checks if the script was actually invoked and then we can tackle the remaining issue separately.

@koto
Copy link
koto commented Oct 8, 2021

I agree. In general the behavior where jQuery takes the script nodes from the HTML string to add them separately feels like an antipattern nowadays (for example, this used to be a CSP strict-dynamic bypass vector), but I understand how it was useful back when the API was created (heck, I used it myself :) ).

In my personal opinion .append(scriptElem) could be a good workaround, and domManip could stop special-casing script nodes, but there are backward compatibility issues that I don't know the full impact of.

@mgol
Copy link
Member Author
mgol commented Oct 8, 2021

Yeah, I'd also love to take out all that script execution stuff, at least as a default, but this may be too big a compatibility break. We'll think if anything can be done to that more generic case.

TBH it's quite weird that Web APIs don't behave like jQuery here natively; it was surprising to me when I first learnt appending script elements directly executes them but not when done via an HTML string or if they're not the top-level element.

mgol added a commit to mgol/jquery that referenced this issue Oct 29, 2021
@mgol
Attributes: Don't stringify attributes in the setter
520f23a 
Stringifying attributes in the setter was needed for IE <=9 but it breaks
trusted types enforcement when setting a script `src` attribute.

Fixes jquerygh-4948
mgol added a commit to mgol/jquery that referenced this issue Oct 29, 2021
@mgol
Attributes: Don't stringify attributes in the setter
8497cd0 
Stringifying attributes in the setter was needed for IE <=9 but it breaks
trusted types enforcement when setting a script `src` attribute.

Note that this doesn't mean script execution works. Since jQuery disables all
scripts by changing their type and then executes them by creating fresh script
tags with proper `src` & possibly other attributes, this unwraps any trusted
`src` wrappers, making the script not execute under strict CSP settings.
We might try to fix it in the future in a separate change.

Fixes jquerygh-4948
mgol added a commit to mgol/jquery that referenced this issue Oct 31, 2021
@mgol
Attributes: Don't stringify attributes in the setter
2773427 
Stringifying attributes in the setter was needed for IE <=9 but it breaks
trusted types enforcement when setting a script `src` attribute.

Note that this doesn't mean script execution works. Since jQuery disables all
scripts by changing their type and then executes them by creating fresh script
tags with proper `src` & possibly other attributes, this unwraps any trusted
`src` wrappers, making the script not execute under strict CSP settings.
We might try to fix it in the future in a separate change.

Fixes jquerygh-4948
@mgol mgol closed this as completed in #4949 Nov 1, 2021
mgol added a commit that referenced this issue Nov 1, 2021
@mgol
Attributes: Don't stringify attributes in the setter
4250b62 
Stringifying attributes in the setter was needed for IE <=9 but it breaks
trusted types enforcement when setting a script `src` attribute.

Note that this doesn't mean script execution works. Since jQuery disables all
scripts by changing their type and then executes them by creating fresh script
tags with proper `src` & possibly other attributes, this unwraps any trusted
`src` wrappers, making the script not execute under strict CSP settings.
We might try to fix it in the future in a separate change.

Fixes gh-4948
Closes gh-4949
@mgol mgol changed the title jQuery.attr: Don't stringify values as it breaks Trusted Types Attributes: Don't stringify values as it breaks Trusted Types Nov 1, 2021
@mgol mgol mentioned this issue Nov 1, 2021
Open
@mgol
Copy link
Member Author
mgol commented Nov 1, 2021

The fix has landed; I extracted the script execution issue to #4963.

@mgol mgol mentioned this issue Oct 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mgol mgol

Labels
Attributes Has Pull Request
Milestone
4.0.0
Development

Successfully merging a pull request may close this issue.

Attributes: Don't stringify attributes in the setter mgol/jquery
3 participants
@koto @mgol and others
0