fix(compiler): disallow i18n event attributes#68821
Conversation
alan-agius4
left a comment
There was a problem hiding this comment.
LGTM
Reviewed-for: fw-security
f980879 to
ca6e783
Compare
alan-agius4
left a comment
There was a problem hiding this comment.
Avoiding DOM schema validation here; a simple check for the on prefix is sufficient
ca6e783 to
d366587
Compare
Hexix23
left a comment
There was a problem hiding this comment.
Thanks Alan, updated to avoid DOM schema validation here and keep the direct on prefix check in the i18n metadata gate.
alan-agius4
left a comment
There was a problem hiding this comment.
LGTM
Reviewed-for: fw-security
|
Looks liek we need a rebase here. |
|
@Hexix23 please rebase. |
Reject translated event-handler attributes so localization cannot bypass Angular event-attribute validation.
d366587 to
c126e5f
Compare
|
Rebased onto latest main. |
josephperrott
left a comment
There was a problem hiding this comment.
LGTM
Reviewed-for: fw-security
|
Hello, It looks like this change introduces a breaking behavior, but it was released as part of a patch version. Could you please advise on the recommended migration path for cases where attributes contain From a consumer perspective, it is unclear how such attributes should now be translated without being incorrectly treated as events. Could you please confirm this was the intended approach, or consider applying a fix if this behavior was not intentional? Thank you. |
|
@Raisolution, with i18n any attribute even unbound ones can theoretically be exploited for XSS during i18n processing, that isn't the root cause here. In this specific scenario, the issue arises because the attribute is literally named #69306 should fix this particular case. |
|
@alan-agius4 thank you for the clarification. Yes, the issue is caused by the attribute name being interpreted as a potential event handler. This is the reason for raising the question - there are many valid translation attributes/messages where the name may start with Could you please advise what the recommended approach is for such valid i18n cases, should consumers rename all such attributes/messages? Edit: Thank you for the fix. It handles the concrete case with the word Thank you. |
|
@Raisolution, we could add a comprehensive list, but it would drastically increase the bundle size by almost 1kb (minified) since there are 73 handlers starting with the Can you provide examples of attributes starting with Generally, I'd recommend keeping attributes starting with |
|
@alan-agius4 thank you for the prompt reaction and for providing the fix. In the specific case, I do not currently have other concrete examples, the concern was more general, as there are valid words/messages starting with That said, the proposed fix addresses the reported scenario, thank you again for the quick turnaround. 🍻 |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Angular disallows binding to event-handler attributes such as
onclickandonerrorthroughvalidateAttribute()/validateProperty(), but the i18n metadata path still allowed the same attribute names to be marked for translation withi18n-on*.This creates an XSS sink in the i18n pipeline: a lower-trust translation file could replace a benign static handler such as
onerror="void 0"with executable JavaScript in the localized build.Affected versions / verification
I reproduced the vulnerable behavior in localized AOT builds against:
21.2.1121.2.1322.0.0-rc.0The #68591 hardening correctly covers schema-driven i18n security contexts such as URL, ResourceURL, iframe policy attributes, and SVG animation attributes. This event-handler case remained separate because
on*attributes are rejected by Angular's event-attribute validation path rather than bySECURITY_SCHEMA.What is the new behavior?
i18n-on*attributes are rejected during i18n metadata collection with the same security error shape used for other disallowed translated attributes.This prevents translated static attributes from bypassing Angular's event-attribute validation invariant.
The implementation intentionally performs the event-attribute check directly in the i18n metadata gate, alongside the existing Trusted Types sink check, so this path rejects translated executable event handlers before they are collected as translatable attribute metadata.
Does this PR introduce a breaking change?
Applications that intentionally translate static event-handler attributes such as
i18n-onerrorwill now fail compilation. This is intentional because translated event-handler attributes are executable JavaScript sinks.Other information
The change is intentionally limited to translated event-handler attributes. Normal non-security-sensitive translated attributes continue to work, and the existing schema-driven i18n sanitizer/validator paths continue to handle URL, ResourceURL, iframe policy, and SVG animation attributes.
Fix validation run locally:
bazelisk test //packages/core/test:testbazelisk test //packages/compiler/test:testpnpm ng-dev commit-message validate-range