Skip to content

Fix the shipped npm advisories, and actually run the UI test suite - #869

Open
potiuk wants to merge 3 commits into
apache:mainfrom
potiuk:fix/npm-advisories
Open

Fix the shipped npm advisories, and actually run the UI test suite#869
potiuk wants to merge 3 commits into
apache:mainfrom
potiuk:fix/npm-advisories

Conversation

@potiuk

@potiuk potiuk commented Aug 3, 2026

Copy link
Copy Markdown
Member

Stacked on #868 — merge that first; this branch is cut from it, so the
diff here will show its 4 commits until it lands.

Picks up the remaining non-blocking items from my 0.43.0 RC0 review, plus
two things found while fixing them.

1. npm advisories in shipped dependencies (16 → 2)

A fresh npm ci from the shipped lockfile reported 16 advisories, 7 of
them outside devDependencies. Checked each against the build sourcemap;
two reached users in the compiled bundle:

  • mdast-util-to-hast 13.1.0 — moderate, unsanitized class attribute.
    28 modules in the bundle, reached via react-markdown, which renders
    trace and chat content. Fixed by 13.2.1.
  • react-router 7.18.1 — high, RSC mode CSRF bypass.

npm audit fix (no --force) resolves everything semver-compatible. The
rest of the 16 (postcss, minimatch, glob, brace-expansion, vite)
are build-time only — confirmed 0 modules each in the shipped bundle.

The 2 that remain are react-router / react-router-dom, still inside
the advisory range at 7.18.2. The only fix is react-router 8.3.0, a major
bump. The advisory covers RSC mode, and the tracking UI is a static
client-side SPA with no RSC and no server actions, so it isn't reachable
here. I'd rather that upgrade be done on its own merits than smuggled in
as a security fix — happy to open it separately if you want it now.

LICENSE-wheel is regenerated for the two bundled version changes. Worth
noting the --check gate from #868 caught this drift on its first real
outing, which is exactly what it's for.

2. useTheme could take down the whole app

useTheme read and wrote window.localStorage directly, guarded only by
typeof window === 'undefined'. That assumption doesn't hold: a sandboxed
iframe raises SecurityError on access, browsers with storage disabled
omit it, and Node's own experimental localStorage global shadows jsdom's
under vitest. In all of those the hook throws during the initial
useState — which fails the entire <App> render, not just the theme.

Reads and writes now go through a helper that returns null when storage
is unreachable. The theme falls back to the system preference and just
isn't persisted.

3. The UI test suite was never run in CI

ui.yml builds, lints and formats, but never invokes vitest — so
App.test.tsx, the only UI test, ran nowhere. It has been failing on
newer Node with no signal at all. Added npx vitest run (plain npm test
starts watch mode and would hang the job).

Unrelated but worth a look some time: ui.yml runs lint:fix and
format:fix in CI, which modify files rather than check them, so lint
and format drift can't fail a build.

Verification

  • npm audit --omit=dev: 7 → 2, both react-router
  • npx vitest run: 1 passed (was 1 failed)
  • npm run build: clean, tsc included
  • generate_wheel_license.py --check: up to date after regeneration
  • pytest tests/: 687 passed, 4 skipped

🤖 Generated with Claude Code

@github-actions github-actions Bot added area/ui Burr UI (telemetry frontend) area/tracking Telemetry, tracing, OpenTelemetry area/ci Workflows, build, release scripts labels Aug 3, 2026
@skrawcz

skrawcz commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

@potiuk mind rebasing please / fixing the conflict?

potiuk added 3 commits August 3, 2026 11:51
A fresh `npm ci` from the shipped lockfile reported 16 advisories, 7 of
them (1 moderate, 6 high) outside devDependencies. Checking each against
the build sourcemap, two reached users in the compiled bundle:

  - mdast-util-to-hast 13.1.0, moderate, unsanitized class attribute.
    28 modules in the bundle, reached via react-markdown, which renders
    trace and chat content. Fixed by 13.2.1.
  - react-router 7.18.1, high, RSC mode CSRF bypass.

`npm audit fix` (no --force) takes care of everything semver-compatible,
including mdast-util-to-hast and postcss; the rest were build-time only
and never shipped. That drops the count from 16 to 2.

The remaining 2 are react-router / react-router-dom, still inside the
advisory range at 7.18.2. The only fix is react-router 8.3.0, a major
bump. The advisory covers RSC mode, and the tracking UI is a static
client-side SPA with no RSC and no server actions, so it is not reachable
here -- the upgrade is worth doing on its own merits, not as a security
fix bundled into a release repair.

LICENSE-wheel is regenerated for the two bundled version changes. The
--check gate added alongside it caught this drift, which is what it is
for.

Generated-by: Claude Opus 5 (1M context)
useTheme read and wrote window.localStorage directly, guarded only by a
`typeof window === 'undefined'` check. That assumption does not hold:
a sandboxed iframe raises SecurityError on access, browsers with storage
disabled omit it entirely, and Node's own experimental localStorage
global shadows jsdom's under vitest, leaving window.localStorage
undefined. In every one of those cases the hook threw during the initial
useState, which takes down the whole App render -- not just the theme.

Route both reads and writes through a helper that returns null when
storage is unreachable and swallows access errors. The theme then falls
back to the system preference and simply is not persisted.

This also makes the existing App.test.tsx pass again on Node 26.

Generated-by: Claude Opus 5 (1M context)
The Node CI workflow built, linted and formatted the UI but never ran
vitest, so App.test.tsx -- the only UI test -- was not executed anywhere.
It had been failing on newer Node with no signal.

`npm test` starts vitest in watch mode and would hang the job, so invoke
`npx vitest run` explicitly.

Generated-by: Claude Opus 5 (1M context)
@potiuk
potiuk force-pushed the fix/npm-advisories branch from b25fc03 to 5a72ce6 Compare August 3, 2026 15:42
@potiuk

potiuk commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

sure :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci Workflows, build, release scripts area/tracking Telemetry, tracing, OpenTelemetry area/ui Burr UI (telemetry frontend)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants