MAINT: Upgrade frontend to react-router 8.3.0 - #2327
Open
romanlutz wants to merge 3 commits into
Open
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-domno longer exists in v8. Its last published version is 7.18.1 and it hard-pinsreact-router@7.18.1, so the dependency is replaced withreact-routerat 8.3.0 and the four files importing from it now import fromreact-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.tsis registered via ts-jest'sastTransformersand rewritesimport.meta.env.Xtoprocess.env.Xso CommonJS output can parse. It works for our ownsrc/**TypeScript. It does not run over.jsfiles at all, because ts-jest appliesastTransformersto TypeScript sources only. Every ESM-only dependency in thetransformIgnorePatternsallowlist has therefore been compiled with itsimport.metareferences untouched. We got away with it only because none of the previously allowlisted packages happened to referenceimport.meta.react-router does:
routeModules.jsguards onimport.meta.hot, which becomesSyntaxError: Cannot use 'import.meta' outside a moduleonce 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/.tsxever appear), including withallowJs: trueplus an explicitrootDir, which additionally trips TS5011 under TypeScript 6.The fix is
frontend/jest-esm-js-transformer.cjs, a thin wrapper that neutralizesimport.metain the source text and then delegates to ts-jest.jest.config.tsnow 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 bothjest.config.tsandjest-import-meta-transformer.tsnow 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) shipsdist/index.mjs, which the old pattern never matched, hence the[cm]?in the new one.Node version
react-router@8.3.0declaresengines: { node: '>=22.22.0' }, so.github/workflows/frontend_tests.ymlmovesNODE_VERSIONfrom"20"to"22"(consumed by 3 jobs). npm does not enforceengineswithoutengine-strict, so this would not have failed loudly, but the bump keeps declared and actual versions consistent.doc/getting_started/install_local_dev.mdalso 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 andbuild_scripts/prepare_package.pypoints contributors at 24.x.react/react-domat 19.2.8 already satisfy v8's>=19.2.7peer requirement.Lockfile note
npm installhad to run through an internal registry proxy, which rewritesresolvedURLs toms-feed-*.pkgs.visualstudio.comwith weak sha1 integrity. Both affected entries were normalized back tohttps://registry.npmjs.org/...with sha512 (each tarball's sha1 verified againstnpm view <pkg> dist.shasumfirst), so the lockfile stays portable and consistent with the other ~800 entries.frontend/.npmrcis deliberately unchanged. Verified afterwards that allresolvedURLs point at registry.npmjs.org and thatnpm cistill 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 cleannpm test: 989/989 across 50 suitesnpm 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 5routing.spec.tstestsManual 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:/attackspath="*"to<Navigate to="/" replace />for unknown paths/attacks/:attackId/conversations/:conversationId: both params extracted correctly, reload hydrates from the URL alone, and back/back/forward each restore the right conversationuseSearchParams: filters round-trip to the query string, survive reload, and the History nav breadcrumb restores them. Confirmedreplace: truekeepshistory.lengthdelta at 0 so filter churn does not pollute the back buttonnavigate()across views. Worth flagging: the e2e config presetspyrit-tour-completedinstorageState, so the suite never exercises this path. Tested by hand forward and backward through all 5 steps, zero errorsOn 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.
matchPathshowed 0 differences across 20 cases (UUIDs,:,@, sub-delims, spaces, Unicode,%,#,?, trailing slash, case sensitivity).generatePathdid change in 4 cases, which is exactly the RFC 3986 pchar change, butApp.tsxnever callsgeneratePath; it uses the hand-rolledattackPath/conversationPathhelpers. 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 valuesfails intermittently, most often under--coveragebut also on plainnpm testruns. 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.jsonandfrontend/package-lock.json. Whichever of the two lands second will need a re-run ofnpm installto resolve the lockfile.