fix(core): Ensure fill only patches functions (#15632)

Author: Lms24

packages/browser-utils/src/instrument/history.ts

@@ -18,7 +18,10 @@ export function addHistoryInstrumentationHandler(handler: (data: HandlerDataHist
   maybeInstrument(type, instrumentHistory);
 }
 
-function instrumentHistory(): void {
+/**
+ * Exported just for testing
+ */
+export function instrumentHistory(): void {
   // The `popstate` event may also be triggered on `pushState`, but it may not always reliably be emitted by the browser
   // Which is why we also monkey-patch methods below, in addition to this
   WINDOW.addEventListener('popstate', () => {

packages/browser-utils/test/instrument/history.test.ts

@@ -0,0 +1,58 @@
+import { describe, expect, it, vi } from 'vitest';
+import { WINDOW } from '../../src/types';
+import { afterEach } from 'node:test';
+
+import { instrumentHistory } from './../../src/instrument/history';
+
+describe('instrumentHistory', () => {
+  const originalHistory = WINDOW.history;
+  WINDOW.addEventListener = vi.fn();
+
+  afterEach(() => {
+    // @ts-expect-error - this is fine for testing
+    WINDOW.history = originalHistory;
+  });
+
+  it("doesn't throw if history is not available", () => {
+    // @ts-expect-error - this is fine for testing
+    WINDOW.history = undefined;
+    expect(instrumentHistory).not.toThrow();
+    expect(WINDOW.history).toBe(undefined);
+  });
+
+  it('adds an event listener for popstate', () => {
+    // adds an event listener for popstate
+    expect(WINDOW.addEventListener).toHaveBeenCalledTimes(1);
+    expect(WINDOW.addEventListener).toHaveBeenCalledWith('popstate', expect.any(Function));
+  });
+
+  it("doesn't throw if history.pushState is not a function", () => {
+    // @ts-expect-error - only partially adding history properties
+    WINDOW.history = {
+      replaceState: () => {},
+      pushState: undefined,
+    };
+
+    expect(instrumentHistory).not.toThrow();
+
+    expect(WINDOW.history).toEqual({
+      replaceState: expect.any(Function), // patched function
+      pushState: undefined, // unpatched
+    });
+  });
+
+  it("doesn't throw if history.replaceState is not a function", () => {
+    // @ts-expect-error - only partially adding history properties
+    WINDOW.history = {
+      replaceState: undefined,
+      pushState: () => {},
+    };
+
+    expect(instrumentHistory).not.toThrow();
+
+    expect(WINDOW.history).toEqual({
+      replaceState: undefined, // unpatched
+      pushState: expect.any(Function), // patched function
+    });
+  });
+});

packages/core/src/utils-hoist/object.ts

@@ -10,6 +10,8 @@ import { truncate } from './string';
 /**
  * Replace a method in an object with a wrapped version of itself.
  *
+ * If the method on the passed object is not a function, the wrapper will not be applied.
+ *
  * @param source An object that contains a method to be wrapped.
  * @param name The name of the method to be wrapped.
  * @param replacementFactory A higher-order function that takes the original version of the given method and returns a
@@ -23,7 +25,13 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa
     return;
   }
 
-  const original = source[name] as () => any;
+  // explicitly casting to unknown because we don't know the type of the method initially at all
+  const original = source[name] as unknown;
+
+  if (typeof original !== 'function') {
+    return;
+  }
+
   const wrapped = replacementFactory(original) as WrappedFunction;
 
   // Make sure it's a function first, as we need to attach an empty prototype for `defineProperties` to work

packages/core/src/utils-hoist/supports.ts

@@ -61,7 +61,7 @@ export function supportsDOMException(): boolean {
  * @returns Answer to the given question.
  */
 export function supportsHistory(): boolean {
-  return 'history' in WINDOW;
+  return 'history' in WINDOW && !!WINDOW.history;
 }
 
 /**

packages/core/test/utils-hoist/object.test.ts

@@ -54,6 +54,19 @@ describe('fill()', () => {
     expect(source.prop()).toEqual(41);
   });
 
+  test.each([42, null, undefined, {}])("does't throw if the property is not a function but %s", (propValue: any) => {
+    const source = {
+      foo: propValue,
+    };
+    const name = 'foo';
+    const replacement = vi.fn().mockImplementationOnce(cb => cb);
+
+    fill(source, name, replacement);
+
+    expect(source.foo).toBe(propValue);
+    expect(replacement).not.toBeCalled();
+  });
+
   test('can do anything inside replacement function', () => {
     const source = {
       foo: (): number => 42,

packages/core/test/utils-hoist/supports.test.ts

@@ -0,0 +1,32 @@
+import { afterEach } from 'node:test';
+import { supportsHistory } from '../../src/utils-hoist/supports';
+import { describe, it, expect } from 'vitest';
+
+describe('supportsHistory', () => {
+  const originalHistory = globalThis.history;
+
+  afterEach(() => {
+    globalThis.history = originalHistory;
+  });
+
+  it('returns true if history is available', () => {
+    // @ts-expect-error - not setting all history properties
+    globalThis.history = {
+      pushState: () => {},
+      replaceState: () => {},
+    };
+    expect(supportsHistory()).toBe(true);
+  });
+
+  it('returns false if history is not available', () => {
+    // @ts-expect-error - deletion is okay in this case
+    delete globalThis.history;
+    expect(supportsHistory()).toBe(false);
+  });
+
+  it('returns false if history is undefined', () => {
+    // @ts-expect-error - undefined is okay in this case
+    globalThis.history = undefined;
+    expect(supportsHistory()).toBe(false);
+  });
+});