fix(core): do not trigger CSP alert/report in Firefox and Chrome #36578

peruukki · 2020-04-11T13:56:26Z

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

The related unit tests in html_sanitizer_spec.ts, "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 #25214.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

If [innerHTML] is used in a component and the Content-Security-Policy does not allow inline styles, a CSP violation will be reported by at least Firefox and Chrome.

Issue Number: #25214

What is the new behavior?

No CSP violation is reported.

Does this PR introduce a breaking change?

Yes
No

Other information

Like mentioned above, there is an existing unit test related to this change that tests that badly-formatted HTML is sanitized properly, and I didn't change the test because 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.

This is my first contribution attempt for Angular. 😃

peruukki · 2020-04-11T16:01:47Z

The lint failures are in the file I changed but not in my changes, not sure if I should fix them.

koto · 2020-04-16T11:51:43Z

As explained in angular/angular.js#17013 (comment), please don't change style to span. Style is crucial for the payload there.

peruukki · 2020-04-17T06:39:19Z

Thanks for the info and help, I'll try to get the fix right in Angular.js first and then come back to update this.

peruukki · 2020-04-23T16:58:32Z

I haven't heard back from the Angular.js pull request but updated this one anyway with a similar fix.

The inertDocument member is still always initialized, which is unnecessary if DOMParser is used, but changing that would seem to require a fair bit of refactoring because we'd need some additional null/undefined checks wherever inertDocument is accessed; it would probably make sense to have separate classes for the two strategies. I'm not sure if such bigger changes would be worthwhile.

peruukki · 2020-04-23T19:44:22Z

Also, like I mentioned in angular/angular.js#17013 (comment), we might want to make the DOMParser availability check stricter and verify that the HTML support version of parseFromString works, if the check is run by default on all browsers.

peruukki · 2020-04-25T12:02:52Z

I refactored the two strategies to separate classes and made the DOMParser availability check more accurate.

Splaktar · 2020-05-19T23:31:54Z

CircleCI is indicating that this change makes the main-2015 bundle 500 bytes smaller.

It wants you to update the following file and push up those changes for that test to pass:

FAIL: Commit e48607b1e0767d70d51945b5a787d8362a5f26a7 uncompressed main-es2015 fell below expected size by 500 bytes or >1% (expected: 452060, actual: 451537).
If this is a desired change, please update the size limits in file '/home/circleci/ng/goldens/size-tracking/aio-payloads.json'.

Splaktar

LGTM

IgorMinar · 2020-06-22T20:49:20Z

lgtm now! thank you very much @peruukki!

We are currently in a code freeze mode so no PRs are being merged until v10.0.0 is cut later this week. I expect this PR to be merged as soon as the code freeze is lifted after the release.


        fix(core): do not trigger CSP alert/report in Firefox and Chrome (#36578

) If [innerHTML] is used in a component and a Content-Security-Policy is set that does not allow inline styles then Firefox and Chrome show the following message: > Content Security Policy: The page’s settings observed the loading of a resource at self (“default-src”). A CSP report is being sent. This message is caused because Angular is creating an inline style tag to test for a browser bug that we use to decide what sanitization strategy to use, which causes CSP violation errors if inline CSS is prohibited. This test is no longer necessary, since the `DOMParser` is now safe to use and the `style` based check is redundant. In this fix, we default to using `DOMParser` if it is available and fall back to `createHTMLDocument()` if needed. This is the approach used by DOMPurify too. The related unit tests in `html_sanitizer_spec.ts`, "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 #25214.


        refactor(core): split inert strategies to separate classes (#36578)

The `inertDocument` member is only needed when using the InertDocument strategy. By separating the DOMParser and InertDocument strategies into separate classes, we can easily avoid creating the inert document unnecessarily when using DOMParser.


        fix(core): determine required DOMParser feature availability (#36578)

Verify that HTML parsing is supported in addition to DOMParser existence. This maybe wasn't as important before when DOMParser was used just as a fallback on Firefox, but now that DOMParser is the default choice, we need to be more accurate.


        ci: decrease expected AIO and integration payload sizes (#36578)

The changes in #36687 removed enough code to exceed the CI check limits, so the expected main-es2015 size needs adjusting.

AndrewKushnir · 2020-06-26T00:41:08Z

Hi @@peruukki, FYI I rebased your PR on top of the latest master to make sure the payload size limits are still in the right range (since I merged a few more PRs). Once CI goes "green", this PR will be a part of the merge queue. Thank you.


        refactor(core): split inert strategies to separate classes (#36578) (#…

…36578) The `inertDocument` member is only needed when using the InertDocument strategy. By separating the DOMParser and InertDocument strategies into separate classes, we can easily avoid creating the inert document unnecessarily when using DOMParser. PR Close #36578


        fix(core): determine required DOMParser feature availability (#36578) (…

…#36578) Verify that HTML parsing is supported in addition to DOMParser existence. This maybe wasn't as important before when DOMParser was used just as a fallback on Firefox, but now that DOMParser is the default choice, we need to be more accurate. PR Close #36578


        ci: decrease expected AIO and integration payload sizes (#36578) (#36578

) The changes in #36687 removed enough code to exceed the CI check limits, so the expected main-es2015 size needs adjusting. PR Close #36578


        fix(core): do not trigger CSP alert/report in Firefox and Chrome (ang…

…ular#36578) If [innerHTML] is used in a component and a Content-Security-Policy is set that does not allow inline styles then Firefox and Chrome show the following message: > Content Security Policy: The page’s settings observed the loading of a resource at self (“default-src”). A CSP report is being sent. This message is caused because Angular is creating an inline style tag to test for a browser bug that we use to decide what sanitization strategy to use, which causes CSP violation errors if inline CSS is prohibited. This test is no longer necessary, since the `DOMParser` is now safe to use and the `style` based check is redundant. In this fix, we default to using `DOMParser` if it is available and fall back to `createHTMLDocument()` if needed. This is the approach used by DOMPurify too. The related unit tests in `html_sanitizer_spec.ts`, "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 angular#25214.


        refactor(core): split inert strategies to separate classes (angular#3…

…6578) The `inertDocument` member is only needed when using the InertDocument strategy. By separating the DOMParser and InertDocument strategies into separate classes, we can easily avoid creating the inert document unnecessarily when using DOMParser.


        fix(core): determine required DOMParser feature availability (angular…

…#36578) Verify that HTML parsing is supported in addition to DOMParser existence. This maybe wasn't as important before when DOMParser was used just as a fallback on Firefox, but now that DOMParser is the default choice, we need to be more accurate.


        ci: decrease expected AIO and integration payload sizes (angular#36578)

The changes in angular#36687 removed enough code to exceed the CI check limits, so the expected main-es2015 size needs adjusting.


        ci: increase payload size limit for integration tests

This commit updates the payload size limit for the `hello_world` test app built using Closure. This is likely an effect of the changes in angular#36578 (that reduces the bundle size for most of the apps) and additional changes in subsequent commits.


        ci: decrease payload size limit for integration tests

This commit updates the payload size limit for the `hello_world` test app built using Closure. This is likely an effect of the changes in angular#36578 (that reduces the bundle size for most of the apps) and additional changes in subsequent commits.


        ci: decrease payload size limit for integration tests (#37784)

This commit updates the payload size limit for the `hello_world` test app built using Closure. This is likely an effect of the changes in #36578 (that reduces the bundle size for most of the apps) and additional changes in subsequent commits. PR Close #37784


        fix(core): do not trigger CSP alert/report in Firefox and Chrome (ang…

…ular#36578) If [innerHTML] is used in a component and a Content-Security-Policy is set that does not allow inline styles then Firefox and Chrome show the following message: > Content Security Policy: The page’s settings observed the loading of a resource at self (“default-src”). A CSP report is being sent. This message is caused because Angular is creating an inline style tag to test for a browser bug that we use to decide what sanitization strategy to use, which causes CSP violation errors if inline CSS is prohibited. This test is no longer necessary, since the `DOMParser` is now safe to use and the `style` based check is redundant. In this fix, we default to using `DOMParser` if it is available and fall back to `createHTMLDocument()` if needed. This is the approach used by DOMPurify too. The related unit tests in `html_sanitizer_spec.ts`, "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 angular#25214.


        refactor(core): split inert strategies to separate classes (angular#3…

…6578) The `inertDocument` member is only needed when using the InertDocument strategy. By separating the DOMParser and InertDocument strategies into separate classes, we can easily avoid creating the inert document unnecessarily when using DOMParser.


        fix(core): determine required DOMParser feature availability (angular…

…#36578) Verify that HTML parsing is supported in addition to DOMParser existence. This maybe wasn't as important before when DOMParser was used just as a fallback on Firefox, but now that DOMParser is the default choice, we need to be more accurate.


        ci: decrease expected AIO and integration payload sizes (angular#36578)

The changes in angular#36687 removed enough code to exceed the CI check limits, so the expected main-es2015 size needs adjusting.

IgorMinar · 2020-06-27T00:54:58Z

I cherry-picked this change in the 10.0.x branch as #37783

This comment was marked as resolved.

Sign in to view

googlebot added the cla: no label Apr 11, 2020

pullapprove bot requested review from IgorMinar and kara Apr 11, 2020

This comment was marked as resolved.

Sign in to view

googlebot added cla: yes and removed cla: no labels Apr 11, 2020

peruukki force-pushed the peruukki:sanitize-inert-remove-inline-style branch 2 times, most recently from 45cb61f to e2c69c2 Apr 11, 2020

atscott added the comp: core label Apr 13, 2020

ngbot bot added this to the needsTriage milestone Apr 13, 2020

peruukki mentioned this pull request Apr 14, 2020

fix($sanitize): do not trigger CSP alert/report in Firefox and Chrome angular/angular.js#17013

Closed

2 of 3 tasks complete

peruukki force-pushed the peruukki:sanitize-inert-remove-inline-style branch from e2c69c2 to 0d02499 Apr 23, 2020

peruukki changed the title ~~fix(core): remove inline style from sanitization Firefox bug detection~~ fix(core): remove browser bug detections from inert strategy selection Apr 23, 2020

peruukki force-pushed the peruukki:sanitize-inert-remove-inline-style branch from 0d02499 to 5ead6d3 Apr 25, 2020

pullapprove bot requested a review from AndrewKushnir Apr 25, 2020

peruukki force-pushed the peruukki:sanitize-inert-remove-inline-style branch from 5ead6d3 to e48607b Apr 25, 2020

pkozlowski-opensource added the comp: security label May 11, 2020

Splaktar approved these changes May 19, 2020

View changes

peruukki force-pushed the peruukki:sanitize-inert-remove-inline-style branch 2 times, most recently from 9615b65 to f00ca94 May 21, 2020

Splaktar added the core: sanitization label Jun 1, 2020

IgorMinar removed the PR action: cleanup label Jun 22, 2020

AndrewKushnir added the PR action: rerun CI at HEAD label Jun 25, 2020

ngbot bot removed the PR action: rerun CI at HEAD label Jun 25, 2020

peruukki added 4 commits Apr 11, 2020

ci: decrease expected AIO and integration payload sizes (#36578)

Loading status checks…

2c8dfaa

The changes in #36687 removed enough code to exceed the CI check limits, so the expected main-es2015 size needs adjusting.

AndrewKushnir force-pushed the peruukki:sanitize-inert-remove-inline-style branch from 4287f92 to 2c8dfaa Jun 26, 2020

AndrewKushnir added PR target: master-only and removed PR target: master & patch labels Jun 26, 2020

AndrewKushnir closed this in b950d46 Jun 26, 2020

IgorMinar mentioned this pull request Jun 26, 2020

10.0.x cp of: fix(core): do not trigger CSP alert/report in Firefox and Chrome #36578 #37783

Open

AndrewKushnir mentioned this pull request Jun 26, 2020

ci: decrease payload size limit for integration tests #37784

Closed

2 of 3 tasks complete

May	JUN	Jul
	27
2019	2020	2021

angular / angular

fix(core): do not trigger CSP alert/report in Firefox and Chrome #36578

fix(core): do not trigger CSP alert/report in Firefox and Chrome #36578

peruukki commented Apr 11, 2020 •

edited

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

peruukki commented Apr 11, 2020

koto commented Apr 16, 2020

peruukki commented Apr 17, 2020

peruukki commented Apr 23, 2020 •

edited

peruukki commented Apr 23, 2020 •

edited

peruukki commented Apr 25, 2020

Splaktar commented May 19, 2020

Splaktar left a comment

IgorMinar commented Jun 22, 2020

AndrewKushnir commented Jun 26, 2020

IgorMinar commented Jun 27, 2020

angular / angular

Join GitHub today

fix(core): do not trigger CSP alert/report in Firefox and Chrome #36578

fix(core): do not trigger CSP alert/report in Firefox and Chrome #36578

Conversation

peruukki commented Apr 11, 2020 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

peruukki commented Apr 11, 2020

koto commented Apr 16, 2020

peruukki commented Apr 17, 2020

peruukki commented Apr 23, 2020 • edited

peruukki commented Apr 23, 2020 • edited

peruukki commented Apr 25, 2020

Splaktar commented May 19, 2020

Splaktar left a comment

IgorMinar commented Jun 22, 2020

AndrewKushnir commented Jun 26, 2020

IgorMinar commented Jun 27, 2020

peruukki commented Apr 11, 2020 •

edited

peruukki commented Apr 23, 2020 •

edited

peruukki commented Apr 23, 2020 •

edited