Skip to content

fix(auth): require an MP security role for contact reads and writes (F1, F3, F10) - #83

Merged
chriskehayias merged 4 commits into
mainfrom
fix/auth-role-gate-contact-features
Sep 12, 2026
Merged

fix(auth): require an MP security role for contact reads and writes (F1, F3, F10)#83
chriskehayias merged 4 commits into
mainfrom
fix/auth-role-gate-contact-features

Conversation

@chriskehayias

@chriskehayias chriskehayias commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Closes the three open findings from the 2026-09-12 auth security review: F1 (High), F3 (Medium) and F10 (Low). Also closes F11 (Low) in passing, since it was one line in a file already being touched.

The findings

F1 (High) — reads were gated on nothing but a session. Contact search, contact details and contact logs — plus the page guard — checked only that auth.api.getSession() returned something. That proves nothing here: MP's OIDC endpoint authenticates any dp_Users record in the domain, and this app fetches all MP data with its own client-credentials service account (dataplatform/scopes/all), so MP's per-user record security never filters what the app returns. Any MP user could read every contact's email and phone number, and every pastoral contact log, through this app. Writes were already role-gated by requireSecurityRoleForWrite; reads were not.

F3 (Medium) — open redirect on /signin. callbackUrl came off the query string and was assigned straight to window.location.href for a visitor who already had a session, so /signin?callbackUrl=https://evil.example bounced the user off-site from a URL that looks like this app's own login page.

F10 (Low) — ContactService.updateContact wrote with no authorization. It took its acting user from SessionContextService.getActingUserIdForWrite, which logs and proceeds when none resolves, and never consulted AuthorizationService. Unreferenced today, but a loaded footgun.

F11 (Low) — getMpTimezone had no check at all. The one server action in the app with no gate of any kind.

Policy

Any Ministry Platform user may sign in. A user with no security role gets a session, the app shell (header, avatar, user menu, sign-out) and the home page.

The contact-lookup and contact-log features require an MP security role — for reads as well as writes. Any user who holds one may read, create, edit and delete any contact log, including one another user created.

Sign-in is deliberately not role-gated: no check in getUserInfo / mapProfileToUser, in customSession / enrichSessionUser, or in AuthWrapper. A role-less user must be able to reach a page that explains the problem and offers a sign-out, not be bounced off the login screen. AuthWrapper is unchanged and remains the authentication gate for the (web) group.

Ownership (Made_By) remains deliberately irrelevant — contact logs are shared pastoral records and staff need to correct each other's entries.

Enforcement layers

Three enforcement layers plus one presentation layer. Each enforcement layer re-checks, because each is independently reachable — a server action is a callable POST endpoint whether or not the page that calls it ever rendered.

Layer File(s) What it does
Page (server) src/app/(web)/contactlookup/layout.tsx hasSecurityRole → redirect("/no-access"). Covers [guid]/page.tsx too: React renders the layout first and only renders children once it returns, so the detail page and its server-action calls never run
src/app/(web)/no-access/page.tsx New. Inside the (web) group so the header and sign-out survive; static, no auto-redirect (the fix is an administrator granting a role in MP)
Server action src/components/contact-lookup/actions.ts searchContacts — gate replaces the bare session check
src/components/contact-lookup-details/actions.ts getContactDetails, getContactLogsByContactId
src/components/contact-logs/actions.ts All six actions; requireSession() is gone, reads gate like writes
src/components/shared-actions/domain.ts getMpTimezone — authenticated-session check (F11). A session check is sufficient: one domain-wide config string, and its only consumer is the now-gated contact page
Service src/services/contactService.ts contactSearch, getContactByGuid, and updateContact (F10 — $userId now comes from the gate)
src/services/contactLogService.ts getContactLogTypes, searchContactLogs, getContactLogById, getContactLogsByContactId, createContactLog, updateContactLog, deleteContactLog
UX only — not a security control src/components/layout/sidebar.tsx Hides the Contact Lookup nav entry
src/components/home-demos/contact-lookup-demo-card.tsx New. The dashboard tile renders null without access; mounted in a <Suspense> boundary by src/app/(web)/page.tsx

The UX layer reads canAccessContactFeatures, a boolean computed server-side by getCurrentUserProfile from the same gate (hasSecurityRole). The client never derives policy from role names, and both consumers test === true so a null or flagless profile fails closed.

Gate API

src/services/authorizationService.ts is still the single source of truth.

Member Signature Use
requireSecurityRole (ctx: { table: string; operation: "read" | "create" | "update" | "delete" }) => Promise<number> The gate. Throws UnauthorizedError; returns the acting MP User_ID
requireSecurityRoleForWrite same, operation narrowed to the write verbs Thin alias, kept so existing call sites and tests work unchanged and so a read at a write boundary is a type error
hasSecurityRole (ctx) => Promise<{ permitted, userId, reason }> Non-throwing form the throwing gate is built on — for redirects and UI affordances, never as the enforcement point

hasSecurityRole reports a denial as permitted: false but still throws on infrastructure failure (MP unreachable, an unusable acting User_ID), so an outage can never read as a company-wide permissions change. Denials log mp.read.unauthorized (reads) or mp.write.unauthorized (writes) — same shape, same no_mp_user / no_security_role / role_not_permitted reasons. Writes still resolve their acting user through getActingUserIdForWrite, so mp.write.non_user fires before a denial.

Caching. The dp_User_Roles read is memoized per request with React's cache(), keyed by User_ID — one MP role read per request no matter how many of the three layers ask. There is still no cross-request cache: no module-level map, no TTL, so a revoked role stops working on the user's very next request.

Environment variable

MP_SECURITY_ROLES is the new general variable — a comma-separated allow-list applying to reads and writes, case- and whitespace-insensitive, unset or blank meaning "any security role".

MP_WRITE_SECURITY_ROLES is deprecated but still read as a fallback when the new variable is unset or blank, so an existing deployment is not silently widened to "any role". It now governs reads too, and MP_SECURITY_ROLES wins where both are set. Documented in .env.example and auth.md.

Behaviour change for a role-less user

Before After
Signs in, reads every contact and contact log Signs in, sees the header, avatar, user menu and sign-out
— Home page loads; the Contact Lookup tile and nav entry are not shown
— Typing /contactlookup or a /contactlookup/<guid> deep link → redirected to /no-access
— Calling any contact server action directly → UnauthorizedError, logged as mp.read.unauthorized / mp.write.unauthorized

A user who already holds any MP security role sees no change at all, unless MP_SECURITY_ROLES / MP_WRITE_SECURITY_ROLES is set to a narrower list.

Tests

897 tests across 54 files, all passing. Coverage thresholds all met, including the src/app/** gate PR #81 added.

New files

File Tests
src/app/(web)/contactlookup/layout.test.tsx 8
src/components/home-demos/contact-lookup-demo-card.test.tsx 6
src/app/(web)/no-access/page.test.tsx 5

Updated files

File Tests (was)
src/services/authorizationService.test.ts 40 (21)
src/components/contact-logs/actions.test.ts 69 (67)
src/services/contactLogService.test.ts 64 (54)
src/components/contact-lookup-details/actions.test.ts 26 (22)
src/app/signin/page.test.tsx 22 (7)
src/services/contactService.test.ts 17 (12)
src/components/shared-actions/user.test.ts 12 (7)
src/components/layout/sidebar.test.tsx 11 (5)
src/components/contact-lookup/actions.test.ts 10 (8)
src/app/(web)/page.test.tsx 7 (6)
src/components/shared-actions/domain.test.ts 5 (3)

Specifically asserted: a session with no MP user is refused by every read action; an MP user with zero roles is refused; a role-holder is allowed; MP_SECURITY_ROLES restricts reads and writes and MP_WRITE_SECURITY_ROLES still works as a fallback (and loses when both are set); service methods deny without a role even when called directly; updateContact passes the gate's User_ID as $userId and throws without a role; callbackUrl=https://evil.example, //evil.example and /\evil.example all resolve to / while /contactlookup?x=1 survives; and the sidebar and dashboard tile hide/show by canAccessContactFeatures, failing closed without it.

A note on cache() under Vitest: outside a React request scope it is a passthrough (React calls straight through with no cache dispatcher installed), so the tests assert only what holds in both environments — "no decision is carried across calls" — and never a hit count that depends on a live memo. This is documented in testing.md and in the service's own comments.

Docs

  • .claude/references/auth.md — rewritten Authorization section, a new "authentication is not authorization" table under Route Protection, updated File Map and Server Actions snippet, and a closed-findings table.
  • .env.example — MP_SECURITY_ROLES with the legacy-fallback note.
  • CLAUDE.md — new key practice: feature server actions and service methods that touch MP data call AuthorizationService, not a bare session check.
  • .claude/references/components.md — home-demos/, the new route files, authorization note on the actions inventory.
  • .claude/references/testing.md — the gate mock pattern, the cache() caveat, refreshed inventory and coverage figures.

Verification

  • npx vitest run --coverage — 897 passed, 54 files, exit 0; 99.47% stmts / 96.65% branch / 98.92% funcs / 99.73% lines, every threshold met.
  • npx tsc --noEmit -p tsconfig.json — clean.
  • npm run lint — clean.
  • No Ministry Platform API call of any kind was made; every MP boundary in the tests is mocked.
  • The working tree's unrelated package-lock.json / deps-known-issues.md / deps-audit changes were left untouched — every commit staged by explicit path.

🤖 Generated with Claude Code

chriskehayias and others added 4 commits September 12, 2026 18:04
@chriskehayias @claude
feat(auth): extend the MP security-role gate to reads, memoized per r…
afef3a9 
…equest

`AuthorizationService` only knew how to gate writes. Generalize it so the same
single source of truth can decide reads too, ahead of wiring it into the read
paths (F1).

What changed:

- `requireSecurityRole({ table, operation })` accepts `read` alongside the three
  write verbs and returns the acting MP `User_ID`.
  `requireSecurityRoleForWrite` stays as a thin alias, with `operation` narrowed
  to the write verbs so a `read` passed at a write boundary is a type error;
  every existing call site and test keeps working.
- `hasSecurityRole()` is the non-throwing form the throwing gate is now built
  on, returning `{ permitted, userId, reason }`. It is what a redirect or a UI
  affordance should ask. It reports a denial but still THROWS on infrastructure
  failure (MP unreachable, an unusable acting `User_ID`), so "MP is down" can
  never be mistaken for "this user is not allowed" — and it logs nothing, since
  it runs on every profile load and the UI asking "may they?" is not an incident.
- Reads resolve the acting user with `SessionContextService.getCurrentUserId`;
  writes keep `getActingUserIdForWrite`, so an unattributed write still emits
  `mp.write.non_user` before the gate refuses it.
- Denials log `mp.read.unauthorized` for reads, parallel to the existing
  `mp.write.unauthorized`. Same shape, same `no_mp_user` / `no_security_role` /
  `role_not_permitted` reasons.
- Policy env var is now `MP_SECURITY_ROLES` (reads and writes).
  `MP_WRITE_SECURITY_ROLES` is read as a deprecated fallback when the new one is
  unset or blank, so an existing deployment is not silently widened to "any
  role" — but it now governs reads as well, and the new var wins where both are
  set.

Caching: the gate will run at up to three layers per request once it is wired
in, so the `dp_User_Roles` read is memoized PER REQUEST with React's `cache()`,
keyed by `User_ID` — one role read per request regardless of how many layers
ask. There is still no cross-request cache of any kind: roles are re-read on the
next request, so a revoked role stops working immediately, and a per-request
memo cannot outlive the request that created it. Outside a React request scope
(Vitest, plain Node) `cache()` is a passthrough, so the tests observe the
uncached behaviour — which is exactly the behaviour that must hold in both
environments.

Verified: `npx vitest run --coverage` (all thresholds met),
`npx tsc --noEmit`, `npm run lint`.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@chriskehayias @claude
fix(auth): require an MP security role for contact reads and writes (…
16c3415 
…F1, F10)

Every read path in the contact features — search, details, logs — and the page
guard checked only that a session existed. That proved nothing: MP's OIDC
endpoint authenticates ANY `dp_Users` record, and this app fetches all MP data
with its own client-credentials service account (`dataplatform/scopes/all`), so
MP's per-user record security never applies to what the app returns. Any MP user
in the domain could read every contact's email and phone, and every pastoral
contact log, through this app. Writes were already role-gated; reads were not.

Policy (from the product owner): any MP user may sign in and use the app shell;
the contact-lookup and contact-log features require an MP security role.
Sign-in is deliberately NOT role-gated — no check in `getUserInfo`,
`mapProfileToUser`, `customSession` or `AuthWrapper` — so a role-less user gets
a session, the header, the user menu and a working sign-out.

Enforced at three independent layers, because each is independently reachable
(a server action is a callable POST endpoint whether or not its page rendered):

- Page: `src/app/(web)/contactlookup/layout.tsx` redirects a role-less user to
  the new `/no-access`. It covers `[guid]/page.tsx` too — React renders the
  layout first and only renders `children` once it returns, so the detail page's
  own action calls never run. `/no-access` lives INSIDE the (web) group on
  purpose: the session is valid, so the user keeps the shell and, crucially,
  sign-out. No auto-redirect — the fix is an administrator granting a role.
- Server action: every exported action in `contact-lookup/`,
  `contact-lookup-details/` and `contact-logs/` now calls the gate instead of
  `requireSession()`. The gate implies an authenticated session, so it replaces
  the session check rather than following it. `shared-actions/domain.ts`
  (`getMpTimezone`) gains a session check — it had none at all (F11); a session
  check is sufficient there, since it returns one domain-wide config string and
  its only consumer is the now-gated contact page.
- Service: the read methods on `ContactService` and `ContactLogService` gate
  too, so a future caller that bypasses the actions still comes up empty. Writes
  take `$userId` from the gate's return value rather than resolving the acting
  user separately; the gate routes through `SessionContextService`, so the
  structured `mp.write.non_user` log still fires before a denial.

F10: `ContactService.updateContact` wrote to `Contacts` using only
`getActingUserIdForWrite`, which logs and proceeds when no user resolves. It now
calls the gate and uses its `User_ID` for `$userId`. Unreferenced today, but it
was a loaded footgun.

UX layer — explicitly not a security control: the sidebar entry and the new
dashboard tile (`components/home-demos/`) are hidden for users without access,
so nobody is handed a link that only redirects them. Both read
`canAccessContactFeatures`, computed SERVER-SIDE by `getCurrentUserProfile` from
the same gate (`hasSecurityRole`), never derived on the client from role names,
and tested to fail closed when the profile or the flag is absent.

Verified: `npx vitest run --coverage` — 897 tests, 54 files, all thresholds met
including the `src/app/**` gate; `npx tsc --noEmit`; `npm run lint`.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@chriskehayias @claude
fix(signin): only honor a same-origin relative callbackUrl (F3)
ee46343 
`callbackUrl` comes straight off the query string and was assigned to
`window.location.href` whenever the visitor already had a session, so
`/signin?callbackUrl=https://evil.example` bounced the user off-site from a URL
that looks like this app's own login page — a credible phishing hop.

`sanitizeCallbackUrl` reduces the value to a same-origin relative path: it must
start with `/` and must not start with `//` (protocol-relative, another origin)
or `/\` (which browsers normalize to `//`). Anything else falls back to `/`.

The sanitized value feeds BOTH sinks — the `location.href` assignment, where no
server is involved at all, and the `callbackURL` handed to `signIn.social`,
which better-auth also validates server-side. Sanitizing at the source rather
than at each sink means a future third use cannot miss it.

Tests cover absolute http/https, protocol-relative, backslash-escaped,
`javascript:` and bare-relative inputs against both sinks, and assert that
legitimate deep links (`/contactlookup?x=1`, `/contactlookup/abc?tab=logs`)
still survive the round trip.

Verified: `npx vitest run --coverage`, `npx tsc --noEmit`, `npm run lint`.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@chriskehayias @claude
docs(auth): document the read gate, MP_SECURITY_ROLES and /no-access
36227eb 
Records the policy and the mechanism behind F1/F3/F10 so the next reader does
not have to reconstruct it from the diff.

- `.claude/references/auth.md`: rewritten "Authorization" section — the decided
  policy (any MP user may sign in; the contact features need a role), why reads
  needed a gate at all, the three enforcement layers plus the UX layer in a
  table with file paths, the gate API, `MP_SECURITY_ROLES` with the deprecated
  `MP_WRITE_SECURITY_ROLES` fallback, per-request memoization via React
  `cache()` and why there is still no cross-request cache, `/no-access`, the F3
  sanitizer, and a closed-findings table dated 2026-09-12. Also updated the File
  Map, a new "authentication is not authorization" table under Route Protection,
  and the "Server Actions" snippet, which now shows the read gate and says which
  two actions may still use a bare session check.
- `.env.example`: `MP_SECURITY_ROLES` with the legacy-fallback note.
- `CLAUDE.md`: new key practice — feature server actions AND service methods
  that touch MP data call `AuthorizationService`, not a bare session check.
- `.claude/references/components.md`: `home-demos/`, the new route files, and an
  authorization note on the actions inventory.
- `.claude/references/testing.md`: the gate mock pattern, the `cache()`
  passthrough caveat for Vitest, and the refreshed inventory and coverage
  figures (897 tests / 54 files; 99.47% stmts, 96.65% branch).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@chriskehayias
chriskehayias merged commit 2713fdd into main Sep 12, 2026
2 checks passed
@chriskehayias
chriskehayias deleted the fix/auth-role-gate-contact-features branch September 12, 2026 22:07
@codecov

codecov Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@chriskehayias