fix(deps): raise the next floor past the three middleware-bypass advisories - #323
Merged
Conversation
…sories Raises next from ^16.1.6 to ^16.3.0. Closes GHSA-26hh-7cqf-hhc6, GHSA-492v-c6pp-mqqv and GHSA-267c-6grr-h53f (the CVE-2026-44573/44574/44575 trio), all Middleware / Proxy bypass advisories against src/proxy.ts, the file Phase 1 (#321) hardened with RBAC, the Origin check, rate limiting, the security headers and audit emission. The floor moves in package.json, not only in bun.lock: @libredb/studio publishes next as a runtime dependency, so an embedder resolving ^16.1.6 independently could still install a vulnerable 16.1.x. bun audit before: next >=16.0.0 <16.2.5, 11 advisories, 3 Middleware / Proxy bypass. bun audit after: next no longer appears in the report.
Next 16.3.0 shifted the timing of the App Router's accessibility route-announcer (the hidden shadow-DOM live region rendered for screen readers on client navigation) relative to 16.1.6. This app sets no per-route <title>, so the announcer's h1-fallback text now settles to 'Admin Dashboard' inside the assertion's polling window, and the bare 'text=Admin Dashboard' locator resolves to two elements - the visible h1 and the announcer - failing with a strict-mode violation. Verified with A/B testing directly against this locator: 0/24 failures across repeated runs on next@16.1.6, 9/9 failures on next@16.3.0 before this change, 40/40 passes on next@16.3.0 after it. The route-announcer component's own source is byte-identical between the two next versions, so the shift is in the router's surrounding timing, not this component. Scoping to the heading role is also consistent with how the rest of this spec file already queries by role.
eslint-config-next stayed pinned at 16.1.6 after the next bump - the caret did not carry it forward on its own relock, leaving the lint layer reasoning about a framework version we no longer run. Bumped to ^16.3.0 and relocked. The bumped eslint-plugin-react-hooks that ships inside it adds a new react-hooks/set-state-in-effect rule, which flagged two calls in use-tab-manager.ts's load/ready hydration handshake. Both are a deliberate two-effect sequencing pattern that predates this change (see the existing LOAD EFFECT / READY EFFECT comments) with no derivable-during- render alternative, so both are suppressed at the call site with a comment explaining why, rather than reshaping a working, tested hook to satisfy a new rule. No other new findings surfaced; lint is back to the pre-existing 77 warnings / 0 errors baseline.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Raises the Next.js dependency floor to 16.3.0, addressing middleware-bypass advisories while preserving framework and lint compatibility.
Changes:
- Updates Next.js and its ESLint configuration to 16.3.0.
- Regenerates the lockfile and transitive dependencies.
- Adjusts an E2E locator and documents intentional hook lint suppressions.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
package.json |
Raises Next.js and ESLint configuration floors. |
bun.lock |
Resolves updated framework dependencies. |
e2e/admin-dashboard.spec.ts |
Uses an unambiguous heading locator. |
src/hooks/use-tab-manager.ts |
Documents required effect-state lint suppressions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cevheri
added a commit
that referenced
this pull request
Aug 9, 2026
The rebase onto main pulled in #323 (next 16.1.6 -> 16.3.0), which is exactly the deferred work C7 described. All three applicable middleware- bypass advisories (CVE-2026-44575, CVE-2026-45109, CVE-2026-44574) are fixed by 16.2.5/16.2.6, both below the now-locked 16.3.0 - verified against bun.lock. Deleted the entry per this file's own convention (delete an entry when the work lands) rather than leave a corrected-but- already-resolved decision record; #323's own post-merge correction note carries the CVE/GHSA mapping history. Renumbered the former C8 to C7.
cevheri
added a commit
that referenced
this pull request
Aug 9, 2026
…build (Phase 2) (#322) * feat(security): classify the repository's secret-scanning history and pin the allowlist * feat(security): add security-scan.yml with a blocking incremental secret scan * feat(security): scan lockfiles on every pull request and gate critical fixable advisories on main * feat(security): scan the published container image daily and report its OS findings * feat(security): attach an attested CycloneDX SBOM to every release * fix(security): type-check the production build instead of ignoring its errors Remove typescript.ignoreBuildErrors from next.config.ts (control 2.3). Measured before deleting: with the flag off, `bun run build` exits 0 today (zero hidden errors), and a deliberately injected type error in src/lib/security/headers.ts does fail the build with the expected "Type error: Type 'string' is not assignable to type 'number'" - so the check the flag was suppressing is real, not vacuous. What this buys is narrow, not "catches type errors in general" - both `bun run typecheck` and the build read the same tsconfig.json, so most errors were already caught by the required typecheck gate. The one gap is that `next build` regenerates .next/types/validator.ts from the current route tree before checking, while `tsc --noEmit` reads whatever the last build left on disk. A route added, renamed or removed can leave typecheck passing against stale generated types while the build's check would not. Removing the flag closes that window. Add tests/unit/next-config-typecheck.test.ts to guard against the flag being restored - it is a two-line block that looks harmless to re-add "just to see the build finish." * docs(security): document the supply-chain scans, the SBOM and the Phase 2 deferrals Resyncs the operator Helm mirror (bun run chart:bump, no version change): this branch forked before phase-1-hardening's own resync commit, so chart:check failed on the pre-existing README/NOTES drift before any Phase 2 edit. * fix(security): close the guard-not-scanner gaps the Phase 2 review found The review's structural finding: three of the four things it could break were guards, not scanners. Apply the same discipline to the YAML that protects the scanners' own configuration. - secret-scan resolves the commit range with `git rev-list --count` under `set -e`, so an unresolvable range (unreachable base.sha, shallow checkout, `.git` as a file) fails loudly instead of reaching gitleaks, which logs that same failure at ERROR and still exits 0. A new step asserts the scanned count is non-zero on a pull request. - the "never runs a container by tag" test matched only the physical `docker run --rm \` line; every image reference sits on a continuation line, so substituting a mutable tag for $TRIVY_IMAGE/$GITLEAKS_IMAGE stayed green. Replaced with a positive, whole-block assertion. - added tests/unit/trivyignore-policy.test.ts: mechanical backing for the suppression policy .trivyignore.yaml's own header and SECURITY.md promise (a statement of meaningful length, an expired_at that parses and is no more than 90 days out). - keyed the security-scan concurrency group by event as well as ref, so a push to main can no longer cancel the daily cron's image scan (same ref, different event). - the dependency gate's failure explainer now checks the gate step's own outcome instead of a bare `if: failure()`, so a Trivy DB timeout or a bun install flake no longer gets told a CRITICAL advisory is present. - the release sbom job checks for Docker Hub credentials and logs in when configured, and retries its trivy pull three times - it now sits on the release path (publish-release needs it) and had no retry. - the sbom's root component is renamed away from Trivy's default ".". - SECURITY.md now says the SBOM covers "the dependency closure of" the packaged artefacts, not the artefacts themselves, and names the bundled Node.js runtime (packaging/*/fetch-node.sh) as an undescribed gap. - docs/BACKLOG.md: relettered the Phase 2 deferrals S1-S7 to C1-C7 (the SQL section already owns S1-S8), amended C7 to name the three Next.js middleware/authorization-bypass CVEs against src/proxy.ts and record the 0.10.0 decision, and added C8 for the Node-runtime SBOM gap. - minor corrections: the gh attestation verify example now uses the <version> placeholder; the security-scan.yml comment blaming GHCR rate limits for DB downloads now names Trivy's actual mirror.gcr.io source; .gitignore no longer points at CONTRIBUTING.md commands that write no report files; the publish-release comment no longer claims release:published fires npm-publish/docker-build-push directly; added a continue-on-error absence test for dependency-scan. Every guard touched here was sabotaged and confirmed red before being reverted; see .superpowers/sdd/2026-08-09-security-phase-2/fix-wave-report.md for the full record. * fix(security): correct the round-1 explainer regression and two guard blind spots Response to the re-review of be5754d. - security-scan.yml: `if: steps.gate.outcome == 'failure'` has no status-check function, so GitHub Actions implicitly prepends `success() &&` - which is already false once the gate step has failed, so the explainer could never fire, including on a genuine CRITICAL-advisory failure on main. Fixed to `if: failure() && steps.gate.outcome == 'failure'`. - security-scan.yml: log_opts and commit_count were two independent reconstructions of "$BASE_SHA..$HEAD_SHA"; a sabotage touching only log_opts could leave commit_count non-zero while the scanner read a different, reversed range. Refactored to a single `range=` assignment that both outputs are derived from. - tests/unit/security-scan-workflow.test.ts: the pinned-image guard checked whether $TRIVY_IMAGE/$GITLEAKS_IMAGE occurred anywhere in a docker run block, so a hardcoded tag plus an unused decoy reference to the variable still read as pinned. Replaced with `imageArgument()`, which finds the token docker actually reads as the image (the first non-flag token after `docker run --rm \`) and checks that specifically. - added a general test asserting any step conditioned on another step's outcome begins with an explicit status function - the same class of bug as the explainer regression, not a patch for that one line. - added the requested one-sentence comment noting that push/schedule/ workflow_dispatch have no non-zero-scan backstop by decision (--all has no equivalent range to miscount), and a pointer to tests/unit/trivyignore-policy.test.ts from both .trivyignore.yaml's header and the dependency gate's failure explainer. Every guard touched was sabotaged and confirmed red before being reverted; see .superpowers/sdd/2026-08-09-security-phase-2/fix-wave-report.md (round 2 section) for the full record. * fix(security): fingerprint the secret-scan allowlist and close two scanner gaps SonarCloud (typescript:S5906): tests/unit/trivyignore-policy.test.ts's length assertion now reports the actual length on failure. Copilot review of #322, five findings, all fixed: - .gitleaks.toml's four path-scoped allowlists exempted every future generic-api-key/jwt/private-key/curl-auth-user finding under their path, not just the 24 historical fabricated ones. Replaced with exact commit:file:rule:startline fingerprints in a new .gitleaksignore (the actual gitleaks 8.30.1 mechanism for this - its [[allowlists]] TOML schema has no fingerprint field). Verified live: a real-shaped private key and generic API key planted under tests/ are now reported; the full history still scans clean. - security-scan.yml's --no-merges left a secret introduced only while resolving a merge conflict unscanned in both parents. Switched the pull_request range to --diff-merges=first-parent, which surfaces that resolution without re-flooding with the other side's unrelated history. Verified live against a synthetic conflict-resolution commit. - The pull request and the workflow's own section header used 'blocks' for a check that is not yet a required branch-protection check. Reworded the workflow header; PR body wording proposed separately. - SECURITY.md's image-SBOM regeneration command used a mutable tag while the prose promised an immutable digest. Resolves the digest first. - CONTRIBUTING.md and SECURITY.md called bun.lock, Cargo.lock and go.mod 'three non-npm lockfiles' - bun.lock is the npm lockfile and go.mod is a manifest, not a lockfile. Named the three ecosystems directly. tests/unit/gitleaks-config.test.ts rewritten for the new split (fingerprint classification moved out; the file now guards the shape of any future value-scoped allowlist). New tests/unit/gitleaksignore.test.ts guards the fingerprint file's shape and count. * fix(security): resolve a ReDoS, correct BACKLOG's CVE mapping, and close two more scanner gaps CodeQL (js/redos): tests/unit/gitleaksignore.test.ts's FINGERPRINT regex had a redundant, ambiguous slash-delimited group - '[^:\\s]+' already matches '/', so '(?:\\/[^:\\s]+)*' let the same character be consumed by either alternative, exponential backtracking on a long run of '/'. Removed the redundant group; the colon delimiters between fingerprint fields already make one '[^:\\s]+' per segment unambiguous. docs/BACKLOG.md C7: resolved all five 'Middleware / Proxy bypass' GHSAs against the next advisories affecting next@16.1.6 against the GitHub advisory database directly rather than trusting a copied CVE number. CVE-2026-44573 (GHSA-36qx-fr4f-26g5) was cited as one of the three that apply here; it does not - it is a Pages Router + i18n advisory and this app has no pages/ directory. The one actually missing was CVE-2026-45109 (GHSA-26hh-7cqf-hhc6), an incomplete-fix follow-up scoped to middleware.ts. Also verified CVE-2026-64642 (GHSA-6gpp-xcg3-4w24, Turbopack + single-locale i18n) does not apply - this app has no i18n config. Recorded the full table with fixed versions so a future reader does not need to re-derive it. security-scan.yml: added a direct 'git rev-parse --is-shallow-repository' guard before the --all full-history scan. That scan skips the pull-request range's non-zero-commit assertion because there is no equivalent range to miscount, but a shallow checkout would make --all scan a truncated slice and report 'no leaks found' indistinguishable from a real clean scan. Sabotage-verified: removing the guard makes its test fail. image-scan: the vuln scan and the SBOM generation each independently resolved the mutable ':latest' tag in separate docker run invocations minutes apart; a release retagging ':latest' mid-run would make them describe two different images with no shared identity. Resolved the digest once and reused it for both. release-artifacts.yml: the SBOM root-component rename set 'name' but left 'version' empty, so a CycloneDX consumer that keys a project by name+version would collapse every release's SBOM into the same unversioned project. Now sets both from the release version. tests/unit/security-scan-workflow.test.ts and tests/unit/release-sbom.test.ts updated for the new steps; tests/unit/gitleaksignore.test.ts unchanged in shape, just its regex. * docs(security): resolve BACKLOG C7 - the next bump landed via #323 The rebase onto main pulled in #323 (next 16.1.6 -> 16.3.0), which is exactly the deferred work C7 described. All three applicable middleware- bypass advisories (CVE-2026-44575, CVE-2026-45109, CVE-2026-44574) are fixed by 16.2.5/16.2.6, both below the now-locked 16.3.0 - verified against bun.lock. Deleted the entry per this file's own convention (delete an entry when the work lands) rather than leave a corrected-but- already-resolved decision record; #323's own post-merge correction note carries the CVE/GHSA mapping history. Renumbered the former C8 to C7. * fix(security): run the release SBOM's Trivy container as the invoking user, not root Copilot found a real defect that would have broken the first 0.10.0 release attempt: aquasec/trivy runs as root by default, so the SBOM it writes into the bind-mounted workspace lands on the host owned by root, mode 644. The very next step patches that same file in place with Node's fs.writeFileSync, which for an existing file opens it for write - permission a non-root runner does not have on a root-owned file. release-artifacts.yml never runs on a pull request, so none of this branch's 18 green checks exercised this path; the first execution would have been the real release, and a failed release burns a patch version rather than retrying the same tag. Reproduced against the real pinned image and real repository lockfiles (bun.lock, Cargo.lock, go.mod): the unpatched command produces a root-owned SBOM, and the exact 'Name and version the SBOM's root component' step's fs.writeFileSync then fails EACCES. Fixed with --user "$(id -u):$(id -g)" on the Trivy invocation - this scan uses --scanners license only, so it needs no vulnerability-DB cache and has no root-owned-cache fallout to work around, unlike the /root/.cache/ trivy-mounted scanners elsewhere in this repository. Verified end-to-end by extracting the actual committed step scripts from the YAML and running them in sequence against the real image: output is now owned by the invoking user, and the patch step succeeds and sets name and version correctly. Comment explains why the other two shapes from review were not taken: sudo chown is reactive and assumes passwordless sudo a self-hosted runner may not grant; a write-sibling-then-rename in the Node step needs no privilege either, but moves the fix somewhere a future editor could 'simplify' back into an in-place write without realising a permission fix depends on it. Audited all docker run invocations across security-scan.yml and release-artifacts.yml for the same class of defect (output written by a container later modified by a host-side step). This is the only one: every other scanner's output is either uploaded/cat'd read-only downstream, rewritten by another container rather than the host, or produces no file at all. tests/unit/release-sbom.test.ts: new guard test, matching the flag immediately after 'docker run --rm' rather than a bare substring - this step's own comment also contains the literal flag text in prose, so a substring check alone would still pass with the flag removed from the actual command. Sabotage-verified: removing the flag from the command (leaving the comment) fails the test; restoring it passes.
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.
What
Raises the
nextdependency floor from^16.1.6to^16.3.0inpackage.jsonand resolvesbun.lockaccordingly. This is a standalone dependency bump, deliberately kept out of the Phase 3 diff.Why this is its own pull request
src/proxy.tsis Next 16's middleware, and Phase 1 (#321) put RBAC, the Origin check, rate limiting, the security headers and audit emission into exactly that file. Three of the elevennextadvisories fixed by this bump are middleware/proxy bypasses against that surface:GHSA-26hh-7cqf-hhc6(high) - Middleware / Proxy bypass via segment-prefetch routes (incomplete-fix follow-up)GHSA-492v-c6pp-mqqv(high) - Middleware / Proxy bypass through dynamic route parameter injectionGHSA-267c-6grr-h53f(high) - Middleware / Proxy bypass via segment-prefetch routesThe floor moves in
package.json, not only inbun.lock, because@libredb/studiopublishesnextas a runtime dependency: an embedder (libredb-platform) resolving^16.1.6independently could legitimately land on a vulnerable16.1.x. Moving the manifest floor to^16.3.0is what actually closes the bypasses for every consumer, not just this repository's own lockfile.Verification
bun auditbefore:next >=16.0.0 <16.2.5, 11 advisories, 3 of them "Middleware / Proxy bypass". (These arebun audit's numbers as measured. The GitHub advisory database reports more of both - see the correction above.)bun auditafter:nextno longer appears in the report at all (0 advisories, 0 middleware/proxy-bypass hits).Local gates, all green:
format,lint,typecheck,knip,test(all 23 groups, 100% pass),build(production build succeeds; theProxy (Middleware)line is present in the route summary, confirming Next 16.3.0 still recognizessrc/proxy.tsas middleware),test:coverage+coverage:check(100.00% line coverage maintained),build:lib(tsup package build succeeds) andattw(all node16/bundler resolutions green; node10 is the pre-existing, intentionally-ignored resolution).One code change was required to accommodate a genuine framework behaviour change, and it is not in
src/proxy.tsor any of Phase 1's five controls - see "Framework behaviour change found and fixed" below.End-to-end (the acceptance criterion for this PR)
Ran the full Playwright suite against a production build, in CI's own configuration (
workers: 1,retries: 2, matchingplaywright.config.ts'sprocess.env.CIbranch), on both browser projects that matter for security coverage:chromiumandwebkit-security. Result: 0 failed, 2 flaky, 39 passed (41 tests total). The security spec is the one to read closely:e2e/security-headers.spec.ts- the CSP spec with thesecuritypolicyviolationcollector - passed on bothchromiumandwebkit-security. In every single attempt across dozens of runs during this verification (isolated, contended, serial, repeated), the collector reported an empty violation array; it was never once the reason a run failed. Two attempts in the CI-matched run needed their built-in retry, both for the same reason:getByRole("button", { name: "PNG" })(after opening the ELK-worker-backed ERD diagram) or the post-loginwaitForURLoccasionally outran its timeout budget on this sandbox's WPE-WebKit build - a UI-timing margin, not a policy violation, and it passed cleanly and quickly (chromium 3.0s, webkit 10.9s) whenever run without contention.admin-dashboard,connection-management,editor-layout,embedded-samples,explain-sqlite,export,functional-smoke,login,offline-editor,query-execution,tab-management) passed stably.No Phase 1 control - the CSP, the Origin check, the RBAC route guard, the rate limiter, or audit emission - behaved differently under 16.3.0.
Framework behaviour change found and fixed
e2e/admin-dashboard.spec.ts's "admin dashboard loads" test failed deterministically under 16.3.0:page.locator("text=Admin Dashboard")resolved to two elements - the visible<h1>and Next's hidden accessibility route-announcer (a shadow-DOM live region announcing client navigations to screen readers), whose h1-fallback text now settles to "Admin Dashboard" within the assertion's polling window (this app sets no per-route<title>, so the announcer's title-vs-h1-fallback logic was already on a knife's edge). The route-announcer component's own source is byte-identical betweennext@16.1.6andnext@16.3.0- the shift is in the surrounding router timing, not that component.Verified by A/B testing directly against this locator, isolating the framework version as the only variable:
Fixed by scoping the locator to the heading role (
page.getByRole("heading", { name: "Admin Dashboard" })), consistent with how the rest of that spec file already queries by role. This particular fix is a test locator, not application code. The PR's onlysrc/edit is two comment lines, described under Scope below; neither touches a Phase 1 control.Scope
Four files.
package.json:next^16.1.6->^16.3.0(the security bump), andeslint-config-next^16.1.6->^16.3.0alongside it - that package is versioned in lockstep withnext, and leaving it behind resolves a second copy of the shared React-hooks pluginbun.lock: resolved accordinglye2e/admin-dashboard.spec.ts: one locator scoped to the heading role (see above)src/hooks/use-tab-manager.ts: twoeslint-disable-next-line react-hooks/set-state-in-effectcomments, each carrying a written justification - see belowNo bump of this package's own
versionfield, anddocs/BACKLOG.mdis untouched (entry C7 lives on the still-open Phase 2 branch, #322): the programme's single 0.10.0 release is carried by Phase 3.The one
src/change, and why it is comment-onlyeslint-config-next@16.3.0carries a stricterreact-hooks/set-state-in-effect, which newly flags both halves ofuse-tab-manager.ts's load/ready handshake. Measured with this branch's toolchain:main's unsuppressed source reports 3 warnings in that file (the two new ones plus a pre-existingexhaustive-deps), and the branch reports 1.Two things bound this change. It failed no gate - the rule is configured
"warn"ineslint.config.mjs, andbun run lintisoxlint && eslint .with no--max-warnings, so the bump was green either way; the suppressions are hygiene, not a fix. And it is comment-only: no executable line changes, so this PR's "no Phase 1 control behaves differently" claim rests on the same evidence it did before.The suppressions are deliberate rather than reflexive.
isWorkspaceHydratedis a latch between two effects whose only job is to stop the SAVE EFFECT writing back a load in progress; it has no value derivable during render, and the flip half must observe the LOAD EFFECT'ssetTabshaving already committed. Both directives are live rather than dead weight -eslint --report-unused-disable-directives-severity errorreports neither as unused.Ten further
set-state-in-effectwarnings exist elsewhere undersrc/and are deliberately untouched: pre-existing, non-blocking, and outside a dependency bump's remit.