Skip to content

fix(core): escape overlapping comment delimiters in escapeCommentText#69343

Merged
thePunderWoman merged 1 commit into
angular:mainfrom
rootvector2:comment-text-overlap-escape
Jun 17, 2026
Merged

fix(core): escape overlapping comment delimiters in escapeCommentText#69343
thePunderWoman merged 1 commit into
angular:mainfrom
rootvector2:comment-text-overlap-escape

Conversation

@rootvector2

@rootvector2 rootvector2 commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

PR Checklist

PR Type

  • 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.dev application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

escapeCommentText matches COMMENT_DISALLOWED globally, and global matches never overlap. when two delimiter sequences share characters the second is skipped, so escapeCommentText('<!-->') only escapes the leading <!-- and returns text that still contains a live --> (same for <!--!> and any value embedding such a sequence). the helper exists to keep programmatically created comment nodes from closing early, which its own docstring calls out as an XSS vector, so that surviving --> lets a value like <!--><img onerror=...> close the comment and run as markup.

found while auditing the comment/dom escaping helpers.

What is the new behavior?

the existing replacement is re-run until the text stabilizes, so overlapping delimiters are all neutralized. non-overlapping inputs are byte-for-byte unchanged (existing specs still pass).

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

added regression tests in dom_spec.ts for the overlapping cases.

@pullapprove
pullapprove Bot requested a review from crisbeto June 13, 2026 17:25
@angular-robot angular-robot Bot added the area: core Issues related to the framework runtime label Jun 13, 2026
@ngbot ngbot Bot added this to the Backlog milestone Jun 13, 2026
@JeanMeche

JeanMeche commented Jun 15, 2026

Copy link
Copy Markdown
Member

Before we get such a fix in, it would be interesting to understand how this could be used as a attack vector and that vector be covered by a test.

@JeanMeche
JeanMeche requested review from JeanMeche and alan-agius4 and removed request for crisbeto June 15, 2026 12:51
@alan-agius4

alan-agius4 commented Jun 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This is only a dev-mode thing so this is not considered a vulnerability, but it's a good thing to do.

https://b2607f8b04800100000680fe9ac10f44e0fa2000000000000000001.proxy.googlers.com/findings?cns_dir=cns__jj-d__home__codemender-collaborators__terjanq__ttl=1y__jetski_validate_experiments__259993459#q=packages%2Fcore%2Fsrc%2Futil%2Fdom.ts;id=ffb23685-90af-4780-9952-01999846808f-61e0d3

alan-agius4
alan-agius4 requested changes Jun 15, 2026
View reviewed changes

@alan-agius4 alan-agius4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of doing a loop the regular expression, the ^ anchor should be removed.

@alan-agius4 alan-agius4 added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Jun 15, 2026
@rootvector2
fix(core): escape overlapping comment delimiters in escapeCommentText
0eb2c24 
`COMMENT_DISALLOWED` is matched globally, so overlapping delimiter
sequences are skipped: `<!-->` only escapes the leading `<!--` and
leaves a live `-->` that can close a programmatically created comment
node early. Drop the `^` anchors so a standalone `>`/`->` is escaped
wherever it appears, which neutralizes the trailing delimiter left
behind by an earlier match.
@rootvector2
rootvector2 force-pushed the comment-text-overlap-escape branch from 7ebdeea to 0eb2c24 Compare June 15, 2026 15:30
@rootvector2

rootvector2 commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

dropped the loop and removed the ^ anchors instead, so a lone > or -> gets escaped wherever it sits. that catches the trailing > left over after <!-- matches in <!-->, which is the case the loop was working around. only spec churn is the two .>/.-> cases that now get escaped.

on the vector: escapeCommentText runs on values written into programmatically created comment nodes (the SSR path called out in the docstring). with the old regex escapeCommentText('<!-->') keeps a live -->, so <!--><img src=x onerror=...> serializes to a comment that closes early and the <img> lands as live markup. added a test that round-trips a comment node through innerHTML and checks the payload stays a single comment node with nothing parsed out of it.

and agreed, this is dev-mode only and not a real vuln, just tightening the helper.

@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 15, 2026
alan-agius4
alan-agius4 approved these changes Jun 15, 2026
View reviewed changes

@alan-agius4 alan-agius4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@JeanMeche JeanMeche added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 17, 2026
@thePunderWoman
thePunderWoman merged commit ea1a3ed into angular:main Jun 17, 2026
26 checks passed
@thePunderWoman

thePunderWoman commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

This PR was merged into the repository. The changes were merged into the following branches:

@thePunderWoman

thePunderWoman commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

@rootvector2 Looks like we have to revert this due to several broken targets inside G3. The revert is here: #69426

@rootvector2

rootvector2 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

makes sense. the breakage is from dropping the ^ anchors: a lone > or -> now gets escaped anywhere in comment text, not just at a delimiter boundary, so any comment that contains a > changes output and trips the snapshot/golden targets in g3.

the overlap can be closed without that churn by keeping the anchored regex and re-running the replacement until it stabilizes. non-overlapping text stays byte-for-byte identical and only <!--> / <!--!> get touched. happy to send that as a follow-up if you want it.

@JeanMeche JeanMeche mentioned this pull request Jul 17, 2026
Open
14 tasks
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Jul 19, 2026

Copy link
Copy Markdown

This pull request 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 19, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JeanMeche JeanMeche JeanMeche approved these changes
@alan-agius4 alan-agius4 alan-agius4 approved these changes

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

4 participants

@rootvector2 @JeanMeche @alan-agius4 @thePunderWoman