refactor(http): change <script>'s ownerDocument in jsonp teardown (#36807)

martinsik · thePunderWoman · commit 909b21aa29c1 · 2022-02-24T17:24:33.000Z
handler Cancel pending json handler by adopting its <script> element into another document (https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block) This way the browser will prevent the script from being parsed and executed. Fixes #34818 PR Close #36807
diff --git a/packages/common/http/src/jsonp.ts b/packages/common/http/src/jsonp.ts
@@ -21,6 +21,12 @@ import {HttpErrorResponse, HttpEvent, HttpEventType, HttpResponse, HttpStatusCod
 // is shared among all applications on the page.
 let nextRequestId: number = 0;
 
+/**
+ * When a pending <script> is unsubscribed we'll move it to this document, so it won't be
+ * executed.
+ */
+let foreignDocument: Document|undefined;
+
 // Error text given when a JSONP script is injected, but doesn't invoke the callback
 // passed in its URL.
 export const JSONP_ERR_NO_CALLBACK = 'JSONP injected script did not invoke callback.';
@@ -101,22 +107,13 @@ export class JsonpClientBackend implements HttpBackend {
       // Whether the response callback has been called.
       let finished: boolean = false;
 
-      // Whether the request has been cancelled (and thus any other callbacks)
-      // should be ignored.
-      let cancelled: boolean = false;
-
       // Set the response callback in this.callbackMap (which will be the window
       // object in the browser. The script being loaded via the <script> tag will
       // eventually call this callback.
       this.callbackMap[callback] = (data?: any) => {
         // Data has been received from the JSONP script. Firstly, delete this callback.
         delete this.callbackMap[callback];
 
-        // Next, make sure the request wasn't cancelled in the meantime.
-        if (cancelled) {
-          return;
-        }
-
         // Set state to indicate data was received.
         body = data;
         finished = true;
@@ -125,29 +122,22 @@ export class JsonpClientBackend implements HttpBackend {
       // cleanup() is a utility closure that removes the <script> from the page and
       // the response callback from the window. This logic is used in both the
       // success, error, and cancellation paths, so it's extracted out for convenience.
-      const cleanup = (removeCallback = true) => {
+      const cleanup = () => {
         // Remove the <script> tag if it's still on the page.
         if (node.parentNode) {
           node.parentNode.removeChild(node);
         }
 
-        if (removeCallback) {
-          // Remove the response callback from the callbackMap (window object in the
-          // browser).
-          delete this.callbackMap[callback];
-        }
+        // Remove the response callback from the callbackMap (window object in the
+        // browser).
+        delete this.callbackMap[callback];
       };
 
       // onLoad() is the success callback which runs after the response callback
       // if the JSONP script loads successfully. The event itself is unimportant.
       // If something went wrong, onLoad() may run without the response callback
       // having been invoked.
       const onLoad = (event: Event) => {
-        // Do nothing if the request has been cancelled.
-        if (cancelled) {
-          return;
-        }
-
         // We wrap it in an extra Promise, to ensure the microtask
         // is scheduled after the loaded endpoint has executed any potential microtask itself,
         // which is not guaranteed in Internet Explorer and EdgeHTML. See issue #39496
@@ -186,10 +176,6 @@ export class JsonpClientBackend implements HttpBackend {
       // a Javascript error. It emits the error via the Observable error channel as
       // a HttpErrorResponse.
       const onError: any = (error: Error) => {
-        // If the request was already cancelled, no need to emit anything.
-        if (cancelled) {
-          return;
-        }
         cleanup();
 
         // Wrap the error in a HttpErrorResponse.
@@ -212,20 +198,25 @@ export class JsonpClientBackend implements HttpBackend {
 
       // Cancellation handler.
       return () => {
-        // Track the cancellation so event listeners won't do anything even if already scheduled.
-        cancelled = true;
-
-        // Remove the event listeners so they won't run if the events later fire.
-        node.removeEventListener('load', onLoad);
-        node.removeEventListener('error', onError);
+        if (!finished) {
+          this.removeListeners(node);
+        }
 
         // And finally, clean up the page.
-        // Unsubscription won't remove the callback handler because there's no way to stop
-        // loading <script> once it has been added to DOM.
-        cleanup(false);
+        cleanup();
       };
     });
   }
+
+  private removeListeners(script: HTMLScriptElement): void {
+    // Issue #34818
+    // Changing <script>'s ownerDocument will prevent it from execution.
+    // https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block
+    if (!foreignDocument) {
+      foreignDocument = (this.document.implementation as DOMImplementation).createHTMLDocument();
+    }
+    foreignDocument.adoptNode(script);
+  }
 }
 
 /**
diff --git a/packages/common/http/test/jsonp_mock.ts b/packages/common/http/test/jsonp_mock.ts
@@ -7,7 +7,7 @@
  */
 
 export class MockScriptElement {
-  constructor() {}
+  constructor(public ownerDocument: MockDocument) {}
 
   listeners: {
     load?: (event: Event) => void,
@@ -28,8 +28,12 @@ export class MockDocument {
   mock!: MockScriptElement|null;
   readonly body: any = this;
 
+  implementation = {
+    createHTMLDocument: () => new MockDocument(),
+  };
+
   createElement(tag: 'script'): HTMLScriptElement {
-    return new MockScriptElement() as any as HTMLScriptElement;
+    return new MockScriptElement(this) as any as HTMLScriptElement;
   }
 
   appendChild(node: any): void {
@@ -42,11 +46,23 @@ export class MockDocument {
     }
   }
 
+  adoptNode(node: any) {
+    node.ownerDocument = this;
+  }
+
   mockLoad(): void {
-    this.mock!.listeners.load!(null as any);
+    // Mimic behavior described by
+    // https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block
+    if (this.mock!.ownerDocument === this) {
+      this.mock!.listeners.load!(null as any);
+    }
   }
 
   mockError(err: Error) {
-    this.mock!.listeners.error!(err);
+    // Mimic behavior described by
+    // https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block
+    if (this.mock!.ownerDocument === this) {
+      this.mock!.listeners.error!(err);
+    }
   }
 }
diff --git a/packages/common/http/test/jsonp_spec.ts b/packages/common/http/test/jsonp_spec.ts
@@ -24,7 +24,7 @@ const SAMPLE_REQ = new HttpRequest<never>('JSONP', '/test');
 
 {
   describe('JsonpClientBackend', () => {
-    let home = {};
+    let home: any;
     let document: MockDocument;
     let backend: JsonpClientBackend;
     beforeEach(() => {
@@ -62,10 +62,19 @@ const SAMPLE_REQ = new HttpRequest<never>('JSONP', '/test');
       });
       document.mockError(error);
     });
-    it('allows the callback to be invoked when the request is cancelled', () => {
-      backend.handle(SAMPLE_REQ).subscribe().unsubscribe();
-      runOnlyCallback(home, {data: 'This is a test'});
+    it('prevents the script from executing when the request is cancelled', () => {
+      const sub = backend.handle(SAMPLE_REQ).subscribe();
+      expect(Object.keys(home).length).toBe(1);
+      const keys = Object.keys(home);
+      const spy = jasmine.createSpy('spy', home[keys[0]]);
+
+      sub.unsubscribe();
+      document.mockLoad();
       expect(Object.keys(home).length).toBe(0);
+      expect(spy).not.toHaveBeenCalled();
+      // The script element should have been transferred to a different document to prevent it from
+      // executing.
+      expect(document.mock!.ownerDocument).not.toEqual(document);
     });
     describe('throws an error', () => {
       it('when request method is not JSONP',
