Instrument urls built from a JavaScript undefined (BL-16666)

src/BloomBrowserUI/lib/errorHandler.ts

@@ -1,5 +1,6 @@
 import * as StackTrace from "stacktrace-js";
 import Axios from "axios";
+import { installUndefinedUrlDetector } from "./undefinedUrlDetector";
 
 // This file implements custom Bloom global error handling.
 // It should be imported by the root module in each bundle.
@@ -156,6 +157,11 @@ if (typeof window !== "undefined") {
         });
         return true; // suppress normal handling.
     };
+
+    // Watch for urls built out of a JavaScript value that wasn't ready. Installed here because
+    // every bundle's root module imports this file, which is exactly the coverage we want: the
+    // bug can be in any of our pages, and the server can't tell us which one. See BL-16577.
+    installUndefinedUrlDetector(reportError);
 }
 
 // Saving this as it MIGHT be useful if we decide to have another go at catching

src/BloomBrowserUI/lib/undefinedUrlDetector.ts

@@ -0,0 +1,187 @@
+// Catches the moment a piece of our front-end code puts a JavaScript value it doesn't actually
+// have into a url.
+//
+// `element.src = someUndefinedVariable` doesn't fail: the DOM turns the value into the *text*
+// "undefined" and asks the server for a file by that name. The server can only report that some
+// file called "undefined" is missing, which tells us nothing about who asked for it - so these
+// have sat in Sentry for years without ever being traceable to a line of code (BL-16666, split from BL-16577). We
+// already fixed one instance the hard way (BL-16447, where the Adjust Timings dialog rendered
+// before its audio url was ready); this exists so the next one names itself.
+//
+// The interception below is deliberately passive: it reports and then lets the assignment
+// proceed exactly as before. What we need from it is the stack, and we get that either way, so
+// there is no reason to change what the app does.
+
+/// The exact strings JavaScript produces when these values are turned into text. We match them
+/// case-sensitively and only as a whole url or a whole path segment, so a real file that happens
+/// to be called "Undefined.png", or a folder called "undefinedThings", is left alone.
+const javascriptValueStrings = ["undefined", "null", "NaN"];
+
+/**
+ * True if this will become a url that is, or contains a path segment that is, the text form of a
+ * JavaScript value. Nothing legitimately loads such a url, so a true here is always one of our bugs.
+ *
+ * Takes the value rather than a string on purpose. At an assignment like `img.src = x` the value
+ * arrives here as the real `undefined`; it is the DOM that turns it into the text "undefined" a
+ * moment later. By the time anyone could see a string it is already too late to know who did it.
+ */
+export function isJavascriptValueUrl(url: unknown): boolean {
+    // These are exactly the values that stringify to the text we are looking for.
+    if (url === undefined || url === null) return true;
+    if (typeof url === "number") return Number.isNaN(url);
+    // Anything else that isn't a string (a URL object, say) stringifies to something real.
+    if (typeof url !== "string" || url.length === 0) return false;
+
+    // Ignore any query string or fragment; only the path can name a file.
+    const path = url.split(/[?#]/)[0];
+    return path
+        .split(/[/\\]/)
+        .some((segment) => javascriptValueStrings.includes(segment));
+}
+
+/**
+ * What we send to the server. Kept separate from the interception so it can be tested, and so the
+ * message reads the same wherever it was caught.
+ */
+export function describeBadUrl(url: unknown, whereItWasSet: string): string {
+    return (
+        `A url was built from a JavaScript value that wasn't ready: ${whereItWasSet} was set to "${String(url)}". ` +
+        `Bloom will now ask the server for a file by that name and fail. See BL-16666.`
+    );
+}
+
+type Reporter = (message: string, stack: string | undefined) => void;
+
+let installed = false;
+
+// Each report is an http post to the server, and the bug that triggers it is typically a render
+// that repeats - the Adjust Timings dialog in BL-16447 would have fired on every open. We only
+// need to learn about each site once, so report a given message once and stop altogether after a
+// handful. Losing the repeat count costs us nothing: the point is to identify the code, and the
+// server-side count in Sentry already tells us how often it happens.
+const maxReports = 10;
+const alreadyReported = new Set<string>();
+
+/** Exported only so tests can start from a clean slate. */
+export function resetReportingForTests(): void {
+    alreadyReported.clear();
+}
+
+/**
+ * Installs the interception. Safe to call more than once; only the first call does anything, so a
+ * bundle that pulls in the bootstrap twice doesn't end up double-reporting.
+ *
+ * @param report how to get the problem back to us - errorHandler's reportError in real use.
+ */
+export function installUndefinedUrlDetector(report: Reporter): void {
+    if (installed || typeof window === "undefined") return;
+    installed = true;
+
+    const check = (url: unknown, whereItWasSet: string) => {
+        if (!isJavascriptValueUrl(url)) return;
+        const message = describeBadUrl(url, whereItWasSet);
+        if (alreadyReported.has(message) || alreadyReported.size >= maxReports)
+            return;
+        alreadyReported.add(message);
+        // The stack of *this* call is the whole point: it names the line that built the url.
+        report(message, new Error().stack);
+    };
+
+    guardSrcProperty(window.HTMLImageElement, "an image's src", check);
+    guardSrcProperty(window.HTMLMediaElement, "an audio/video src", check);
+    guardSrcProperty(window.HTMLIFrameElement, "an iframe's src", check);
+    guardSrcProperty(window.HTMLScriptElement, "a script's src", check);
+    guardSetAttribute(check);
+    guardFetch(check);
+    guardXmlHttpRequest(check);
+}
+
+/**
+ * Wraps the `src` property of one element type. We call the original setter afterwards, so the
+ * element behaves exactly as it did before; we are only listening.
+ */
+function guardSrcProperty(
+    elementType: { prototype: object } | undefined,
+    description: string,
+    check: (url: unknown, whereItWasSet: string) => void,
+): void {
+    // Not every environment has every element type (jsdom, for one), and a browser could in
+    // principle define src without a setter. Nothing to wrap in that case.
+    if (!elementType) return;
+    const original = Object.getOwnPropertyDescriptor(
+        elementType.prototype,
+        "src",
+    );
+    if (!original || !original.set) return;
+
+    Object.defineProperty(elementType.prototype, "src", {
+        ...original,
+        set(value: unknown) {
+            check(value, description);
+            original.set!.call(this, value);
+        },
+    });
+}
+
+function guardSetAttribute(
+    check: (url: unknown, whereItWasSet: string) => void,
+): void {
+    const originalSetAttribute = Element.prototype.setAttribute;
+    Element.prototype.setAttribute = function (
+        name: string,
+        value: string,
+    ): void {
+        // Only src: an <a href> of "undefined" is a dead link rather than a failed request, and
+        // is not what we are hunting.
+        if (name === "src") check(value, `setAttribute("src")`);
+        originalSetAttribute.call(this, name, value);
+    };
+}
+
+/**
+ * The BL-16447 case came in through a library (WaveSurfer) fetching the url rather than putting it
+ * on an element, so element interception alone would have missed it.
+ */
+function guardFetch(
+    check: (url: unknown, whereItWasSet: string) => void,
+): void {
+    if (typeof window.fetch !== "function") return;
+    const originalFetch = window.fetch;
+    window.fetch = function (input: RequestInfo | URL, init?: RequestInit) {
+        check(urlOfFetchInput(input), "a fetch()");
+        // Call it on `window`, not on `this`. A bare `fetch(url)` inside a module gives us
+        // `this === undefined`, and the browser rejects fetch invoked on anything that isn't the
+        // window ("Illegal invocation") - which would break every fetch in Bloom.
+        return originalFetch.call(window, input, init);
+    };
+}
+
+function urlOfFetchInput(input: RequestInfo | URL): string | undefined {
+    if (typeof input === "string") return input;
+    if (input instanceof URL) return input.href;
+    // A Request object
+    if (input && typeof (input as Request).url === "string")
+        return (input as Request).url;
+    return undefined;
+}
+
+function guardXmlHttpRequest(
+    check: (url: unknown, whereItWasSet: string) => void,
+): void {
+    if (typeof XMLHttpRequest !== "function") return;
+    const originalOpen = XMLHttpRequest.prototype.open;
+    // Deliberately untyped rest args: open() has two overloads and we only care about the url.
+    XMLHttpRequest.prototype.open = function (
+        method: string,
+        url: string | URL,
+        ...rest: unknown[]
+    ) {
+        check(typeof url === "string" ? url : url?.href, "an XMLHttpRequest");
+        return (originalOpen as (...args: unknown[]) => void).call(
+            this,
+            method,
+            url,
+            ...rest,
+        );
+    };
+}

src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts

@@ -0,0 +1,150 @@
+import { beforeEach, describe, expect, it, vi } from "vitest";
+import {
+    describeBadUrl,
+    installUndefinedUrlDetector,
+    isJavascriptValueUrl,
+    resetReportingForTests,
+} from "./undefinedUrlDetector";
+
+describe("isJavascriptValueUrl", () => {
+    it("catches a url that is nothing but the value", () => {
+        expect(isJavascriptValueUrl("undefined")).toBe(true);
+        expect(isJavascriptValueUrl("null")).toBe(true);
+        expect(isJavascriptValueUrl("NaN")).toBe(true);
+    });
+
+    it("catches the value as a path segment, which is the shape we see in Sentry", () => {
+        // The real reported paths look like this, because a bare "undefined" src resolves
+        // against a document whose base is the temp folder.
+        expect(
+            isJavascriptValueUrl(
+                "C:/Users/someone/AppData/Local/Temp/undefined",
+            ),
+        ).toBe(true);
+        expect(isJavascriptValueUrl("audio/undefined")).toBe(true);
+        expect(isJavascriptValueUrl("undefined/audio/abc.mp3")).toBe(true);
+    });
+
+    it("ignores the query string and fragment", () => {
+        expect(isJavascriptValueUrl("audio/undefined?assetv=1")).toBe(true);
+        expect(isJavascriptValueUrl("audio/real.mp3?name=undefined")).toBe(
+            false,
+        );
+        expect(isJavascriptValueUrl("page.htm#undefined")).toBe(false);
+    });
+
+    it("catches the values themselves, which is what an assignment actually passes", () => {
+        // `img.src = x` hands us the real value; the DOM stringifies it afterwards. Catching it
+        // here is the only moment at which we still know who set it.
+        expect(isJavascriptValueUrl(undefined)).toBe(true);
+        expect(isJavascriptValueUrl(null)).toBe(true);
+        expect(isJavascriptValueUrl(Number.NaN)).toBe(true);
+    });
+
+    it("leaves real files and folders alone", () => {
+        expect(isJavascriptValueUrl("audio/abc123.mp3")).toBe(false);
+        expect(isJavascriptValueUrl("undefined.png")).toBe(false);
+        expect(isJavascriptValueUrl("undefinedThings/x.png")).toBe(false);
+        expect(isJavascriptValueUrl("images/Undefined")).toBe(false);
+        expect(isJavascriptValueUrl("")).toBe(false);
+        // A real number or a URL object stringifies to something legitimate.
+        expect(isJavascriptValueUrl(17)).toBe(false);
+        expect(isJavascriptValueUrl(new URL("http://x/y.png"))).toBe(false);
+    });
+});
+
+describe("describeBadUrl", () => {
+    it("names both the value and where it was set", () => {
+        const message = describeBadUrl(undefined, "an image's src");
+        expect(message).toContain("an image's src");
+        expect(message).toContain('"undefined"');
+        expect(message).toContain("BL-16666");
+    });
+});
+
+describe("installUndefinedUrlDetector", () => {
+    // Installed once for the whole file, as it is in the real app: the installer deliberately
+    // ignores repeat calls so a bundle can't double-report.
+    const report = vi.fn();
+    installUndefinedUrlDetector(report);
+
+    beforeEach(() => {
+        report.mockClear();
+        // Each test would otherwise be silenced by the previous test's identical message.
+        resetReportingForTests();
+    });
+
+    it("reports an image src assigned an undefined variable, with a stack", () => {
+        report.mockClear();
+        const image = document.createElement("img");
+        let notReadyYet: string | undefined;
+
+        image.src = notReadyYet as unknown as string;
+
+        expect(report).toHaveBeenCalledTimes(1);
+        const [message, stack] = report.mock.calls[0];
+        expect(message).toContain("an image's src");
+        // The stack is the entire point of this layer: it is what names the offending line.
+        expect(stack).toBeTruthy();
+        expect(stack).toContain("undefinedUrlDetectorSpec");
+    });
+
+    it("still actually sets the src, so nothing behaves differently", () => {
+        report.mockClear();
+        const image = document.createElement("img");
+
+        image.src = "audio/real.mp3";
+
+        expect(report).not.toHaveBeenCalled();
+        expect(image.getAttribute("src")).toBe("audio/real.mp3");
+    });
+
+    it("reports setAttribute('src', undefined) too", () => {
+        report.mockClear();
+        const image = document.createElement("img");
+
+        image.setAttribute("src", undefined as unknown as string);
+
+        expect(report).toHaveBeenCalledTimes(1);
+        expect(report.mock.calls[0][0]).toContain('setAttribute("src")');
+    });
+
+    it("leaves other attributes alone", () => {
+        report.mockClear();
+        const div = document.createElement("div");
+
+        div.setAttribute("title", "undefined");
+
+        expect(report).not.toHaveBeenCalled();
+        expect(div.getAttribute("title")).toBe("undefined");
+    });
+
+    it("reports a repeating bug only once, so a re-rendering component can't flood the server", () => {
+        const image = document.createElement("img");
+
+        image.src = undefined as unknown as string;
+        image.src = undefined as unknown as string;
+        image.src = undefined as unknown as string;
+
+        expect(report).toHaveBeenCalledTimes(1);
+    });
+
+    it("reports a fetch of an undefined url, which is how BL-16447 escaped", () => {
+        // Called bare, exactly as application code calls it - which means the wrapper receives
+        // `this === undefined` and must not pass that on to the real fetch.
+        void fetch("audio/undefined").catch(() => {
+            // jsdom has no real network; the rejection is expected and irrelevant here.
+        });
+
+        expect(report).toHaveBeenCalledTimes(1);
+        expect(report.mock.calls[0][0]).toContain("a fetch()");
+    });
+
+    it("leaves an ordinary fetch alone", () => {
+        void fetch("audio/real.mp3").catch(() => {
+            // Again, no network in jsdom.
+        });
+
+        expect(report).not.toHaveBeenCalled();
+    });
+});

src/BloomExe/web/BloomHttpListenerContext.cs

@@ -42,6 +42,13 @@ public interface IHttpListenerRequest
         string HttpMethod { get; }
         System.IO.Stream InputStream { get; }
         string RawUrl { get; }
+
+        /// <summary>
+        /// The url of the document that asked for this, or null if the browser didn't say.
+        /// Only diagnostics use this: when a request is obviously bogus, the referrer is what
+        /// tells us which of our pages produced it. See BL-16666.
+        /// </summary>
+        string Referer { get; }
         System.Uri Url { get; }
     }
 
@@ -64,6 +71,11 @@ public string ContentType
             get { return _actualRequest.ContentType; }
         }
 
+        public string Referer
+        {
+            get { return _actualRequest.Headers["Referer"]; }
+        }
+
         public bool HasEntityBody
         {
             get { return _actualRequest.HasEntityBody; }