From 93b1204087fec7a0cb6b18daf511d9757e4c8a72 Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Sat, 10 Oct 2026 10:32:48 -0400 Subject: [PATCH] chore(todo): close owner checklist and drop held/step7/step9 TODOs Owner checklist is complete. The repo-settings, proc-allowlist, step 7 and step 9 items will not be done here: this is a reference/template repo and those decisions belong to downstream clones, or the changes cause more problems than they solve. Also trims the security README's "Still open" list to the CSP walk and rewords the procedure doc's allowlist note to stop pointing at a TODO. Co-Authored-By: Claude Opus 5.5 (1M context) --- ...9-30-held-github-repo-security-settings.md | 20 ---- ...6-09-30-held-stored-procedure-allowlist.md | 24 ----- ...-step7-write-audit-and-datetime-cleanup.md | 31 ------ .../2026-09-30-step9-ci-setup-and-docs.md | 24 ----- ...0-10-owner-ops-checklist-before-release.md | 96 ------------------- .claude/TODO/INDEX.md | 48 +++------- .../mp-provider/services/procedure.md | 2 +- .claude/references/security/README.md | 26 ----- 8 files changed, 16 insertions(+), 255 deletions(-) delete mode 100644 .claude/TODO/2026-09-30-held-github-repo-security-settings.md delete mode 100644 .claude/TODO/2026-09-30-held-stored-procedure-allowlist.md delete mode 100644 .claude/TODO/2026-09-30-step7-write-audit-and-datetime-cleanup.md delete mode 100644 .claude/TODO/2026-09-30-step9-ci-setup-and-docs.md delete mode 100644 .claude/TODO/2026-10-10-owner-ops-checklist-before-release.md 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.