Navigation Menu

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.

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

Chrome 77: sanitize() returns TrustedHTML-Object instead of string #361

Closed
Uzlopak opened this issue Sep 11, 2019 · 32 comments
Closed

Chrome 77: sanitize() returns TrustedHTML-Object instead of string #361

Uzlopak opened this issue Sep 11, 2019 · 32 comments

Comments

@Uzlopak
Copy link

Uzlopak commented Sep 11, 2019

In our usecase Chrome77 breaks our code. The sanitize method returns TrustedHTML instead of a string. We fixed it by adding a .toString() to the result of sanitize().

Bug

Input

console.log(dompurify.sanitize("a"))

Given output

TrustedHTML-Object;

Expected output

"a"

@abduwaly
Copy link

Hi @Uzlopak !
We encountered with the same issue as well and found that .toString() will fix this issue.

dompurify.sanitize("a").toString()

@cure53
Copy link
Owner

cure53 commented Sep 11, 2019

Hey all, thanks for the report. This is intended behavior however when working with Trusted Types.

See here: https://github.com/cure53/DOMPurify#what-about-dompurify-and-trusted-types

@koto Maybe we should update the docs, is this on by default now?

@Uzlopak
Copy link
Author

Uzlopak commented Sep 11, 2019

@mayrbenjamin92
Copy link

mayrbenjamin92 commented Sep 11, 2019

I am still a bit confused about what has changed in Chrome 77? Did they change something related to the trusted API? Does anybody have a link to the changelog in Chrome so that we can see, why this was changed?

@cure53
Copy link
Owner

cure53 commented Sep 11, 2019

This is something @koto is likely going to be able to answer

@marcneander
Copy link

This change in API is risky. I fear a bunch of apps are now experiencing issues with this.

@abduwaly
Copy link

abduwaly commented Sep 11, 2019

Screen Shot 2019-09-11 at 8 56 10 PM

This is why it crashed on Chrome 77. It's an experimental API and it's been here from chrome 73 - 76 ( but needs users to manually enable it ). But from Chrome 77, it's Enabled by default.

see : MDN trusted-types

@mayrbenjamin92

cure53 added a commit that referenced this issue Sep 11, 2019
Added clear documentation about the C77 Trysuted Types issue
@cure53
Copy link
Owner

cure53 commented Sep 11, 2019

The README file has been updated to reflect the changes in Chrome's behavior/config.
A PR has been sent to DefinitelyTyped: DefinitelyTyped/DefinitelyTyped#38298

Hope that clarifies things a bit.

cure53 added a commit that referenced this issue Sep 11, 2019
Added Chrome 77 to tested browses
@mayrbenjamin92
Copy link

Thank you guys!

@koto
Copy link
Contributor

koto commented Sep 11, 2019

Trusted Types should not be enabled by default on Chrome 77. We're investigating what happened in https://bugs.chromium.org/p/chromium/issues/detail?id=1002685

@koto
Copy link
Contributor

koto commented Sep 11, 2019

That said you should really not alter the sanitize function output, Trusted Types only makes it clear. The API will ship in one of future releases, there will be a proper Intent To Ship, so there's still time to update the code.

@trymoto
Copy link

trymoto commented Sep 11, 2019

In May i saw #316
because our website broke under experimental flag

  • "Oh, well, there will be major version and all of this good stuff before this whole TrustedHTML thingie ships to stable" :)
    Today we saw it breaking a lot of production though.

IMHO it's a broken versioning and we're left with:

  1. Wait for Chrome folks to fix 77 and do nothing until it happens again
  2. Rely on people to hot-fix their own feature-detection / duck-typing / toString stuff in upcoming days when v77 adoption rate will go to the moon
  3. Do a minor bump and save people

What do you think?

@cure53
Copy link
Owner

cure53 commented Sep 12, 2019

@trymoto @koto I honestly understand both viewpoints here and the fact that things break in production is sub-optimal to say the least.

I am wondering what DOMPurify could do to fix things? Is there anything constructive we can do right now?

@Uzlopak
Copy link
Author

Uzlopak commented Sep 12, 2019

activating TrustedHTML by parameter instead of Feature detection would be a solution but you would need to release a new major as it is a breaking change.

But that would fix it perfectly.

@koto
Copy link
Contributor

koto commented Sep 12, 2019

Regardless of the Trusted Types angle, I think the change DOMPurify might want is for the value returned from sanitize to be immutable. I would think this should be the default, as otherwise users may attempt to process the data unsafely, and e.g. do string replacement on HTML, which leads to bugs. This is what breakages surfaced. But that's just my opinion.

This can be achieved by e.g. returning an object with a toString method instead of a string (or feature detected TrustedHTML which has the same behavior).

If needed this behavior could be turned off with RETURN_STRING config parameter or sth. Alternatively, the users might be told to use toString() with the caveat that this is risky pattern.

Now - this is a breaking change. So the plan might be to:

  • keep the default to RETURN_STRING and change the default to RETURN_IMMUTABLE in a next major release (if you're into semver)
  • Do it (i.e. change the default to RETURN_IMMUTABLE) soon with a major release with no transitional period.

@cure53
Copy link
Owner

cure53 commented Sep 12, 2019

Regardless of the Trusted Types angle, I think the change DOMPurify might want is for the value returned from sanitize to be immutable.

DOMPurify is not a Sheriff that controls what people do with the sanitized result. It's a sanitizer library that sanitizes - and it does so really well.

What people do with the result afterwards is their choice. Even if they do the seemingly wrong thing according to this or the other philosophy. So, the immutability thing, while sounding right when being looked at from a certain angle, is not what we will do. @koto

I fully see your point and support it - when looking at things from a pentester's angle. But I don't like it looking at things from a developer's angle. :)

activating TrustedHTML by parameter instead of Feature detection would be a solution but you would need to release a new major as it is a breaking change.

I like this idea much more. It gives people a choice. This means we would go 2.0.0 correct? Not a versioning expert, hence asking :D @Uzlopak

@Uzlopak
Copy link
Author

Uzlopak commented Sep 12, 2019

@cure53

Yes, according to semver you would increase the version to 2.0.0 because there could already people using TrustedHTML feature because you already documented it as the expected behaviour (and thus make their use of your product not wrong) and you would break the expected behaviour.

@cure53
Copy link
Owner

cure53 commented Sep 12, 2019

Cool, thanks :) So what would have to be done is:

  • Make the return type be configurable, i.e. via RETURN_TRUSTED_TYPE
  • In any other case return a string
  • Update README

Anything I have forgotten?

@Uzlopak
Copy link
Author

Uzlopak commented Sep 12, 2019

Just for clarification, because i dont know what RETURN_TRUSTED_TYPE means:
So the default behaviour would be that .sanitize() returns string and if you enable Trusted Types by a flag, you would get Trusted Type?

Two more possible points:

@cure53
Copy link
Owner

cure53 commented Sep 12, 2019

So the default behaviour would be that .sanitize() returns string and if you enable Trusted Types by a flag, you would get Trusted Type?

Yes

@Uzlopak
Copy link
Author

Uzlopak commented Sep 12, 2019

I like that.

@koto
Copy link
Contributor

koto commented Sep 12, 2019

I like that as well.

Re: breakages for existing code bases, this will likely not happen again, as we're renaming window.TrustedTypes to window.trustedTypes (so the existing DOMPurify 1.x will never feature detect TT, even if not upgraded) - the version that will actually launch will be `trustedTypes'.

@cure53
Copy link
Owner

cure53 commented Sep 12, 2019

@koto should we incorporate that change into the new code as well? i.e. test for both once the flag has been set to true?

I am working on it as we speak, hence asking. I don't want the library to run after Chrome with a bucket in its hand - so maybe better do it now than later :D

@koto
Copy link
Contributor

koto commented Sep 12, 2019

I'd say only check for window.trustedTypes. Ignore browsers and polyfills that used PascalCase, it's deprecated. If needed, clients in those environments can reenable this by doing window.TrustedTypes = window.trustedTypes on their own.

@cure53
Copy link
Owner

cure53 commented Sep 12, 2019

Gotcha, makes it hard to write tests though.

@cure53
Copy link
Owner

cure53 commented Sep 12, 2019

@koto Let me actually push the changes with window.TrustedTypes because I need to see what happens on BrowserStack.

After that, I would wait for your go to make the second change and use window.trustedTypes.

cure53 added a commit that referenced this issue Sep 12, 2019
Added first logic to enable RETURN_TRUSTED_TYPE config flag
cure53 added a commit that referenced this issue Sep 12, 2019
Added simple tests for new return type
cure53 added a commit that referenced this issue Sep 12, 2019
Adjusted the tests slightly to match Chrome >77 as well
cure53 added a commit that referenced this issue Sep 12, 2019
Changed README to reflect on the changes with Trusted Types
cure53 added a commit that referenced this issue Sep 12, 2019
Made some minimal README changes
cure53 added a commit that referenced this issue Sep 12, 2019
Finalized first new version with Trusted Types switch
cure53 added a commit that referenced this issue Sep 12, 2019
Tweaked the test suite runner to make sure we get correct results.
@cure53
Copy link
Owner

cure53 commented Sep 12, 2019

@koto Changes are in, now waiting for the BrowserStack results - also running Chrome 77 on BS now.

cure53 added a commit that referenced this issue Sep 12, 2019
Removed Chrome 77 again, not supoorted by BS yet
@cure53
Copy link
Owner

cure53 commented Sep 12, 2019

Okay, we should be good. CI is complaining but for different reasons :)

@Uzlopak
Copy link
Author

Uzlopak commented Sep 12, 2019

Can you provide the PR so that we can have a look over it, please :)?

@cure53
Copy link
Owner

cure53 commented Sep 13, 2019

All relevant commits are linked in this ticket, see above.

@koto
Copy link
Contributor

koto commented Sep 13, 2019

I think we might also need a bugfix release that removes the TT as a return value from 1.x.x? Since this was the one that introduced a breaking change. I don't see branches in your repo, would that be a manual code change & release?

@cure53
Copy link
Owner

cure53 commented Sep 13, 2019

No need for that, we will not start maintain different branches or bug-fix releases because of this rather small issue.

I would go as far as to close this issue for now. As far as I can see, the tests are green, the offending feature has been placed behind a flag and the library behaves again as expected.

@cure53 cure53 closed this as completed Sep 13, 2019
LeSuisse added a commit to Enalean/tuleap that referenced this issue Sep 13, 2019
This version is tagged as major due to a breaking change
in the API.
This upgrade is forced by an unexpected change in Chrome 77 [0]
(see implications for DOMPurify in [1]). Even if this is not
supposed to impact Tuleap, it can be hard to troubleshoot in
production since this is caused by a specific version of one
browser. It is best to upgrade now to avoid the potential pain.

[0] https://bugs.chromium.org/p/chromium/issues/detail?id=1002685
[1] cure53/DOMPurify#361

Change-Id: Ibc852c866d23824964ee8c0299ffb8457bf07b1b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants