fix($sanitize): do not trigger CSP alert/report in Firefox and Chrome

[peruukki]

Default to using DOMParser if it is available and fall back to createHTMLDocument if needed. This is the approach suggested in the related pull request #17013 and used by DOMPurify too. It also safely avoids using an inline style tag that causes CSP violation errors if inline CSS is prohibited.

The related unit tests in sanitizeSpec.js, "should not allow JavaScript execution when creating inert document" and "should not allow JavaScript hidden in badly formed HTML to get through sanitization (Firefox bug)", are left untouched to assert that the behavior hasn't changed in those scenarios.

Fixes #16463.

AngularJS is in LTS mode

We are no longer accepting changes that are not critical bug fixes into this project.
See https://blog.angular.io/stable-angularjs-and-long-term-support-7e077635ee9c for more detail.

Does this PR fix a regression since 1.7.0, a security flaw, or a problem caused by a new browser version?
This fixes a regression that came with commit 7673ca7 that based on its tags appeared in version 1.7.0. It's not clear to me if it's considered a security flaw but it does hinder setting a strict Content-Security-Policy that doesn't allow inline styles.

What is the current behavior? (You can also link to an open issue here)
If ngSanitize is added as a module dependency and the Content-Security-Policy does not allow inline styles, a CSP violation will be reported by at least Firefox and Chrome.

Issue #16463.

What is the new behavior (if this is a feature change)?
No CSP violation is reported.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Fix/Feature: Docs have been added/updated
• Fix/Feature: Tests have been added; existing tests pass

Other information:
Like mentioned above, there are existing unit tests related to this change that test that badly-formatted HTML is sanitized properly, and the behavior shouldn't have changed on that regard. I don't think there is a convenient way to test CSP violations, so I didn't write any new tests.

I submitted the same fix to Angular in angular/angular#36578, it is still being processed.

[koto]

The change from <style> to <span> has security consequences. The original payload (similar to what DOMPurify used) was tailored to detect the bug in Firefox HTML parser used in innerHTML. The actual check done after payload processing is kind of wrong (the fact whether img is under svg is irrelevant, what is important is whether any element exists with an onerror attribute), but incidentally - the modified payload will hinder the check even if it was right. style is the symptom that triggers the FF bug, as the parsing context is different - changing to span breaks that - AngularJS would not be testing for the right thing.

A proper patch would be to default to DOMParser without testing. DOMPurify stopped testing for the FF (and Safari) bugs long time ago, and is defaulting to DOMParser for other reasons now.

[peruukki]

Thanks for the info @koto, this fix did seem suspiciously simple for an issue that had been waiting for 2 years to be resolved. 😄 So if I understood correctly, we should try to use DOMParser (getInertBodyElement_DOMParser) and fall back to createHTMLDocument (getInertBodyElement_InertDocument) if DOMParser is not available, and drop getInertBodyElement_XHR altogether. I'll see if I can achieve that.

[peruukki]

I made the changes, now DOMParser is used in modern browsers and the fallback was used at least on my Internet Explorer 11 virtual machine. I don't think there is still any practical way to write a test for this change as the changes are internal.

[peruukki]

One detail I noticed: the Angular repository already has an isDOMParserAvailable function that uses a simpler method for the detection, !!(window as any).DOMParser, whereas I actually test the DOMParser functionality using !!getInertBodyElement_DOMParser(''). I chose the latter because the parseFromString method with HTML support seems to be less supported than just the DOMParser property but I don't know if it makes any real-life difference and which one would be preferable.

[koto]

It sounds like a good choice. Thanks!

[Splaktar]

The test failures just appear to be caused by connection flakiness with Saucelabs. Can someone on the @angular/angular-1-contributors team restart Travis?

[Splaktar]

LGTM. I'm thankful for the great input from @koto and to @peruukki for posting this PR and getting the discussion started.

[petebacondarwin]

@koto could you give this your approval if you feel it is a safe change to make?

[koto]

This looks good, thanks Harri!

[petebacondarwin]

Thanks @peruukki - I'm changed the commit message to refer to the thing it is fixing rather than the detail of the implementation. Closed via 2fab3d4