8000 Revert "fix(core): hardening rules related to the attribute order on … · angular/angular@13b863a · GitHub
[go: up one dir, main page]
Skip to content

Commit 13b863a

Browse files
AndrewKushnirAndrewKushnir
committed
Revert "fix(core): hardening rules related to the attribute order on iframe elements (#47935)" (#47959)
This reverts commit 2d08965. The reason for revert is that we've identified some issues with implementation. The issues will get addressed soon and the fix would be re-submitted. PR Close #47959
1 parent 2d08965 commit 13b863a

File tree

17 files changed

+19
-1113
lines changed

17 files changed

+19
-1113
lines changed

‎goldens/public-api/core/errors.md

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,6 @@ export const enum RuntimeErrorCode {
101101
// (undocumented)
102102
UNKNOWN_ELEMENT = 304,
103103
// (undocumented)
104-
UNSAFE_IFRAME_ATTRS = 910,
105-
// (undocumented)
106104
UNSAFE_VALUE_IN_RESOURCE_URL = 904,
107105
// (undocumented)
108106
UNSAFE_VALUE_IN_SCRIPT = 905,

‎goldens/size-tracking/aio-payloads.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
"aio-local": {
1313
"uncompressed": {
1414
"runtime": 4325,
15-
"main": 456041,
15+
"main": 455228,
1616
"polyfills": 33952,
1717
"styles": 73964,
1818
"light-theme": 78157,

‎goldens/size-tracking/integration-payloads.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@
4848
"animations": {
4949
"uncompressed": {
5050
"runtime": 1070,
51-
"main": 156446,
51+
"main": 155920,
5252
"polyfills": 33814
5353
}
5454
},

‎packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/elements/security_sensitive_constant_attributes.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ consts: [
66
],
77
template: function MyComponent_Template(rf, ctx) {
88
if (rf & 1) {
9-
$r3$.ɵɵelement(0, "embed", 0)(1, "iframe", 1, null, i0.ɵɵvalidateIframeStaticAttributes)(2, "object", 2)(3, "embed", 0)(4, "img", 3);
9+
$r3$.ɵɵelement(0, "embed", 0)(1, "iframe", 1)(2, "object", 2)(3, "embed", 0)(4, "img", 3);
1010
}
1111
…
1212
}

‎packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

Lines changed: 0 additions & 212 deletions
Original file line numberDiff line numberDiff line change
@@ -7506,218 +7506,6 @@ function allTests(os: string) {
75067506
});
75077507
});
75087508

7509-
describe('iframe processing', () => {
7510-
it('should generate attribute and property bindings with a validator fn when on <iframe>',
7511-
() => {
7512-
env.write('test.ts', `
7513-
import {Component} from '@angular/core';
7514-
7515-
@Component({
7516-
template: \`
7517-
<iframe src="http://angular.io"
7518-
[sandbox]="''" [attr.allow]="''"
7519-
[title]="'Hi!'"
7520-
></iframe>
7521-
\`
7522-
})
7523-
export class SomeComponent {}
7524-
`);
7525-
7526-
env.driveMain();
7527-
const jsContents = env.getContents('test.js');
7528-
7529-
// Only `sandbox` has an extra validation fn (since it's security-sensitive),
7530-
// the `title` property doesn't have an extra validation fn.
7531-
expect(jsContents)
7532-
.toContain(
7533-
'ɵɵproperty("sandbox", "", i0.ɵɵvalidateIframeAttribute)("title", "Hi!")');
7534-
7535-
// The `allow` property is also security-sensitive, thus an extra validation fn.
7536-
expect(jsContents).toContain('ɵɵattribute("allow", "", i0.ɵɵvalidateIframeAttribute)');
7537-
7538-
// Expect an extra validation function on the `element` instruction for an <iframe>
7539-
// to validate static attributes.
7540-
expect(jsContents)
7541-
.toContain('ɵɵelement(0, "iframe", 0, null, i0.ɵɵvalidateIframeStaticAttributes)');
7542-
});
7543-
7544-
it('should generate attribute and property bindings with a validator fn when on <iframe> ' +
7545-
'with an *ngIf on it as well',
7546-
() => {
7547-
env.write('test.ts', `
7548-
import {Component} from '@angular/core';
7549-
7550-
@Component({
7551-
template: \`
7552-
<iframe
7553-
*ngIf="visible"
7554-
src="http://angular.io"
7555-
[sandbox]="''"
7556-
></iframe>
7557-
\`
7558-
})
7559-
export class SomeComponent {
7560-
visible = true;
7561-
}
7562-
`);
7563-
7564-
env.driveMain();
7565-
const jsContents = env.getContents('test.js');
7566-
7567-
// Expect an extra validation function on the `element` instruction for an <iframe>
7568-
// to validate static attributes.
7569-
expect(jsContents)
7570-
.toContain('ɵɵelement(0, "iframe", 1, null, i0.ɵɵvalidateIframeStaticAttributes)');
7571-
7572-
// The `template` instruction that represents an <iframe> remains as is
7573-
// (without additional validation fns).
7574-
expect(jsContents)
7575-
.toContain('ɵɵtemplate(0, SomeComponent_iframe_0_Template, 1, 1, "iframe", 0)');
7576-
});
7577-
7578-
it('should generate an element instruction with a validator function for <iframe>s ' +
7579-
'(making sure it\'s case-insensitive, since this is allowed in Angular templates)',
7580-
() => {
7581-
env.write('test.ts', `
7582-
import {Component} from '@angular/core';
7583-
7584-
@Component({
7585-
template: \`
7586-
<IFRAME
7587-
src="http://angular.io"
7588-
[attr.SANDBOX]="''"
7589-
></IFRAME>
7590-
\`
7591-
})
7592-
export class SomeComponent {}
7593-
`);
7594-
7595-
env.driveMain();
7596-
const jsContents = env.getContents('test.js');
7597-
7598-
// Make sure that the `sandbox` has an extra validation fn,
7599-
// and the check is case-insensitive (since the `setAttribute` DOM API
7600-
// is case-insensitive as well).
7601-
expect(jsContents).toContain('ɵɵattribute("SANDBOX", "", i0.ɵɵvalidateIframeAttribute)');
7602-
7603-
// Expect an extra validation function on the `element` instruction for an <iframe>
7604-
// to validate static attributes.
7605-
expect(jsContents)
7606-
.toContain('ɵɵelement(0, "IFRAME", 0, null, i0.ɵɵvalidateIframeStaticAttributes)');
7607-
});
7608-
7609-
it('should include static attribute validation fn when on <iframe> ' +
7610-
'(even if there are no static attributes)',
7611-
() => {
7612-
env.write('test.ts', `
7613-
import {Component} from '@angular/core';
7614-
7615-
@Component({
7616-
template: \`
7617-
<iframe [src]="'http://angular.io'"></iframe>
7618-
\`
7619-
})
7620-
export class SomeComponent {}
7621-
`);
7622-
7623-
env.driveMain();
7624-
const jsContents = env.getContents('test.js');
7625-
7626-
// Expect an extra validation function on the `element` instruction for an <iframe>
7627-
// to validate static attributes even if there are no static attributes present
7628-
// on an <iframe>. There might be directives with static host attributes mathcing
7629-
// this <iframe>, the validation function will be used to check the final set of
7630-
// static attributes (from all directives and an element itself).
7631-
expect(jsContents)
7632-
.toContain('ɵɵelement(0, "iframe", 0, null, i0.ɵɵvalidateIframeStaticAttributes)');
7633-
});
7634-
7635-
it('should *not* generate a validator fn for attribute and property bindings when *not* on <iframe>',
7636-
() => {
7637-
env.write('test.ts', `
7638-
import {Component, Directive} from '@angular/core';
7639-
7640-
@Directive({
7641-
standalone: true,
7642-
selector: '[sandbox]',
7643-
inputs: ['sandbox']
7644-
})
7645-
class Dir {}
7646-
7647-
@Component({
7648-
standalone: true,
7649-
imports: [Dir],
7650-
template: \`
7651-
<div [sandbox]="''" [title]="'Hi!'"></div>
7652-
\`
7653-
})
7654-
export class SomeComponent {}
7655-
`);
7656-
7657-
env.driveMain();
7658-
const jsContents = env.getContents('test.js');
7659-
7660-
// Note: no extra validation fn, since a security-sensitive attribute is *not* on an
7661-
// <iframe>.
7662-
expect(jsContents).toContain('ɵɵproperty("sandbox", "")("title", "Hi!")');
7663-
7664-
// No extra validation fn on an `element` instruction too, since it's
7665-
// not an <iframe>.
7666-
expect(jsContents).toContain('ɵɵelement(0, "div", 0)');
7667-
});
7668-
7669-
it('should generate a validator fn for attribute and property host bindings on a directive',
7670-
() => {
7671-
env.write('test.ts', `
7672-
import {Directive} from '@angular/core';
7673-
7674-
@Directive({
7675-
host: {
7676-
'[sandbox]': "''",
7677-
'[attr.allow]': "''",
7678-
'src': 'http://angular.io'
7679-
}
7680-
})
7681-
export class SomeDir {}
7682-
`);
7683-
7684-
env.driveMain();
7685-
const jsContents = env.getContents('test.js');
7686-
7687-
// The `sandbox` is potentially a security-sensitive attribute of an <iframe>.
7688-
// Generate an extra validation function to invoke at runtime, which would
7689-
// check if an underlying host element is an <iframe>.
7690-
expect(jsContents)
7691-
.toContain('ɵɵhostProperty("sandbox", "", i0.ɵɵvalidateIframeAttribute)');
7692-
7693-
// Similar to the above, but for an attribute binding (host attributes are
7694-
// represented via `ɵɵattribute`).
7695-
expect(jsContents).toContain('ɵɵattribute("allow", "", i0.ɵɵvalidateIframeAttribute)');
7696-
});
7697-
7698-
it('should generate a validator fn for attribute host bindings on a directive ' +
7699-
'(making sure the check is case-insensitive)',
7700-
() => {
7701-
env.write('test.ts', `
7702-
import {Directive} from '@angular/core';
7703-
7704-
@Directive({
7705-
host: {
7706-
'[attr.SANDBOX]': "''"
7707-
}
7708-
})
7709-
export class SomeDir {}
7710-
`);
7711-
7712-
env.driveMain();
7713-
const jsContents = env.getContents('test.js');
7714-
7715-
// Make sure that we generate a validation fn for the `sandbox` attribute,
7716-
// even when it was declared as `SANDBOX`.
7717-
expect(jsContents).toContain('ɵɵattribute("SANDBOX", "", i0.ɵɵvalidateIframeAttribute)');
7718-
});
7719-
});
7720-
77217509
describe('undecorated providers', () => {
77227510
it('should error when an undecorated class, with a non-trivial constructor, is provided directly in a module',
77237511
() => {

‎packages/compiler/src/render3/r3_identifiers.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -346,8 +346,4 @@ export class Identifiers {
346346
static trustConstantHtml: o.ExternalReference = {name: 'ɵɵtrustConstantHtml', moduleName: CORE};
347347
static trustConstantResourceUrl:
348348
o.ExternalReference = {name: 'ɵɵtrustConstantResourceUrl', moduleName: CORE};
349-
static validateIframeAttribute:
350-
o.ExternalReference = {name: 'ɵɵvalidateIframeAttribute', moduleName: CORE};
351-
static validateIframeStaticAttributes:
352-
o.ExternalReference = {name: 'ɵɵvalidateIframeStaticAttributes', moduleName: CORE};
353349
}

‎packages/compiler/src/render3/view/compiler.ts

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import * as core from '../../core';
1212
import {AST, ParsedEvent, ParsedEventType, ParsedProperty} from '../../expression_parser/ast';
1313
import * as o from '../../output/output_ast';
1414
import {ParseError, ParseSourceSpan, sanitizeIdentifier} from '../../parse_util';
15-
import {isIframeSecuritySensitiveAttr} from '../../schema/dom_security_schema';
1615
import {CssSelector} from '../../selector';
1716
import {ShadowCss} from '../../shadow_css';
1817
import {BindingParser} from '../../template_parser/binding_parser';
@@ -575,19 +574,6 @@ function createHostBindingsFunction(
575574
const instructionParams = [o.literal(bindingName), bindingExpr.currValExpr];
576575
if (sanitizerFn) {
577576
instructionParams.push(sanitizerFn);
578-
} else {
579-
// If there was no sanitization function found based on the security context
580-
// of an attribute/property binding - check whether this attribute/property is
581-
// one of the security-sensitive <iframe> attributes.
582-
// Note: for host bindings defined on a directive, we do not try to find all
583-
// possible places where it can be matched, so we can not determine whether
584-
// the host element is an <iframe>. In this case, if an attribute/binding
585-
// name is in the `IFRAME_SECURITY_SENSITIVE_ATTRS` set - append a validation
586-
// function, which would be invoked at runtime and would have access to the
587-
// underlying DOM element, check if it's an <iframe> and if so - runs extra checks.
588-
if (isIframeSecuritySensitiveAttr(bindingName)) {
589-
instructionParams.push(o.importExpr(R3.validateIframeAttribute));
590-
}
591577
}
592578

593579
updateVariables.push(...bindingExpr.stmts);

‎packages/compiler/src/render3/view/template.ts

Lines changed: 2 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import {mapLiteral} from '../../output/map_util';
2323
import * as o from '../../output/output_ast';
2424
import {ParseError, ParseSourceSpan, sanitizeIdentifier} from '../../parse_util';
2525
import {DomElementSchemaRegistry} from '../../schema/dom_element_schema_registry';
26-
import {isIframeSecuritySensitiveAttr} from '../../schema/dom_security_schema';
2726
import {isTrustedTypesSink} from '../../schema/trusted_types_sinks';
2827
import {CssSelector} from '../../selector';
2928
import {BindingParser} from '../../template_parser/binding_parser';
@@ -676,16 +675,6 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
676675
const refs = this.prepareRefsArray(element.references);
677676
parameters.push(this.addToConsts(refs));
678677

679-
// If this element is an <iframe>, append an extra validation
680-
// function, which would be invoked at runtime to make sure that
681-
// all security-sensitive attributes defined statically (both on
682-
// the element itself as well as on all matched directives) are
683-
// set on the underlying <iframe> *before* setting its `src` or
684-
// `srcdoc` (otherwise they'd not be taken into account).
685-
if (isIframeElement(element.name)) {
686-
parameters.push(o.importExpr(R3.validateIframeStaticAttributes));
687-
}
688-
689678
const wasInNamespace = this._namespace;
690679
const currentNamespace = this.getNamespaceInstruction(namespaceKey);
691680

@@ -794,19 +783,8 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
794783
const params: any[] = [];
795784
const [attrNamespace, attrName] = splitNsName(input.name);
796785
const isAttributeBinding = inputType === BindingType.Attribute;
797-
let sanitizationRef = resolveSanitizationFn(input.securityContext, isAttributeBinding);
798-
if (!sanitizationRef) {
799-
// If there was no sanitization function found based on the security context
800-
// of an attribute/property - check whether this attribute/property is
801-
// one of the security-sensitive <iframe> attributes (and that the current
802-
// element is actually an <iframe>).
803-
if (isIframeElement(element.name) && isIframeSecuritySensitiveAttr(input.name)) {
804-
sanitizationRef = o.importExpr(R3.validateIframeAttribute);
805-
}
806-
}
807-
if (sanitizationRef) {
808-
params.push(sanitizationRef);
809-
}
786+
const sanitizationRef = resolveSanitizationFn(input.securityContext, isAttributeBinding);
787+
if (sanitizationRef) params.push(sanitizationRef);
810788
if (attrNamespace) {
811789
const namespaceLiteral = o.literal(attrNamespace);
812790

@@ -2233,10 +2211,6 @@ function isTextNode(node: t.Node): boolean {
22332211
return node instanceof t.Text || node instanceof t.BoundText || node instanceof t.Icu;
22342212
}
22352213

2236-
function isIframeElement(tagName: string): boolean {
2237-
return tagName.toLowerCase() === 'iframe';
2238-
}
2239-
22402214
function hasTextChildrenOnly(children: t.Node[]): boolean {
22412215
return children.every(isTextNode);
22422216
}

‎packages/compiler/src/schema/dom_security_schema.ts

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -76,28 +76,3 @@ export function SECURITY_SCHEMA(): {[k: string]: SecurityContext} {
7676
function registerContext(ctx: SecurityContext, specs: string[]) {
7777
for (const spec of specs) _SECURITY_SCHEMA[spec.toLowerCase()] = ctx;
7878
}
79-
80-
/**
81-
* The set of security-sensitive attributes of an `<iframe>` that *must* be
82-
* applied before setting the `src` or `srcdoc` attribute value.
83-
* This ensures that all security-sensitive attributes are taken into account
84-
* while creating an instance of an `<iframe>` at runtime.
85-
*
86-
* Keep this list in sync with the `IFRAME_SECURITY_SENSITIVE_ATTRS` token
87-
* from the `packages/core/src/sanitization/iframe_attrs_validation.ts` script.
88-
*
89-
* Avoid using this set directly, use the `isIframeSecuritySensitiveAttr` function
90-
* in the code instead.
91-
*/
92-
export const IFRAME_SECURITY_SENSITIVE_ATTRS = new Set(
93-
['sandbox', 'allow', 'allowfullscreen', 'referrerpolicy', 'loading', 'csp', 'fetchpriority']);
94-
95-
/**
96-
* Checks whether a given attribute name might represent a security-sensitive
97-
* attribute of an <iframe>.
98-
*/
99-
export function isIframeSecuritySensitiveAttr(attrName: string): boolean {
100-
// The `setAttribute` DOM API is case-insensitive, so we lowercase the value
101-
// before checking it against a known security-sensitive attributes.
102-
return IFRAME_SECURITY_SENSITIVE_ATTRS.has(attrName.toLowerCase());
103-
}

0 commit comments

Comments
 (0)
0