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

fix(core): fix possible XSS attack in development through SSR #40525

Closed
wants to merge 1 commit into from

Conversation

mhevery
Copy link
Contributor

@mhevery mhevery commented Jan 22, 2021

This is a follow up fix for 894286d.

It turns out that comments can be closed in several ways:

  • <!-->
  • <!-- -->
  • <!-- --!>

All of the above are valid ways to close comment per:
https://html.spec.whatwg.org/multipage/syntax.html#comments

The new fix surrounds < and > with zero width space so that it
renders in the same way, but it prevents the comment to be closed eagerly.

PR Checklist

Please check if your PR fulfills the following requirements:

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?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla google-cla bot added the cla: yes label Jan 22, 2021
@mhevery mhevery added the area: security Issues related to built-in security features, such as HTML sanitation label Jan 22, 2021
@ngbot ngbot bot added this to the Backlog milestone Jan 22, 2021
@mhevery mhevery added area: core Issues related to the framework runtime action: merge The PR is ready for merge by the caretaker labels Jan 22, 2021
@mhevery
Copy link
Contributor Author

mhevery commented Jan 22, 2021

presubmit

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with a few nits. Can you please rework the tests to be more clear and have better coverage? thanks

packages/core/test/acceptance/security_spec.ts Outdated Show resolved Hide resolved
packages/core/src/util/dom.ts Outdated Show resolved Hide resolved
@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 Jan 22, 2021
@IgorMinar
Copy link
Contributor

and the CI seems unhappy, PTAL

@mhevery mhevery removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 22, 2021
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the updates!

@mhevery mhevery added the target: patch This PR is targeted for the next patch release label Jan 22, 2021
jessicajaniuk pushed a commit that referenced this pull request Jan 22, 2021
This is a follow up fix for
894286d.

It turns out that comments can be closed in several ways:
- `<!-->`
- `<!-- -->`
- `<!-- --!>`

All of the above are valid ways to close comment per:
https://html.spec.whatwg.org/multipage/syntax.html#comments

The new fix surrounds `<` and `>` with zero width space so that it
renders in the same way, but it prevents the comment to be closed eagerly.

PR Close #40525
jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this pull request Jan 23, 2021
…angular#40525)"

This reverts commit bb3b315.

Reason for Revert: Issues with Google3 TAP Failures
jessicajaniuk pushed a commit that referenced this pull request Jan 23, 2021
…#40525)" (#40533)

This reverts commit bb3b315.

Reason for Revert: Issues with Google3 TAP Failures

PR Close #40533
jessicajaniuk pushed a commit that referenced this pull request Jan 23, 2021
…#40525)" (#40533)

This reverts commit bb3b315.

Reason for Revert: Issues with Google3 TAP Failures

PR Close #40533
@jessicajaniuk
Copy link
Contributor

We had to rollback this due to legitimate failing targets in google3. Please take a look.

@jessicajaniuk jessicajaniuk reopened this Jan 23, 2021
@AndrewKushnir AndrewKushnir added state: blocked and removed action: merge The PR is ready for merge by the caretaker labels Jan 23, 2021
packages/core/src/util/dom.ts Outdated Show resolved Hide resolved
packages/core/src/util/dom.ts Outdated Show resolved Hide resolved
@mhevery
Copy link
Contributor Author

mhevery commented Jan 25, 2021

presubmit

This is a follow up fix for
angular@894286d.

It turns out that comments can be closed in several ways:
- `<!-->`
- `<!-- -->`
- `<!-- --!>`

All of the above are valid ways to close comment per:
https://html.spec.whatwg.org/multipage/syntax.html#comments

The new fix surrounds `<` and `>` with zero width space so that it
renders in the same way, but it prevents the comment to be closed eagerly.
@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Jan 26, 2021
jessicajaniuk pushed a commit that referenced this pull request Jan 26, 2021
This is a follow up fix for
894286d.

It turns out that comments can be closed in several ways:
- `<!-->`
- `<!-- -->`
- `<!-- --!>`

All of the above are valid ways to close comment per:
https://html.spec.whatwg.org/multipage/syntax.html#comments

The new fix surrounds `<` and `>` with zero width space so that it
renders in the same way, but it prevents the comment to be closed eagerly.

PR Close #40525
@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 Feb 26, 2021
mhevery added a commit that referenced this pull request Mar 18, 2021
This is a follow up fix for
894286d.

It turns out that comments can be closed in several ways:
- `<!-->`
- `<!-- -->`
- `<!-- --!>`

All of the above are valid ways to close comment per:
https://html.spec.whatwg.org/multipage/syntax.html#comments

The new fix surrounds `<` and `>` with zero width space so that it
renders in the same way, but it prevents the comment to be closed eagerly.

PR Close #40525
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 area: core Issues related to the framework runtime area: security Issues related to built-in security features, such as HTML sanitation cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants