From ea3853dedb63eebc4cd0f15f049f2214bfbd5f5a Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Tue, 29 Sep 2026 18:28:47 -0400 Subject: [PATCH] docs: accuracy pass across CLAUDE.md, README and references Verified against the code; fixes factual errors, drift and broken guidance. - CLAUDE.md: $userId example now takes the acting User_ID from requireSecurityRole (was Contact_ID); real Zod v4 differences; accurate coverage thresholds (only proxy.ts is 100%); Discord workflow; npx tsx; sign-out-broadcast; more reference docs. - README: security notice as an advisory table; JWE sessions and the CurrentUserProfile DTO; no hard-coded test/coverage counts; Node 22 minimum; project tree and Documentation links brought up to date; generator sample output consistent; Contributing/Support expanded. - Node 22 minimum: setup.ts check and "engines" in package.json. - References: sanitized worked $filter example; .pick() allowlist; stale counts and line numbers replaced; lockfile CI job runs in parallel; x-nonce header and force-dynamic pages; sql.db.md confirm-first note; TestCoverage.md marked historical; stale vitest.config comments. - Security docs: sign-in advisory affected range + merge-base checks and fix date; direct private-reporting link in SECURITY.md; downstream playbook F3b grep covers src/app/signin/; write-attribution playbook marked superseded in part. - Commands: /audit-deps relocks after any npm install; /release honors --draft/--prerelease/--target. - Provider/shared-actions READMEs: CurrentUserProfile return type, react-server condition, guards.ts, one MP host placeholder everywhere. Co-Authored-By: Claude Opus 5.5 (1M context) --- .claude/commands/audit-deps.md | 22 +++- .claude/commands/release.md | 4 +- .claude/docs/TestCoverage.md | 9 +- .../playbooks/port-downstream-hardening.md | 2 +- .../playbooks/port-mp-write-attribution.md | 8 ++ .../port-security-review-2026-09-28.md | 2 +- .claude/references/auth.md | 24 ++-- .claude/references/components.md | 8 +- .claude/references/deps-known-issues.md | 17 +-- .../ministryplatform.datetimehandling.md | 2 +- .../ministryplatform.query-syntax.md | 12 +- .claude/references/security-headers.md | 11 +- .claude/references/sql.db.md | 4 + .claude/references/testing.md | 90 ++++++++------- CLAUDE.md | 35 ++++-- README.md | 104 +++++++++++++----- SECURITY.md | 5 +- docs/OAUTH_LOGOUT_SETUP.md | 2 +- docs/security/2026-09-25-signin-hardening.md | 16 ++- .../security/downstream-hardening-playbook.md | 2 +- package-lock.json | 3 + package.json | 3 + scripts/setup.ts | 6 +- src/components/shared-actions/README.md | 19 +++- .../ministry-platform/docs/README.md | 10 +- .../ministry-platform/scripts/README.md | 2 +- vitest.config.mts | 9 +- 27 files changed, 283 insertions(+), 148 deletions(-) diff --git a/.claude/commands/audit-deps.md b/.claude/commands/audit-deps.md index 9b316bec..876c55a1 100644 --- a/.claude/commands/audit-deps.md +++ b/.claude/commands/audit-deps.md @@ -31,7 +31,10 @@ Know exactly what you may do without asking: **Allowed without asking** - Every read-only command (`npm ls`, `npm audit`, `npm outdated`, `npm view`, `git status`, `git diff`). -- `npm install` with **no package argument** (syncs the tree to the lockfile). +- `npm install` with **no package argument** (syncs the tree to the lockfile). If it changes + `package-lock.json`, regenerate it with `npm run deps:relock` and check it with + `npm run deps:verify` — a bare Windows `npm install`/`npm dedupe` must never produce the + committed lockfile (CLAUDE.md § Dependency Rule). - **`npm update`** with no package argument — applies patch/minor updates inside the existing ranges. This is the phase 6 workhorse; it never edits `package.json`. - `npm run deps:relock` / `npm run deps:verify` — required after any `npm update`. @@ -77,10 +80,13 @@ reading a single advisory. 2. `node --version` and `npm --version` — note them in the report; some advisories and peer ranges are engine-dependent. 3. `npm ls --all 2>&1 | grep -iE "invalid|missing|UNMET"` — any hit means the installed - tree does not satisfy `package.json`. Run `npm install`, then re-check. + tree does not satisfy `package.json`. Run `npm install`, then re-check. If that changed + `package-lock.json`, run `npm run deps:relock` then `npm run deps:verify` before going on. 4. Confirm `package.json` and the lockfile agree. Prefer `npm ls` cleanliness plus `npm install --package-lock-only --dry-run` (non-destructive). Reach for `npm ci` only - if you must, and warn the user first — it **deletes and rebuilds `node_modules`**. + if you must, and warn the user first — it **deletes and rebuilds `node_modules`**. Have + the user stop `next dev` before it runs: a running dev server locks a native `.node` + file, so `npm ci` aborts after the delete and leaves `node_modules` half-installed. 5. Only proceed once the tree matches `package.json`. State the baseline explicitly: "audited tree = package.json as of \". @@ -208,13 +214,17 @@ recommendation. Then ask. For each approved major, one at a time: -1. Edit the range in `package.json`, `npm install`. Watch for `ERESOLVE` and for - `invalid:` in `npm ls ` — either means surrounding tooling hasn't adopted it. +1. Edit the range in `package.json`, `npm install`, then `npm run deps:relock` and + `npm run deps:verify` (the bare install's lockfile is never the one you commit). Watch + for `ERESOLVE` and for `invalid:` in `npm ls ` — either means surrounding tooling + hasn't adopted it. 2. Apply the code changes the migration guide requires. 3. Run the full phase 7 verification. 4. **On failure, revert cleanly and completely:** restore the `package.json` range, `git checkout -- package-lock.json`, `npm install`, then confirm `git status` is clean - for those files and the build passes again. Record the failure and its cause in the + for those files (if `npm install` rewrote the restored lockfile, discard that with + `git checkout -- package-lock.json` again rather than committing it — or relock and + verify) and the build passes again. Record the failure and its cause in the known-issues file with today's date so the next run doesn't retry it blindly. ## Phase 9 — Report, persist, commit diff --git a/.claude/commands/release.md b/.claude/commands/release.md index f69ff703..83d447ee 100644 --- a/.claude/commands/release.md +++ b/.claude/commands/release.md @@ -41,8 +41,10 @@ Create a GitHub release with auto-generated release notes from merged pull reque - Apply any requested changes 6. **Create the release:** - - Run `gh release create --target main --title "" --notes ""` + - Run `gh release create --target --title "" --notes ""`, where `` is the `--target` argument (default `main`) + - Append `--draft` if the `--draft` argument was given, and `--prerelease` if `--prerelease` was given - Use a HEREDOC for the notes body to handle multiline content + - If the release is a draft, the tag is not created until it is published — say so, and skip the tag-sync check in step 7 - Run `git fetch --tags` to sync the new tag locally 7. **Post-creation:** diff --git a/.claude/docs/TestCoverage.md b/.claude/docs/TestCoverage.md index ce9d3c5d..c30a4310 100644 --- a/.claude/docs/TestCoverage.md +++ b/.claude/docs/TestCoverage.md @@ -1,5 +1,10 @@ # MPNext — Application & Unit Test Coverage Review +> **Historical snapshot.** This review describes commit `5bc505a` as of 2026-09-12 and is not kept up +> to date. The coverage gates that apply today are the thresholds in `vitest.config.mts`, documented in +> [Testing Reference](../references/testing.md) § Coverage; run `npm run test:coverage` for current figures. +> The `.claude/TODO/` folder it mentions no longer exists. + **Date:** 2026-09-12 **Reviewed commit:** `5bc505a` (branch `docs/release-readiness-refresh`) **Scope:** whole application — `src/**` excluding generated MP models, codegen scripts and `src/components/ui/` @@ -30,7 +35,7 @@ Three things matter more than the headline number: |---|---| | **Measurement was inflated ~2.2×.** With no explicit `coverage.include`, every file no test imported dropped out of the denominator. | **Fixed.** `vitest.config.mts` now sets an explicit `include`, plus per-glob `thresholds` that fail the run on regression. | | **`testing.md` claimed 95.39% coverage** — not reproducible under any configuration. | **Fixed.** Rewritten against measured numbers, with the new mock patterns documented. | -| **Coverage was pointed away from the risk.** Two `'use server'` actions have no session check at all, and both sat at 100% line coverage. | **Documented, then fixed.** Every finding in §5 is closed — §5.1 (filter injection), §5.2/§5.3 (missing auth), §5.4/§5.5 (missing authz, duplicated User_ID lookup), §5.6 (N+1 lookup), §5.7 (token lifetime), §5.8 (tests asserting against a copy of the logic). No test-derived item remains in `.claude/TODO/`. | +| **Coverage was pointed away from the risk.** Two `'use server'` actions have no session check at all, and both sat at 100% line coverage. | **Documented, then fixed.** Every finding in §5 is closed — §5.1 (filter injection), §5.2/§5.3 (missing auth), §5.4/§5.5 (missing authz, duplicated User_ID lookup), §5.6 (N+1 lookup), §5.7 (token lifetime), §5.8 (tests asserting against a copy of the logic). No test-derived item remained in `.claude/TODO/` (since removed). | The shape of the original problem is worth restating, because the new number does not make it go away: **high coverage is not evidence of correctness.** The filter-injection path in §5.1 lived in a @@ -176,7 +181,7 @@ safeguards), `contact-logs.tsx:387` and `user-menu.tsx:35`. None is worth the co **This is the most important section.** Each item below was fully covered by passing tests and was still wrong. The original coverage work **documented rather than fixed** them — one file per issue in -`.claude/TODO/` — and the fixed items have since been closed out by follow-up work; each carries a +`.claude/TODO/` (a folder that has since been removed) — and the fixed items have since been closed out by follow-up work; each carries a regression test that would have caught the defect. ### 5.1 Numeric IDs are interpolated into MP filters unsanitized ✅ FIXED diff --git a/.claude/playbooks/port-downstream-hardening.md b/.claude/playbooks/port-downstream-hardening.md index 1ddeaafa..e3210151 100644 --- a/.claude/playbooks/port-downstream-hardening.md +++ b/.claude/playbooks/port-downstream-hardening.md @@ -101,7 +101,7 @@ grep -rn "console\.log\|console\.debug\|console\.info" src/ --include="*.ts" --i grep -rn "callbackUrl" src/ | grep -i "location.href\|sanitize" # F3b: the 2026-09-12 sanitizer only checked a leading `//` and `/\` — tab/CR/LF bypass it grep -rqF 'startsWith("/\\")' src/ && echo "✗ F3b (weak leading-/\\ check)" || echo "✓ no weak check" -grep -rqF '\u001f' src/components/sign-in/ && echo "✓ control chars refused" || echo "✗ F3b (no control-char rule)" +grep -rqF '\u001f' src/components/sign-in/ src/app/signin/ 2>/dev/null && echo "✓ control chars refused" || echo "✗ F3b (no control-char rule)" # F12: is ID-token sign-in refused? (live whenever discoveryUrl is set on better-auth >= 1.7) grep -q "discoveryUrl" src/lib/auth.ts && ! grep -q "ID_TOKEN_SIGN_IN_DISABLED" src/lib/auth.ts \ diff --git a/.claude/playbooks/port-mp-write-attribution.md b/.claude/playbooks/port-mp-write-attribution.md index d561febb..76c691c0 100644 --- a/.claude/playbooks/port-mp-write-attribution.md +++ b/.claude/playbooks/port-mp-write-attribution.md @@ -1,5 +1,13 @@ # Playbook: Port Ministry Platform Write Attribution Into This Repo +> **Superseded in part — read this first.** +> +> - **Anonymous writes are now refused upstream.** MPNext resolves the acting `User_ID` inside `AuthorizationService.requireSecurityRole` (`src/services/authorizationService.ts`, `hasSecurityRole`): writes go through `SessionContextService.getActingUserIdForWrite`, which still logs `mp.write.non_user`, but a `null` result is then refused as `no_mp_user`. The gate returns the `User_ID`, and services pass that as `$userId`. `CLAUDE.md` (Key Development Practice 10) requires this role gate on every service method that touches MP data. Outcome 4 below ("anonymous / unresolved writes do not throw") no longer describes upstream. +> - **If this fork has ported the security playbooks** ([`port-downstream-hardening.md`](port-downstream-hardening.md), [`port-security-review-2026-09-28.md`](port-security-review-2026-09-28.md)) or the [Downstream Hardening Playbook](../../docs/security/downstream-hardening-playbook.md), follow those where they disagree with this one. The session-side resolver and `SessionContextService` below are still the basis they build on. +> - **`.claude/references/ministryplatform.writeattribution.md` does not exist upstream.** The steps below that create and cite it are optional and fork-local; upstream documents attribution in `CLAUDE.md` and `.claude/references/auth.md`. +> +> The body below is kept as written for forks porting the original change. + You are Claude Code running in a repo that integrates with **Ministry Platform** (MP). Another team has closed a class of audit-trail gaps where MP write APIs were called without `$userId`, so every change recorded against MP's audit log was attributed to the OAuth integration account rather than the user who performed it. This playbook ports that fix into the repo you're in. **Outcome you're driving toward** diff --git a/.claude/playbooks/port-security-review-2026-09-28.md b/.claude/playbooks/port-security-review-2026-09-28.md index 7651a061..8999c9da 100644 --- a/.claude/playbooks/port-security-review-2026-09-28.md +++ b/.claude/playbooks/port-security-review-2026-09-28.md @@ -299,7 +299,7 @@ The service account holds `dataplatform/scopes/all`. Everything this client send 3. **Stored procedures are deny-all.** `ALLOWED_PROCEDURES = []` is built in. `new MPHelper({ allowedProcedures: ['api_MyChurch_Get_Stats'] })` enables names **per instance**, and each name is validated at construction. The service that calls a procedure builds its own helper with a fixed module-level list, so the name is reviewed next to its caller. **Never build that list from request input.** Before you merge, list every procedure this fork calls and ask the user to confirm each one. 4. **Trusted sender.** `createCommunication(content, sender: { authorUserId, fromContactId }, attachments?)` and `sendMessage(content, sender: { fromAddress }, attachments?)`. Author and From fields on the content object are ignored. The caller must be a service method that has already run `requireSecurityRole`, and it builds `sender` from the `User_ID` the gate returned plus a server-side lookup. **Never from form fields, action arguments or query strings.** The service account can send as anyone, so this is the only thing stopping a caller from impersonating the senior pastor. Also refuse CR/LF (C0 controls) in subjects and display names, which would be header injection. 5. **Remove `$ignorePermissions`** from the helper, provider and types. Requests already run as the admin-level service account, so the flag could only widen what they see. Where a caller-shaped params object is forwarded (upstream: `DomainService.getGlobalFilters`), forward an explicit allowlist (only `$userId`), so a smuggled `$ignorePermissions` is dropped even when an untyped caller sets it. -6. **Codegen escaping** (`scripts/generate-types.ts`). Emit MP metadata (table and column names, descriptions) through `JSON.stringify` so that `"`, `\` and control characters come out as a valid TS string literal. Hostile or odd metadata must not be able to inject code into generated models. +6. **Codegen escaping** (`src/lib/providers/ministry-platform/scripts/generate-types.ts`). Emit MP metadata (table and column names, descriptions) through `JSON.stringify` so that `"`, `\` and control characters come out as a valid TS string literal. Hostile or odd metadata must not be able to inject code into generated models. 7. **Log `errorName(error)` only** everywhere in these services. **Tests.** Path traversal attempts (`../procs/x`, `%2e%2e`, `a/b`, arrays, objects) throw before any fetch. A non-allowlisted procedure throws. A sender missing or on the content object is refused or ignored. Upstream's `helper.wiring.test.ts` checks that `MPHelper` actually forwards `allowedProcedures` and `sender` to the provider. diff --git a/.claude/references/auth.md b/.claude/references/auth.md index fb9287c7..01878378 100644 --- a/.claude/references/auth.md +++ b/.claude/references/auth.md @@ -94,7 +94,9 @@ clears that browser's cookies, nothing more. > `/get-session` reads, so a copied pair lived to the 12 h cap, and no logout > URL had an `id_token_hint`. `sharedInstance` now caches the instance on > `globalThis`, so every layer shares one store. The 1 h figure below assumes -> that; `src/auth.shared-instance.test.ts` pins it across two module copies. The settings in `src/lib/auth.ts` +> that; `src/auth.shared-instance.test.ts` pins it across two module copies. + +The settings in `src/lib/auth.ts` (documented on `SESSION_EXPIRES_IN_SECONDS`) instead put hard ceilings on every session, pinned by the clock-walk suite `src/auth.session-lifetime.test.ts` (better-auth 1.7.4, real `auth` instance, mock OIDC code flow, fake clock): @@ -738,7 +740,7 @@ exposes. /signed-out, which starts no OAuth) ``` -`client_id` is always sent and `id_token_hint` whenever it is available. Without either, an IdentityServer-style OP (MP) cannot tell which client's post-logout URIs to check: it shows a "log out?" prompt and does not redirect, so a user who closes the tab there leaves the MP SSO session alive on a shared PC. The `id_token` comes only from the in-memory account row of the process that handled sign-in (there is no account cookie — see Account cookie above; all bundle layers share one `auth`, see Session lifetime), so on another serverless instance only `client_id` is sent. **Tested against MP 2026-09-29 (Playwright):** with `id_token_hint`, MP logs out with no prompt and redirects back; with `client_id` alone, MP shows "Would you like to logout?" with a **Yes** button, and redirects only after Yes. `handleSignOut()` throws, after clearing the local session, if `OIDC_CLIENT_ID` is unset, or if `MINISTRY_PLATFORM_BASE_URL` or `BETTER_AUTH_URL`/`NEXTAUTH_URL` is unset or fails the `src/lib/env.ts` checks (non-https, malformed, credentials or query in the URL) — there is no localhost fallback. The `post_logout_redirect_uri` is the origin of `BETTER_AUTH_URL` (`getAuthBaseUrl()`: a path is refused, a single trailing `/` is dropped) and must be registered in the MP OAuth client configuration exactly as that origin. +`client_id` is always sent and `id_token_hint` whenever it is available. Without either, an IdentityServer-style OP (MP) cannot tell which client's post-logout URIs to check: it shows a "log out?" prompt and does not redirect, so a user who closes the tab there leaves the MP SSO session alive on a shared PC. The `id_token` comes only from the in-memory account row of the process that handled sign-in (there is no account cookie — see [Session Strategy](#session-strategy); all bundle layers share one `auth`, see Session lifetime), so on another serverless instance only `client_id` is sent. **Tested against MP 2026-09-29 (Playwright):** with `id_token_hint`, MP logs out with no prompt and redirects back; with `client_id` alone, MP shows "Would you like to logout?" with a **Yes** button, and redirects only after Yes. `handleSignOut()` throws, after clearing the local session, if `OIDC_CLIENT_ID` is unset, or if `MINISTRY_PLATFORM_BASE_URL` or `BETTER_AUTH_URL`/`NEXTAUTH_URL` is unset or fails the `src/lib/env.ts` checks (non-https, malformed, credentials or query in the URL) — there is no localhost fallback. The `post_logout_redirect_uri` is the origin of `BETTER_AUTH_URL` (`getAuthBaseUrl()`: a path is refused, a single trailing `/` is dropped) and must be registered in the MP OAuth client configuration exactly as that origin. Sign-out is entirely server-side (`auth.api.signOut()`, called in-process from the server action) — the browser never calls a `/sign-out` HTTP endpoint, which @@ -1209,7 +1211,7 @@ server-side; defence in depth). | **F10** (Low) — `ContactService.updateContact` wrote with no authorization | 2026-09-12 | Calls `requireSecurityRole({ table: "Contacts", operation: "update" })` and uses its `User_ID` for `$userId` | | **F11** (Low) — `getMpTimezone` had no check at all | 2026-09-12 | Authenticated-session check (its only consumer is the role-gated contact page) | | **F5** (Medium) — member PII and pastoral notes written to server logs at info level | 2026-09-12 | Removed all `console.log`/`.debug`/`.info` from non-script `src/`; error logs now carry identifiers/shape only (no request bodies, result sets, `Notes`, or `$filter`/full URLs); see § Logging policy above | -| **F4** (Medium) — contact-log writes accepted `Made_By`/`Contact_ID` from the caller | 2026-09-12 | `ContactLogService` stamps `Made_By` from the gate and strips both keys via the schema `.omit()`; `Contact_ID` is never sent on update; see § Attribution is server-authoritative above | +| **F4** (Medium) — contact-log writes accepted `Made_By`/`Contact_ID` from the caller | 2026-09-12 | `ContactLogService` stamps `Made_By` from the gate and strips both keys at runtime with allowlist `.pick()` schemas; `Contact_ID` is never sent on update, and since 2026-09-29 (`dc4a25b`) neither is `Made_By`, so an edit keeps the original author; see § Attribution is server-authoritative above | | **F2** (High) — a shared MP email could merge two people onto one better-auth user | 2026-09-12 | `accountLinking.enabled: false`, a synthetic `email` derived from `sub`, the real address moved to `mpEmail`, and `emailVerified` from the provider's own claim; see § Email is never a key and § Account linking | | **F7** (Low) — OAuth failures landed on better-auth's built-in error page | 2026-09-12 | `onAPIError.errorURL: "/auth-error"` plus the route allowlist, which no longer exposes `GET /error`; see § OAuth Flow | | **id_token substitution** (Low) — `POST /sign-in/social` with an attacker's id_token and a victim's access token minted the victim's session | 2026-09-28 | `refuseIdTokenSignIn` (`hooks.before`), the `getUserInfo` sub binding, and the route's `/sign-in/social` body filter; see § id_token sign-in is disabled | @@ -1289,7 +1291,7 @@ failure modes are auth's too: ## Better Auth Upgrade Checklist `npm audit fix` or a manual `npm update` can bump `better-auth` across **minor** -versions (e.g. 1.4 → 1.6). (`npm run setup` no longer can: it installs with `npm ci` +versions (e.g. 1.7 → 1.8). (`npm run setup` no longer can: it installs with `npm ci` from the lockfile and never runs `npm update`.) CI runs lint + `tsc --noEmit`, the unit tests (both Vitest projects), `next build` + the prerender check, and the lockfile check. The auth suites drive the real `auth` instance against a mock OIDC @@ -1305,8 +1307,8 @@ the `better-auth` version, do this before merging: `/api/auth/oauth2/callback/:id` → `/api/auth/callback/:id`). A moved callback needs the new redirect URI registered on the MP OAuth client in **every** environment before deploy — nothing in CI catches this. -3. **Run the auth tests**: `npm run test:run src/auth.test.ts`. Four tests are - real library guards, not simulations: +3. **Run the auth tests**: `npm run test:run -- src/auth`. These are real + library guards, not simulations: - `better-auth 1.6 guard` — `userGuid` still survives provider-profile parsing. - `better-auth 1.7 guard` (getUserInfo) — the profile still carries `sub`, which the OIDC `accountSubject` resolver reads. @@ -1371,12 +1373,10 @@ the `better-auth` version, do this before merging: ## Known Limitations 1. **No database (top refactor priority)**: With no `database` in the config, Better Auth uses an in-memory adapter. Sessions live only in the in-memory store + cookies, so a process restart ends each session once its 1-hour cookie cache expires. On serverless/Vercel this is severe: **every cold start or new function instance has an empty session store**, so once the 1-hour JWE cookie cache expires, a request that lands on a fresh instance returns `null` and the user is sent back through sign-in (usually silent while the MP session is alive). This also makes auth bugs hard to reproduce. The adapter also **grows without bound** (every sign-in adds user/account/session rows; nothing prunes them, bounded only by the sign-in rate limit) and has **no unique constraint**, so concurrent first sign-ins can create duplicate user rows (harmless today — nothing keys on them). **Recommendation:** configure a persistent database adapter or `secondaryStorage` (e.g. a Vercel Marketplace Postgres/Neon, or SQLite for local dev) before relying on this in production. -2. ~~**mapProfileToUser type narrowness**~~ *(resolved in better-auth 1.7)*: `mapProfileToUser` now returns `OAuthMappedUser`, which permits arbitrary extra keys, so the old `as Record` cast is gone. The type does forbid returning `id` — provider identity is owned by `accountSubject`. -3. **userGuid type cast**: `session.user.userGuid` requires a type cast because `customSessionClient` doesn't infer `additionalFields` from `genericOAuth`. This is a Better Auth type limitation. -4. **Token refresh**: None, by design. `offline_access` is not requested and the user's access/refresh tokens are not retained (see Account cookie above); the app never calls MP with the user's token. -5. ~~**Cookie cache staleness**~~ *(not a limitation)*: `customSession` output is never stored in the cookie — it runs on every `/get-session` — so a change to it takes effect on the next request. Only better-auth's own user/session fields (name, `userGuid`, `mpEmail`, `expiresAt`, …) are cached, for up to 1 h. -6. **No server-side revocation, no MP re-validation of a live session** *(accepted 2026-09-29)*: sign-out cannot revoke a copied cookie pair, and nothing re-checks the `dp_Users` login while a session is live (only `dp_User_Roles` is re-read per request, and `userIdCache` re-resolves `User_ID` every 15 min without ending the session). Bounded by the 12 h / 1 h ceilings in [Session lifetime and revocation](#session-lifetime-and-revocation-stateless); closed properly only by a server-side store (§ 1). -7. **Cookie hardening residue (accepted, theoretical):** +2. **userGuid type cast**: `session.user.userGuid` requires a type cast because `customSessionClient` doesn't infer `additionalFields` from `genericOAuth`. This is a Better Auth type limitation. +3. **Token refresh**: None, by design. `offline_access` is not requested and the user's access/refresh tokens are not retained (see [Session Strategy](#session-strategy)); the app never calls MP with the user's token. +4. **No server-side revocation, no MP re-validation of a live session** *(accepted 2026-09-29)*: sign-out cannot revoke a copied cookie pair, and nothing re-checks the `dp_Users` login while a session is live (only `dp_User_Roles` is re-read per request, and `userIdCache` re-resolves `User_ID` every 15 min without ending the session). Bounded by the 12 h / 1 h ceilings in [Session lifetime and revocation](#session-lifetime-and-revocation-stateless); closed properly only by a server-side store (§ 1). +5. **Cookie hardening residue (accepted, theoretical):** - **No `__Host-` prefix.** better-auth uses `__Secure-` and has no `__Host-` option. better-call keeps the first of duplicate cookies and browsers send longer-`Path` cookies first, so a *sibling subdomain* could plant state/session cookies (login CSRF). Relevant only on a custom domain with untrusted sibling subdomains (`*.vercel.app` and `*.azurewebsites.net` are on the Public Suffix List). Host the app on a dedicated subdomain. - **OAuth state is not one-time** in cookie mode (10-minute replay window) — see [`nonce` binding is off](#nonce-binding-is-off-and-must-stay-off). - **A null `/get-session` does not clear stale cookies.** customSession returns `ctx.json(null)` and drops better-auth's cookie deletions (`plugins/custom-session/index.mjs`). Not exploitable — `AuthWrapper` still redirects — and the route now adds `Cache-Control: no-store` to every `/api/auth` response, the null one included. diff --git a/.claude/references/components.md b/.claude/references/components.md index dc6017c8..1d7319a2 100644 --- a/.claude/references/components.md +++ b/.claude/references/components.md @@ -250,20 +250,20 @@ untimezoned parse left in `src/components/`. ## Compliance Summary -Verified by inspection of the tree on `docs/release-readiness-refresh`. +Verified by inspection of the tree on 2026-09-29. | Criterion | Status | Notes | |-----------|--------|-------| | File naming (kebab-case) | PASS | Every file and folder under `src/components/` and `src/app/` | | Component naming (PascalCase) | PASS | | -| Named exports only (no default) | PASS in `src/components/` | Zero `export default` under `src/components/`. The 14 in `src/app/` are App Router framework requirements (`page`/`layout`/`error`/`global-error`) | +| Named exports only (no default) | PASS in `src/components/` | Zero `export default` under `src/components/`. The 15 in `src/app/` (non-test files) are App Router framework requirements (`page`/`layout`/`error`/`global-error`) | | `@/` alias for imports | PASS with one exception | `(web)/contactlookup/page.tsx` imports `@/components/contact-lookup/contact-lookup` — the deep path, bypassing the barrel | | Barrel exports for features | PASS | All seven component folders with a component have `index.ts`; `shared-actions/` intentionally has none (it exports actions, not components) | | `"use client"` where needed | PASS | | | `"use server"` for actions | PASS | All six actions files | | Actions call services, not MPHelper | PASS | No `MPHelper` import anywhere in `src/components/` or `src/app/` | | `requireSecurityRole` on MP-touching actions | PASS with documented carve-outs | See the table in § Authorization | -| No `console.log`/`.info`/`.debug` in `src/` | PASS | Only `console.error` is used. The `console.log` hits in `src/lib/providers/ministry-platform/helper.ts` are inside `@example` JSDoc blocks, not executable code; the rest are in `scripts/`, which the rule exempts | +| No `console.log`/`.info`/`.debug` in `src/` | PASS | Only `console.error` and `console.warn` are used (the lint rule allows both). The `console.log` hits in `src/lib/providers/ministry-platform/helper.ts` are inside `@example` JSDoc blocks, not executable code; the rest are in `scripts/`, which the rule exempts | | Co-located tests | PASS except `ui/` | `sign-in/sign-in.test.tsx` unit-tests `sanitizeCallbackUrl`; the component itself is exercised through `src/app/signin/page.test.tsx`. Every `src/app/` route file has one | | TypeScript strict typing | PASS | | @@ -350,7 +350,7 @@ Components interact with these service classes: | ContactService | `@/services/contactService` | contact-lookup, contact-lookup-details | | ContactLogService | `@/services/contactLogService` | contact-logs, contact-lookup-details | | UserService | `@/services/userService` | shared-actions/user | -| AuthorizationService | `@/services/authorizationService` | contact-logs, contact-lookup, contact-lookup-details, shared-actions/user, `(web)/contactlookup/layout.tsx` | +| AuthorizationService | `@/services/authorizationService` | contact-logs, contact-lookup, contact-lookup-details, shared-actions/user, `(web)/contactlookup/layout.tsx`, `(web)/contactlookup/[guid]/page.tsx` | | DomainTimezoneService | `@/services/domainTimezoneService` | shared-actions/domain (and internally by ContactLogService) | `SessionContextService` (`@/services/sessionContextService`) is not called from diff --git a/.claude/references/deps-known-issues.md b/.claude/references/deps-known-issues.md index 7f805db3..69c2b059 100644 --- a/.claude/references/deps-known-issues.md +++ b/.claude/references/deps-known-issues.md @@ -16,7 +16,9 @@ npm 11.16.0. **2026-09-29 (auth security review, wave 3):** `next`/`eslint-config-next` `16.3.5` → `16.3.7` for GHSA-vcvr-r3jv-pc5j, and `server-only` added. Relocked with `npm run deps:relock`; `deps:verify` clean; the lockfile diff was only the -`next` family and `server-only`. +`next` family and `server-only`. Installed per `package-lock.json` after it: +`next@16.3.7`, `eslint-config-next@16.3.7`, `better-auth@1.7.4`, +`typescript@6.0.3`, `eslint@9.39.5`, `@types/node@24.13.4`, `vitest@4.1.11`. ## Accepted advisories (triaged as not exploitable) @@ -111,7 +113,7 @@ with `npm run deps:relock`; `deps:verify` clean; the lockfile diff was only the | Package | Item | Outcome | Date | |---|---|---|---| -| `next` | Pre-announced **critical** vulnerability (announced 2026-08-20, scheduled 2026-08-26) | **Shipped early, 2026-08-25** as `16.3.3` (Active LTS) and `15.5.24` (Maintenance LTS) — the `backport` dist-tag moved `15.5.23` → `15.5.24`, confirming run 2's prediction. Two critical unauthenticated RCEs, both now in the accepted table above. Repo upgraded `16.3.2` → `16.3.3`; build/lint/582 tests green. [Advisory](https://nextjs.org/blog/august-2026-security-release) | 2026-08-27 | +| `next` | Pre-announced **critical** vulnerability (announced 2026-08-20, scheduled 2026-08-26) | **Shipped early, 2026-08-25** as `16.3.3` (Active LTS) and `15.5.24` (Maintenance LTS) — the `backport` dist-tag moved `15.5.23` → `15.5.24`, confirming run 2's prediction. Two critical unauthenticated RCEs, both now in the accepted table above. Repo upgraded `16.3.2` → `16.3.3`; build/lint/582 tests (at the time) green. [Advisory](https://nextjs.org/blog/august-2026-security-release) | 2026-08-27 | > **Run 2's read was correct and worth repeating:** `16.3.2` was *not* the security release > despite landing during the announcement window. Confirm a security release by its advisory, @@ -141,15 +143,15 @@ with `npm run deps:relock`; `deps:verify` clean; the lockfile diff was only the | Package | Was | Why removed | Date | |---|---|---|---| -| `openai` | `dependencies: ^6.32.0` | Zero references repo-wide — no `import`/`require` in `src/` or `scripts/`, no `OPENAI*` env var, no config reference. Removing it dropped exactly 1 package; build, lint, and 279 tests stayed green. If AI features are added later, install fresh at `^7`. | 2026-08-21 | +| `openai` | `dependencies: ^6.32.0` | Zero references repo-wide — no `import`/`require` in `src/` or `scripts/`, no `OPENAI*` env var, no config reference. Removing it dropped exactly 1 package; build, lint, and 279 tests (at the time) stayed green. If AI features are added later, install fresh at `^7`. | 2026-08-21 | ## Applied majors (for the record) | Package | Change | Verified by | Date | |---|---|---|---| -| `jsdom` | `^29.0.0` → `^30.0.1` | 279/279 tests pass | 2026-08-21 | +| `jsdom` | `^29.0.0` → `^30.0.1` | 279/279 tests pass (at the time) | 2026-08-21 | | `chalk` | `^5.6.2` → `^6.0.0` | `npm run setup:check` renders colored output, all 8 checks run | 2026-08-21 | -| `@testing-library/jest-dom` | `^6.9.1` → `^7.0.1` | 279/279 tests pass; `@testing-library/dom@^10.4.1` promoted transitive → explicit `devDependency` as v7 requires | 2026-08-21 | +| `@testing-library/jest-dom` | `^6.9.1` → `^7.0.1` | 279/279 tests pass (at the time); `@testing-library/dom@^10.4.1` promoted transitive → explicit `devDependency` as v7 requires | 2026-08-21 | ## Lockfile platform drift (Windows -> Linux CI) @@ -250,7 +252,8 @@ reports `ajv: 6.15.0 -> 8.20.0` and a missing `fast-uri`, which is what `npm ci` - **CI** — the `lockfile` job in `.github/workflows/test.yml`, on every push to `main` and every PR targeting `main` (the workflow's only triggers — a push to a feature branch with no open PR runs nothing). This is the authoritative check; it runs on Linux, on Node 22, - ahead of the `test` job, and cannot be skipped. + in parallel with the `test`, `lint` and `build` jobs (none has a `needs:`, so a + lockfile failure does not stop the others from running, but it still fails the run). Offline behavior: the check needs the registry. Locally it warns and passes when npm is unreachable (so an offline commit is not blocked); in CI (`process.env.CI`) it fails instead. @@ -273,4 +276,4 @@ entanglement is inherent to those upstream packages, so the guard is the fix, no | Item | Outcome | Date | |---|---|---| -| `vitest.config.ts` CJS/ESM warning | **Fixed** — renamed to `vitest.config.mts`, so Vite loads it as native ESM and the `configLoader: 'native'` warning is gone. Not a pure rename: the config used `__dirname`, which does not exist in an ESM `.mts` file, so the `@` alias would have silently resolved wrong. It now uses `fileURLToPath(new URL('./src', import.meta.url))` — `fileURLToPath` specifically, because `new URL(...).pathname` yields `/S:/MP/MPNext/src` on Windows. `tsconfig.json` also needed `**/*.mts` added to `include`, since `**/*.ts` does not match `.mts` and the config would otherwise have dropped out of type checking. Verified: 582/582 tests, coverage thresholds still enforced, build and lint clean. | 2026-08-27 | +| `vitest.config.ts` CJS/ESM warning | **Fixed** — renamed to `vitest.config.mts`, so Vite loads it as native ESM and the `configLoader: 'native'` warning is gone. Not a pure rename: the config used `__dirname`, which does not exist in an ESM `.mts` file, so the `@` alias would have silently resolved wrong. It now uses `fileURLToPath(new URL('./src', import.meta.url))` — `fileURLToPath` specifically, because `new URL(...).pathname` yields `/S:/MP/MPNext/src` on Windows. `tsconfig.json` also needed `**/*.mts` added to `include`, since `**/*.ts` does not match `.mts` and the config would otherwise have dropped out of type checking. Verified: 582/582 tests (at the time), coverage thresholds still enforced, build and lint clean. | 2026-08-27 | diff --git a/.claude/references/ministryplatform.datetimehandling.md b/.claude/references/ministryplatform.datetimehandling.md index 46e6778b..a1ab07b1 100644 --- a/.claude/references/ministryplatform.datetimehandling.md +++ b/.claude/references/ministryplatform.datetimehandling.md @@ -186,7 +186,7 @@ The shared signature of these bugs: a `Date` object that crosses a zone boundary ## Windows ↔ IANA zone names -MP's `/domain` endpoint returns `TimeZoneName` as a **Windows** zone (e.g. `"Eastern Standard Time"`). `Intl.DateTimeFormat` requires **IANA** (e.g. `"America/New_York"`). `resolveIanaTimezone` maps between them via the `WINDOWS_TO_IANA` table in `domainTimezoneService.ts` — 137 entries, the standard Windows zone list. If a new MP deployment surfaces an unmapped zone it throws ``Unknown time zone "" — add it to the Windows→IANA mapping in domainTimezoneService.ts``; extend the table rather than silently falling back to the server's local zone. +MP's `/domain` endpoint returns `TimeZoneName` as a **Windows** zone (e.g. `"Eastern Standard Time"`). `Intl.DateTimeFormat` requires **IANA** (e.g. `"America/New_York"`). `resolveIanaTimezone` maps between them via the `WINDOWS_TO_IANA` table in `domainTimezoneService.ts`, which covers the standard Windows zone list. If a new MP deployment surfaces an unmapped zone it throws ``Unknown time zone "" — add it to the Windows→IANA mapping in domainTimezoneService.ts``; extend the table rather than silently falling back to the server's local zone. Resolution order, before the table is consulted: diff --git a/.claude/references/ministryplatform.query-syntax.md b/.claude/references/ministryplatform.query-syntax.md index 3fd7aff4..8e157477 100644 --- a/.claude/references/ministryplatform.query-syntax.md +++ b/.claude/references/ministryplatform.query-syntax.md @@ -157,6 +157,12 @@ the smallest case that exercises both `_TABLE` rules at once. The option keys ar Two parallel queries on `MPHelper.getTableRecords` covering (a) primary contact of the group, and (b) leader-role participation: ```typescript +import { sanitizeNumericId } from "@/lib/providers/ministry-platform/utils/filter-sanitize"; + +// Validate once; throws "Invalid Contact ID" unless it is a positive safe integer +// (a number, or a digits-only string) +const id = sanitizeNumericId(contactId, "Contact ID"); + // Query A: groups where the user is Primary_Contact { table: "Groups", @@ -173,7 +179,7 @@ Two parallel queries on `MPHelper.getTableRecords` covering (a) primary contact "Groups.End_Date", ].join(", "), filter: - `Groups.Primary_Contact = ${contactId} ` + + `Groups.Primary_Contact = ${id} ` + `AND (Groups.End_Date IS NULL OR Groups.End_Date > GETDATE())`, orderBy: "Groups.Group_Name", } @@ -194,7 +200,7 @@ Two parallel queries on `MPHelper.getTableRecords` covering (a) primary contact "Group_ID_TABLE.End_Date AS End_Date", ].join(", "), filter: - `Participant_ID_TABLE.Contact_ID = ${contactId} ` + + `Participant_ID_TABLE.Contact_ID = ${id} ` + `AND Group_Role_ID_TABLE.Group_Role_Type_ID = 1 ` + `AND (Group_Participants.End_Date IS NULL OR Group_Participants.End_Date > GETDATE()) ` + `AND (Group_ID_TABLE.End_Date IS NULL OR Group_ID_TABLE.End_Date > GETDATE())`, @@ -270,7 +276,7 @@ caller without a permitted role gets one answer ("not authorized") and learns no arguments the endpoint would have accepted. The **path** is guarded too, independently of the filter. Table and procedure names must be plain -identifiers (`sanitizeIdentifier` in `services/guards.ts`: `^[A-Za-z_][A-Za-z0-9_]*$`, ≤ 128 chars; +identifiers (`sanitizeIdentifier` in `src/lib/providers/ministry-platform/services/guards.ts`: `^[A-Za-z_][A-Za-z0-9_]*$`, ≤ 128 chars; throws `Invalid `), so `GET /tables/{table}` cannot be pointed at another endpoint. Below that, `HttpClient` refuses any endpoint that does not start with `/` or that contains `..`, `?`, `#`, `\`, `%2e`/`%2f`/`%5c` (any case) or a control character, and re-checks that the resolved URL stays under diff --git a/.claude/references/security-headers.md b/.claude/references/security-headers.md index 7e04cd16..3825a63a 100644 --- a/.claude/references/security-headers.md +++ b/.claude/references/security-headers.md @@ -132,6 +132,13 @@ header that the proxy sets on `NextResponse.next({ request: { headers } })`. A page prerendered at build time has no request, so no nonce, so under enforcement its bootstrap script is blocked and the page never hydrates. +The proxy also sets the raw nonce as an `x-nonce` request header (the +convention from Next's CSP guide). Nothing in `src/` reads it today — Next takes +the nonce from the CSP header, not from `x-nonce` — so it is only there for a +server component that needs to nonce its own `