Skip to content

MAINT: Upgrade frontend to react-router 8.3.0 - #2327

Open
romanlutz wants to merge 3 commits into
microsoft:mainfrom
romanlutz:romanlutz-react-router-v8-upgrade
Open

MAINT: Upgrade frontend to react-router 8.3.0#2327
romanlutz wants to merge 3 commits into
microsoft:mainfrom
romanlutz:romanlutz-react-router-v8-upgrade

Conversation

@romanlutz

Copy link
Copy Markdown
Contributor

Description

Resolves Dependabot alert #216 (GHSA-qwww-vcr4-c8h2, "React Router: RSC Mode CSRF Bypass Allows Action Execution Before 400 Response"), patched in 8.3.0.

Severity context, so reviewers can calibrate: the advisory explicitly states it only affects applications using the unstable RSC APIs. This frontend is a pure client-side SPA (BrowserRouter / Routes / Route / useNavigate) with no RSC and no server actions, so the vulnerability was never reachable. This upgrade clears the alert and keeps us current; it is not a patch for an exploitable hole.

The package swap

react-router-dom no longer exists in v8. Its last published version is 7.18.1 and it hard-pins react-router@7.18.1, so the dependency is replaced with react-router at 8.3.0 and the four files importing from it now import from react-router. Every API in use (Routes, Route, Navigate, useNavigate, useLocation, useSearchParams, matchPath, BrowserRouter, MemoryRouter) exists unchanged in v8; only the module specifier moved.

The part worth reviewing: ts-jest never runs astTransformers over .js

This repo has been carrying a latent trap that is independent of react-router, and it is the main thing I would like eyes on.

frontend/jest-import-meta-transformer.ts is registered via ts-jest's astTransformers and rewrites import.meta.env.X to process.env.X so CommonJS output can parse. It works for our own src/** TypeScript. It does not run over .js files at all, because ts-jest applies astTransformers to TypeScript sources only. Every ESM-only dependency in the transformIgnorePatterns allowlist has therefore been compiled with its import.meta references untouched. We got away with it only because none of the previously allowlisted packages happened to reference import.meta.

react-router does: routeModules.js guards on import.meta.hot, which becomes SyntaxError: Cannot use 'import.meta' outside a module once emitted as CommonJS. Extending the existing AST transformer does not fix this. I verified that empirically by logging every filename the transformer receives (only .ts/.tsx ever appear), including with allowJs: true plus an explicit rootDir, which additionally trips TS5011 under TypeScript 6.

The fix is frontend/jest-esm-js-transformer.cjs, a thin wrapper that neutralizes import.meta in the source text and then delegates to ts-jest. jest.config.ts now routes ^.+\.tsx?$ to ts-jest directly and ^.+\.[cm]?jsx?$ through the wrapper.

The two-entry transform is load-bearing and must not be collapsed back into a single ^.+\.[tj]sx?$ pattern. Comments in both jest.config.ts and jest-import-meta-transformer.ts now say so explicitly, so the next person adding an ESM-only dependency does not have to rediscover this the hard way.

Related detail: cookie-es (react-router's only dependency) ships dist/index.mjs, which the old pattern never matched, hence the [cm]? in the new one.

Node version

react-router@8.3.0 declares engines: { node: '>=22.22.0' }, so .github/workflows/frontend_tests.yml moves NODE_VERSION from "20" to "22" (consumed by 3 jobs). npm does not enforce engines without engine-strict, so this would not have failed loudly, but the bump keeps declared and actual versions consistent.

doc/getting_started/install_local_dev.md also told contributors "Version 18 or higher is recommended", which would now walk someone into a broken setup, so it says 22+ instead. Every other Node reference was already above the floor and needed no change: the devcontainer installs 24.x and build_scripts/prepare_package.py points contributors at 24.x.

react / react-dom at 19.2.8 already satisfy v8's >=19.2.7 peer requirement.

Lockfile note

npm install had to run through an internal registry proxy, which rewrites resolved URLs to ms-feed-*.pkgs.visualstudio.com with weak sha1 integrity. Both affected entries were normalized back to https://registry.npmjs.org/... with sha512 (each tarball's sha1 verified against npm view <pkg> dist.shasum first), so the lockfile stays portable and consistent with the other ~800 entries. frontend/.npmrc is deliberately unchanged. Verified afterwards that all resolved URLs point at registry.npmjs.org and that npm ci still validates integrity.

Tests and Documentation

No new test files. The existing Jest and Playwright suites already cover this surface; the work was making them pass against an ESM-only dependency.

Automated:

  • npm ci, npm run build, npm run lint: all clean
  • npm test: 989/989 across 50 suites
  • npm run test:coverage: thresholds pass (93.5% statements / 86.49% branches / 92.33% functions / 95.46% lines vs 85/85/90/90)
  • npx playwright test --project mock: 80/80 passing, including all 5 routing.spec.ts tests

Manual GUI testing against the real production dist/ bundle (not the dev server) with a live PyRIT backend, since a router upgrade deserves more than unit coverage:

  • All 5 sidebar routes, browser back/forward 5 deep each way (symmetric), 13 deep links including trailing slashes and bare /attacks
  • Catch-all path="*" to <Navigate to="/" replace /> for unknown paths
  • Nested /attacks/:attackId/conversations/:conversationId: both params extracted correctly, reload hydrates from the URL alone, and back/back/forward each restore the right conversation
  • useSearchParams: filters round-trip to the query string, survive reload, and the History nav breadcrumb restores them. Confirmed replace: true keeps history.length delta at 0 so filter churn does not pollute the back button
  • Onboarding tour, which drives navigate() across views. Worth flagging: the e2e config presets pyrit-tour-completed in storageState, so the suite never exercises this path. Tested by hand forward and backward through all 5 steps, zero errors
  • 40 rapid sidebar clicks plus 24 history jumps with no settle time: no errors, warnings, or leaks

On the 8.3.0 encoding change specifically: rather than reasoning from the changelog, I installed 7.18.1 and 8.3.0 side by side and diffed them on this app's exact route patterns. matchPath showed 0 differences across 20 cases (UUIDs, :, @, sub-delims, spaces, Unicode, %, #, ?, trailing slash, case sensitivity). generatePath did change in 4 cases, which is exactly the RFC 3986 pchar change, but App.tsx never calls generatePath; it uses the hand-rolled attackPath / conversationPath helpers. So the one behavior that actually changed is unreachable from this codebase.

No JupyText run: this PR touches no notebooks or Python code samples. The only doc change is a one-line Node version correction in install_local_dev.md.

Known unrelated flake

InitializerParametersDialog > submits toggled boolean and selected multiselect values fails intermittently, most often under --coverage but also on plain npm test runs. It passes 9/9 in isolation, reproduces on the unmodified baseline, and touches no react-router code. Not addressed here. Same flake is described on #2323.

Merge-order note

#2323 also touches frontend/package.json and frontend/package-lock.json. Whichever of the two lands second will need a re-run of npm install to resolve the lockfile.

Copilot AI added 3 commits August 4, 2026 08:22
react-router-dom is gone in v8, so swap the dependency for react-router and
update the four files that imported from it. This clears Dependabot alert
GHSA-qwww-vcr4-c8h2; the advisory only affects the unstable RSC APIs, which
this client-side SPA does not use.

react-router v8 ships ESM only, so it and its cookie-es dependency join the
transformIgnorePatterns allowlist. ts-jest applies astTransformers to
TypeScript sources only, which left react-router's import.meta.hot guard
intact and unparseable once emitted as CommonJS; JavaScript now goes through
a thin wrapper transformer that neutralizes import.meta first.

react-router@8.3.0 requires Node >=22.22.0, so bump the frontend workflow's
NODE_VERSION from 20 to 22.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9009f005-342b-41a1-8513-74cbf282a9a4
The split transform in jest.config.ts is load-bearing, not cosmetic: ts-jest
applies astTransformers to TypeScript sources only, so
jest-import-meta-transformer.ts silently misses every node_modules dependency.
Spell that out where someone would be tempted to merge the two patterns back
together, and note the scope limit on the AST transformer itself.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9009f005-342b-41a1-8513-74cbf282a9a4
react-router 8.3.0 declares engines node >=22.22.0, so the local dev setup
guide telling contributors that Node 18 is enough is now wrong. Other Node
references were already consistent: the devcontainer installs 24.x,
prepare_package.py points at 24.x, and CI now uses 22.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9009f005-342b-41a1-8513-74cbf282a9a4
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.

2 participants