Skip to content

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

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

Conversation

IgorMinar
Copy link
Contributor

cherry-pick of #36578

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@IgorMinar IgorMinar added cla: yes action: merge The PR is ready for merge by the caretaker PR target: patch-only and removed cla: no labels Jun 26, 2020
@ngbot
Copy link

ngbot bot commented Jun 26, 2020

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "cla/google" is failing
    pending status "ci/circleci: setup" is pending
    pending status "google3" is pending
    pending status "pullapprove" is pending
    pending missing required status "ci/circleci: build"
    pending missing required status "ci/circleci: lint"
    pending missing required status "ci/circleci: publish_snapshot"
    pending missing required status "ci/angular: size"
    pending 4 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@IgorMinar IgorMinar requested a review from AndrewKushnir June 26, 2020 22:19
@IgorMinar IgorMinar added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jun 26, 2020
@IgorMinar
Copy link
Contributor Author

IgorMinar commented Jun 26, 2020

merge-assistance: global-self-approval since this is a cherry-pick

@mary-poppins
Copy link

You can preview ab12b6a at https://pr37783-ab12b6a.ngbuilds.io/.

@IgorMinar IgorMinar added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 26, 2020
peruukki added 3 commits June 26, 2020 17:04
…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.
…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.
…#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.
@IgorMinar IgorMinar force-pushed the sanitize-inert-remove-inline-style-10.0.x branch from ab12b6a to c35c07b Compare June 27, 2020 00:04
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@mary-poppins
Copy link

You can preview c35c07b at https://pr37783-c35c07b.ngbuilds.io/.

…troy (angular#37543)

Invoking a callback registered through `ViewRef.onDestroy` throws an error, because we weren't registering it correctly in the internal data structure. These changes also remove the `storeCleanupFn` function, because it was mostly identical to `storeCleanupWithContext` and was only used in one place.

Fixes angular#36213.

PR Close angular#37543
@IgorMinar IgorMinar force-pushed the sanitize-inert-remove-inline-style-10.0.x branch from c35c07b to f82c0f9 Compare June 27, 2020 00:48
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@IgorMinar IgorMinar added cla: yes and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: no labels Jun 27, 2020
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@mary-poppins
Copy link

You can preview f82c0f9 at https://pr37783-f82c0f9.ngbuilds.io/.

AndrewKushnir pushed a commit that referenced this pull request Jun 29, 2020
) (#37783)

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.

PR Close #37783
AndrewKushnir pushed a commit that referenced this pull request Jun 29, 2020
…37783)

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 #37783
AndrewKushnir pushed a commit that referenced this pull request Jun 29, 2020
…#37783)

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 #37783
AndrewKushnir pushed a commit that referenced this pull request Jun 29, 2020
…troy (#37543) (#37783)

Invoking a callback registered through `ViewRef.onDestroy` throws an error, because we weren't registering it correctly in the internal data structure. These changes also remove the `storeCleanupFn` function, because it was mostly identical to `storeCleanupWithContext` and was only used in one place.

Fixes #36213.

PR Close #37543

PR Close #37783
@AndrewKushnir
Copy link
Contributor

FYI, this PR is now merged to the patch branch. Thank you.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants