diff --git a/src/BloomBrowserUI/lib/errorHandler.ts b/src/BloomBrowserUI/lib/errorHandler.ts index 1a4a96bb3934..d1f5b08ad281 100644 --- a/src/BloomBrowserUI/lib/errorHandler.ts +++ b/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,26 @@ 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-16666. + installUndefinedUrlDetector((message, error) => { + // Source-map the stack the same way we do for unhandled errors above. Without this the + // report points into a bundle, which would defeat the purpose: the only reason to catch + // these in the browser at all is to name the line of *our* source that built the url. + // Unlike window.onerror we don't send a preliminary unmapped report first - nothing is + // about to crash and take the mapping with it, and these are diagnostics we deliberately + // keep quiet, so one report per problem is enough. + StackTrace.fromError(error) + .then((stackframes) => + reportError( + message, + stackframes.map((sf) => sf.toString()).join("\n"), + ), + ) + .catch(() => reportError(message, error.stack)); + }); } // Saving this as it MIGHT be useful if we decide to have another go at catching diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts new file mode 100644 index 000000000000..a59ca387c994 --- /dev/null +++ b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts @@ -0,0 +1,230 @@ +// 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. +// +// Coverage, so nobody reads a silence here as proof of innocence: this patches the prototypes of +// *the window it is installed in*. Bloom's edit screen puts the book page and the toolbox in +// iframes, and elements created inside a frame are instances of that frame's own constructors. We +// are installed in every frame that runs one of our bundles, since each bundle's root module +// imports errorHandler - but parent-frame code that reaches into a child frame's document +// (`pageIframe.contentDocument.createElement("img").src = x`, a pattern bookEdit does use) is +// creating elements from the child's constructors while running in the parent, and is not seen. + +/// 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.` + ); +} + +/** + * Takes the Error rather than its stack text, so the caller can source-map it. A raw stack points + * into a bundle, which is close to useless for a feature whose whole job is to name a line of our + * source. errorHandler does that mapping, since it already owns it for window.onerror. + */ +type Reporter = (message: string, error: Error) => 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 problem 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. +// +// "A given problem" has to mean the call site, not the message. Every component that sets an +// image src to undefined produces the *same* message, so keying on that would report whichever +// one happened to run first and hide every other one for the rest of the session - which is +// exactly the kind of blind spot this whole feature exists to remove. +const maxReports = 10; +const alreadyReported = new Set(); + +/** + * What makes two reports "the same problem": the message plus the whole stack. We use the entire + * stack rather than trying to pick out "the caller" — any rule for finding that frame has to guess + * which frames are ours, and a wrong guess lands on a frame two different call sites share, which + * silently reinstates the blind spot. The stack is stable for repeats from one site, and if two + * routes reach the same bug, hearing about both is useful rather than noise. + */ +function reportKey(message: string, error: Error): string { + return message + "|" + (error.stack ?? ""); +} + +/** 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); + // The stack of *this* call is the whole point: it names the line that built the url. + const error = new Error(message); + const key = reportKey(message, error); + if (alreadyReported.has(key) || alreadyReported.size >= maxReports) + return; + alreadyReported.add(key); + report(message, error); + }; + + 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); + }, + }); +} + +/** + * The other way an element's src gets set. Wrapping `setAttribute` on `Element` covers every + * element type at once, including any we didn't wrap a `src` property for. + */ +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 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); + }; +} + +/** + * fetch() accepts a string, a URL, or a Request; pull the url out of whichever we were given so + * the check sees the same thing the network will. + */ +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; +} + +/** + * Older code (and some libraries) still request through XMLHttpRequest rather than fetch, and + * `open` is where the url is named, so that is what we wrap. + */ +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, + ); + }; +} diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts b/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts new file mode 100644 index 000000000000..b875c572d262 --- /dev/null +++ b/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts @@ -0,0 +1,183 @@ +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, error] = 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. We + // hand over the Error itself so the caller can source-map it before reporting. + expect(error).toBeInstanceOf(Error); + expect(error.stack).toBeTruthy(); + expect(error.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("stops reporting after a handful, so nothing can flood the server", () => { + // Two protections: identical repeats collapse (same message and stack), and a hard cap + // bounds everything else. The cap is what we can pin down in a test — the collapse depends + // on the stack being identical, which a loop here does give us but a caller at a different + // line would not, so the cap is what actually guarantees the bound in the wild. + for (let i = 0; i < 40; i++) { + document.createElement("img").src = undefined as unknown as string; + } + + expect(report).toHaveBeenCalledTimes(1); + + // ...and distinct problems keep being reported only up to the cap. + report.mockClear(); + resetReportingForTests(); + const sites = [ + () => (document.createElement("img").src = undefined as never), + () => (document.createElement("img").src = null as never), + () => (document.createElement("iframe").src = undefined as never), + ]; + sites.forEach((site) => site()); + expect(report).toHaveBeenCalledTimes(3); + }); + + it("still reports a second, different call site with the same message", () => { + // Every component that sets an image src to undefined produces the same message, so + // keying the once-only rule on the message alone would report whichever ran first and + // hide all the others for the rest of the session — the exact blind spot this feature + // exists to remove. These two helpers stand in for two unrelated components. + const firstComponent = () => { + document.createElement("img").src = undefined as unknown as string; + }; + const secondComponent = () => { + document.createElement("img").src = undefined as unknown as string; + }; + + firstComponent(); + secondComponent(); + + expect(report).toHaveBeenCalledTimes(2); + }); + + 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(); + }); +}); diff --git a/src/BloomExe/web/BloomHttpListenerContext.cs b/src/BloomExe/web/BloomHttpListenerContext.cs index 95fef614dd57..3e753731bac2 100644 --- a/src/BloomExe/web/BloomHttpListenerContext.cs +++ b/src/BloomExe/web/BloomHttpListenerContext.cs @@ -42,6 +42,13 @@ public interface IHttpListenerRequest string HttpMethod { get; } System.IO.Stream InputStream { get; } string RawUrl { get; } + + /// + /// 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. + /// + 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; } diff --git a/src/BloomExe/web/BloomServer.cs b/src/BloomExe/web/BloomServer.cs index 39f7adfc62c6..43ba3ec017a9 100644 --- a/src/BloomExe/web/BloomServer.cs +++ b/src/BloomExe/web/BloomServer.cs @@ -1430,7 +1430,7 @@ private bool ProcessAnyFileContent(IRequestInfo info, string localPath) { if (ShouldReportFailedRequest(info, CurrentBook?.FolderPath)) { - ReportMissingFile(localPath, path); + ReportMissingFile(localPath, path, info); } return false; // from here we head off to BloomServer.MakeReply() which now uses the same ShouldReportFailedRequest() method. } @@ -1444,13 +1444,45 @@ private bool IsAudioFileWhichCanHaveCompressedCounterpart(string path) return path.EndsWith($".{AudioRecording.kRecordableExtension}"); } - private static void ReportMissingFile(string localPath, string path) + private static void ReportMissingFile(string localPath, string path, IRequestInfo info) { if (path == null) { path = "(was null)"; } + // A url containing a JavaScript value is our bug, and there is nothing the user can do + // about it, so record it for ourselves without interrupting them. This matters because + // ShouldReportFailedRequest deliberately stopped suppressing these: without this branch, + // un-suppressing them would start showing "Cannot Find File" toasts (and, on beta, a + // modal) to users on pages that previously failed silently. The distinct message also + // gives this class its own Sentry issue rather than burying it in the general one. + // + // We ask IsJavascriptValueRequest(info) rather than testing localPath, so that this + // decision cannot disagree with the one ShouldReportFailedRequest made from the same + // request. If they ever diverged, a request it had decided to stop suppressing would + // fall through to the loud branch below - which is precisely the toast this exists to + // prevent. See BL-16666. + if (IsJavascriptValueRequest(info)) + { + NonFatalProblem.Report( + ModalIf.None, + PassiveIf.None, + "Url built from a JavaScript value", + String.Format( + "Server could not find the file {0}. LocalPath was {1}{2}{3}", + path, + localPath, + GetJavascriptValueDiagnostics(info), + // Both kinds of detail, because they answer different questions and this + // request can raise both: a bare "undefined" has no directory AND is a + // JavaScript value. + GetBareNameDiagnostics(localPath) + ) + ); + return; + } + // we have any number of incidences where something asks for a page after we've navigated from it. E.g. BL-3715, BL-3769. // I suspect our disposal algorithm is just flawed: the page is removed from the _url cache as soon as we navigated away, // which is too soon. But that will take more research and we're trying to finish 3.7. @@ -1493,6 +1525,8 @@ private static void ReportMissingFile(string localPath, string path) "Server could not find the file {0}. LocalPath was {1}{2}{3}", path, localPath, + // Not extraDiagnostics: that only ever has content for a JavaScript-value url, + // and those returned above. GetBareNameDiagnostics(localPath), Environment.NewLine ); @@ -1500,6 +1534,55 @@ private static void ReportMissingFile(string localPath, string path) } } + /// + /// True if any part of the requested path is literally "undefined", "null" or "NaN" - what a + /// JavaScript value of that kind becomes when something puts it into a url without checking. + /// No real file is named any of those, so such a request is always a front-end bug rather + /// than a genuinely missing file. See BL-16666 (split from BL-16577), and BL-16447 for one instance we have fixed. + /// + internal static bool LooksLikeAJavascriptValueInAUrl(string localPath) + { + if (String.IsNullOrEmpty(localPath)) + return false; + foreach (var segment in localPath.Split('/', '\\')) + { + // Not case-insensitive: JavaScript produces exactly these spellings, and being + // strict keeps us from flagging a book or file someone really did call "Undefined". + if (segment == "undefined" || segment == "null" || segment == "NaN") + return true; + } + return false; + } + + /// + /// Whether this request is the JavaScript-value bug above. Everything that needs to know + /// asks *this*, from the request itself, so that the decision to keep reporting it (in + /// ShouldReportFailedRequest) and the decision to report it quietly (in ReportMissingFile) + /// can never be made from two separately-derived strings and disagree. See BL-16666. + /// + private static bool IsJavascriptValueRequest(IRequestInfo info) + { + return LooksLikeAJavascriptValueInAUrl(GetLocalPathWithoutQuery(info)); + } + + /// + /// The extra detail we attach when the request looks like the JavaScript-value bug above. + /// The referrer is the whole point: it names the page that issued the bogus request, which + /// is what lets us find the code responsible. See BL-16666. + /// + private static string GetJavascriptValueDiagnostics(IRequestInfo info) + { + if (!IsJavascriptValueRequest(info)) + return ""; + return String.Format( + "{0}Part of this path is a JavaScript value turned into text, so some of our front-end code built a url out of something it did not have yet." + + " The page that asked for it was {1}. The raw url was {2}.", + Environment.NewLine, + String.IsNullOrEmpty(info.Referer) ? "(the browser did not say)" : info.Referer, + info.RawUrl + ); + } + /// /// Extra detail for the report when the request had no directory at all, e.g. "Checkbox.js". /// Those are almost always files we ship: a bundle we inject into a page at the server root @@ -2201,7 +2284,12 @@ internal async Task MakeReplyAsync(IRequestInfo info) private void ReportMissingFile(IRequestInfo info) { var localPath = GetLocalPathWithoutQuery(info); - Logger.WriteEvent("**{0}: File Missing: {1}", GetType().Name, localPath); + Logger.WriteEvent( + "**{0}: File Missing: {1}{2}", + GetType().Name, + localPath, + GetJavascriptValueDiagnostics(info) + ); } /// @@ -2226,6 +2314,13 @@ protected bool ShouldReportFailedRequest( return false; var localPath = GetLocalPathWithoutQuery(info); + + // A path containing "undefined"/"null"/"NaN" is one of our own bugs, never a file the + // user is responsible for, so none of the suppressions below should hide it - and the + // book-folder one in particular was hiding most of them, since a bogus url built by a + // book page resolves inside that book's folder. See BL-16666. + if (IsJavascriptValueRequest(info)) + return true; var localFolderTestPath = localPath; // We don't need even a toast for missing files in the book folder. That's the user's problem // and should be adequately documented by the browser message saying the file is missing. diff --git a/src/BloomExe/web/IRequestInfo.cs b/src/BloomExe/web/IRequestInfo.cs index 8f4103e55882..2b6412a4a6f1 100644 --- a/src/BloomExe/web/IRequestInfo.cs +++ b/src/BloomExe/web/IRequestInfo.cs @@ -20,6 +20,13 @@ public interface IRequestInfo string RequestContentType { get; } string ResponseContentType { set; } string RawUrl { get; } + + /// + /// The url of the document that asked for this, or null if the browser didn't say. + /// Diagnostics only - it tells us which of our pages issued a request we can't satisfy. + /// See BL-16666. + /// + string Referer { get; } bool HaveFullyProcessedRequest { get; } void WriteCompleteOutput(string s); void ReplyWithFileContent(string path, string originalPath = null); diff --git a/src/BloomExe/web/RequestInfo.cs b/src/BloomExe/web/RequestInfo.cs index ec18b134228c..8d3d3c1b05df 100644 --- a/src/BloomExe/web/RequestInfo.cs +++ b/src/BloomExe/web/RequestInfo.cs @@ -653,6 +653,11 @@ public string RawUrl get { return _actualContext.Request.RawUrl; } } + public string Referer + { + get { return _actualContext.Request.Referer; } + } + HttpMethods IRequestInfo.HttpMethod { get diff --git a/src/BloomTests/PretendRequestInfo.cs b/src/BloomTests/PretendRequestInfo.cs index 9891e1db462f..828aae0297b1 100644 --- a/src/BloomTests/PretendRequestInfo.cs +++ b/src/BloomTests/PretendRequestInfo.cs @@ -25,10 +25,12 @@ public PretendRequestInfo( string url, HttpMethods httpMethod = HttpMethods.Get, bool forPrinting = false, - bool forSrcAttr = false + bool forSrcAttr = false, + string referer = null ) { HttpMethod = httpMethod; + Referer = referer; if (forPrinting) url = url.Replace("/bloom/", "/bloom/OriginalImages/"); @@ -55,6 +57,11 @@ public PretendRequestInfo( public string LocalPathWithoutQuery { get; set; } + /// + /// The url of the document that made the request, as the constructor was told it. + /// + public string Referer { get; } + public string RequestContentType { get; } public string ResponseContentType { private get; set; } diff --git a/src/BloomTests/web/BloomServerTests.cs b/src/BloomTests/web/BloomServerTests.cs index 6912a3864623..79fc37f6dc0a 100644 --- a/src/BloomTests/web/BloomServerTests.cs +++ b/src/BloomTests/web/BloomServerTests.cs @@ -249,6 +249,76 @@ public void ReportsMissingFile() } } + /// + /// A url containing the literal text "undefined" means some front-end code built it out of a + /// JavaScript value it did not have yet - always our bug, never a file the user is missing. + /// The referrer is what tells us which page did it, so it has to reach the report. BL-16666. + /// + [Test] + public void ReportsMissingFile_UndefinedInPath_LogsTheReferringPage() + { + using (var server = CreateBloomServer()) + { + var transaction = new PretendRequestInfo( + BloomServer.ServerUrlWithBloomPrefixEndingInSlash + "audio/undefined", + referer: "http://localhost:8089/bloom/SomeBook/SomePage.htm" + ); + + server.MakeReply(transaction); + + Assert.That( + Logger.LogText, + Contains.Substring("SomeBook/SomePage.htm"), + "The referring page should be in the report; without it we cannot tell which of our pages built the bad url." + ); + Assert.That( + Logger.LogText, + Contains.Substring("JavaScript value turned into text") + ); + } + } + + /// + /// The referrer header is optional, so the diagnostics must still say something useful (and + /// not crash) when the browser doesn't send one. + /// + [Test] + public void ReportsMissingFile_UndefinedInPathAndNoReferer_StillReports() + { + using (var server = CreateBloomServer()) + { + var transaction = new PretendRequestInfo( + BloomServer.ServerUrlWithBloomPrefixEndingInSlash + "undefined" + ); + + server.MakeReply(transaction); + + Assert.That(Logger.LogText, Contains.Substring("the browser did not say")); + } + } + + [TestCase("audio/undefined", true)] + [TestCase("undefined", true)] + [TestCase("C:/Users/joe/AppData/Local/Temp/undefined", true)] + [TestCase("images/null", true)] + [TestCase("pages/NaN.htm", false, Description = "NaN.htm is a filename, not a bare value")] + [TestCase("undefined.png", false, Description = "so is undefined.png")] + [TestCase( + "audio/Undefined", + false, + Description = "JavaScript never produces this spelling" + )] + [TestCase("undefinedThings/x.png", false)] + [TestCase("myBook/audio/abc123.mp3", false)] + [TestCase("", false)] + public void LooksLikeAJavascriptValueInAUrl_Works(string localPath, bool expected) + { + Assert.That( + BloomServer.LooksLikeAJavascriptValueInAUrl(localPath), + Is.EqualTo(expected) + ); + } + [Test] public void SupportsHandlerInjection() { diff --git a/src/BloomTests/web/RequestInfoTests.cs b/src/BloomTests/web/RequestInfoTests.cs index b46b2c9bc1b3..804618121a7b 100644 --- a/src/BloomTests/web/RequestInfoTests.cs +++ b/src/BloomTests/web/RequestInfoTests.cs @@ -120,6 +120,7 @@ private class TestHttpListenerRequest : IHttpListenerRequest public string HttpMethod { get; private set; } public Stream InputStream { get; private set; } public string RawUrl { get; private set; } + public string Referer { get; set; } public Uri Url { get; private set; } public void SetRawUrl(string rawUrl)