build(deps): clear every high and critical npm advisory, and record the ledger in docs/operations/ - #231
Conversation
`npm audit` reported 45 advisories — 1 critical, 24 high — and now reports 11, none above moderate. `package.json` moves only for `sharp`; every other version change is a lockfile resolution the committed ranges already permitted. Two of the twenty-five had no in-range escape and decided the shape of this change: - `sharp` needed 0.34.5 -> 0.35.3, a major. None of its breaking changes touches what this repository calls — `.tint().blur().jpeg()` in the post thumbnail route and the module handed to Payload as a config value — and 0.35's Node floor of >=20.9.0 sits under this project's >=24. It also drops sharp's install script, which shortens the install-time supply chain by one entry. - `next` had to reach 16.3.0 regardless: every 16.2.x through 16.2.12 pins `sharp ^0.34.5` as an optional dependency, so anything below 16.3.0 reintroduces a vulnerable sharp nested under `next` however the root manifest reads. `payload` 3.82.1 -> 3.87.1 carries six of the highs at once: 3.87.1 drops `image-size`, whose every published version is affected and which therefore has no other exit, and lifts `undici` to 7.29.0, `uuid` to 13.0.2, and `@payloadcms/drizzle`'s `drizzle-orm` to 0.45.2. The `@payloadcms/*` packages peer-depend on each other at an exact version, so `npm audit fix` and `npm update` both refuse the move — it has to happen as one resolution, which is why the lockfile is regenerated rather than patched. That takes every other in-range update with it; 31 of 61 direct dependencies move. The rest of this commit is what the regeneration required to keep the tree green. Biome 2.4.11 -> 2.5.7 rejects the 2.4 config outright, so `biome migrate` rewrites it: the schema version, `recommended: true` -> `preset: "recommended"`, and six nursery rules relocated to the `style` and `suspicious` groups they graduated into, comments intact. Its new checks then found four things — a deprecated `FormEvent` import, which React 19.2 replaces with `SubmitEvent` for a form's `onSubmit`; two spec assertions whose optional chain short-circuited into a non-optional member access; and `public/images/brush-grunge.svg`, newly in scope now that Biome checks standalone SVG assets, where an accessible name is meaningless because the file is consumed as a CSS `mask-image` and never reaches the accessibility tree. That last one is an override rather than a fix. `next.config.ts` loses `experimental.viewTransition`, which 16.3 removed; the feature is React's `<ViewTransition>` component, imported directly in `blog-post-header/loaded.tsx` and unaffected. Refs #225 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012EBvZfUJFqpE68Cqbp1fuC
The Open Graph thumbnail for a post rendered as a blank white image. The route loads its font relative to its own module, and it derived that directory from `new URL(import.meta.url).pathname` — which is the file URL's *raw* pathname, so the brackets of the `[slug]` segment it lives under stay percent-encoded. `readFile` was handed `app/(app)/posts/%5Bslug%5D/thumbnail.png/_assets/ibm-plex-sans-jp-700.ttf`, a directory that does not exist, and the render failed with ENOENT. `fileURLToPath` is the conversion that decodes them, and is what should have been used all along. The index thumbnail route shares neither the bug nor the fix: its own path contains only parentheses, which a URL pathname does not encode. Found by the e2e snapshot for `post.thumbnail`, which is exactly the failure that check exists to catch — it compared a blank frame against the committed thumbnail. Refs #225 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012EBvZfUJFqpE68Cqbp1fuC
`classifyLinkHref` classified any `http(s)` destination as external without asking whether it resolved anywhere, so `https://` — a scheme and nothing else — rendered as a live anchor with `target="_blank"` and `rel="noopener noreferrer"`. It now has to carry a host, and a host-less one is refused like any other unrenderable destination. That string is not hypothetical. `@payloadcms/richtext-lexical` 3.87.1 added `sanitizeUrl()` to its Lexical-to-markdown export, and its refusal value is the literal `https://`. So a `javascript:` destination authored through the admin or the MCP server no longer reaches this helper intact; it arrives as `https://` and, until this change, rendered as an anchor pointing at it. The dangerous protocol was still neutralized — no `javascript:` href ever reached the DOM — but the guarantee `docs/specs/markdown-rendering.md` states is stronger than that: a refused destination loses its `href`, never its text. An anchor to `https://` is not a refused destination, it is a broken external link. This restores the documented behaviour and does not depend on Payload continuing to substitute anything, since a host-less `http(s)` URL is unrenderable on its own terms. The spec's two affected rows are updated: the guarantee row names the host requirement, and the note that the CMS is not a filter records the substitution as an upstream convenience this site does not rely on. Caught by the `post.content.links` e2e scenario, which is tagged `must`. Refs #225 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012EBvZfUJFqpE68Cqbp1fuC
…l at it The only record of the outstanding advisories was a line in `.github/dependabot.yml` saying they were "their own piece of work" — which named the work but neither the reasoning nor a destination. A deferral nobody can find reads the same as an oversight, which is what #225 was filed about. `docs/operations/dependency-advisories.md` is that destination: the current counts, the standing position that no `high` or `critical` finding is left without a written reachability judgement, the reachability of each moderate and low still open, and a procedure for reconciling the ledger when the count moves. `operations/` rather than `conventions/` or `decisions/` because it is a ledger that moves with upstream, not a rule a change follows or a one-time constraint. Three things are written down because each would otherwise look like an oversight to the next reader: - **No CI step runs `npm audit`**, decided deliberately: the count is watched by tooling outside this repository, and a gate that fails on a freshly published upstream advisory turns an unrelated pull request red, which is how audit gates get switched off. - **`npm audit fix` cannot move a cross-pinned family.** The `@payloadcms/*` packages peer-depend on each other at an exact version, so npm's incremental resolver fails with `ERESOLVE` and leaves the findings standing. Regenerating the lockfile resolves them together. - **A `fixAvailable` naming a lower version is a downgrade**, reported identically to an upgrade. `@payloadcms/plugin-mcp@3.76.1` is the live example. The `tar` critical is kept as a worked example even though it is fixed, because it is the shape of judgement the ledger asks for: `re2` reaches the server graph, but `node-gyp` is its install script and the archives `tar` unpacks there are Node headers, so nothing a request influences reaches that parser. `docs/conventions/security.md` points at the ledger from its Dependencies section, and `docs/index.md` lists it under Operations. Refs #225 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012EBvZfUJFqpE68Cqbp1fuC
|
@claude review Generated by Claude Code |
🚀 Preview deploymenthttps://btnopen-pr-231.vercel.app Deployed |
Code review0 important, 0 nits. Mandatory checks (REVIEW.md):
Also independently verified (not just taken on faith from the PR body):
No bugs, logic errors, or CLAUDE.md/skill violations found in the reviewable diff (excluding |
`main` moved to a73195e, which migrated the unit suite from Jest to Vitest (#230). Everything merged cleanly except `package-lock.json`, where both sides had regenerated the tree — this branch to clear the advisories, `main` to swap `jest`/`@jest/globals` for `vite`/`vitest`. A lockfile conflict is not resolvable hunk by hunk, so the file is regenerated from the merged manifest rather than hand-merged. The result keeps this branch's `sharp ^0.35.3` and its `payload` 3.87.1 resolution and picks up `vitest` 4.1.10, and `npm audit` still reports 11 findings with nothing above moderate. Re-verified against the merged tree: lint, typecheck, `vitest run` (31 files, 325 tests), `next build`, the full Playwright suite in production mode (267 passed, no snapshot changes), and the scenario-coverage gate. Refs #225 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012EBvZfUJFqpE68Cqbp1fuC
|
@claude review Generated by Claude Code |
🚀 Preview deploymenthttps://btnopen-pr-231.vercel.app Deployed |
🚀 Preview deploymentTorn down — the isolated Turso database ( |
Code review0 important, 0 nits. Mandatory checks (REVIEW.md):
Also independently verified (not just taken on faith from the PR body):
No bugs, logic errors, or CLAUDE.md/skill violations found in the reviewable diff (excluding |
Integrates the seven commits main gained since this branch's merge base (#228, #229, #230, #231, #234, #233, #236). Merged rather than rebased: the branch carries 31 commits and several conflicting files were touched by more than one of them, so a rebase would replay each conflict once per commit. One merge resolves each once and needs no force-push. Conflict resolutions: - jest.config.cjs: deleted. #230 removed it; this branch's edits to it are obsolete under Vitest. - app/(app)/_/repositories/shared-types.ts: stays deleted. #234 deleted it on main and this branch deleted it too, so no conflict arose. - .claude/skills/project-structure/references/component-conventions.md and testing-conventions.md: accepted main's deletion. #228 retired the repository-owned skills into docs/; the rule changes relocate there. - app/(app)/layout.tsx: kept both sides. #236's AnalyticsConsentProvider, Footer, and AnalyticsConsentBanner wiring survives alongside this branch's Suspense-boundary comments, and the third bare <Suspense> #236 introduced around <Footer> gets a comment of its own. - app/(app)/_/translations/catalogs.spec.ts: took main's Vitest import with this branch's relocated relative paths. The four auto-merged files — both locale catalogs, blog-post-list/loaded.module.css, and loading-placeholder.module.css — were each read back rather than trusted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FjDKycriTUnQXxYT9mgjgu
Summary
npm auditreported 45 advisories — 1 critical, 24 high — and reports 11 after this change, none above moderate.npm audit --audit-level=highis clean.package.jsonmoves for exactly one package,sharp. Every other version change is a lockfile resolution the committed ranges already permitted.Two findings had no in-range escape, and they decided the shape of the change:
sharpneeded a major, 0.34.5 → 0.35.3. None of 0.35's breaking changes touches what this repository calls —.tint().blur().jpeg({ quality: 90 })in the post thumbnail route, and the module handed to Payload as a config value — and its Node floor of>=20.9.0sits under this project's>=24. It also removes sharp's install script, shortening the install-time supply chain by one entry.nexthad to reach 16.3.0 regardless. Every 16.2.x release through the last one, 16.2.12, pinssharp ^0.34.5as an optional dependency, so anything below 16.3.0 reintroduces a vulnerablesharpnested undernexthowever the root manifest reads.payload3.82.1 → 3.87.1 carries six of the highs at once. 3.87.1 dropsimage-size, whose every published version is affected and which therefore has no other exit, and liftsundicito 7.29.0,uuidto 13.0.2, and@payloadcms/drizzle'sdrizzle-ormto 0.45.2. The@payloadcms/*packages peer-depend on each other at an exact version, so bothnpm audit fixandnpm updaterefuse the move — it has to happen as one resolution, which is why the lockfile is regenerated rather than patched. That takes every other in-range update with it: 31 of 61 direct dependencies move.Three source changes are fallout from that, each caught by a check rather than by reading:
new URL(import.meta.url).pathname, whose brackets stay percent-encoded, so the font resolved to a%5Bslug%5Ddirectory that does not exist and the render failed with ENOENT.fileURLToPathis the conversion that decodes them. The index thumbnail route shares neither the bug nor the fix — its path has only parentheses, which a URL pathname does not encode.@payloadcms/richtext-lexical3.87.1 addedsanitizeUrl()to its Lexical-to-markdown export, whose refusal value is the literal stringhttps://. Ajavascript:destination therefore no longer reachesclassifyLinkHrefintact; it arrives ashttps://and rendered as a live anchor withtarget="_blank". The dangerous protocol was still neutralized, butdocs/specs/markdown-rendering.mdguarantees something stronger — a refused destination loses itshref, never its text — soclassifyLinkHrefnow requires anhttp(s)destination to carry a host. That holds independently of Payload continuing to substitute anything.biome migraterewrites it, relocating six nursery rules into thestyleandsuspiciousgroups they graduated into with their comments intact. Its new checks then found a deprecatedFormEventimport (React 19.2 replaces it withSubmitEventfor a form'sonSubmit), two spec assertions whose optional chain short-circuited into a non-optional member access, and one decorative SVG asset.Finally,
.github/dependabot.yml's "their own piece of work" comment now points atdocs/operations/dependency-advisories.md, which holds the counts, the reachability judgement behind everything still open, and the decision that no CI step runsnpm audit.Related issues
Closes #225
Verification
Every command was run in this session on Linux, against the committed lockfile.
npm auditnpm audit --audit-level=highnpm run lintnpm run typechecknpm run test:unitnpm run buildnpx playwright test(PLAYWRIGHT_SERVER_MODE=production)node e2e/check-scenario-coverage.mjsnpm ls sharp0.35.3, deduped undernextCI on
6d036beis green across Lint, Typecheck, Unit Tests, Payload Artifacts, Preflight, and Deploy Preview.Acceptance criteria from the issue:
npm audit --audit-level=highreports zero findings — met, so no high/critical needed a written rationale.package.jsonandpackage-lock.jsonmoved in the same commit — met (e71169d).npm run lint,typecheck,test:unit,buildpass — met.npm ls sharpresolves a single0.35.x— met.thumbnail.pnge2e checks pass with no snapshot changes — met.sharp0.35 (libvips 8.18.x) produced byte-identical thumbnails, so no image comparison was needed; the post thumbnail did have to be fixed first, for the unrelated font-path reason above.docs/operations/dependency-advisories.mdexists, is listed indocs/index.md, cross-referenced fromdocs/conventions/security.md— met.dependabot.yml's deferral comment links to it — met.Local database migrations (
npm run migrate:up) were applied before the build, sincenext buildprerenders/sitemap.xmlthrough Payload.Risks and breaking changes
The blast radius is the 31 moved direct dependencies, not the two source fixes.
payloadmoves five minors andnextone;@vercel/blob2.3.3 → 2.7.0,@clerk/nextjs7.5.19 → 7.7.1,@base-ui/react1.6.0 → 1.7.0,@playwright/test1.61.1 → 1.62.1 and@biomejs/biome2.4.11 → 2.5.7 all move too.react/react-domare pinned exactly and do not.The full e2e suite passing at 267/267 with zero snapshot drift is the strongest evidence here — a Playwright bump alongside a
sharpmajor and anextminor could each have shifted rendered pixels, and none did.Two things worth scrutiny:
classifyLinkHrefis a content-safety control, and this change widens what it refuses. It is called from bothrehypeAllowedLinkProtocolsand the link component, whichdocs/conventions/security.mdrequires to stay a single source of truth — both still call the one helper.app/(payload)/admin/importMap.jswas regenerated (npx payload generate:importmap) and gained a JSDoc type annotation from Payload 3.87.1's generator.payload/types.tsregenerated identically, and no migration was needed.Not included, and left for the maintainer to decide: Next.js 16.3's
next devwrites a<!-- BEGIN:nextjs-agent-rules -->block intoCLAUDE.mdon every run. It appeared during verification and was reverted rather than committed —CLAUDE.mdis this repository's working agreement, and vendor-injected text in it is a call for its owner, not a mechanical consequence of a bump. The cost of leaving it out is that anyone runningnpm run devwill see a dirty working tree until it is either committed or suppressed.Notes for reviewers
Read e71169d's message first — it is where the resolution reasoning lives, and the lockfile diff is unreadable without it. Then the two
fix(app)commits, which are small and independent of each other.The plan was approved at revision
sha256:5c30e6d8…, and the scope grew after that. The approved plan expectednpm audit fixto clear 24 findings without touchingpackage.json. It did not: the@payloadcms/*exact-peer cross-pin blocked the payload family, which forced the lockfile regeneration, which pulled innext16.3.0 andbiome2.5.7 and everything downstream. The maintainer was asked in-session which way to take it and chose to absorb the Biome 2.5 migration into this pull request rather than pin Biome back or leave the six payload-family highs open. The plan text in #225 was deliberately left at the approved revision rather than rewritten, so it stays a record of what was approved; this section is where the delivered scope is stated.Three judgement calls that a reviewer might reasonably decide differently:
public/**/*.svggets ana11y/noSvgWithoutTitleoverride rather than a<title>element.brush-grunge.svgis consumed as a CSSmask-image, so it never reaches the accessibility tree and has nowhere to carry an accessible name. Biome only began checking standalone SVG assets in 2.5.biome migrateappended them, at the end of their groups, rather than re-sorted alphabetically. Thesuspiciousgroup already carried a run appended by an earlier migration, so this matches the file's existing shape and keeps the diff to what the tool did.docs/operations/rather thanconventions/ordecisions/for the ledger: it moves with upstream, so it is neither a rule a change follows nor a one-time constraint.