From c44d7ad09098c7daffe5c288c0bf75212a8d8d43 Mon Sep 17 00:00:00 2001
From: Andrew Kushnir <akushnir@google.com>
Date: Mon, 7 Nov 2022 14:24:38 -0800
Subject: [PATCH] fixup! fix(core): hardening attribute and property binding
 rules for <iframe> elements

---
 .../sanitization/iframe_attrs_validation.ts   | 17 ++---
 .../core/test/acceptance/security_spec.ts     | 62 +++++++++++++------
 2 files changed, 52 insertions(+), 27 deletions(-)

diff --git a/packages/core/src/sanitization/iframe_attrs_validation.ts b/packages/core/src/sanitization/iframe_attrs_validation.ts
index 5b210a29079467..07e08eb64254fa 100644
--- a/packages/core/src/sanitization/iframe_attrs_validation.ts
+++ b/packages/core/src/sanitization/iframe_attrs_validation.ts
@@ -10,6 +10,8 @@ import {RuntimeError, RuntimeErrorCode} from '../errors';
 import {getTemplateLocationDetails} from '../render3/instructions/element_validation';
 import {TNodeType} from '../render3/interfaces/node';
 import {RComment, RElement} from '../render3/interfaces/renderer_dom';
+import {RENDERER} from '../render3/interfaces/view';
+import {nativeRemoveNode} from '../render3/node_manipulation';
 import {getLView, getSelectedTNode} from '../render3/state';
 import {getNativeByTNode} from '../render3/util/view_utils';
 import {trustedHTMLFromString} from '../util/security/trusted_types';
@@ -32,15 +34,16 @@ export function ɵɵvalidateIframeAttribute(attrValue: any, tagName: string, att
   // Restrict any dynamic bindings of security-sensitive attributes/properties
   // on an <iframe> for security reasons.
   if (tNode.type === TNodeType.Element && tagName.toLowerCase() === 'iframe') {
+    const iframe = element as HTMLIFrameElement;
+
     // Unset previously applied `src` and `srcdoc` if we come across a situation when
     // a security-sensitive attribute is set later via an attribute/property binding.
-    const iframe = element as HTMLIFrameElement;
-    if (iframe.src) {
-      iframe.src = '';
-    }
-    if (iframe.srcdoc) {
-      iframe.srcdoc = trustedHTMLFromString('') as unknown as string;
-    }
+    iframe.src = '';
+    iframe.srcdoc = trustedHTMLFromString('') as unknown as string;
+
+    // Also remove the <iframe> from the document.
+    nativeRemoveNode(lView[RENDERER], iframe);
+
     const errorMessage = ngDevMode &&
         `Angular has detected that the \`${attrName}\` was applied ` +
             `as a binding to an <iframe>${getTemplateLocationDetails(lView)}. ` +
diff --git a/packages/core/test/acceptance/security_spec.ts b/packages/core/test/acceptance/security_spec.ts
index e19f4e58783a06..2f2ad12a79ef45 100644
--- a/packages/core/test/acceptance/security_spec.ts
+++ b/packages/core/test/acceptance/security_spec.ts
@@ -7,7 +7,7 @@
  */
 
 import {NgIf} from '@angular/common';
-import {Component, Directive, inject, Type} from '@angular/core';
+import {Component, Directive, inject, TemplateRef, Type, ViewChild, ViewContainerRef} from '@angular/core';
 import {RuntimeErrorCode} from '@angular/core/src/errors';
 import {global} from '@angular/core/src/util/global';
 import {ComponentFixture, TestBed} from '@angular/core/testing';
@@ -51,6 +51,12 @@ describe('iframe processing', () => {
     return new RegExp(errorMessagePart);
   }
 
+  function ensureNoIframePresent(fixture?: ComponentFixture<unknown>) {
+    // Note: a `fixture` may not exist in case an error was thrown at creation time.
+    const iframe = fixture?.nativeElement.querySelector('iframe');
+    expect(!!iframe).toBeFalse();
+  }
+
   function expectIframeCreationToFail<T>(component: Type<T>): ComponentFixture<T> {
     let fixture: ComponentFixture<T>|undefined;
     expect(() => {
@@ -58,25 +64,7 @@ describe('iframe processing', () => {
       fixture.detectChanges();
     }).toThrowError(getErrorMessageRegexp());
 
-    // Verify that there are no <iframe>s with `src` and `srcdoc` (that they are reset).
-    // Note: a `fixture` may not exist in case an error was thrown at creation time.
-    const iframe = fixture?.nativeElement.querySelector('iframe');
-    if (iframe) {
-      // Check that we unset the `src` and `srcdoc`.
-      // * Domino returns `/` as a default value for `src`.
-      // * After the reset (when we assign `src` to an empty string),
-      //   a browser returns either an empty string (when `src` is set
-      //   using a property) or a current page location (when `src` is set
-      //   using an attribute).
-      // Check all the mentioned values, since these tests are executed
-      // in both NodeJS (using Domino) and a browser-based environments on CI.
-      expect(
-          iframe.src === '' || iframe.src === '/' ||
-          (isBrowser && iframe.src === window.location.href))
-          .toEqual(true);
-      expect(iframe.srcdoc).toEqual('');
-    }
-
+    ensureNoIframePresent(fixture);
     return fixture!;
   }
 
@@ -667,6 +655,40 @@ describe('iframe processing', () => {
            expectIframeToBeCreated(IframeComp, {src: TEST_IFRAME_URL});
          });
 
+
+      it('should error when creating a view that contains an <iframe> ' +
+             'with security-sensitive attributes set via property bindings',
+         () => {
+           @Component({
+             standalone: true,
+             selector: 'my-comp',
+             template: `
+                <ng-container #container></ng-container>
+                <ng-template #template>
+                  <iframe src="${TEST_IFRAME_URL}" [sandbox]="''"></iframe>
+                </ng-template>
+              `,
+           })
+           class IframeComp {
+             @ViewChild('container', {read: ViewContainerRef}) container!: ViewContainerRef;
+             @ViewChild('template') template!: TemplateRef<unknown>;
+
+             createEmbeddedView() {
+               this.container.createEmbeddedView(this.template);
+             }
+           }
+
+           const fixture = TestBed.createComponent(IframeComp);
+           fixture.detectChanges();
+
+           expect(() => {
+             fixture.componentInstance.createEmbeddedView();
+             fixture.detectChanges();
+           }).toThrowError(getErrorMessageRegexp());
+
+           ensureNoIframePresent(fixture);
+         });
+
       describe('i18n', () => {
         it('should error when a security-sensitive attribute is set as ' +
                'a property binding on an <iframe> inside i18n block',