Skip to content

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

Open
JohnThomson wants to merge 8 commits into
masterfrom
BL-16666-undefined-url-instrumentation
Open

Instrument urls built from a JavaScript undefined (BL-16666)#8175
JohnThomson wants to merge 8 commits into
masterfrom
BL-16666-undefined-url-instrumentation

Conversation

@JohnThomson

@JohnThomson JohnThomson commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

What this is

Sentry issue 2699459502 has years of "Cannot Find
File" reports for paths like C:/Users/<user>/AppData/Local/Temp/undefined.

BL-16577 framed these as C# building a temp path from a name that was undefined. That framing is
wrong
, and a guard there would catch nothing. What actually happens is that some front-end code
does the equivalent of element.src = aVariableThatIsUndefined: 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 a
file called undefined is missing — never who asked — which is why finding BL-16447, the one
instance we have fixed, took so much guessing.

This does not fix any particular instance. It makes the next one name itself.

Two other things fell out of the investigation and are worth knowing:

  • The temp folder is incidental. A bare "undefined" src resolves against the document's base url,
    and ShouldReportFailedRequest suppresses reports for anything under the current book folder — so
    these bugs are invisible when they happen on a book page. Everything reaching Sentry came from
    documents Bloom writes into temp and navigates to. We have been seeing a filtered slice.
  • React src={undefined} is safe (React omits the attribute). Only the stringifying forms bite:
    el.src = x, setAttribute("src", x), new Audio(x), and template literals.

Layer A — server side, with the referrer

Referer now flows through IHttpListenerRequestIRequestInfo → the report, so a bogus request
names the page that issued it.

undefined/null/NaN are matched as a whole path segment, case-sensitively, so a real
undefined.png, a folder called undefinedThings, or a file called Undefined are untouched.

This will increase reported volume. These reports are no longer suppressed under the current book
folder — that suppression is exactly what was hiding most instances. That is the intent, and the
extra reports should be actionable, but whoever watches Sentry should expect it.

Layer B — front-end interception, for the stack

New lib/undefinedUrlDetector.ts, installed from lib/errorHandler.ts — the module every bundle's
root already imports, so coverage is automatic. It intercepts src assignment,
setAttribute("src"), fetch and XMLHttpRequest.open, and reports with a JavaScript stack, which
names the offending line.

It is deliberately passive: it reports and lets the assignment proceed. The stack is what we
need and we get it either way, so there is no reason to change what the app does. (The plan called
for throwing in Debug/Alpha; I left that out for the same reason. Easy to add.)

Two things worth knowing if you change this later:

  1. The check takes the value, not a string. At img.src = x the value arrives as the real
    undefined — the DOM stringifies it inside the native setter. My first version checked strings
    only and therefore caught nothing at the assignment site; the tests caught that.
  2. fetch/XHR had to be wrapped too. BL-16447 came in through WaveSurfer fetching the url
    rather than putting it on an element, so element interception alone would have missed the one
    case we actually know about.

It also reports each distinct problem once, capped at 10 per session — each report is an http post,
and these bugs live in components that re-render.

Worth a careful look

  • The merge with master had one conflict, in ReportMissingFile: PR Add diagnostics for missing files requested by bare name (BL-16577) #8171 (now landed) added
    GetBareNameDiagnostics to the same format call this adds extraDiagnostics to. I kept both
    they answer different questions, and a bare undefined request legitimately raises both.
  • The fetch wrapper calls through on window, not this. A bare fetch(url) in a module gives
    this === undefined, and the browser rejects fetch invoked on anything but the window — that would
    have broken every fetch in Bloom.

Testing

  • C#: 78 pass across BloomServerTests, RequestInfoTests, BloomFileLocatorTests (13 new).
  • Front end: 13 new tests in undefinedUrlDetectorSpec; full vitest suite green.
  • typecheck, lint clean; production bundle compiles via build/agent-vite.sh (worth doing — this
    touches a module every bundle imports).

Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16666 (split from
https://issues.bloomlibrary.org/youtrack/issue/BL-16577)

Devin review


This change is Reviewable

JohnThomson and others added 4 commits August 7, 2026 08:38
Sentry has years of "Cannot Find File" reports for paths like
Temp/undefined. The old card framed these as C# building a temp path from a
name that was undefined; that is wrong, and a guard there would catch
nothing. What happens is that some front-end code does the equivalent of
`element.src = aVariableThatIsUndefined`: 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 a file called "undefined" is missing, which never says who
asked - so finding BL-16447, the one instance we have fixed, took a lot of
guessing.

This does not fix any particular instance. It makes the next one name
itself, in two layers.

Server side: carry the HTTP Referer through to the report, so a bogus
request names the page that issued it. "undefined", "null" and "NaN" are
matched as a whole path segment, case-sensitively, so a real file called
undefined.png or a folder called undefinedThings is untouched. We also stop
suppressing these reports when the path is under the current book folder.
That suppression is what has been hiding most instances, since a bogus url
built by a book page resolves inside that book's folder - so expect more of
these reports, not fewer. That is the intent; they are now actionable.

Front end: a new lib/undefinedUrlDetector, installed from lib/errorHandler
because every bundle's root module already imports it. It intercepts src
assignment, setAttribute("src"), fetch and XMLHttpRequest.open and reports
with a JavaScript stack, which names the offending line. It is deliberately
passive - it reports and lets the assignment proceed - and reports each
distinct problem once, capped per session, because these bugs live in
components that re-render.

Two things worth knowing for anyone changing this later. The check takes the
value rather than a string because at `img.src = x` the value arrives as the
real undefined; the DOM stringifies it inside the native setter, and by the
time a string exists it is too late to know who set it. And fetch/XHR needed
wrapping because BL-16447 came in through WaveSurfer fetching the url rather
than putting it on an element.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…url-instrumentation

# Conflicts:
#	src/BloomExe/web/BloomServer.cs
…16666)

Two things Devin caught on the first pass.

Keep the new reports quiet. ShouldReportFailedRequest deliberately stopped
suppressing requests whose path contains a JavaScript value, so that we
finally see them. But the report they fall through to uses PassiveIf.All and
ModalIf.Beta, which means a toast on every channel and a modal on beta - so
un-suppressing them would have started interrupting ordinary users on pages
that previously failed silently. There is nothing a user can do about our
bug, so this class now reports with ModalIf.None/PassiveIf.None: still
logged and still sent to Sentry, but invisible to the user. It also gets its
own short message, so it forms its own Sentry issue instead of being buried
in the general "Cannot Find File" one.

Fix the spec assertion. When the card references were repointed from
BL-16577 to BL-16666, the message in describeBadUrl changed but the spec's
assertion did not, so that test could never pass. The C# suite was re-run
after that rename but the front-end suite was not, which is exactly how it
slipped through.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts
Comment thread src/BloomExe/web/BloomServer.cs
Comment thread src/BloomBrowserUI/lib/undefinedUrlDetector.ts
Devin caught that the merge left a contradiction. The JavaScript-value
branch returns early, so by the time control reaches the later branches
extraDiagnostics is always empty - which meant the "both kinds of detail"
comment was false, the bare-name detail never reached a report about a
JavaScript-value url, and the general branch had gained an argument that
could never be filled.

Now the early branch emits both (a bare "undefined" genuinely has no
directory AND is a JavaScript value), and the later branches ask only for
the bare-name detail, which is the only one they can ever have.

Also records what the front-end detector does and does not cover: it patches
the prototypes of the window it is installed in, so a frame gets it by
running one of our bundles, but parent-frame code reaching into a child
frame's document is not seen. Worth knowing before reading a silence there
as proof that nothing went wrong.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/BloomExe/web/BloomServer.cs
Comment thread src/BloomBrowserUI/lib/undefinedUrlDetector.ts
…L-16666)

Devin's point, and a good one: ShouldReportFailedRequest decided from
GetLocalPathWithoutQuery(info) while the quiet branch in ReportMissingFile
decided from the localPath that ProcessAnyFileContent happened to be
holding. They are the same string today, but they are computed
independently, and if they ever diverged the consequence is exactly the
thing the quiet branch exists to prevent: the request would fall through to
the ModalIf.Beta/PassiveIf.All branch and interrupt the user.

Both now ask IsJavascriptValueRequest(info), so they cannot disagree.
ReportMissingFile takes the request rather than a pre-computed diagnostics
string, which is what made the divergence possible.

Also adds the missing explanatory comments on three helpers in the front-end
detector, per AGENTS.md.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/BloomExe/web/BloomServer.cs
Comment thread src/BloomBrowserUI/lib/undefinedUrlDetector.ts
…(BL-16666)

Devin's point: errorHandler runs unhandled-error stacks through
stacktrace-js so the report names a line in our source, but the new detector
was handing over a raw stack, which points into a bundle. For a feature
whose entire purpose is to name the line that built the bad url, that mostly
defeats it.

The detector now hands over the Error itself rather than its stack text, and
errorHandler - which already owns the mapping - maps it before reporting,
falling back to the raw stack if the mapping fails. 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 deliberately quiet diagnostics,
so one report per problem is enough.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/BloomBrowserUI/lib/errorHandler.ts
Comment thread src/BloomBrowserUI/lib/undefinedUrlDetector.ts
Comment thread src/BloomExe/web/BloomServer.cs
Devin: "Only the first bad-url report of each kind is ever sent, so later
problems stay invisible." Right, and it mattered. Every component that sets
an image src to undefined produces the same message, so keying the
report-once rule on the message meant we would report whichever component
ran first and hide every other one for the rest of the session - which is
precisely the blind spot this feature exists to remove.

The key is now the message plus the stack. Deliberately the whole stack
rather than an attempt 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
that two different call sites share, silently reinstating the blind spot. I
tried the clever version first and a test caught it doing exactly that.

That leaves the hard cap as the real flood protection rather than the
collapse, which the reworked test now says out loud - identical repeats do
still collapse, but only when the stack is identical, which a re-render
gives us and a caller at a different line does not. The cap bounds it either
way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/BloomBrowserUI/lib/undefinedUrlDetector.ts
Comment thread src/BloomExe/web/BloomServer.cs
Comment thread src/BloomBrowserUI/lib/errorHandler.ts
@JohnThomson
JohnThomson marked this pull request as ready for review August 7, 2026 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant