release: v2026.09.13.1431 - #30
Merged
Merged
Conversation
## Summary
Establishes the two-trunk flow: feature branches off `dev`, `dev` as
staging, `dev` → `main` as the production promotion.
```
feature/fix --(squash PR)--> dev --(merge PR + tag)--> main
staging production
```
## Changes in this PR
- **`.github/workflows/test.yml`** — run tests on push/PR to `dev` as
well as `main`. Previously `main`-only, which under the new flow would
have left the staging trunk completely ungated.
- **`CLAUDE.md`** — new *Branching & Release* section documenting the
flow, protection rules, and hotfix path.
- **`.claude/commands/release.md`** — `/release` now promotes `dev` →
`main` via a merge-commit PR before tagging, and passes `--target main`
explicitly (required now that `dev` is the default branch).
## Repo-side config (already applied)
- Default branch → `dev`
- Squash + merge commit enabled, rebase disabled, auto-delete merged
branches
- Ruleset **`dev-staging`**: PR required, `test` must pass, squash-only,
no force-push, no deletion
- Ruleset **`main-production`**: PR required, `test` must pass,
merge-commit-only, no force-push, no deletion
This PR is itself the first run through the new flow.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
`getCurrentUserProfile(id)` accepted a caller-supplied MP `User_GUID` behind
only a "a session exists" check. A server action is a callable POST endpoint
whose payload the caller shapes, so any authenticated MP user could read any
other user's name, email, mobile phone, photo GUID, and full role and
user-group list.
The parameter is removed rather than validated, making the rule-12 carve-out
("the user's own profile") enforceable by the type signature. The guard moves
from `session?.user?.id` to a non-empty `userGuid` check, which fails closed.
Also gates `generateSampleTemplate` (an undocumented fifth carve-out) with the
address-label tool's gate, and deletes the dead `getCurrentUserIdFromSession`
action and `UserService.getUserIdByGuid`. The deleted action was an exported
endpoint taking a session object as a parameter, whose docstring contradicted
rule 13 by claiming to be the canonical source of `$userId`.
Verified by negative control: restoring the vulnerable body fails 4 of 6 tests.
805 tests pass, eslint clean, build green.
Follow-up: ~20 reference mentions of the now-deleted `getUserIdByGuid` remain.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… 5 stack (#25) Full dependency audit: in-range updates, per-major evaluation, an advisory sweep beyond `npm audit`, and a repeatable process so this does not have to be re-derived next time. ## Security: 14 → 0 Every advisory was cleared by in-range updates; **no major bump was required for security**. The headline is **`next` 16.2.10 → 16.3.5**, which carried a **critical** advisory plus ten others. Three matter structurally for this app: - [GHSA-p293-qw3h-jr36](GHSA-p293-qw3h-jr36) — **critical**, unauthenticated RCE on **Windows-hosted** servers. - [GHSA-6gpp-xcg3-4w24](GHSA-6gpp-xcg3-4w24) — proxy/middleware bypass in App Router + Turbopack. Route protection lives in `src/proxy.ts`, so a bypass here is an authentication bypass. - [GHSA-955p-x3mx-jcvp](GHSA-955p-x3mx-jcvp) — unauthenticated disclosure of internal Server Function endpoints, which is where MP data access lives. Also closed: `postcss` (XSS + `sourceMappingURL` arbitrary `.map` read), `@xmldom/xmldom` via docxtemplater, `undici`, `sharp`, `svgo`, `js-yaml`, `nanoid`, `@vitest/mocker`. ### Sweep beyond `npm audit` `npm audit` reports only GitHub-**reviewed** advisories, so all **472** packages in the resolved tree were additionally queried against OSV.dev. One hit, **confirmed false positive**: `grapesjs@0.22.16` → GHSA-589f-c66p-hxr4. The advisory text says fixed in 0.19.5; the machine-readable record declares `{"introduced": "0"}` with **no `fixed` event**, so it matches every version ever published. Documented so it is not re-triaged. Supply chain: no deprecated packages in the tree. `zod@4.6.4` was published the **same day** it was installed — verified before accepting (sole maintainer `colinhacks`, published by GitHub Actions with npm provenance attestations, zero runtime deps, no install scripts). ## Majors adopted Vitest 5 + `@vitest/coverage-v8` 5, jsdom 30, jest-dom 7, chalk 6 — each applied and verified **individually**. Two notes worth reading: - **jest-dom 7 needed a source change.** It moved Vitest `expect` type augmentation to the `@testing-library/jest-dom/vitest` entry point. The bare import still registers matchers at runtime, so **805 tests passed while `next build` gained 60 type errors**. `src/test-setup.ts` now imports the `/vitest` entry. - **Vitest 5 relocated reporter output to `.vitest/`.** Coverage was not moved, but it was verified explicitly that `test:coverage` still writes `coverage/coverage-final.json` — the exact path `test.yml` uploads to Codecov. Had it moved, CI would have gone green while uploading nothing. `engines.node`: `>=20` → `^22.22.2 || ^24.15.0 || >=26.0.0`, mirroring the strictest dev dependency (jsdom 30). Node 20 reached EOL in April 2026; CI already runs Node 24. ## Majors held back Each recorded with the upstream condition that clears it and a one-line re-check command: | Package | Blocked by | |---|---| | `typescript` 7 | No stable Compiler API until 7.1; `typescript-eslint` peers `typescript: >=4.8.4 <6.1.0` | | `eslint` 10 | Attempted and reverted — `eslint-plugin-react` calls a removed rule-context method. `7.37.5` is the **latest** release and peers `eslint ^9.7`, so no npm `override` fixes it | | `grapesjs` 0.23 | `@grapesjs/react@2.0.0` (latest) peers `grapesjs ^0.22.5` | ## Removed — 5 unreferenced packages `openai` (zero imports — also makes the pending 6 → 7 major moot), `@types/js-cookie` (runtime package not installed, and misfiled under `dependencies`), `@types/react-syntax-highlighter` (runtime package not installed), `autoprefixer` (not in `postcss.config.mjs`; Tailwind v4 prefixes via Lightning CSS), `@tailwindcss/typography` (never registered with `@plugin`, no `prose` usage). ## Process - **`/update-deps`** (`.claude/commands/update-deps.md`) makes this repeatable. - **`.claude/packages/`** holds one record per audit. The "held back" section is the point — it keeps the next audit from re-deriving the same blockers. - `CLAUDE.md` gains a Dependencies section with the audit history and current holds. `.gitignore` was silently swallowing `.claude/packages/` via a NuGet rule inherited from the Visual Studio template (`**/[Pp]ackages/*`); negated. ## Pre-existing, NOT fixed here `npm run build` already fails on `dev` with **12 type errors** in three test files (MP fixtures using snake_case wire names against camelCase types). Confirmed pre-existing — reproduced when only `package.json`/`package-lock.json` differed. **CI does not catch this**: `test.yml` runs only `npm run test:coverage`, never `npm run build`. Being handled separately. Adding `npm run build` (or `tsc --noEmit`) to CI afterwards would stop it regressing. ## Verification | | Before | After | |---|---|---| | `npm audit` | 14 (1 critical, 9 high, 4 moderate) | **0** | | Tests | 805 passing / 50 files | **805 passing / 50 files** | | `eslint .` | clean | clean | | `next build` type errors | 12 (pre-existing) | 12 (unchanged) | | Direct dependencies | 60 | 55 | 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The F-UPDATE-USER advisory requires the disabled endpoints to 404 at the Better
Auth router. This repo asserted only that `disabledAuthPaths` — an exported
array constant — contains them. Nothing tied that array to the running router:
commenting out `disabledPaths: disabledAuthPaths` deleted the defence-in-depth
control and all 805 tests stayed green. Verified by doing it, not inferred.
`route.test.ts` did not cover the gap either, contrary to how it reads — it
mocks `@/lib/auth` to `{}` and mocks `toNextJsHandler`, so it exercises the
allowlist wrapper against a stub and never reaches real Better Auth.
These drive `auth.handler` with real Requests, bypassing Next.js routing and the
allowlist, since the advisory is explicit that the allowlist is an addition to
`disabledPaths` and not a replacement — both must hold independently.
- POST /update-user with a well-formed foreign User_GUID -> 404
- every other disabledAuthPaths entry, table-driven -> 404
- CONTROL: /get-session is NOT 404, so the block cannot pass vacuously
Unwiring disabledPaths now fails 4 tests, including /update-user. /set-password
and /delete-user/callback pass in both states (Better Auth does not mount them
without an email/password provider), so they assert intent, not protection.
812 tests pass, eslint clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pins the project to a single Node major so local, CI, and Vercel all run
the same runtime.
## Why
`engines.node` was a three-arm range (`^22.22.2 || ^24.15.0 ||
>=26.0.0`), which let every environment pick a different arm. Vercel
only offers **major** versions — `24.x` (its current default), `22.x`,
`20.x` — and `engines.node` **overrides** the project's Build &
Deployment setting, so the deployed runtime was effectively 24 while
nothing else guaranteed it.
`@types/node` was at `^26.5.1`, two majors ahead of what actually runs.
Typings for APIs that don't exist in the deployed Node compile clean and
fail at request time.
## Changes
| File | Change |
|---|---|
| `package.json` | `engines.node` → **`^24.15.0`**; `@types/node`
`^26.5.1` → **`^24.13.4`** |
| `.nvmrc` (new) | `24` |
| `.github/workflows/test.yml` | `node-version: 24` →
`node-version-file: .nvmrc` — removes the second hardcoded copy |
| `scripts/setup.ts` | `REQUIRED_NODE_VERSION = 20` →
`REQUIRED_NODE_MAJOR`/`MINOR` (24/15); `getNodeVersion()` returns `{
major, minor }`. Now rejects **any** major other than 24, not just older
ones — someone on 26 previously passed setup while deploying to 24. |
| `CLAUDE.md` | Node bullet rewritten with the pin and the condition
that releases it; `@types/node` 25+ row added to **Current holds** |
| `README.md` | Prerequisite is now v24.15.0+ on the 24.x line,
`.nvmrc`-driven |
The `24.15` floor is jsdom 30's requirement (strictest dev dependency);
every `24.x` Vercel ships satisfies it.
## Vercel coverage
`engines.node: ^24.15.0` resolves to the latest `24.x` on Vercel and
overrides the dashboard setting, covering **both builds and functions**.
There is no Node-version field in `vercel.json`, so no such file was
added — `engines` is the correct and complete mechanism. See [Supported
Node.js
versions](https://vercel.com/docs/functions/runtimes/node-js/node-js-versions).
## Hold condition
Hold this pin until Vercel's default Node version moves forward. Then
bump `engines.node`, `.nvmrc`, `@types/node`, and `REQUIRED_NODE_MAJOR`
in `scripts/setup.ts` **together**.
## Verification
- `npm install` clean — 0 vulnerabilities
- `npm run test:run` — 812 tests / 50 files pass
- `eslint .` — clean
- `npm run setup:check` — `[1/8] Node.js version... ✓ v24.18.0`
⚠️ Pre-existing, unrelated: `tsc --noEmit` reports 18 type errors in MP
provider test files (`CommunicationInfo` / `MessageInfo` /
`FileUploadParams` shape mismatches). The count is **identical before
and after** this change. Since CI gates tests only, `npm run build` is
already broken on `dev` independently of this PR.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#28) ## The reported coverage number was wrong `vitest.config.ts` set no `coverage.include`, so the v8 provider only instrumented files that some test had already imported. **131 of 176 source files were absent from the report entirely** rather than counted as 0%. It read 83.14% while true coverage was 48.41% — and this is the report CI uploads to Codecov. That's how `src/services/familyService.ts` (756 lines) and `src/app/(web)/tools/layout.tsx` (the `/tools` authorization gate) sat at zero tests without anyone seeing it. ## Results Measured on the same denominator before and after: | Metric | Before | After | |---|---|---| | Statements | 49.67% (1,814/3,652) | **98.84%** (3,610/3,652) | | Lines | 50.20% | **99.70%** (3,363/3,373) | | Branches | 37.51% | **92.21%** | | Functions | 38.35% | **98.81%** | | Test files | 50 | **116** | | Test cases | 812 | **1,535** | Scope excludes vendored shadcn/ui primitives, generated MP models, build-time scripts, `loading.tsx` skeletons, barrels, and type-only modules. ## Config changes - **`coverage.include: ['src/**/*.{ts,tsx}']`** — the actual fix. Worth knowing: `coverage.all` was **removed in Vitest 5**, so setting it is a type error that does nothing. `include` is the only lever. - **Directory exclusions now end in `**`.** A bare `'src/components/ui/'` matches nothing, which had quietly kept those files in the denominator. - **`thresholds: { statements: 97, lines: 98 }`**, enforced by the existing `test:coverage` CI job. Branches and functions are deliberately unenforced — a single defensive guard can trip them, and a threshold people learn to override is worse than none. I verified the gate actually fires by setting it to an impossible value and confirming exit 1. - **`vitest.config.ts` → `.mts`.** As `.ts` it loaded as CommonJS and Vite warned it breaks when `configLoader: 'native'` becomes the default. The rename exposed a second instance of the same problem: `__dirname` only worked *because* of the CJS load. As real ESM it would have thrown at config load and stopped every test from starting. Now `import.meta.dirname`. ## `npm run build` was already broken on `dev` `tsconfig.json` includes `**/*.ts`, `next.config.ts` sets no `ignoreBuildErrors`, and CI never runs `next build` — so two committed test files failed type checking while CI stayed green. Their fixtures built `CommunicationInfo` / `MessageInfo` / `FileUploadParams` values from raw MP column names (`Author_User_ID`, `From_Contact`) instead of the DTO fields the helper requires. The tests passed because the mocks never inspected the missing fields. Fixtures now carry explicit type annotations so the compiler enforces the shape. **Build succeeds.** ## Runner output: 1,507 lines → 17, zero warnings - Barcode tests mocked react-pdf's `View` as the string `'View'`, which React read as a capitalised unknown tag — two warnings per element, **797 lines** for one 65-bar barcode. Now `rpdf-view`; the dash matters, since any undashed unknown tag still draws "unrecognized in this browser". - Added console spies to the 15 test files that deliberately drive error paths. The MP logger is unconditional by design, so those were expected logs plus stack traces. The spies use `mockImplementation`, so they still record and assertions on logged content keep working. ## One source fix (14 lines) `Select` fields in the group wizard and Add/Edit Family passed `value={... : undefined}`, leaving them uncontrolled until first selection and logging a React controlled/uncontrolled warning. Silencing that in tests would have hidden a real bug: while a field is uncontrolled, React state is not the source of truth for it, so `form.reset()` between wizard runs isn't guaranteed to clear the trigger. Changed to `""`, Radix's documented "no selection" value. Diff is exactly 14 lines, nothing else. ## Review pointers 113 files, but only a handful need real attention: - `vitest.config.mts` — the config contract and thresholds - the 5 `.tsx` source files — 14 identical one-line changes - `helper.test.ts` / `provider.test.ts` — the corrected fixtures - everything else is new `*.test.*` files and docs ## TODOs: 8 opened, 4 resolved **9 open — 3 high, all found by reading code while writing tests for it:** - **Filter injection shape** — `FamilyService` interpolates client-supplied `envelopeNo`/`donorId` into MP `$filter` strings with no validation; the only filters in that file lacking a `validatePositiveInt` guard. - **Silent field loss** — `removeGroup` guards `groupedFields` but not `groupOrder`, so a non-empty group can be stripped from the order while its fields remain. `buildSavePayload` then drops them. Real MP page-config data loss, no error surfaced. - **Address PII into logs** — `mergeTemplate` logs the raw docxtemplater error, which attaches the live merge scope (names, addresses). Plus 4 medium (incl. no type-check gate in CI) and 2 low. Docs updated: `CLAUDE.md`, `DECISIONS.md`, testing references, new facts snapshot. ## Verification | Gate | Result | |---|---| | `npm run test:run` | exit 0 — 116 files, 1,535 tests, 17 lines, no warnings | | `npm run test:coverage` | exit 0 — thresholds pass | | `npx tsc --noEmit` | 0 errors | | `npx eslint .` | 0 problems | | `npm run build` | exit 0 | 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes all three high-severity TODOs from the coverage review, plus the
CI gap that let a type error sit on `dev` unnoticed.
## 1. Unvalidated numeric fields reaching MP `$filter` strings
`saveFamily` now runs `HouseholdSchema.safeParse()` before the payload
reaches `FamilyService`. A server action is a public POST endpoint and
its TypeScript annotation is erased at runtime, so `envelopeNo: number`
guaranteed nothing.
The rejection reports field **paths** only (`members.0.envelopeNo`),
never the submitted values — rule 14 applies to returned error strings,
not just logs. Authorization still runs first, so an unauthorized caller
learns nothing about the schema.
Defence in depth where the string is actually built:
`resolveUniqueEnvelopeNo` validates `requested` and `excludeDonorId`,
and re-validates `candidate` each loop iteration so a future change to
`getNextEnvelopeNumber()` can't reintroduce an unchecked value.
`upsertDonor` validates the id it uses to target the `Donors` update.
`null` and `0` stay valid as the "no donor" sentinels.
### Correction to the original report — please read before assessing
risk
The classic injection string was **already blocked**, but incidentally
rather than by design. `upsertDonor` guards on `envelopeNo > 0`, and JS
coerces `"1 OR 1=1"` to `NaN`, so that comparison is false and the
crafted string never reached the filter.
What *did* get through: values that survive numeric coercion without
being positive integers — `1.5`, `Infinity`, `1e21` (interpolating as
the malformed `1e+21`), numeric strings. That's malformed filters and
confusing 500s, **not** a filter-injection primitive. The practical
severity was below "high" as filed.
The fix still stands: relying on an incidental coercion side effect to
block injection is fragile, and a schema change or a new call path that
drops the `> 0` guard would turn it into the real thing with nothing to
catch it.
## 2. `removeGroup` silently dropped a non-empty group's fields
The guard existed on `groupedFields` but not `groupOrder`, and
`buildSavePayload()` iterates `groupOrder` — so the fields vanished from
the save payload and from live MP page configuration, with no error
shown to the admin. The guard now sits at the top of the callback,
reading rendered state, so both updates agree.
**Worth knowing for review:** the obvious fix doesn't work. A `let
removed = false` flag set inside the `setGroupedFields` updater is still
`false` when `setGroupOrder` is reached, because React runs updaters
during render, not at call time. It would have read as correct and kept
the bug. I wrote it that way first; it's recorded in the TODO so it
isn't retried.
## 3. Raw error objects logged household PII
docxtemplater attaches the live merge scope to `err.properties.scope` on
a scope-parser failure — in this feature that scope **is** the household
list, so `console.error('...', error)` wrote every printable name and
mailing address for the batch into server logs.
New `describeError()` reduces a throw to `{ name, message }` plus
docxtemplater's own `id`/`explanation`, which carry no caller data.
Applied to `mergeTemplate`, `generateLabelPdf`, `generateLabelDocx`.
**One extra beyond the three highs:** I also fixed the identical
anti-pattern in `addeditfamily/page.tsx` (filed separately as medium,
and a file this branch already touches). Shipping a redaction fix that
skips the file next door would leave two standards in one codebase. Say
the word if you'd rather it were split out.
## CI gates
`npm run typecheck` and `npm run lint` now run as **steps of the
existing `test` job** — steps, not new jobs, because branch protection
requires the check named `test`, and separate jobs would be
green-but-unrequired until someone also edited the protection rules. A
gate that doesn't gate is worse than none.
Two more issues found in the same file:
- **`npm install` → `npm ci`.** `npm install` resolves fresh versions
and rewrites `package-lock.json` inside CI, so CI could be testing a
dependency tree no developer had. Verified with `npm ci --dry-run`.
- **No `concurrency` group** — superseded PR runs burned minutes. Now
cancels in-flight runs per ref, *except* on `main` and `dev`, whose runs
gate merges and releases and must not be killed.
Plus `permissions: contents: read`; the workflow only reads the repo,
and Codecov uses its own token.
## Review pointers
| File | What to look at |
|---|---|
| `familyService.ts` | +20 lines of validation; check the `0`/`null`
sentinel handling |
| `addeditfamily/actions.ts` | schema parse; confirm the error leaks no
values |
| `use-field-order-state.ts` | guard moved to top of callback,
`groupedFields` added to deps |
| `address-labels/actions.ts` | new `describeError()` |
| `.github/workflows/test.yml` | steps vs jobs, and the `concurrency`
exemption for the trunks |
Everything else is tests (+28) and TODO/doc updates.
## Status
TODOs: **9 open → 4** (2 medium, 2 low, all in `src/components`). No
high-severity or security items remain open.
| Gate | Result |
|---|---|
| `npm run lint` | 0 problems |
| `npm run typecheck` | 0 errors |
| `npm run test:coverage` | exit 0 — 116 files, 1,563 tests, 98.85%
stmts / 99.70% lines |
| `npm run build` | exit 0 |
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
1 participant
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.
Promotes verified
devintomainfor release v2026.09.13.1431.Includes PRs #23–#29:
disabledPathsis wired, not just declaredgetCurrentUserProfileCI green on
6aaa57b.🤖 Generated with Claude Code