From a96f751b8e7781d4d018ecb1314b8b5987dc407c Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Sun, 13 Sep 2026 10:26:36 -0400 Subject: [PATCH] fix: close 3 high-severity findings, gate lint + types in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three high-severity TODOs from the coverage review, plus the CI gap that let a type error reach `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, never the submitted values — rule 14 applies to returned error strings too. Authorization still runs first, so an unauthorized caller learns nothing about the schema. Defence in depth where the string is built: `resolveUniqueEnvelopeNo` validates `requested` and `excludeDonorId`, and re-validates `candidate` each loop iteration so a future change to `getNextEnvelopeNumber()` cannot 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: the classic injection string was already blocked, but incidentally — `upsertDonor` guards on `envelopeNo > 0` and JS coerces "1 OR 1=1" to NaN. What actually got through were values surviving numeric coercion without being positive integers (1.5, Infinity, 1e21 interpolating as the malformed "1e+21"). That is malformed filters and confusing 500s, not an injection primitive, so the practical severity was below "high". The fix stands: relying on an incidental coercion side effect is fragile, and a schema change or a new call path dropping that guard would make it real. 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. The guard now sits at the top of the callback, reading the rendered state, so both updates agree. The obvious fix does not work and is worth not retrying: 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 looked correct and kept the bug. 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 to 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 and generateLabelDocx. Also fixed the identical anti-pattern one file over, in `addeditfamily/page.tsx` (filed separately as medium). Shipping a redaction fix that skips the file next door would leave two standards in one codebase. CI gates Added `npm run typecheck` (`tsc --noEmit`) and `npm run lint` 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. Two more issues found in the same file: - `npm install` -> `npm ci`. `npm install` resolves fresh versions and rewrites the lockfile inside CI, so CI could test 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. Plus `permissions: contents: read` — the workflow only reads the repo. Tests: 1,535 -> 1,563. Coverage 98.85% statements / 99.70% lines. TODOs: 9 open -> 4 (2 medium, 2 low, all in src/components). No high or security items remain open. Verified: lint 0 · typecheck 0 errors · test:coverage exit 0 · next build exit 0. Co-Authored-By: Claude Opus 5 (1M context) --- ...mergetemplate-logs-address-pii-on-error.md | 30 +++- .../2026-09-13-page-logs-raw-error-object.md | 28 +++- ...-09-13-removegroup-order-guard-mismatch.md | 42 ++++- ...6-09-13-testing-no-typecheck-gate-in-ci.md | 36 ++++- ...nvalidated-envelope-donor-ids-in-filter.md | 47 +++++- .claude/TODO/INDEX.md | 54 +++---- .github/workflows/test.yml | 39 ++++- CLAUDE.md | 15 +- package.json | 1 + .../(web)/tools/addeditfamily/actions.test.ts | 132 ++++++++++++++++ src/app/(web)/tools/addeditfamily/actions.ts | 31 +++- .../(web)/tools/addeditfamily/page.test.tsx | 49 +++++- src/app/(web)/tools/addeditfamily/page.tsx | 8 +- src/components/address-labels/actions.test.ts | 141 +++++++++++++++++ src/components/address-labels/actions.ts | 37 ++++- .../use-field-order-state.test.ts | 80 ++++++++-- .../field-management/use-field-order-state.ts | 39 +++-- src/services/familyService.test.ts | 148 ++++++++++++++++++ src/services/familyService.ts | 20 +++ 19 files changed, 912 insertions(+), 65 deletions(-) diff --git a/.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md b/.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md index 77ba706..dc9d811 100644 --- a/.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md +++ b/.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md @@ -6,7 +6,7 @@ area: components files: [src/components/address-labels/actions.ts] discovered: 2026-09-13 discovered_by: coverage-agent-address-labels -status: open +status: resolved --- ## Problem @@ -56,3 +56,31 @@ scope parser causes every printable household's name and mailing address for that batch to be written to application/server logs (e.g. Vercel function logs), which are typically retained, more widely readable, and less access controlled than the MP database itself. + +--- + +## Resolution (2026-09-13) +Added a `describeError()` helper in `src/components/address-labels/actions.ts` +and routed all three `console.error` calls through it — +`generateLabelPdf`, `generateLabelDocx`, and `mergeTemplate`. + +It reduces a caught value to `{ name, message }`, plus docxtemplater's own +`properties.id` and `properties.explanation` when present. Those two are +genuinely useful for telling which failure mode occurred and contain no caller +data. Everything else on `properties` — crucially `scope`, which in this +feature IS the household list — is dropped. A non-`Error` throw is reported as +`{ name: 'NonError', type: typeof error }` rather than by value, since a thrown +string can itself be data-derived. + +The user-facing return value still carries `error.message`. That is the +diagnostic the person fixing their template needs, and docxtemplater's messages +reference tag names from the uploaded template, not merge data. + +### Tests +`src/components/address-labels/actions.test.ts` — 5 cases driving a +docxtemplater-shaped error whose `properties.scope` holds a real-looking name +and address, asserting none of it reaches the log while the identifiers do. +One asserts on the logged object's KEYS rather than substrings: the word +"scope" legitimately appears inside the safe identifier +`scopeparser_execution_failed`, so a naive `not.toContain('scope')` fails for +the wrong reason. diff --git a/.claude/TODO/2026-09-13-page-logs-raw-error-object.md b/.claude/TODO/2026-09-13-page-logs-raw-error-object.md index 14cf70b..77fac66 100644 --- a/.claude/TODO/2026-09-13-page-logs-raw-error-object.md +++ b/.claude/TODO/2026-09-13-page-logs-raw-error-object.md @@ -6,7 +6,7 @@ area: components files: [src/app/(web)/tools/addeditfamily/page.tsx] discovered: 2026-09-13 discovered_by: coverage-agent-addeditfamily -status: open +status: resolved --- ## Problem @@ -60,3 +60,29 @@ resolution has already failed, and only to server logs. But it is the second place after `tool-params.server.ts`'s example that this exact swallow-and-warn shape appears, and it is the one that gets it wrong; left uncorrected, it is an easy pattern to copy into the next new page. + +--- + +## Resolution (2026-09-13) +Fixed alongside the identical anti-pattern in +`src/components/address-labels/actions.ts` — shipping a redaction fix for one +file while leaving the same bug in the feature next door would have left the +codebase with two standards. + +```ts +console.warn("addeditfamily.resolve_contact_id_failed", { + table: params.pageData.Table_Name, + name: error instanceof Error ? error.name : "NonError", +}); +``` + +The table name is a configuration identifier, not record content, and rule 14 +explicitly allows "table, IDs, HTTP status". The error's `message` is dropped +because `resolveContactIdFromPage` builds an MP `$filter` from the record id +and column path, and MP surfaces that filter back inside its error text. + +### Tests +`src/app/(web)/tools/addeditfamily/page.test.tsx` — the existing swallow test +now asserts the redacted shape, plus two new cases: a rejection whose message +contains a filter string never reaches the log, and a non-`Error` throw is +described as `NonError` rather than by value. diff --git a/.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md b/.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md index 0cab77d..94a32e7 100644 --- a/.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md +++ b/.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md @@ -6,7 +6,7 @@ area: components files: [src/components/field-management/use-field-order-state.ts] discovered: 2026-09-13 discovered_by: coverage-agent-field-management -status: open +status: resolved --- ## Problem @@ -87,3 +87,43 @@ surfaced to the admin performing the edit. This directly corrupts live Ministry Platform page field configuration (view order, group assignment, required/hidden/filter settings all lost for the affected fields) until someone notices fields missing from the page and manually re-adds them. + +--- + +## Resolution (2026-09-13) +Applied option 1 from the proposed fix: `groupOrder` is now guarded the same +way `groupedFields` was, so a non-empty group is a complete no-op. + +The guard moved OUT of the `setGroupedFields` updater and up to the top of the +callback, reading the rendered `groupedFields` (added to the dependency array, +matching how `addGroup` already reads `isFlat`): + +```ts +const removeGroup = useCallback( + (name: string) => { + if ((groupedFields[name] || []).length > 0) return; + setGroupedFields((prev) => { const { [name]: _, ...rest } = prev; return rest; }); + setGroupOrder((prev) => prev.filter((g) => g !== name)); + setIsDirty(true); + }, + [groupedFields], +); +``` + +### A wrong first attempt, recorded so it is not retried +The obvious fix — set a `let removed = false` flag inside the +`setGroupedFields` updater and check it before calling `setGroupOrder` — does +not work. React runs state updaters during the render phase, not synchronously +at call time, so the flag is still `false` when the second setter is reached. +It would have looked correct, passed a casual read, and silently kept the bug. +The decision has to be made once, up front, from state both setters agree on. + +`isDirty` is no longer set when the removal is refused: nothing changed, so +there is nothing to save. + +### Tests +`src/components/field-management/use-field-order-state.test.ts` — the test that +documented the broken behaviour was replaced with four that pin the fix: both +halves of state left intact, fields still present in `buildSavePayload()`, +`isDirty` untouched on refusal, and a group still removable once emptied (with +its field surviving). diff --git a/.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md b/.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md index e3cfd4f..2ea39f5 100644 --- a/.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md +++ b/.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md @@ -6,7 +6,7 @@ area: testing files: [.github/workflows/test.yml, package.json] discovered: 2026-09-13 discovered_by: coverage-review-orchestrator -status: open +status: resolved --- ## Problem @@ -83,3 +83,37 @@ The specific breakage is repaired, but the gap that allowed it is still open. A type error reaching `dev` breaks `npm run build` and therefore a Vercel production deploy, while CI stays green — so it is found at deploy time by whoever is shipping, not at PR time by whoever wrote it. + +--- + +## Resolution (2026-09-13) +Added `"typecheck": "tsc --noEmit"` to `package.json` and wired it into +`.github/workflows/test.yml`, along with two other gaps found while in there. + +**Added as STEPS, not new jobs.** Branch protection on `dev` and `main` +requires the status check named `test`. Lint and typecheck as separate jobs +would have been green-but-unrequired until someone also edited the protection +rules — a gate that does not gate. As steps in the existing `test` job they are +covered by the rule already in place, with no admin change needed. + +Order is install -> lint -> typecheck -> test:coverage, so the cheap checks +fail first. The Codecov step keeps `if: always()` and `fail_ci_if_error: false`, +so an early failure that produces no coverage file does not itself fail the +build. + +### Two other CI issues fixed in the same pass +- **`npm install` -> `npm ci`.** `npm install` resolves fresh versions and + rewrites `package-lock.json` inside CI, so CI could be testing a different + dependency tree than any developer had. `npm ci` installs exactly what the + lockfile pins and fails outright if `package.json` and the lockfile disagree. + Verified with `npm ci --dry-run` before committing. +- **No `concurrency` group.** Rapid pushes to a PR branch left superseded runs + burning minutes. Now cancels in-progress runs for the same ref, except on + `main` and `dev`, whose runs gate merges and releases and must not be killed. + +Also added `permissions: contents: read` — this workflow only reads the repo, +and Codecov authenticates with its own token rather than `GITHUB_TOKEN`. + +`CLAUDE.md` previously documented "CI gates tests only ... Type-check locally" +as the mitigation. That line should now be read alongside this change: the +honour system has been replaced by a gate. diff --git a/.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md b/.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md index a59cd7a..ae949b0 100644 --- a/.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md +++ b/.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md @@ -6,7 +6,7 @@ area: services files: [src/services/familyService.ts, src/app/(web)/tools/addeditfamily/actions.ts, src/lib/dto/family.ts] discovered: 2026-09-13 discovered_by: coverage-agent-services -status: open +status: resolved --- ## Problem @@ -75,3 +75,48 @@ not a plain positive integer. Depending on how the MP OData-style `$filter` parser handles the resulting string, this ranges from a confusing 400/500 error to a filter-injection primitive against the `Donors` table — the same class of risk rule 11 exists to close off for string fields. + +--- + +## Resolution (2026-09-13) +Both layers of the proposed fix were applied. + +**1. Parse at the action boundary.** `saveFamily` now runs +`HouseholdSchema.safeParse(household)` before the payload reaches +`FamilyService`, and returns an `ActionError` on failure. The error reports the +offending field PATHS only (`members.0.envelopeNo`), never the submitted +values — rule 14 applies to returned error strings, which travel further than +logs. Authorization still runs first, so an unauthorized caller learns nothing +about the schema. + +**2. Validate where the filter is built.** `resolveUniqueEnvelopeNo` calls +`validatePositiveInt` on `requested` and on `excludeDonorId`, and re-validates +`candidate` on every loop iteration so a future change to +`getNextEnvelopeNumber()` cannot reintroduce an unchecked value. `upsertDonor` +validates `existingDonorId` before using it to target the `Donors` update. +`null` and `0` are preserved as the "no donor to exclude" sentinels rather than +being rejected. + +### Correction to the original severity assessment +Re-reading this while fixing it: the classic injection string was already +blocked, but incidentally rather than by design. `upsertDonor` guards with +`envelopeNo > 0`, and JS coerces `"1 OR 1=1"` to `NaN`, making that comparison +false — so the crafted string never reached the filter. What *did* get through +were values that survive numeric coercion but are not positive integers: +`1.5`, `Infinity`, `1e21` (which interpolates as the malformed `1e+21`), and +numeric strings. Those produce malformed filters and confusing 400/500s, not a +filter-injection primitive. + +So the practical severity was lower than "high" as originally filed. The fix is +still correct and worth keeping: 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. Recorded here so the next reader is not misled by the original framing. + +### Tests +- `src/app/(web)/tools/addeditfamily/actions.test.ts` — 11 cases: injection-shaped + `envelopeNo`/`donorId`, non-integer/array/object/boolean fields, missing + top-level fields, path reporting, no value echoed back, authorize-before-validate. +- `src/services/familyService.test.ts` — 7 cases asserting no malformed value + ever reaches an MP query, that `donorId: 0` is still accepted as "none", and + that well-formed input produces the exact expected filter string. diff --git a/.claude/TODO/INDEX.md b/.claude/TODO/INDEX.md index adc8793..e6b6dc6 100644 --- a/.claude/TODO/INDEX.md +++ b/.claude/TODO/INDEX.md @@ -22,14 +22,19 @@ Severity tiers: - **medium**: doc drift, missing test, refactor with real cost - **low**: nits, minor doc fixes, stylistic improvements -Total: **9 open TODOs**. +Total: **4 open TODOs**. > **2026-09-13 — unit-test coverage push.** Statement coverage over authored > code went from 49.67% to 98.84% (3,610/3,652), lines to 99.70%, across 1,535 > tests in 116 files. Most of the items below were opened during that work, > found by reading code while writing tests for it. A follow-up pass cleaned -> the runner output from 1,507 lines to 17 with zero warnings. Details in each -> file. +> the runner output from 1,507 lines to 17 with zero warnings. +> +> **2026-09-13 (later) — remediation.** All three high-severity items and the +> CI gap are fixed and closed; see each file's Resolution section. CI now runs +> lint and `tsc --noEmit` as required steps, and installs with `npm ci`. +> Everything still open is `medium` or below, and all of it sits in +> `src/components`. --- @@ -38,18 +43,12 @@ Total: **9 open TODOs**. ### Critical (0) _none open_ -### High (3) -| Area | Tags | Title | File | -|---|---|---|---| -| services | security, bug | Unvalidated client-supplied numeric fields interpolated into MP `$filter` strings in FamilyService | [→](2026-09-13-unvalidated-envelope-donor-ids-in-filter.md) | -| components | bug | `removeGroup` drops a non-empty group's fields from the save payload | [→](2026-09-13-removegroup-order-guard-mismatch.md) | -| components | security, bug | `mergeTemplate` console.errors the raw docxtemplater error, which can carry household addresses | [→](2026-09-13-mergetemplate-logs-address-pii-on-error.md) | +### High (0) +_none open_ -### Medium (4) +### Medium (2) | Area | Tags | Title | File | |---|---|---|---| -| testing | bug, testing, drift | Add a type-check gate to CI (stale fixtures had silently broken `npm run build`) | [→](2026-09-13-testing-no-typecheck-gate-in-ci.md) | -| components | security, drift | `AddEditFamilyPage` logs the raw error object instead of an identifier | [→](2026-09-13-page-logs-raw-error-object.md) | | components | bug, drift | Template editor ignores pageID/recordID (no MP persistence) | [→](2026-04-17-components-template-editor-no-mp-persistence.md) | | components | bug, refactor | Merge tokens `{{Field_Name}}` have no resolver anywhere | [→](2026-04-17-components-template-editor-merge-token-resolver.md) | @@ -63,23 +62,15 @@ _none open_ ## By tag -### security (3) -- unvalidated-envelope-donor-ids-in-filter — high -- mergetemplate-logs-address-pii-on-error — high -- page-logs-raw-error-object — medium +### security (0) +_none open_ -### bug (6) -- unvalidated-envelope-donor-ids-in-filter — high -- removegroup-order-guard-mismatch — high -- mergetemplate-logs-address-pii-on-error — high -- testing-no-typecheck-gate-in-ci — medium +### bug (3) - components-template-editor-no-mp-persistence — medium - components-template-editor-merge-token-resolver — medium - search-empty-state-never-renders — low -### drift (3) -- testing-no-typecheck-gate-in-ci — medium -- page-logs-raw-error-object — medium +### drift (1) - components-template-editor-no-mp-persistence — medium ### missing-test (1) @@ -89,8 +80,8 @@ _none open_ - components-template-editor-merge-token-resolver — medium - dead-empty-fields-branch-handlenext — low -### testing (1) -- testing-no-typecheck-gate-in-ci — medium +### testing (0) +_none open_ ### doc (0) _none open_ @@ -104,9 +95,9 @@ _none open_ | Area | Count | |---|---| -| components | 6 | -| testing | 1 | -| services | 1 | +| components | 4 | +| testing | 0 | +| services | 0 | | auth | 0 | | mp-provider | 0 | | utils | 0 | @@ -126,6 +117,11 @@ _none open_ | 2026-09-13 | No tests for `src/components/template-editor/` | [→](2026-04-17-components-template-editor-missing-tests.md) | | 2026-09-13 | `vitest.config.ts` loaded as CommonJS (renamed to `.mts`) | [→](2026-09-13-testing-vitest-config-loaded-as-cjs.md) | | 2026-09-13 | Radix Select fields switched uncontrolled -> controlled | [→](2026-09-13-components-select-uncontrolled-to-controlled.md) | +| 2026-09-13 | Unvalidated numeric fields reaching MP `$filter` strings | [→](2026-09-13-unvalidated-envelope-donor-ids-in-filter.md) | +| 2026-09-13 | `removeGroup` dropped a non-empty group's fields from the save payload | [→](2026-09-13-removegroup-order-guard-mismatch.md) | +| 2026-09-13 | Raw docxtemplater error logged household addresses | [→](2026-09-13-mergetemplate-logs-address-pii-on-error.md) | +| 2026-09-13 | `AddEditFamilyPage` logged the raw error object | [→](2026-09-13-page-logs-raw-error-object.md) | +| 2026-09-13 | No type-check gate in CI (also `npm ci`, lint, concurrency) | [→](2026-09-13-testing-no-typecheck-gate-in-ci.md) | --- diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 923cdea..1a14124 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -6,7 +6,24 @@ on: pull_request: branches: [main, dev] +# A new push to the same branch supersedes the run in flight. Pushes to the +# protected trunks are never cancelled — their runs gate merges and releases. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.ref != 'refs/heads/main' && github.ref != 'refs/heads/dev' }} + +# Least privilege: this workflow only reads the repo. The Codecov upload +# authenticates with its own token, not the GITHUB_TOKEN. +permissions: + contents: read + jobs: + # Deliberately ONE job, not several. + # + # Branch protection on `dev` and `main` requires the status check named + # `test`. Adding lint/typecheck as separate jobs would leave them + # unrequired — green-but-ignored — until someone also updated the + # protection rules. As steps here they are covered by the existing rule. test: runs-on: ubuntu-latest @@ -18,9 +35,27 @@ jobs: node-version-file: .nvmrc cache: npm - - run: npm install + # `npm ci`, not `npm install`: it installs exactly what the lockfile + # pins and fails if package.json and package-lock.json disagree, rather + # than silently resolving new versions and rewriting the lockfile in CI. + - run: npm ci + + - name: Lint + run: npm run lint + + # The gate this repo was missing. + # + # `tsconfig.json` includes `**/*.ts` and `**/*.tsx`, and `next.config.ts` + # sets no `ignoreBuildErrors` — so a type error in a TEST file breaks + # `npm run build`, and therefore a production deploy, while the suite + # itself stays green. That is not hypothetical: two committed test files + # sat on `dev` in exactly that state, caught only by a manual local run. + - name: Type check + run: npm run typecheck - - run: npm run test:coverage + # Enforces the coverage thresholds in vitest.config.mts. + - name: Test with coverage + run: npm run test:coverage - uses: codecov/codecov-action@v5 if: always() diff --git a/CLAUDE.md b/CLAUDE.md index 5ff425a..5eb1a22 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -7,6 +7,7 @@ This guide provides essential information for AI assistants (like Claude) workin - **Dev**: `npm run dev` (Next.js dev server) - **Build**: `npm run build` (builds SQL install script, then production build with Turbopack + type checking) - **Lint**: `npm run lint` (ESLint CLI — `next lint` was removed in Next.js 16) +- **Type check**: `npm run typecheck` (`tsc --noEmit`; enforced in CI) - **Generate MP Types**: `npm run mp:generate:models` (generates TypeScript types + Zod schemas from Ministry Platform API, cleans output directory first) - **Generate MP Stored Procs**: `npm run mp:generate:storedprocs` (generates stored procedure reference from Ministry Platform API) - **Build MP SQL Install**: `npm run mp:build:install` (combines SQL files from `db/` into unified `_INSTALL/ministryplatform-install.sql`, skips if unchanged) @@ -239,7 +240,7 @@ await mp.createTableRecords('Contact_Log', records, { - **MPHelper mock**: Use mock class (`MPHelper: class { method = mockFn; }`), not `vi.fn().mockImplementation()` - **Singleton reset**: Reset `(ServiceClass as any).instance = undefined` in `beforeEach` to prevent state leakage - **Server action tests**: Mock `@/lib/auth` (`auth.api.getSession`), `next/headers` (`headers()`), and service singletons -- **Type-check locally before pushing**: CI does not run `tsc`, and `tsconfig.json` includes `**/*.ts`/`**/*.tsx`, so a type error in a *test* file breaks `npm run build` while CI stays green. This has happened — see `.claude/TODO/2026-09-13-testing-no-typecheck-gate-in-ci.md`. +- **CI type-checks**: `npm run typecheck` (`tsc --noEmit`) runs as a required step in the `test` job, alongside `npm run lint`. `tsconfig.json` includes `**/*.ts`/`**/*.tsx`, so a type error in a *test* file breaks `npm run build` — that used to reach `dev` unnoticed because CI ran tests only. - See **[Testing Reference](.claude/references/testing/README.md)** for all mock patterns, coverage data, and test inventory ## Dependencies @@ -263,8 +264,16 @@ condition that clears it. Do not re-derive that analysis. with , then bump `engines.node`, `.nvmrc`, `@types/node`, and `REQUIRED_NODE_MAJOR` in `scripts/setup.ts` together. -- **CI gates tests only** — `.github/workflows/test.yml` runs `npm run test:coverage` - and never `npm run build`, so type errors do not fail CI. Type-check locally. +- **CI gates lint, types and tests** — `.github/workflows/test.yml` runs + `npm run lint`, `npm run typecheck` and `npm run test:coverage`, in that + order, as steps of the single `test` job. They are steps rather than separate + jobs on purpose: branch protection requires the check named `test`, so extra + jobs would be green-but-unrequired until someone also edited the protection + rules. CI still does not run `npm run build`, but `typecheck` now covers the + part of it that used to break. +- **CI installs with `npm ci`**, not `npm install` — it installs exactly what + `package-lock.json` pins and fails if the lockfile and `package.json` + disagree, instead of silently resolving new versions inside CI. - **Coverage path is load-bearing**: CI uploads `coverage/coverage-final.json` to Codecov. Verify that exact path still exists after any Vitest major. diff --git a/package.json b/package.json index 3faf4a4..f404496 100644 --- a/package.json +++ b/package.json @@ -10,6 +10,7 @@ "build": "tsx src/lib/providers/ministry-platform/scripts/build-sql-install.ts && next build", "start": "next start", "lint": "eslint .", + "typecheck": "tsc --noEmit", "test": "vitest", "test:run": "vitest run", "test:coverage": "vitest run --coverage", diff --git a/src/app/(web)/tools/addeditfamily/actions.test.ts b/src/app/(web)/tools/addeditfamily/actions.test.ts index 67a80b2..07bb76c 100644 --- a/src/app/(web)/tools/addeditfamily/actions.test.ts +++ b/src/app/(web)/tools/addeditfamily/actions.test.ts @@ -382,3 +382,135 @@ describe("addeditfamily actions", () => { }); }); }); + +/** + * Runtime validation of the saveFamily payload. + * + * `saveFamily(household: Household)` is a server action — a public POST + * endpoint whose TypeScript annotation is erased at runtime. Downstream, + * `FamilyService` interpolates `envelopeNo` and `donorId` into MP `$filter` + * strings and uses `donorId` to target a `Donors` update, so the payload has + * to be parsed, not merely typed. + * + * See `.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md`. + */ +describe("saveFamily payload validation", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockRequireSecurityRole.mockResolvedValue(42); + }); + + /** Bypass the compile-time type the way a hand-crafted POST body does. */ + function malformed(household: Record): Household { + return household as unknown as Household; + } + + it("rejects a filter-injection-shaped envelopeNo before it reaches the service", async () => { + const household = makeHousehold(); + const bad = malformed({ + ...household, + members: [{ ...household.members[0], envelopeNo: "1 OR 1=1" }], + }); + + const result = await saveFamily(bad); + + expect(result.success).toBe(false); + expect(mockSaveHousehold).not.toHaveBeenCalled(); + }); + + it("rejects a filter-injection-shaped donorId before it reaches the service", async () => { + const household = makeHousehold(); + const bad = malformed({ + ...household, + members: [{ ...household.members[0], donorId: "5; DROP" }], + }); + + const result = await saveFamily(bad); + + expect(result.success).toBe(false); + expect(mockSaveHousehold).not.toHaveBeenCalled(); + }); + + it.each([ + ["a non-integer envelopeNo", { envelopeNo: 1.5 }], + ["an array envelopeNo", { envelopeNo: [7] }], + ["an object donorId", { donorId: { id: 7 } }], + ["a boolean contactId", { contactId: true }], + ])("rejects %s", async (_label, patch) => { + const household = makeHousehold(); + const bad = malformed({ + ...household, + members: [{ ...household.members[0], ...patch }], + }); + + const result = await saveFamily(bad); + + expect(result.success).toBe(false); + expect(mockSaveHousehold).not.toHaveBeenCalled(); + }); + + it("rejects a household missing required top-level fields", async () => { + const result = await saveFamily(malformed({ householdId: 1 })); + + expect(result.success).toBe(false); + expect(mockSaveHousehold).not.toHaveBeenCalled(); + }); + + it("reports offending field paths so the user can fix the form", async () => { + const household = makeHousehold(); + const bad = malformed({ + ...household, + members: [{ ...household.members[0], envelopeNo: "1 OR 1=1" }], + }); + + const result = await saveFamily(bad); + + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error).toMatch(/members\.0\.envelopeNo/); + }); + + it("never echoes the submitted value back in the error message", async () => { + // CLAUDE.md rule 14: error messages travel further than logs do. Paths + // are safe to report; the value that was rejected is not. + const household = makeHousehold(); + const bad = malformed({ + ...household, + members: [ + { ...household.members[0], envelopeNo: "1 OR 1=1", emailAddress: "secret@example.com" }, + ], + }); + + const result = await saveFamily(bad); + + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error).not.toContain("1 OR 1=1"); + expect(result.error).not.toContain("secret@example.com"); + }); + + it("still authorizes before validating, so an unauthorized caller learns nothing about the schema", async () => { + mockRequireSecurityRole.mockRejectedValueOnce(new Error("Forbidden")); + + const result = await saveFamily(malformed({ householdId: 1 })); + + expect(result).toEqual({ success: false, error: "Forbidden" }); + expect(mockSaveHousehold).not.toHaveBeenCalled(); + }); + + it("passes a well-formed household straight through to the service", async () => { + const household = makeHousehold(); + const progress: SaveProgress = { + mainAddressId: 1, + altAddressId: null, + householdId: 1, + members: [], + }; + mockSaveHousehold.mockResolvedValueOnce(progress); + + const result = await saveFamily(household); + + expect(result).toEqual({ success: true, progress }); + expect(mockSaveHousehold).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/app/(web)/tools/addeditfamily/actions.ts b/src/app/(web)/tools/addeditfamily/actions.ts index af7abfa..6c36b17 100644 --- a/src/app/(web)/tools/addeditfamily/actions.ts +++ b/src/app/(web)/tools/addeditfamily/actions.ts @@ -3,6 +3,7 @@ import { FamilyService, PartialSaveError } from "@/services/familyService"; import { AuthorizationService } from "@/services/authorizationService"; import { GooglePlacesService } from "@/services/googlePlacesService"; +import { HouseholdSchema } from "@/lib/dto/family"; import type { ContactSearchResult, FamilyDefaults, @@ -139,8 +140,36 @@ export async function saveFamily( ): Promise<{ success: true; progress: SaveProgress } | ActionError> { try { await requireAccess("Households", "update"); + + /** + * Parse before the payload reaches the service. + * + * `household: Household` is a compile-time annotation only — this is a + * server action, i.e. a public POST endpoint, and the types are erased at + * runtime. Fields from this object are interpolated into MP `$filter` + * strings and used to target `Donors`/`Contacts` updates downstream, so + * "it is typed `number`" is not a runtime guarantee of anything. + * + * The error deliberately reports field PATHS only, never the submitted + * values (CLAUDE.md rule 14: error messages travel further than logs). + */ + const parsed = HouseholdSchema.safeParse(household); + if (!parsed.success) { + const paths = [ + ...new Set( + parsed.error.issues.map((issue) => + issue.path.length > 0 ? issue.path.join(".") : "(root)", + ), + ), + ]; + return { + success: false, + error: `Invalid family data. Check these fields: ${paths.join(", ")}`, + }; + } + const service = await FamilyService.getInstance(); - const progress = await service.saveHousehold(household); + const progress = await service.saveHousehold(parsed.data); return { success: true, progress }; } catch (error) { if (error instanceof PartialSaveError) { diff --git a/src/app/(web)/tools/addeditfamily/page.test.tsx b/src/app/(web)/tools/addeditfamily/page.test.tsx index 301a751..d1c6d4d 100644 --- a/src/app/(web)/tools/addeditfamily/page.test.tsx +++ b/src/app/(web)/tools/addeditfamily/page.test.tsx @@ -133,10 +133,53 @@ describe("AddEditFamilyPage", () => { render(jsx); expect(screen.getByTestId("initial-contact-id").textContent).toBe("null"); - expect(warnSpy).toHaveBeenCalledWith( - "Failed to resolve Contact_ID from page record:", - expect.any(Error), + // Identifiers and shape only — never the raw error, which can carry the + // interpolated $filter string (CLAUDE.md rule 14). + expect(warnSpy).toHaveBeenCalledWith("addeditfamily.resolve_contact_id_failed", { + table: "Households", + name: "Error", + }); + }); + + it("never writes the raw error — or its message — to the log", async () => { + mockParseToolParams.mockResolvedValue({ + recordID: 42, + pageData: { + Table_Name: "Households", + Primary_Key: "Household_ID", + Contact_ID_Field: "Contact_ID", + }, + }); + mockResolveContactIdFromPage.mockRejectedValue( + new Error("Invalid filter: Household_ID = 42 AND Secret = 'value'"), ); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + render(await AddEditFamilyPage({ searchParams: searchParamsOf({ recordID: "42" }) })); + + const logged = JSON.stringify(warnSpy.mock.calls); + expect(logged).not.toContain("Invalid filter"); + expect(logged).not.toContain("Secret"); + }); + + it("describes a non-Error throw by shape", async () => { + mockParseToolParams.mockResolvedValue({ + recordID: 42, + pageData: { + Table_Name: "Households", + Primary_Key: "Household_ID", + Contact_ID_Field: "Contact_ID", + }, + }); + mockResolveContactIdFromPage.mockRejectedValue("a bare string"); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + render(await AddEditFamilyPage({ searchParams: searchParamsOf({ recordID: "42" }) })); + + expect(warnSpy).toHaveBeenCalledWith("addeditfamily.resolve_contact_id_failed", { + table: "Households", + name: "NonError", + }); }); it("passes params through to AddEditFamily", async () => { diff --git a/src/app/(web)/tools/addeditfamily/page.tsx b/src/app/(web)/tools/addeditfamily/page.tsx index 065e0ed..096736b 100644 --- a/src/app/(web)/tools/addeditfamily/page.tsx +++ b/src/app/(web)/tools/addeditfamily/page.tsx @@ -26,7 +26,13 @@ export default async function AddEditFamilyPage({ searchParams }: AddEditFamilyP params.pageData.Contact_ID_Field, ); } catch (error) { - console.warn("Failed to resolve Contact_ID from page record:", error); + // Identifiers and shape only (CLAUDE.md rule 14). The raw error can + // carry the interpolated $filter string built inside + // resolveContactIdFromPage, and a $filter embeds record values. + console.warn("addeditfamily.resolve_contact_id_failed", { + table: params.pageData.Table_Name, + name: error instanceof Error ? error.name : "NonError", + }); } } diff --git a/src/components/address-labels/actions.test.ts b/src/components/address-labels/actions.test.ts index 7192bac..f3dc4e3 100644 --- a/src/components/address-labels/actions.test.ts +++ b/src/components/address-labels/actions.test.ts @@ -669,3 +669,144 @@ describe('generateLabelPdf error branches', () => { if (!result.success) expect(result.error).toBe('PDF generation failed'); }); }); + +/** + * Error-logging redaction. + * + * docxtemplater attaches the live merge scope to `err.properties.scope` when + * its scope parser fails. In this feature that scope IS the household list, so + * `console.error('mergeTemplate error:', error)` wrote every printable name + * and mailing address for the batch into server logs — exactly what CLAUDE.md + * rule 14 forbids. + * + * See `.claude/TODO/2026-09-13-mergetemplate-logs-address-pii-on-error.md`. + */ +describe('address-label actions redact errors before logging', () => { + const config: LabelConfig = { + stockId: '5160', + addressMode: 'household', + startPosition: 1, + includeMissingBarcodes: true, + barcodeFormat: 'postnet', + mailerId: '', + serviceType: '040', + }; + + const labels: LabelData[] = [ + { + name: 'Jane Householder', + addressLine1: '742 Evergreen Terrace', + city: 'Springfield', + state: 'IL', + postalCode: '62704', + }, + ]; + + /** Mirrors the shape docxtemplater throws on a scope-parser failure. */ + function scopeParserError(): Error { + const error = new Error('Scope parser execution failed') as Error & { + properties?: Record; + }; + error.properties = { + id: 'scopeparser_execution_failed', + explanation: 'The tag {Name} failed to parse', + scope: labels.map((l) => ({ + Name: l.name, + AddressLine1: l.addressLine1, + City: l.city, + State: l.state, + PostalCode: l.postalCode, + })), + }; + return error; + } + + function loggedText(): string { + const spy = console.error as unknown as { mock: { calls: unknown[][] } }; + return JSON.stringify(spy.mock.calls); + } + + beforeEach(() => { + mockRequireSecurityRole.mockResolvedValue(42); + mockGetSession.mockResolvedValue({ user: { id: 'user-1' } }); + }); + + it('never writes the merge scope — household names and addresses — to the log', async () => { + mockDocxtemplaterRender.mockImplementationOnce(() => { + throw scopeParserError(); + }); + + await mergeTemplate(Buffer.from('x').toString('base64'), labels, config); + + const logged = loggedText(); + expect(logged).not.toContain('Jane Householder'); + expect(logged).not.toContain('742 Evergreen Terrace'); + expect(logged).not.toContain('Springfield'); + expect(logged).not.toContain('62704'); + + // The word "scope" legitimately appears inside the safe identifiers + // ("scopeparser_execution_failed"), so assert on the KEY, not the text: + // no `scope` property may survive into the logged object. + const spy = console.error as unknown as { mock: { calls: unknown[][] } }; + const payload = spy.mock.calls.at(-1)?.[1] as Record; + expect(payload).not.toHaveProperty('scope'); + expect(Object.keys(payload).sort()).toEqual(['explanation', 'id', 'message', 'name']); + }); + + it('still logs the identifiers needed to diagnose the failure', async () => { + mockDocxtemplaterRender.mockImplementationOnce(() => { + throw scopeParserError(); + }); + + await mergeTemplate(Buffer.from('x').toString('base64'), labels, config); + + expect(console.error).toHaveBeenCalledWith('mergeTemplate error:', { + name: 'Error', + message: 'Scope parser execution failed', + id: 'scopeparser_execution_failed', + explanation: 'The tag {Name} failed to parse', + }); + }); + + it('describes a non-Error throw by shape rather than value', async () => { + mockDocxtemplaterRender.mockImplementationOnce(() => { + // A thrown string could itself be attacker- or data-derived. + throw 'Jane Householder, 742 Evergreen Terrace'; + }); + + await mergeTemplate(Buffer.from('x').toString('base64'), labels, config); + + expect(console.error).toHaveBeenCalledWith('mergeTemplate error:', { + name: 'NonError', + type: 'string', + }); + expect(loggedText()).not.toContain('Evergreen'); + }); + + it('omits properties that are absent rather than logging undefined keys', async () => { + mockDocxtemplaterRender.mockImplementationOnce(() => { + throw new Error('plain failure'); + }); + + await mergeTemplate(Buffer.from('x').toString('base64'), labels, config); + + expect(console.error).toHaveBeenCalledWith('mergeTemplate error:', { + name: 'Error', + message: 'plain failure', + }); + }); + + it('applies the same redaction in generateLabelPdf', async () => { + mockToBlob.mockRejectedValueOnce(scopeParserError()); + + await generateLabelPdf(labels, config); + + const logged = loggedText(); + expect(logged).not.toContain('Jane Householder'); + expect(logged).not.toContain('Evergreen'); + expect(console.error).toHaveBeenCalledWith( + 'generateLabelPdf error:', + expect.objectContaining({ id: 'scopeparser_execution_failed' }), + ); + }); +}); diff --git a/src/components/address-labels/actions.ts b/src/components/address-labels/actions.ts index 24f699c..7f509a0 100644 --- a/src/components/address-labels/actions.ts +++ b/src/components/address-labels/actions.ts @@ -141,6 +141,37 @@ export async function fetchAddressLabels( return { printable: [], skipped: [] }; } +/** + * Reduce a caught error to something safe to write to a log. + * + * CLAUDE.md rule 14: log identifiers and shape, never record content. + * `console.error('...', error)` serialises the whole object, and 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 the raw + * object carries every printable name and mailing address for the batch + * straight into server logs. + * + * docxtemplater's own `id` and `explanation` are useful for diagnosing which + * failure mode occurred and contain no caller data, so they are kept + * explicitly. Nothing else from `properties` is. + */ +function describeError(error: unknown): Record { + if (!(error instanceof Error)) { + return { name: 'NonError', type: typeof error }; + } + + const described: Record = { name: error.name, message: error.message }; + + const properties = (error as { properties?: unknown }).properties; + if (properties && typeof properties === 'object') { + const { id, explanation } = properties as { id?: unknown; explanation?: unknown }; + if (typeof id === 'string') described.id = id; + if (typeof explanation === 'string') described.explanation = explanation; + } + + return described; +} + export async function generateLabelPdf( labels: LabelData[], config: LabelConfig @@ -186,7 +217,7 @@ export async function generateLabelPdf( return { success: true, data: base64 }; } catch (error) { - console.error('generateLabelPdf error:', error); + console.error('generateLabelPdf error:', describeError(error)); return { success: false, error: error instanceof Error ? error.message : 'PDF generation failed', @@ -229,7 +260,7 @@ export async function generateLabelDocx( return { success: true, data: base64 }; } catch (error) { - console.error('generateLabelDocx error:', error); + console.error('generateLabelDocx error:', describeError(error)); return { success: false, error: error instanceof Error ? error.message : 'Word generation failed', @@ -325,7 +356,7 @@ export async function mergeTemplate( return { success: true, data: base64 }; } catch (error) { - console.error('mergeTemplate error:', error); + console.error('mergeTemplate error:', describeError(error)); const message = error instanceof Error ? error.message : 'Template merge failed'; if (message.includes('tag')) { return { success: false, error: `Template error: ${message}. Check that merge tokens are correctly formatted.` }; diff --git a/src/components/field-management/use-field-order-state.test.ts b/src/components/field-management/use-field-order-state.test.ts index 54e97b6..2eb3c0e 100644 --- a/src/components/field-management/use-field-order-state.test.ts +++ b/src/components/field-management/use-field-order-state.test.ts @@ -383,27 +383,89 @@ describe('useFieldOrderState > removeGroup', () => { expect(result.current.isDirty).toBe(true); }); - it('is a no-op on groupedFields when the group still has fields, but still removes it from groupOrder', () => { + /** + * Regression: removing a non-empty group used to strip its name from + * `groupOrder` while leaving its fields in `groupedFields`. Because + * `buildSavePayload()` iterates `groupOrder`, those fields silently + * disappeared from the payload — live Ministry Platform page configuration + * lost with no error shown. + * See `.claude/TODO/2026-09-13-removegroup-order-guard-mismatch.md`. + */ + it('is a complete no-op when the group still has fields', () => { const fields: PageField[] = [ makeField({ Page_Field_ID: 1, Field_Name: 'A', Group_Name: '1 - First', View_Order: 1 }), ]; const { result } = renderHook(() => useFieldOrderState(fields)); + const orderBefore = result.current.groupOrder; act(() => { result.current.removeGroup('1 - First'); }); - // Documented actual behavior: groupedFields keeps the non-empty group (guard - // in removeGroup refuses to drop its fields), but groupOrder unconditionally - // filters the name out regardless of that guard — see filed TODO - // 2026-09-13-removegroup-order-guard-mismatch.md. + // Both halves of the state stay intact, not just groupedFields. expect(result.current.groupedFields['1 - First']).toEqual([1]); - expect(result.current.groupOrder).not.toContain('1 - First'); + expect(result.current.groupOrder).toEqual(orderBefore); + expect(result.current.groupOrder).toContain('1 - First'); + }); + + it("keeps a non-empty group's fields in the save payload after a refused removal", () => { + const fields: PageField[] = [ + makeField({ Page_Field_ID: 1, Field_Name: 'A', Group_Name: '1 - First', View_Order: 1 }), + makeField({ Page_Field_ID: 2, Field_Name: 'B', Group_Name: '1 - First', View_Order: 2 }), + ]; + const { result } = renderHook(() => useFieldOrderState(fields)); + + act(() => { + result.current.removeGroup('1 - First'); + }); - // Consequence: buildSavePayload iterates groupOrder, so field 1 is silently - // dropped from the save payload even though it still exists in groupedFields. const payload = result.current.buildSavePayload(); - expect(payload.find((p) => p.Field_Name === 'A')).toBeUndefined(); + expect(payload.map((p) => p.Field_Name).sort()).toEqual(['A', 'B']); + }); + + it('does not mark the form dirty when the removal was refused', () => { + // A refused removal changed nothing, so there is nothing to save. + const fields: PageField[] = [ + makeField({ Page_Field_ID: 1, Field_Name: 'A', Group_Name: '1 - First', View_Order: 1 }), + ]; + const { result } = renderHook(() => useFieldOrderState(fields)); + + act(() => { + result.current.removeGroup('1 - First'); + }); + + expect(result.current.isDirty).toBe(false); + }); + + it('still removes a group once its fields have been moved out', () => { + const fields: PageField[] = [ + makeField({ Page_Field_ID: 1, Field_Name: 'A', Group_Name: '1 - First', View_Order: 1 }), + ]; + const { result } = renderHook(() => useFieldOrderState(fields)); + + act(() => { + result.current.removeGroup('1 - First'); + }); + expect(result.current.groupOrder).toContain('1 - First'); + + // Empty it the way the UI does (drag the field elsewhere), then retry. + act(() => { + result.current.moveHiddenToOther(); + }); + act(() => { + result.current.updateField(1, { Hidden: true }); + }); + act(() => { + result.current.moveHiddenToOther(); + }); + act(() => { + result.current.removeGroup('1 - First'); + }); + + expect(result.current.groupOrder).not.toContain('1 - First'); + expect(result.current.groupedFields['1 - First']).toBeUndefined(); + // The field itself survived the group's removal. + expect(result.current.buildSavePayload().map((p) => p.Field_Name)).toContain('A'); }); it('is a no-op when the group name does not exist in groupedFields at all', () => { diff --git a/src/components/field-management/use-field-order-state.ts b/src/components/field-management/use-field-order-state.ts index fc59666..084f8cc 100644 --- a/src/components/field-management/use-field-order-state.ts +++ b/src/components/field-management/use-field-order-state.ts @@ -120,15 +120,36 @@ export function useFieldOrderState(fields: PageField[]): FieldOrderState { [isFlat] ); - const removeGroup = useCallback((name: string) => { - setGroupedFields((prev) => { - if ((prev[name] || []).length > 0) return prev; - const { [name]: _, ...rest } = prev; - return rest; - }); - setGroupOrder((prev) => prev.filter((g) => g !== name)); - setIsDirty(true); - }, []); + /** + * Remove an empty group. + * + * Both pieces of state must agree. `buildSavePayload()` iterates + * `groupOrder`, not `Object.keys(groupedFields)`, so dropping a name from + * the order while its fields remain in `groupedFields` silently omits those + * fields from the payload — they vanish from the Ministry Platform page's + * field list on the next save, with no error shown to the admin. + * + * The guard therefore lives on the shared decision, not inside one setter. + * A non-empty group is a no-op: the UI only renders the delete control for + * empty groups, but this is a plain callback with no such enforcement. + */ + const removeGroup = useCallback( + (name: string) => { + // Decide once, up front, from the rendered state — NOT inside one of the + // setters. A flag set inside a `setGroupedFields` updater cannot drive + // the `setGroupOrder` call: React runs updaters during render, so the + // flag is still false when the second setter is reached. + if ((groupedFields[name] || []).length > 0) return; + + setGroupedFields((prev) => { + const { [name]: _, ...rest } = prev; + return rest; + }); + setGroupOrder((prev) => prev.filter((g) => g !== name)); + setIsDirty(true); + }, + [groupedFields], + ); const moveHiddenToOther = useCallback(() => { setGroupedFields((prev) => { diff --git a/src/services/familyService.test.ts b/src/services/familyService.test.ts index 908f1e9..447912a 100644 --- a/src/services/familyService.test.ts +++ b/src/services/familyService.test.ts @@ -804,3 +804,151 @@ describe("FamilyService", () => { }); }); }); + +/** + * Defence in depth at the point the `$filter` string is built. + * + * `saveFamily` parses the payload with `HouseholdSchema` before it gets here, + * so these values should never be malformed in practice. This layer exists + * because `resolveUniqueEnvelopeNo` interpolates them into a raw filter and + * `upsertDonor` uses `donorId` to target which `Donors` row gets written — a + * future caller that skips the action, or a schema change that loosens a + * field, must not silently reopen that. + * + * See `.claude/TODO/2026-09-13-unvalidated-envelope-donor-ids-in-filter.md`. + */ +describe("FamilyService envelope/donor filter hardening", () => { + beforeEach(() => { + vi.clearAllMocks(); + + (FamilyService as any).instance = undefined; + mockRequireSecurityRole.mockResolvedValue(42); + mockGetDomainInfo.mockResolvedValue({ TimeZoneName: "Eastern Standard Time" }); + }); + + /** Bypass the compile-time type the way an unvalidated caller would. */ + function donorMember(overrides: Record) { + return { + contactId: 900, + firstName: "Ada", + middleName: "", + maidenName: "", + lastName: "Lovelace", + nickname: "", + prefixId: 0, + suffixId: 0, + birthDate: null, + genderId: 0, + maritalStatusId: 0, + mobilePhone: "", + emailAddress: "", + bulkEmailOpt: false, + envelopeNo: 1001, + contactStatusId: 1, + primaryLanguageId: null, + faithBackgroundId: null, + householdPositionId: 1, + participant: null, + donorId: null, + isDonor: true, + ...overrides, + }; + } + + function householdWith(overrides: Record): Household { + return makeHousehold({ + householdId: 601, + members: [donorMember(overrides)], + + } as any) as unknown as Household; + } + + async function saveAndCatch(household: Household): Promise { + const service = await FamilyService.getInstance(); + try { + await service.saveHousehold(household); + } catch (error) { + return error as Error; + } + throw new Error("expected saveHousehold to reject"); + } + + it.each([ + ["a filter-injection string", "1 OR 1=1"], + ["a non-integer", 1.5], + ["Infinity", Number.POSITIVE_INFINITY], + ["NaN", Number.NaN], + ])("refuses to build a Donors filter from %s as envelopeNo", async (_label, envelopeNo) => { + const error = await saveAndCatch(householdWith({ envelopeNo })); + + expect(error).toBeInstanceOf(PartialSaveError); + + // The malformed value must never have reached an MP query. + const donorQueries = mockGetTableRecords.mock.calls.filter( + (call) => call[0]?.table === "Donors" && typeof call[0]?.filter === "string", + ); + for (const [args] of donorQueries) { + expect(args.filter).not.toContain(String(envelopeNo)); + } + }); + + it("refuses to build a Donors filter from an injection-shaped donorId", async () => { + const error = await saveAndCatch( + householdWith({ envelopeNo: 1001, donorId: "7 OR 1=1" }), + ); + + expect(error).toBeInstanceOf(PartialSaveError); + const donorQueries = mockGetTableRecords.mock.calls.filter( + (call) => call[0]?.table === "Donors", + ); + for (const [args] of donorQueries) { + expect(String(args.filter ?? "")).not.toContain("OR 1=1"); + } + expect(mockUpdateTableRecords).not.toHaveBeenCalledWith( + "Donors", + expect.anything(), + expect.anything(), + ); + }); + + it("still treats donorId 0 as 'no existing donor' rather than rejecting it", async () => { + // 0 is the historical "none" sentinel alongside null; validation must not + // turn it into an error. + mockGetTableRecords.mockResolvedValueOnce([]); // no envelope conflict + mockCreateTableRecords + .mockResolvedValueOnce([{ Address_ID: 501 }]) + .mockResolvedValueOnce([{ Contact_ID: 900 }]) + .mockResolvedValueOnce([{ Donor_ID: 901 }]); + + const service = await FamilyService.getInstance(); + const household = householdWith({ envelopeNo: 1001, donorId: 0, contactId: 900 }); + + await service.saveHousehold(household).catch(() => { + // Later stages of the save are not the subject here; what matters is + // that validation did not reject donorId: 0 up front. + }); + + const donorFilters = mockGetTableRecords.mock.calls + .filter((call) => call[0]?.table === "Donors") + .map((call) => call[0].filter as string); + expect(donorFilters.some((f) => f === "Envelope_No = 1001")).toBe(true); + }); + + it("builds the exact expected filter for well-formed values", async () => { + mockGetTableRecords.mockResolvedValueOnce([]); + mockCreateTableRecords + .mockResolvedValueOnce([{ Address_ID: 501 }]) + .mockResolvedValueOnce([{ Contact_ID: 900 }]); + mockUpdateTableRecords.mockResolvedValue([{}]); + + const service = await FamilyService.getInstance(); + await service + .saveHousehold(householdWith({ envelopeNo: 1001, donorId: 77, contactId: 900 })) + .catch(() => {}); + + const donorFilters = mockGetTableRecords.mock.calls + .filter((call) => call[0]?.table === "Donors") + .map((call) => call[0].filter as string); + expect(donorFilters).toContain("Envelope_No = 1001 AND Donor_ID <> 77"); + }); +}); diff --git a/src/services/familyService.ts b/src/services/familyService.ts index f3b9e98..ba24c8c 100644 --- a/src/services/familyService.ts +++ b/src/services/familyService.ts @@ -688,8 +688,26 @@ export class FamilyService { requested: number, excludeDonorId: number | null, ): Promise<{ envelopeNo: number; bumped: boolean }> { + // Both values land in a raw $filter string below. They originate from a + // client-supplied Household, and a server action is a public POST endpoint + // whose TypeScript types are erased at runtime — so `number` here proves + // nothing. The action parses with HouseholdSchema; this is the second + // layer, at the point where the string is actually built. + validatePositiveInt(requested); + // 0 and null both mean "no donor to exclude" and are handled by the + // ternary below, so only a value that will actually be interpolated is + // required to be a positive integer. + if (excludeDonorId !== null && excludeDonorId !== 0) { + validatePositiveInt(excludeDonorId); + } + let candidate = requested; for (let attempt = 0; attempt < 5; attempt++) { + // `candidate` is either `requested` (validated above) or the result of + // getNextEnvelopeNumber(), which is derived server-side — but validate + // each iteration anyway so no future change to that method can quietly + // reintroduce an unchecked value here. + validatePositiveInt(candidate); const filter = excludeDonorId && excludeDonorId > 0 ? `Envelope_No = ${candidate} AND Donor_ID <> ${excludeDonorId}` @@ -726,6 +744,8 @@ export class FamilyService { } if (existingDonorId && existingDonorId > 0) { + // Client-supplied, and it selects which Donors row gets written. + validatePositiveInt(existingDonorId); await this.mp!.updateTableRecords( "Donors", [{ Donor_ID: existingDonorId, Envelope_No: finalEnvelopeNo }],