fix(security): align SVG, i18n, and host-binding sinks with Angular v22 - #334
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2806b8621
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_759d00f2-30f8-4a36-a6bf-f592d036fdac) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_346b568d-16dc-4542-b4d4-c30da0af2790) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 626d83bc9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Issue #315 is still open on main, which vendors @angular/compiler v22.0.0. The previous branch pinned v21.2.7 and stripped namespaces before lookup. v22 keeps :svg: and :math: in the security key and strips script elements instead of treating script|src as a resource URL. - Register the v22 DOM security schema, including SVG animation attributes as ATTRIBUTE_NO_BINDING and namespaced MathML hrefs. - Compute host-binding security from the directive selector, including :not() and svg/math promotion of unknown element names. - Map ATTRIBUTE_NO_BINDING to ɵɵvalidateAttribute. - Reject i18n of Trusted Types sinks without stripping a namespace prefix. - Drop script and :svg:script while lowering templates, and look security up on the qualified element name.
626d83b to
d80213b
Compare
Brooooooklyn
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d80213b53a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
v22 stays the default. Compiling for an earlier version uses that version's security schema instead of the namespaced v22 keys. - Before 21.1: script|src only. - 21.1 through 21.2.6: script|href, MathML hrefs, attributeName no-binding. - 21.2.4+: i18n rejects iframe|src. - 21.2.7 through 21.x: bare animate/set to, from, and values. - 22+: namespaced keys, and :svg:script is stripped.
Trusted Types compares the parser's full element name and does not drop a non-svg/math prefix. Script stripping matches only `script` and `:svg:script`. Implicit i18n attributes that name a sink are rejected on extract and merge, same as an explicit i18n-* marker.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a55970ea14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`<MyComp:iframe i18n-src>` was matched as the class name. Trusted Types now uses the host tag, including an explicit namespace. A component with no host tag is not a sink. Real `<script>` and `<style>` elements are still stripped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b25c6409b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A selectorless component inside `<svg>` or `<math>` takes that namespace, the same way Angular builds `tagName`. `<svg><MyComp i18n-innerHTML>` is `:svg:ng-component` and is rejected. `<svg><MyComp:iframe i18n-src>` is `:svg:iframe` and is not the `iframe|src` sink. `foreignObject` still resets the namespace to HTML.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 933649f7f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6bf367e01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6bf367e01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fab161c64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Upstream backported the security-schema fixes non-monotonically, so a flat major/minor gate mis-assigns keys on several versions: - 21.0.6 dropped the legacy ping/cite/applet/media keys before script|href landed in 21.0.7, so it needs its own profile. - 20.3.16-20.3.21 keep the legacy URL keys alongside script|href; the key removal was never backported to the 20.3 line. - 21.2.14 is namespaced but lacks :svg:a|href (added in 20.3.22 / 21.2.15). - The preparser classified :svg:style as a style element only on 20.3.22 and 21.2.14 (STYLE_ELEMENTS); it was reverted everywhere else.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee63c9e9f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Upstream _getPrefix passes getNsPrefix(parentName) to children verbatim, so a parent like :xml:div resolves its children to :xml:* and normalizeTagName then drops the non-svg/math prefix for the security lookup. The previous ElementNamespace stack could only represent svg/math/html, so an arbitrary prefix fell back to the surrounding namespace and <svg><xml:div><iframe [src]> was treated as :svg:iframe, losing the iframe|src sanitizer. The stack now stores the inheritable prefix string. resolve_element_name reproduces mergeNsAndName(_getPrefix()) for element names, emitted tag names, selectorless host tags, and isVoid lookups, while security_lookup_name applies normalizeTagName to the resolved name.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0324bb1a9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
calcPossibleSecurityContexts only promotes an unknown bare selector to :svg:/:math: form on the namespaced schema (20.3.22+, 21.2.14+, v22). Promoting unconditionally made <MyComp:animate [attr.to]> miss the bare animate|to key on 21.2.7-21.2.13, dropping ɵɵvalidateAttribute. Gate the promotion on the schema kind; host tags pass through verbatim on earlier versions, matching upstream.
The 19.2 release line received the same security backports as 20.3/21.x, but every 19.x target fell through to the legacy schema: - 19.2.17 hardened (MathML hrefs, a|xlink:href, attributeNoBinding) while keeping the legacy URL keys - 19.2.18-19.2.22 added script|href; iframe|src joined the Trusted Types sinks at 19.2.20 - 19.2.23+ has the namespaced schema including :svg:a|href, and 19.2.23 is the only 19.x tag where the preparser strips :svg:style
Upstream resolve_sanitizers.ts falls back to ɵɵvalidateIframeAttribute when a Property/Attribute/DomProperty op on an iframe got no sanitizer from its security context. The path exists on versions without the iframe attributeNoBinding schema keys (removed upstream in 19.2.17 / 20.3.15 / 21.0.2), so gate it to the legacy schema. Also pass angular_version into TransformOptions in the integration test helper so version-targeted tests compile templates against the matching security schema.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5150f3ab6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Upstream added `normalizeTagName` to `securityContext` one release after the namespaced schema keys: 19.2.23, 20.3.22, and 21.2.15. At 21.2.14 (and every earlier version) the tag is lowercased verbatim, so `:xml:iframe|src` misses the schema and produces no sanitizer. Our lookup stripped the prefix unconditionally, emitting `ɵɵsanitizeResourceUrl` for targets where upstream assigns NONE. `get_security_context_for` now takes the verbatim resolved name and a new `normalizes_tag_names` profile flag (the V22 schema kind) decides whether `normalizeTagName` applies.
|
Round of fixes against upstream tags landed: 82c7bea — 19.2.x schema cutovers. The 19.2 line got the same backports as 20.3/21.x but every 19.x target fell through to the legacy schema. Verified per tag: f5150f3 — legacy da571ca — Verified against the vendored v22 sources plus |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da571ca492
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`extract_messages`/`merge_translations` hard-coded the v22 sink list via `is_trusted_types_sink`, so a 21.2.3 target that compiles `iframe i18n-src` fine was still rejected during extraction. Both APIs now take the target Angular version and the visitor uses `is_trusted_types_sink_at`, keeping extraction, merge, and template compilation on the same 21.2.4 cutoff.
Upstream's selector regexp has no `*` production, so `CssSelector.parse` leaves `element` unset for `*` and `*[x]`, and `calcPossibleSecurityContexts` expands them over all known element names. Our parser set `element` to `*`, so a directive like `*[feature]` with a `src` host binding only hit the `*|src` schema key and got no sanitizer instead of `sanitizeUrlOrResourceUrl`. The one place upstream keeps a literal `*` element is a `:not(...)`-only selector; it looks up the `*|attr` key verbatim there, so the expansion in `collect_namespaced_contexts` is removed to match.
|
Latest round of fixes against upstream tags landed: f103a7f — universal-selector host security. Our All threads resolved; every finding so far verified against vendored |
|
Post-merge adversarial review verified against upstream tags. One finding was a real divergence from the v22.0.0 baseline: element bindings skipped the unknown-element All other findings describe upstream behavior that landed after our vendored v22.0.0 and don't apply to this baseline: |
1 participant
Summary
Closes #315.
mainvendors@angular/compilerv22.0.0. Passing no version, or v22+, uses that schema. Passing an earlierangularVersionkeeps the schema that release actually shipped, so a v21 build does not pick up v22-only namespaced keys.script|srcis a resource URL. No SVG animation sinks.script|href, MathMLhref, andattributeNameno-binding.iframe|src.animate/setto,from, andvalues.:svg:animate|todoes not match.:svg:scriptis kept.:svg:and:math:.script|srcis gone.:svg:scriptis stripped.ATTRIBUTE_NO_BINDINGemitsɵɵvalidateAttribute.Host bindings use the same version: v22 promotes a bare selector element to
:svg:/:math:and lowercases:not(). v21 matches the element string as written.Validation
cargo test -p oxc_angular_compilercargo run -p oxc_angular_conformance: 1264/1264 (default schema is v22)