fix(deps): migrate react-router-dom to react-router v8 (GHSA-qwww-vcr4-c8h2)#1178
Merged
Conversation
react-router-dom is removed entirely in v8; react-router-dom@^7.18.1 is also the source of both npm audit findings for GHSA-qwww-vcr4-c8h2 (RSC-mode CSRF bypass). Charon's frontend never uses RSC/data-router APIs, so the vulnerable code path was never reachable, but upgrading is the only real fix (npm audit's own downgrade suggestion isn't a genuine patch). - Replace react-router-dom@^7.18.1 with react-router@^8.3.0 in frontend/package.json; regenerate package-lock.json. - Update every `from 'react-router-dom'` import to `from 'react-router'` across 72 files (components, pages, hooks, and their tests) plus vite.config.ts's vendor-react chunk-splitting check. - Verified against the published react-router@8.3.0 package contents that all exports this app uses — including BrowserRouter and MemoryRouter — live on the main `react-router` entry point in this version; `react-router/dom` only exports RSC-hydration APIs this app doesn't use, so no react-router/dom import was needed anywhere. - Update docs/security/vulnerability-analysis-2026-07-24.md: mark the advisory remediated, correct the earlier react-router/dom assumption with the verified finding above, and record the E2E baseline comparison used to confirm this migration introduced no regressions. frontend/npm audit is clean of GHSA-qwww-vcr4-c8h2 after this change. Claude-Session: https://claude.ai/code/session_01TJBaYmvRyNJ31KSS87qRLr
…te links Pre-existing bug found while root-causing E2E failures for the react-router migration PR (confirmed present on unmodified development via baseline comparison, unrelated to routing). CertificateService.ListCertificates() caches the "in_use" flag for up to 5 minutes (scanTTL) and InvalidateCache() was never called from the proxy host CRUD path. Linking a certificate to a new/updated/deleted proxy host therefore left the certificates list's in_use status (and the delete button's disabled/tooltip state) stale for up to 5 minutes. Add a minimal CertificateCacheInvalidator interface so ProxyHostService can invalidate CertificateService's cache without a hard dependency between the two services, wire it in routes.go where both are already constructed, and invalidate on Create/Update/Delete. Covered by new unit tests at both the service and HTTP-handler layer. Claude-Session: https://claude.ai/code/session_01TJBaYmvRyNJ31KSS87qRLr
Pre-existing bug found while root-causing E2E failures for the react-router migration PR (confirmed present on unmodified development via baseline comparison, unrelated to routing). Every certificate delete triggers a backup on the backend (CertificateHandler.Delete -> BackupService.CreateBackup), guarded by a non-blocking TryLock: only one backup can run at a time, and a losing concurrent request fails immediately with ErrBackupInProgress rather than queueing. useBulkDeleteCertificates fired all deletes at once via Promise.allSettled(uuids.map(...)), so only the first ever won the backup lock — every other cert in the batch 500'd on its backup step and silently counted as "failed" in the partial-failure toast, even though the user asked to delete them all. Await each delete in a sequential loop instead, so each backup completes (and releases the lock) before the next delete starts. Added a regression test asserting the deletes never run concurrently. Claude-Session: https://claude.ai/code/session_01TJBaYmvRyNJ31KSS87qRLr
…st bugs Found and root-caused while baseline-comparing E2E failures for the react-router migration against unmodified development (per that PR's review requirements). All confirmed present on development too via repeated isolated re-runs — none are routing regressions. - certificate-delete.spec.ts / certificate-bulk-delete.spec.ts: the shared createCustomCertViaAPI test helper round-tripped through the certificates LIST endpoint to resolve a numeric certificate ID that endpoint has never returned (CertificateInfo only exposes "uuid"), so certResult.id was always undefined. Passing that into certificate_id in a POST body meant JSON.stringify silently dropped the field, so proxy hosts created "with a certificate" never actually linked one — and every cert this helper created leaked into the shared E2E database, since deleteCertViaAPI(baseURL, undefined) 404'd silently in every afterAll cleanup. Both files now use the certificate's UUID throughout (which the backend already accepts for both certificate_id resolution and DELETE), matching the actual API contract. - certificate-delete.spec.ts / certificate-bulk-delete.spec.ts: the "no delete button for valid production LE cert" checks excluded expired certs from the assertion but not staging or expiring ones, even though frontend/src/utils/certificateUtils.ts's isDeletable() correctly treats letsencrypt-staging and status==='expiring' certs as deletable too. Broadened the exclusion regexes to match. - tests/utils/ui-helpers.ts: getToastLocator's "any toast" fallback used `[data-testid^="toast-"]`, which also matches the always- present, normally-empty `data-testid="toast-container"` wrapper the app's separate custom ToastContainer component renders. Since that element sorts first in the DOM, `.first()` locked onto it instead of the actual react-hot-toast toast. Added the same `:not([data-testid="toast-container"])` exclusion wait-helpers.ts's waitForToast already had. - proxy-groups.spec.ts: even after the locator fix above, the create- group toast check raced react-hot-toast's own 5000ms auto-dismiss duration against Playwright's global 5000ms default assertion timeout — zero shared margin for any latency between the API response and the toast actually rendering. Gave it an explicit, more generous timeout. - settings/user-lifecycle.spec.ts: the STEP 3 logout->redirect check used a 5000ms timeout, the only one in the file not already using 10000-15000ms for the identical check (see fixtures/auth-fixtures.ts and this file's own other waitForURL(/login/) calls). Aligned it with the rest of the file's convention. Claude-Session: https://claude.ai/code/session_01TJBaYmvRyNJ31KSS87qRLr
Contributor
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Contributor
✅ Supply Chain Verification Results✅ PASSED 📦 SBOM Summary
🔍 Vulnerability Scan
📎 Artifacts
Generated by Supply Chain Verification workflow • View Details |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…lder GHSA-r277-6w6q-xmqw (ValidationHandler.Load() fail-open authentication bypass via NoopAuthenticationFunc default) in github.com/getkin/kin-openapi was flagged CRITICAL by Grype against the built image, embedded in /usr/local/bin/crowdsec and /usr/local/bin/cscli. It is not a direct or transitive dependency of backend/go.mod — it's pulled in by CrowdSec's own module graph when the Dockerfile builds CrowdSec from source. Pin kin-openapi to v0.144.0 (fixed version) in the crowdsec-builder stage's existing "patch dependencies to fix CVEs in transitive dependencies" chain, matching the established pattern used for the 9 other CVEs already patched there (grpc-go, pgx/v4, aws-sdk-go-v2, go-ntlmssp, quic-go, jsonparser, etc). Verified: - Isolated `docker build --target crowdsec-builder` rebuild: `go version -m` confirms both /crowdsec-out/crowdsec and /crowdsec-out/cscli now embed kin-openapi v0.144.0. - Full image rebuild + fresh Syft SBOM + Grype scan (matching supply-chain-pr.yml's exact tool versions/config): 0 Critical, 0 High findings; kin-openapi no longer appears in the Grype match list at all. - Trivy scan (aquasec/trivy:0.72.0, --scanners vuln --severity CRITICAL,HIGH --ignorefile .trivyignore, matching docker-build.yml's "Security Scan PR Image" job) against the same image: 0 vulnerabilities across every scanned binary, including crowdsec and cscli. - Confirmed kin-openapi is not separately vendored anywhere else in the image (e.g. via Caddy's own module graph in caddy-builder) — it only appears in the crowdsec-builder stage's output binaries.
TestPendingRestoreBootSwap_AcrossRealProcessBoundary was flaky in CI, intermittently failing with: Error: file ".../charon.db-wal" exists Root cause: cmd/pending-restore-harness's boot() calls database.Connect, which launches its post-connect integrity check (PRAGMA quick_check) on a background goroutine holding its own separate SQLite connection (database.Connect's launchQuickCheck). That goroutine is fire-and-forget — Connect does not wait for it before returning. The real server (cmd/api/main.go) never exits right after Connect, so that connection eventually closes on its own, well before the process does. This test harness is different: boot() calls os.Exit shortly after Connect returns, with no server loop in between. Whether the goroutine's connection had closed (triggering SQLite's WAL checkpoint that removes -wal/-shm) by the time os.Exit ran was therefore a genuine race, not a test artifact. Fix: call database.SyncIntegrityCheckForTesting() at the top of boot(), making the check run synchronously within Connect so its connection is guaranteed closed before Connect returns. This is the same fix already applied via TestMain in this package's own database_test.go, cmd/api/main_test.go, and internal/api/handlers/testmain_test.go — but those only affect the process that calls them, and the harness runs as a separately exec'd process, so it needs its own call. Verified: go test -run TestPendingRestoreBootSwap_AcrossRealProcessBoundary -count=20 passes 20/20; also passes 10/10 under -race with CHARON_HARNESS_GOCOVERDIR set (matching the CI coverage pipeline exactly). Full backend suite (go build ./... && go test -race ./...) passes.
… image scan The "Security Scan PR Image" Trivy gate was still failing after the kin-openapi fix: its blocking SARIF-format step evaluates findings across all severities (not just the configured CRITICAL,HIGH), unlike its sibling "table output" step. The one live, unsuppressed finding was GHSA-gcjh-h69q-9w9g (cel-go JSON private fields exposed via NativeTypes/ParseStructTag("json") not honoring "json:\"-\""), MEDIUM severity, in github.com/google/cel-go v0.28.1 embedded in /usr/bin/caddy. Bumping cel-go to the v0.29.0 fix is not currently safe: Caddy v2.11.4 (the latest available Caddy release) calls interpreter.NewCall(id, function, overload, []interpreter.Interpretable, impl) in modules/caddyhttp/celmatcher.go, and v0.29.0 changed that parameter's type to []interpreter.InterpretableV2 — a superset interface that also requires an Exec(*ExecutionFrame) method. This is a genuine source-breaking change, confirmed by an isolated `docker build --target caddy-builder` rebuild that failed to compile with the naive version bump. No Caddy release newer than v2.11.4 exists yet with celmatcher.go updated for the new interpreter API, so there is no safe upgrade path today. Added a suppression to .trivyignore with full justification, following this repo's existing pattern for advisories with no safe upstream fix. Exploitability is low: Caddy's CEL `expression` matcher only evaluates admin-authored, static expressions from the Caddyfile/JSON config against Caddy's own fixed request-context fields — it never evaluates untrusted, user-submitted CEL source against attacker-controlled native struct types, which is the advisory's actual exploit precondition. Verified: rescanned the full image (charon:local) with Trivy 0.72.0 (matching docker-build.yml's pinned version) using the updated .trivyignore, both `--format json` (all severities) and `--format sarif` (matching the actual blocking CI step) — zero results in both, versus one MEDIUM result before this change. Grype rescan (matching supply-chain-pr.yml) remains 0 Critical/0 High, confirming no regression.
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
2 participants
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.
Summary
react-router-dom@^7.18.1toreact-router@^8.3.0(react-router-dom is removed entirely in v8). Fixes the twonpm auditfindings for GHSA-qwww-vcr4-c8h2 (React Router RSC-mode CSRF bypass) — Charon's frontend never uses RSC/data-router APIs so the vulnerable path was never reachable, but this is the only real fix (npm audit's suggested downgrade isn't a genuine patch).from 'react-router-dom'import tofrom 'react-router'across 72 files. Verified against the publishedreact-router@8.3.0package contents that all exports this app needs (includingBrowserRouter) live on the mainreact-routerentry point in this version —react-router/domonly exports RSC-hydration APIs this app doesn't use, so noreact-router/domimport was needed anywhere, contrary to the original risk-assessment doc's assumption (corrected in this PR).developmentvia baseline comparison — none are routing regressions, see commit messages for full root-cause detail):CertificateService's "in_use" cache was never invalidated when a proxy host's certificate link changed, leaving the delete button's disabled/tooltip state stale for up to 5 minutes.undefined, leaking test data into the shared DB on every run; incomplete cert-status exclusions in two assertions; a toast-locator collision with an unrelated always-present DOM element; and two test timeouts inconsistent with the same repo's own conventions elsewhere.Verification
go build ./...,go vet ./...,go test ./...— all pass. New unit tests added for the cache-invalidation fix (service + HTTP handler layer).npm run type-check,npm run build, full unit suite (3167 tests) — all pass, zero failures. New regression test added for the sequential-delete fix.npm auditinfrontend/— 0 vulnerabilities (both GHSA-qwww-vcr4-c8h2 findings cleared).lefthook run pre-commitandlefthook run codeql— both clean (0 lint errors, 0 blocking Semgrep/CodeQL findings for Go and JS).--project=firefox), full 940-test suite, run in CI's actual configuration (CI=true: sequential workers + automatic retries, matching.github/workflows' real execution model): 901 passed, 36 skipped, 3 flaky-but-passed-on-retry, 0 net failures.developmentrunning the identical test subset under identical conditions, confirmed as real regressions, fixed, and reverified clean across 10+ repeated runs (isolated and full-file) before being folded into the pre-existing-bugs commit alongside the other fixes above.Test plan
cd frontend && npm audit— cleancd backend && go test ./...— passcd frontend && npm run test:ci— pass (3167/3167)npx playwright test --project=firefox(CI-mode) — pass (901 passed, 36 skipped, 0 failed)lefthook run pre-commit/lefthook run codeql— cleanSee
docs/security/vulnerability-analysis-2026-07-24.mdfor the original CVE risk assessment and remediation record.