Don't crash when window.onerror is a Firefox Restricted handler (#839) - #1488
devtools-agent[bot] wants to merge 2 commits into
Conversation
In Firefox, an onerror handler installed by a browser extension (axe
DevTools, Code Climate, content scripts using Sentry, ...) is exposed to
the page as an opaque `Restricted {}` object, and every property read on
it throws "Permission denied to access property". captureUncaughtExceptions
read `window.onerror._rollbarOldOnError` unguarded, so `new Rollbar()` /
`Rollbar.init()` threw and took the host page down with it.
- Walk the `_rollbarOldOnError` chain with a guarded read and treat an
unreadable handler as the end of the chain.
- Invoke the previous handler with Function.prototype.apply.call rather
than `old.apply`, which is itself a property read on that object.
- Guard setupUnhandledCapture in the npm/core entry point the same way the
snippet shim already does, so a failure while hooking globals is logged
as an internal error instead of thrown from the constructor.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rollbar-circleci-machine
left a comment
There was a problem hiding this comment.
AI Agent Review (openai, openai-astra)
Summary
This PR fixes #839: in Firefox, a browser extension can install an onerror handler that the page sees as a Restricted {} object, and reading any property from it throws. The PR adds _unwrapOnError(), which stops walking the _rollbarOldOnError chain when a property read throws. It also calls the old handler with Function.prototype.apply.call(old, …) so it never reads old.apply, and wraps setupUnhandledCapture() in try/catch so new Rollbar() / configure() can't throw while hooking page globals.
What I checked
globalSetup.js: the new unwrap loop matches the oldwhileloop for normal chains. The shim path (shim.js:57-58setshandler._rollbarOldOnError = window.onerrordirectly) and the core path (core.js:226, noshimflag) both end at the Restricted handler without reading any property from it.typeofand truthiness checks on that handler don't read properties either.Function.prototype.apply.callis ES5-safe. For a non-callableoldit fails the same wayold.applydid before, so nothing regresses.- Tests:
test/browser.globalSetup.test.tsmatches the WTR globs (web-test-runner.config.js:5-7), andtsconfig.test.json(non-strict,checkJs: false) covers it. The Proxy stand-in only trapsget, so[[Call]]passes through to the spy, as the tests expect. Each new core test restoreswindow.onerror/ components in afinally.
One issue (below): the new try/catch covers the whole setup routine at once. If wrapGlobals throws (the exact case the new core test simulates), the initialized flag is never set, so every later configure() repeats the partial setup. Unhandled-rejection capture is also silently skipped.
Not a finding, worth a thought: the tests run only in Chromium and use a Proxy as the stand-in, so they don't show that Firefox's real Restricted wrapper can actually be called from the page. If calling it throws ("Permission denied to access object"), globalSetup.js:71 would throw out of Rollbar's onerror after the item has been logged. A try/catch around the chained call would make this code safe either way.
No tests were run as part of this review.
Addresses review feedback on #1488. - setupUnhandledCapture now guards each hooking step on its own and sets the *Initialized flags before running them. A failure in wrapGlobals no longer skips unhandled-rejection capture, and a later configure() no longer reruns a half-finished setup, which stacked a second onerror handler and more addEventListener wrappers and re-logged the error. - _rollbarWindowOnError updates anonymousErrorsPending in a `finally`, so a chained handler that throws (including a Restricted one Firefox won't let the page call) can't skip it. The handler's own error still propagates rather than being swallowed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #839. Linear: SDK-697.
Problem
In Firefox, if a browser extension has installed its own
window.onerror(reported with axe DevTools, the Code Climate extension, and content scripts that initialize Sentry), the page doesn't get a normal function back. It gets an opaqueRestricted {}object, and any property read on it throwsPermission denied to access property "...".captureUncaughtExceptionsinsrc/browser/globalSetup.jswalks the handler chain with an unguardedwindow.onerror._rollbarOldOnError. So withcaptureUncaught: true,new Rollbar()/Rollbar.init()threw:In bundled apps (Next.js etc.) that exception escapes app bootstrap and the page renders blank. Reported on wise.com, gog.com, app.circleci.com and DatoCMS. The
<script>snippet mostly avoided this becauseshim.jswraps setup in_wrapInternalErr. The npm entry point (src/browser/core.js) had no such guard.Fix
src/browser/globalSetup.js_unwrapOnError()walks the_rollbarOldOnErrorchain with a guarded read. If a handler can't be read, it can't have been installed by Rollbar, so it's treated as the end of the chain. Rollbar still chains to it rather than silently dropping the extension's handler.Function.prototype.apply.call(old, window, args)instead ofold.apply(window, args).old.applyis itself a property read, so it would throw on the same object every time an error happened.src/browser/core.js:setupUnhandledCapture()runs each hooking step (captureUncaughtExceptions,wrapGlobals,captureUnhandledRejections) through_hookGlobals(), which catches and logs[Rollbar]: Internal error, matching the snippet shim. If hooking page globals fails for any reason, the page keeps working instead of the constructor throwing. Each step is guarded on its own, so one failure doesn't skip the others. The*Initializedflags are set before the hooks run, so a laterconfigure()doesn't rerun a half-finished setup (that would chain a second Rollbaronerrorand stack moreaddEventListenerwrappers)._rollbarWindowOnErrorupdatesanonymousErrorsPendingin afinally, so a chained handler that throws can't skip it. That covers a Restricted handler Firefox won't let the page call. The handler's own error still propagates, so real bugs in a page'sonerrorstay visible.Repro steps
Real browser (from the issue and its comments):
new Rollbar({ captureUncaught: true, ... }), e.g. an app using the npm package.[Rollbar]: Internal error Error: "Permission denied to access property "_rollbarOldOnError""or an uncaughtPermission denied..., and in bundled apps the page is blank. Disabling the extension or using a private window makes it go away.In the test suite (no Firefox or extension needed): the tests model the
Restricted {}handler as a callableProxywhosegettrap throwsPermission denied to access property "<prop>". That's exactly how the object behaves from the page's side: callable, but every property read throws.Tests
test/browser.globalSetup.test.ts(new): unit tests forcaptureUncaughtExceptions:window.onerroris Restrictedthisonerrortest/browser.core.test.ts:new Rollbar({ captureUncaught: true })with a Restrictedwindow.onerrordoesn't throw, captures a real uncaught error from the fixture page, and calls the extension's handlerwrapGlobalscomponent) doesn't propagate out of the constructor, doesn't skip unhandled-rejection capture, and isn't retried by a laterconfigure()(wrapGlobalsruns once,window.onerrorisn't replaced again)Proof the tests catch the bug:
globalSetup.jschange, the 4 Restricted-handler tests fail. The constructor throwsPermission denied to access property "_rollbarOldOnError", or, with only the core guard in place, errors are no longer captured.core.jschange, the constructor-guard test fails. Against the first revision of this PR (single try/catch), it fails because rejection capture is skipped.finallyinglobalSetup.js, the chained-handler-throws test fails (anonymousErrorsPendingstays 0).Validation
npm run test:wtr: 53 files, 628 passed (Chromium)npm run test:server: 149 passingnpx eslintandprettier --checkon the changed files: cleannpm run typecheck,npm run typecheck:tests: cleannpm run build && npm run validate:es5: ES5 check passes.dist/isn't committed; it's regenerated at release.Caveat
I couldn't verify this in real Firefox with the extension installed. The fix doesn't depend on that: every property read on the handler is now guarded. The one open question is whether Firefox lets page script call a Restricted handler. If it doesn't, that call throws out of Rollbar's
onerrorafter the item has already been reported and after Rollbar's own bookkeeping, the same as any previous handler throwing. It doesn't block init or reporting.🤖 Generated with Claude Code