diff --git a/.claude/TODO/2026-09-30-held-github-repo-security-settings.md b/.claude/TODO/2026-09-30-held-github-repo-security-settings.md deleted file mode 100644 index 584bc6f..0000000 --- a/.claude/TODO/2026-09-30-held-github-repo-security-settings.md +++ /dev/null @@ -1,20 +0,0 @@ ---- -title: "Held — Enable GitHub repo security settings (private vuln reporting, secret scanning, push protection, Dependabot security updates)" -severity: low -tags: [security, doc] -area: commands -files: [SECURITY.md] -discovered: 2026-09-30 -discovered_by: playbook-audit (port-security-review-2026-09-28.md P15) -status: open ---- - -## Problem -Owner decision (2026-09-30): **hold.** These are repo-admin toggles only the owner can set. - -## Proposed fix -GitHub → Settings → Code security: enable private vulnerability reporting, secret scanning, push protection, -Dependabot alerts and security updates. Point `SECURITY.md` at private vulnerability reporting. - -## Impact if not fixed -Leaked secrets aren't blocked at push; reporters have no private channel. diff --git a/.claude/TODO/2026-09-30-held-stored-procedure-allowlist.md b/.claude/TODO/2026-09-30-held-stored-procedure-allowlist.md deleted file mode 100644 index 5ef1a59..0000000 --- a/.claude/TODO/2026-09-30-held-stored-procedure-allowlist.md +++ /dev/null @@ -1,24 +0,0 @@ ---- -title: "Held — Deny-all stored-procedure allowlist in the MP provider" -severity: medium -tags: [security] -area: mp-provider -files: [src/lib/providers/ministry-platform/services/procedure.service.ts] -discovered: 2026-09-30 -discovered_by: playbook-audit (port-security-review-2026-09-28.md P9) -status: open ---- - -## Problem -Owner decision (2026-09-30): **skip for now.** Upstream restricts `executeProcedure*` to an explicit allowlist so -a future call path can't reach arbitrary `api_*` procs via the service account. Procs this fork calls: -`api_MPNextTools_GetPages`, `api_MPNextTools_GetPageFields`, `api_MPNextTools_UpdatePageFieldOrder`, -`api_Tools_GetPageData`, `api_Tools_GetUserTools`, `api_Common_GetSelection`, `api_dev_DeployTool` -(plus `api_dev_GetProcedureDefinition` from the dev panel). - -## Proposed fix -Confirm each proc with the owner, then add a frozen allowlist checked in `procedure.service.ts`, with a test that -an unlisted name is refused before any HTTP call. - -## Impact if not fixed -Defence in depth only — every current caller is gated and passes a literal proc name. diff --git a/.claude/TODO/2026-09-30-step7-write-audit-and-datetime-cleanup.md b/.claude/TODO/2026-09-30-step7-write-audit-and-datetime-cleanup.md deleted file mode 100644 index aab567b..0000000 --- a/.claude/TODO/2026-09-30-step7-write-audit-and-datetime-cleanup.md +++ /dev/null @@ -1,31 +0,0 @@ ---- -title: "Step 7 — Write-audit gaps and datetime drift cleanup" -severity: medium -tags: [drift, missing-test] -area: services -files: [src/services/familyService.ts, src/services/groupService.ts, src/components/add-edit-family/add-edit-family.tsx, src/services/authorizationService.ts, src/lib/providers/ministry-platform/db/] -discovered: 2026-09-30 -discovered_by: playbook-audit (port-mp-write-attribution.md, port-mp-datetime-handling.md) -status: open ---- - -## Problem -- Stored-proc writes (`api_MPNextTools_UpdatePageFieldOrder`, `api_dev_DeployTool`) pass `$userId` in the query - string but neither proc declares `@UserID` or writes `dp_Audit_Log` — MP almost certainly doesn't audit DML inside - a proc, so page-field reorders leave no audit trail. -- `familyService.ts:83-87` `toDatetime()` builds `` `${value}T00:00:00` `` itself for `Date_of_Birth`, - `Season_Start/End` instead of `DomainTimezoneService`; `includes("T")` passes `Z`-tagged values through. -- Edit prefill (`groupService.getGroup` ~`:227,261`, `add-edit-family.tsx:1239`) uses `.split('T')[0]` — breaks on - MP's space-separated `YYYY-MM-DD HH:MM:SS`. -- `mp.write.non_user` is `console.warn(name, obj)` not JSON with an `event` key, and fires on refused reads. -- `familyService.test.ts` asserts `$userId` on only some update calls. Default tz mock is `"UTC"` (hides drift). -- Datetime reference doc is thinner than upstream (132 vs 230 lines); not linked from INDEX / services README. - Note: Git Bash on Windows drops `TZ` values containing `/` — use PowerShell `$env:TZ`. - -## Proposed fix -Explicit `@UserID` + `dp_Audit_Log`/`dp_Audit_Detail` inserts in the proc SQL (then `npm run mp:build:install`), or -document the gap. Route family dates through `tz.toMpSqlDatetime`; `replace(' ', 'T').slice(0,10)` prefill; -JSON log for writes only; loop `$userId` assertion over every update call; non-UTC tz in tests; port the fuller doc. - -## Impact if not fixed -No audit trail for domain-wide page-field changes; latent date drift if a caller ever sends a zoned value. diff --git a/.claude/TODO/2026-09-30-step9-ci-setup-and-docs.md b/.claude/TODO/2026-09-30-step9-ci-setup-and-docs.md deleted file mode 100644 index 8d97f7c..0000000 --- a/.claude/TODO/2026-09-30-step9-ci-setup-and-docs.md +++ /dev/null @@ -1,24 +0,0 @@ ---- -title: "Step 9 — CI supply-chain hardening, setup wizard env writer, docs and CLAUDE.md rules" -severity: low -tags: [security, doc] -area: commands -files: [.github/workflows/test.yml, .github/dependabot.yml, scripts/setup.ts, CLAUDE.md, .env.example, .claude/references/security/README.md] -discovered: 2026-09-30 -discovered_by: playbook-audit (port-security-review-2026-09-28.md P15/P16, port-downstream-hardening.md P13) -status: open ---- - -## Problem -- Actions pinned by tag not SHA; no `persist-credentials: false`; no `dependabot.yml`. -- `scripts/setup.ts:273-288` writes env via string `replace` (`$&` expansion), unquoted values, no mode 0600, runs - `npm install`/`npm update`, doesn't check a hand-entered secret's length. -- CLAUDE.md lacks: "sanitize every `$filter` value, number-typed included" and "better-auth body options / idToken — - re-check on upgrade". Security README lacks F12. `mp.write.non_user` undocumented. - -## Proposed fix -SHA-pin actions, `persist-credentials: false`, add Dependabot config; replacer-function env writes, quoted values, -0600; add the two CLAUDE.md rules; F12 + F3b rows in the security README. - -## Impact if not fixed -Tag-hijack risk in CI; setup can corrupt a secret containing `$&`. diff --git a/.claude/TODO/2026-10-10-owner-ops-checklist-before-release.md b/.claude/TODO/2026-10-10-owner-ops-checklist-before-release.md deleted file mode 100644 index b7afc0e..0000000 --- a/.claude/TODO/2026-10-10-owner-ops-checklist-before-release.md +++ /dev/null @@ -1,96 +0,0 @@ ---- -title: "Owner checklist — environment, MP and GitHub settings to do before (and right after) the next /release" -severity: high -tags: [security, doc] -area: auth -files: [.env.example, src/lib/env.ts, src/services/authorizationService.ts, SECURITY.md, .claude/references/security/README.md, .claude/references/DECISIONS.md] -discovered: 2026-10-10 -discovered_by: todo-review (units 0–1 after PRs #40 and #41) -status: open ---- - -## Problem -`dev` now carries security Steps 1–5, the template-editor removal (#40) and -the single-MP-client change (#41). Several things these need are settings, -not code: Vercel env vars, the MP API Client record, and GitHub repo toggles. -Only the owner can change those. If they aren't done, the next `/release` to -`main` either locks everyone out or breaks sign-in. This file is the -consolidated checklist. Where an item already has its own TODO, that file has -the detail and stays open until its own steps are done. - -**Environments to cover for every env-var item:** Vercel Production, Vercel -Preview (staging), Vercel Development, each developer's `.env.local`, and any -customer-deployed clone. - -## Before the next `/release` - -- [x] **1. Set `MP_SECURITY_ROLES=*` in every environment.** _Done 2026-10-10._ - It fails closed now: unset, blank or `","` means nobody can use any tool, so - every user is redirected to `/no-access`. Use `*` (any MP security role) for - now; a comma list of role names later. - -- [ ] **2. Check the redirect URIs on the MP client.** - Sign-in now runs as `MINISTRY_PLATFORM_CLIENT_ID` (ADR-027). In each - environment, that client's MP API Client record must list: - - redirect URI: `/api/auth/callback/ministryplatform` - - post-logout redirect URI: `` (exact, no trailing slash) - - Any environment that used a separate sign-in client (`MP_OIDC_CLIENT_ID`) - will fail to sign in until these are added. Nothing broader belongs on the - record: this one secret is both data access and sign-in. - -- [ ] **3. Delete stale `MP_OIDC_CLIENT_ID` / `MP_OIDC_CLIENT_SECRET` values.** - Remove them from Vercel (all three environments) and every `.env.local`. - The app ignores them since #41, but the secret is still live. If a separate - MP API Client was registered for sign-in, retire it in MP too. - -- [x] **4. Rotate `BETTER_AUTH_SECRET`** _Done 2026-10-10 (incl. audit-log review)._ (≥ 32 chars: `openssl rand -base64 32`) - in every environment, at the same deploy that ships Step 3. Everyone is - signed out once either way. Then review `dp_Audit_Log` for writes made - through this app between 2026-07-09 and the date `e94e549` reached each - environment, looking for `User_ID`s that don't match the signed-in person. - -## Right after the release deploys - -- [ ] **5. Real-MP sign-in smoke test** in Production and staging: sign in; - `/api/auth/get-session` has a `userGuid`; the function logs show no - `auth.oidc.discovery_failed` or `auth.userinfo.id_token_unverified`; sign-out - ends the MP session without a "log out?" prompt; a tool page opens instead of - `/no-access`. This is still outstanding from Step 4 and now also covers - items 1–3. - -## Anytime (not release-blocking) - -- [ ] **6. Ask MP admins to restrict who can create or rename Security Roles.** - `MP_SECURITY_ROLES` matches roles by **name**, so anyone who can create a - role with a listed name gets access. (With `*`, any role at all gets access.) - -- [ ] **7. GitHub → Settings → Code security:** enable private vulnerability - reporting, secret scanning, push protection, Dependabot alerts and Dependabot - security updates. `SECURITY.md` already points reporters at private - reporting. - Detail: [`2026-09-30-held-github-repo-security-settings.md`](2026-09-30-held-github-repo-security-settings.md). - -- [ ] **8. Tell downstream clones** to do items 1–4 themselves. Each has its - own secret, roles and MP client. `SECURITY.md` § Downstream clones already - covers the secret rotation. - -## Evidence -- `src/services/authorizationService.ts` `resolveRolePolicy()`: unset, blank - or `","` resolves to `unconfigured` (nobody). -- `src/lib/env.ts` `getMpClient()`: sign-in, id_token audience and sign-out - `client_id` all use `MINISTRY_PLATFORM_CLIENT_ID` (#41, ADR-027). -- `.claude/references/security/README.md` § open items: redirect-URI check, - stale `MP_OIDC_*`, smoke test, repo settings. - -## Proposed fix -Work through the checkboxes. Tick each one in this file (or note the date per -environment beside it). Close the linked TODOs as their items finish, and close -this file once items 1–5 are done in every environment. - -## Impact if not fixed -- Item 1 skipped: environment-wide lockout on release. -- Items 2–3 skipped: sign-in fails where a separate client was in use; a live - but unused secret keeps sitting in env stores. -- Item 4 skipped: a forged-identity session cookie from the exposure window stays valid. -- Items 6–7 skipped: role-name spoofing, and leaked secrets that nothing blocks at push. diff --git a/.claude/TODO/INDEX.md b/.claude/TODO/INDEX.md index 4497a73..743bdfc 100644 --- a/.claude/TODO/INDEX.md +++ b/.claude/TODO/INDEX.md @@ -18,13 +18,10 @@ Severity tiers: - **medium**: doc drift, missing test, refactor with real cost - **low**: nits, minor doc fixes, stylistic improvements -Total: **11 open TODOs**. +Total: **6 open TODOs**. -> **Start here for owner (non-code) work:** [`2026-10-10-owner-ops-checklist-before-release.md`](2026-10-10-owner-ops-checklist-before-release.md) -> collects every env-var, MP and GitHub setting to do before and right after the next `/release`. - -> **`step6`…`step9`** are the remaining steps from the 2026-09-30 MPNext playbook -> audit (one PR each). `held-*` items wait on an owner decision. +> **`step6`** and **`step8`** are the remaining steps from the 2026-09-30 MPNext +> playbook audit (one PR each). `held-*` items wait on an owner decision. --- @@ -33,25 +30,19 @@ Total: **11 open TODOs**. ### Critical (0) _none open_ -### High (1) -| Area | Tags | Title | File | -|---|---|---|---| -| auth | security, doc | Owner checklist — settings to do before/after the next /release | [→](2026-10-10-owner-ops-checklist-before-release.md) | +### High (0) +_none open_ -### Medium (4) +### Medium (2) | Area | Tags | Title | File | |---|---|---|---| | routing | security, drift | Step 6 — Headers, next.config, server-only, prerender + build in CI | [→](2026-09-30-step6-headers-csp-build-guards.md) | -| services | drift, missing-test | Step 7 — Write-audit gaps and datetime drift cleanup | [→](2026-09-30-step7-write-audit-and-datetime-cleanup.md) | | routing | security, doc | Held — CSP notes for customer-deployed clones | [→](2026-09-30-held-csp-customer-deployments.md) | -| mp-provider | security | Held — Deny-all stored-procedure allowlist | [→](2026-09-30-held-stored-procedure-allowlist.md) | -### Low (6) +### Low (4) | Area | Tags | Title | File | |---|---|---|---| | components | security, bug | Step 8 — Address-label caps, error boundaries, tool-params decode, codegen escaping | [→](2026-09-30-step8-fork-specific-low-risk-items.md) | -| commands | security, doc | Step 9 — CI supply chain, setup env writer, docs | [→](2026-09-30-step9-ci-setup-and-docs.md) | -| commands | security, doc | Held — GitHub repo security settings | [→](2026-09-30-held-github-repo-security-settings.md) | | components | bug | Add/Edit Family search: "No contacts found" empty state never renders | [→](2026-09-13-search-empty-state-never-renders.md) | | components | refactor, missing-test | Unreachable "empty STEP_FIELDS" branch in `GroupWizard.handleNext` | [→](2026-09-13-dead-empty-fields-branch-handlenext.md) | | services | security | `ToolService.resolveContactIds` takes table/column names from its caller (dev panel) | [→](2026-10-10-dev-panel-resolvecontactids-caller-identifiers.md) | @@ -60,36 +51,27 @@ _none open_ ## By tag -### security (8) -- owner-ops-checklist-before-release — high +### security (4) - step6-headers-csp-build-guards — medium - held-csp-customer-deployments — medium -- held-stored-procedure-allowlist — medium - step8-fork-specific-low-risk-items — low -- step9-ci-setup-and-docs — low -- held-github-repo-security-settings — low - dev-panel-resolvecontactids-caller-identifiers — low ### bug (2) - step8-fork-specific-low-risk-items — low - search-empty-state-never-renders — low -### drift (2) +### drift (1) - step6-headers-csp-build-guards — medium -- step7-write-audit-and-datetime-cleanup — medium -### missing-test (2) -- step7-write-audit-and-datetime-cleanup — medium +### missing-test (1) - dead-empty-fields-branch-handlenext — low ### refactor (1) - dead-empty-fields-branch-handlenext — low -### doc (4) -- owner-ops-checklist-before-release — high +### doc (1) - held-csp-customer-deployments — medium -- step9-ci-setup-and-docs — low -- held-github-repo-security-settings — low ### perf (0) _none open_ @@ -101,11 +83,11 @@ _none open_ | Area | Count | |---|---| | components | 3 | -| auth | 1 | | routing | 2 | -| commands | 2 | -| services | 2 | -| mp-provider | 1 | +| services | 1 | +| auth | 0 | +| commands | 0 | +| mp-provider | 0 | | testing | 0 | | utils | 0 | | mp-schema | 0 | diff --git a/.claude/references/mp-provider/services/procedure.md b/.claude/references/mp-provider/services/procedure.md index 84afde9..e3f06b0 100644 --- a/.claude/references/mp-provider/services/procedure.md +++ b/.claude/references/mp-provider/services/procedure.md @@ -62,7 +62,7 @@ function isDevProcedure(procedure: string): boolean { ## How it works - Both execute methods call `resolveHttpClient(procedure)` which branches on `isDevProcedure` and invokes the matching `ensureValid*Token` + `get*HttpClient` pair (`procedure.service.ts:87-95`). -- Procedure name is validated with `sanitizeIdentifier` (`^[A-Za-z_][A-Za-z0-9_]*$`, ≤128) and then URL-encoded, before any token work; a bad name throws `Invalid procedure name`. There is **no** allowlist of callable procedures (held; see `.claude/TODO/2026-09-30-held-stored-procedure-allowlist.md`). +- Procedure name is validated with `sanitizeIdentifier` (`^[A-Za-z_][A-Za-z0-9_]*$`, ≤128) and then URL-encoded, before any token work; a bad name throws `Invalid procedure name`. There is **no** allowlist of callable procedures — by design for this template; every caller is gated and passes a literal proc name. A downstream clone that wants defence in depth can add one here. - Errors propagate after `logger.error('mp.procs.execute_failed', { procedure, error: errorName(e) })`. ## Stored procedures called by this codebase diff --git a/.claude/references/security/README.md b/.claude/references/security/README.md index fa25780..379d350 100644 --- a/.claude/references/security/README.md +++ b/.claude/references/security/README.md @@ -505,32 +505,6 @@ and no code-bearing URLs in logs. ### Still open — needs a human -- **The MP OAuth client's registered redirect URI must be updated.** Better Auth - 1.7 changed the callback path, so the `redirect_uri` this app sends is now - `/api/auth/callback/ministryplatform` — previously - `/api/auth/oauth2/callback/ministry-platform`. Note BOTH halves changed: the - 1.7 upgrade moved `/oauth2/callback/` to `/callback/`, and the provider id was - renamed from `ministry-platform` to `ministryplatform`. Register the new value in the - Ministry Platform OAuth client **before** deploying, or MP will reject the - authorization request outright. - **The enforced-CSP browser walk has not been done.** Headers and nonce coverage were verified against `next start`; a human still needs to click through every Radix surface under enforcement. -- **`BETTER_AUTH_SECRET` rotated** (done 2026-10-10; owner decision 2026-09-30) in every - Vercel environment and `.env.local` — this deployment was exposed to - F-UPDATE-USER from 2026-07-09 to 2026-09-13. Patching does not revoke sessions - already forged, and with `refreshCache` on (fixed 2026-09-30) they could keep - re-signing themselves; with no database there is no session table to clear. -- **Repo settings**: GitHub private vulnerability reporting, secret scanning, - push protection and Dependabot security updates were all **disabled** on - 2026-09-30 (public repo). `SECURITY.md` points reporters at private - vulnerability reporting, so it needs enabling. -- **Check the MP client's redirect URIs** in every environment: - `MINISTRY_PLATFORM_CLIENT_ID`'s MP record must list exactly - `/api/auth/callback/ministryplatform` and post-logout - ``. Remove `MP_OIDC_CLIENT_ID` / `_SECRET` from any - environment that set them (ignored now, but stale secrets). -- **Real-MP smoke test of Step 4 not yet done** (a real sign-in): sign in, - confirm `/api/auth/get-session` has a `userGuid`, no - `auth.oidc.discovery_failed` / `auth.userinfo.id_token_unverified` in the - log, and sign-out ends the MP session without a "log out?" prompt.