From d1dcd16c5b40291aa3fa2dc84d22842cd657b201 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 11 Mar 2026 09:56:18 +0100 Subject: [PATCH] fix(core): sanitize translated form attributes Fixes that we weren't sanitizing the `form` and `formaction` attributes when they're used together with translations. (cherry picked from commit de0eb4c6566011e1a34d529a273ec3d5b6bf17d5) --- packages/core/src/render3/i18n/i18n_parse.ts | 8 +++--- .../core/src/sanitization/html_sanitizer.ts | 26 ++++++++++++++----- packages/core/test/acceptance/i18n_spec.ts | 22 ++++++++++++++++ .../core/test/acceptance/security_spec.ts | 16 ++++++++++++ 4 files changed, 61 insertions(+), 11 deletions(-) diff --git a/packages/core/src/render3/i18n/i18n_parse.ts b/packages/core/src/render3/i18n/i18n_parse.ts index d2ebb0b92bb..cfb150ee085 100644 --- a/packages/core/src/render3/i18n/i18n_parse.ts +++ b/packages/core/src/render3/i18n/i18n_parse.ts @@ -11,7 +11,7 @@ import '../../util/ng_i18n_closure_mode'; import {XSS_SECURITY_URL} from '../../error_details_base_url'; import { getTemplateContent, - URI_ATTRS, + SENSITIVE_ATTRS, VALID_ATTRS, VALID_ELEMENTS, } from '../../sanitization/html_sanitizer'; @@ -388,7 +388,7 @@ export function i18nAttributesFirstPass(tView: TView, index: number, values: str previousElementIndex, attrName, countBindings(updateOpCodes), - URI_ATTRS[attrName.toLowerCase()] ? _sanitizeUrl : null, + SENSITIVE_ATTRS[attrName.toLowerCase()] ? _sanitizeUrl : null, ); } } @@ -816,7 +816,7 @@ function walkIcuTree( newIndex, attr.name, 0, - URI_ATTRS[lowerAttrName] ? _sanitizeUrl : null, + SENSITIVE_ATTRS[lowerAttrName] ? _sanitizeUrl : null, ); } else { ngDevMode && @@ -827,7 +827,7 @@ function walkIcuTree( ); } } else if (VALID_ATTRS[lowerAttrName]) { - if (URI_ATTRS[lowerAttrName]) { + if (SENSITIVE_ATTRS[lowerAttrName]) { // Don't sanitize, because no value is acceptable in sensitive attributes. // Translators are not allowed to create URIs. if (typeof ngDevMode !== 'undefined' && ngDevMode) { diff --git a/packages/core/src/sanitization/html_sanitizer.ts b/packages/core/src/sanitization/html_sanitizer.ts index e2f5a72bedb..1238ea8bf53 100644 --- a/packages/core/src/sanitization/html_sanitizer.ts +++ b/packages/core/src/sanitization/html_sanitizer.ts @@ -13,14 +13,16 @@ import {trustedHTMLFromString} from '../util/security/trusted_types'; import {getInertBodyHelper, InertBodyHelper} from './inert_body'; import {_sanitizeUrl} from './url_sanitizer'; -function tagSet(tags: string): {[k: string]: boolean} { - const res: {[k: string]: boolean} = {}; +type BooleanRecord = Record; + +function tagSet(tags: string): BooleanRecord { + const res: BooleanRecord = {}; for (const t of tags.split(',')) res[t] = true; return res; } -function merge(...sets: {[k: string]: boolean}[]): {[k: string]: boolean} { - const res: {[k: string]: boolean} = {}; +function merge(...sets: BooleanRecord[]): BooleanRecord { + const res: BooleanRecord = {}; for (const s of sets) { for (const v in s) { if (s.hasOwnProperty(v)) res[v] = true; @@ -66,7 +68,7 @@ const INLINE_ELEMENTS = merge( ), ); -export const VALID_ELEMENTS: {[k: string]: boolean} = merge( +export const VALID_ELEMENTS: BooleanRecord = merge( VOID_ELEMENTS, BLOCK_ELEMENTS, INLINE_ELEMENTS, @@ -74,7 +76,7 @@ export const VALID_ELEMENTS: {[k: string]: boolean} = merge( ); // Attributes that have href and hence need to be sanitized -export const URI_ATTRS: {[k: string]: boolean} = tagSet( +const URI_ATTRS: BooleanRecord = tagSet( 'background,cite,href,itemtype,longdesc,poster,src,xlink:href', ); @@ -105,7 +107,7 @@ const ARIA_ATTRS = tagSet( // can be sanitized, but they increase security surface area without a legitimate use case, so they // are left out here. -export const VALID_ATTRS: {[k: string]: boolean} = merge(URI_ATTRS, HTML_ATTRS, ARIA_ATTRS); +export const VALID_ATTRS: BooleanRecord = merge(URI_ATTRS, HTML_ATTRS, ARIA_ATTRS); // Elements whose content should not be traversed/preserved, if the elements themselves are invalid. // @@ -114,6 +116,16 @@ export const VALID_ATTRS: {[k: string]: boolean} = merge(URI_ATTRS, HTML_ATTRS, // don't want to preserve the content, if the elements themselves are going to be removed. const SKIP_TRAVERSING_CONTENT_IF_INVALID_ELEMENTS = tagSet('script,style,template'); +/** + * Attributes that are potential attach vectors and may need to be sanitized. + */ +export const SENSITIVE_ATTRS: BooleanRecord = merge( + URI_ATTRS, + // Note: we don't include these attributes in `URI_ATTRS`, because `URI_ATTRS` also + // determines whether an attribute should be dropped when sanitizing an HTML string. + tagSet('action,formaction,data,codebase'), +); + /** * SanitizingHtmlSerializer serializes a DOM fragment, stripping out any unsafe elements and unsafe * attributes. diff --git a/packages/core/test/acceptance/i18n_spec.ts b/packages/core/test/acceptance/i18n_spec.ts index ff9583b31d1..5ae825e7fa3 100644 --- a/packages/core/test/acceptance/i18n_spec.ts +++ b/packages/core/test/acceptance/i18n_spec.ts @@ -13,6 +13,7 @@ import {CommonModule, DOCUMENT, registerLocaleData} from '@angular/common'; import localeEs from '@angular/common/locales/es'; import localeRo from '@angular/common/locales/ro'; import {computeMsgId} from '@angular/compiler'; +import {isBrowser} from '@angular/private/testing'; import { Attribute, Component, @@ -3598,6 +3599,27 @@ describe('runtime i18n', () => { expect(link).toBeTruthy(); expect(link.getAttribute('href')).toMatch(/^unsafe:/); }); + + it('should sanitize action binding', () => { + const fixture = initWithTemplate( + SanitizeAppComp, + '
', + ); + const form: HTMLFormElement = fixture.nativeElement.querySelector('form'); + expect(form.getAttribute('action')).toMatch(/^unsafe:/); + }); + + // Skip this test in Node, because Domino doesn't support `formAction`. + if (isBrowser) { + it('should sanitize formaction binding', () => { + const fixture = initWithTemplate( + SanitizeAppComp, + '', + ); + const input: HTMLInputElement = fixture.nativeElement.querySelector('input'); + expect(input.getAttribute('formaction')).toMatch(/^unsafe:/); + }); + } }); }); diff --git a/packages/core/test/acceptance/security_spec.ts b/packages/core/test/acceptance/security_spec.ts index e0da38ba043..b95d2705ada 100644 --- a/packages/core/test/acceptance/security_spec.ts +++ b/packages/core/test/acceptance/security_spec.ts @@ -752,3 +752,19 @@ describe('SVG animation processing', () => { ); }); }); + +describe('innerHTML processing', () => { + it('should drop risky attributes from elements created with innerHTML', () => { + @Component({ + template: '
', + }) + class App { + html = '
'; + } + + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(fixture.nativeElement.innerHTML).not.toContain('action'); + }); +});