Skip to content

fix(core): block creation of sensitive URI attributes from ICU messages#67183

Merged
thePunderWoman merged 1 commit intoangular:mainfrom
dgp1130:sanitize-i18n
Feb 24, 2026
Merged

fix(core): block creation of sensitive URI attributes from ICU messages#67183
thePunderWoman merged 1 commit intoangular:mainfrom
dgp1130:sanitize-i18n

Conversation

@dgp1130
Copy link
Contributor

@dgp1130 dgp1130 commented Feb 20, 2026

Translators are not allowed to write HTML which creates URI attributes. I opted to ban any values going into an attribute at all, to prevent even links to malicious content, rather than just sanitizing URIs.

I also converted this blocklist into an allowlist. Now, we only allowing setting known attributes (while sanitizing URI attributes). This significantly reduces risk of missing a vulnerable attribute and does not require an exhaustive list of all potential attributes.

BREAKING CHANGE: Angular now only applies known attributes from HTML in translated ICU content. Unknown attributes are dropped and not rendered.

See: b/483947315

/cc @AndrewKushnir @securityMB @aaronshim

@dgp1130 dgp1130 requested a review from alan-agius4 February 20, 2026 22:09
@dgp1130 dgp1130 added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Feb 20, 2026
@pullapprove pullapprove bot requested a review from atscott February 20, 2026 22:09
@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Feb 20, 2026
@ngbot ngbot bot added this to the Backlog milestone Feb 20, 2026
@angular-robot angular-robot bot added the detected: breaking change PR contains a commit with a breaking change label Feb 20, 2026
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix @dgp1130 👍

Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM, just a NIT

Translators are not allowed to write HTML which creates URI attributes. I opted to ban any values going into an attribute at all, to prevent even links to malicious content, rather than just sanitizing URIs.

I also converted this blocklist into an allowlist. Now, we only allowing setting known attributes (while sanitizing URI attributes). This significantly reduces risk of missing a vulnerable attribute and does not require an exhaustive list of all potential attributes.

BREAKING CHANGE: Angular now only applies known attributes from HTML in translated ICU content. Unknown attributes are dropped and not rendered.
@dgp1130 dgp1130 removed the request for review from atscott February 23, 2026 18:38
@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Feb 23, 2026
@JeanMeche JeanMeche added the action: merge The PR is ready for merge by the caretaker label Feb 23, 2026
@thePunderWoman
Copy link
Contributor

@dgp1130 This has the breaking changes label, which means it can only land in target: major.

@thePunderWoman thePunderWoman merged commit 306f367 into angular:main Feb 24, 2026
20 checks passed
@thePunderWoman
Copy link
Contributor

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

@dgp1130 dgp1130 deleted the sanitize-i18n branch February 24, 2026 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime detected: breaking change PR contains a commit with a breaking change target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants