fix(security): harden MP data access and upgrade better-auth to 1.7.4 - #22
Merged
Merged
Conversation
…r boundaries Applies the downstream hardening playbook (upstream MPNext 436466d..5bc505a) to the findings that affect this repo. Verified against a real `next start`, not only unit tests. F1/F10/F11 — authentication is not authorization MP's OIDC endpoint authenticates ANY dp_Users record, and this app reads MP with its own client-credentials service account, so MP's per-user record security never applies to what it returns. Every feature action was gated on "a session exists", which proves nothing. The sharpest surface was field management: updatePageFieldOrder rewrites dp_Page_Fields for the ENTIRE MP domain, not just the caller. Adds AuthorizationService with two entry points — requireSecurityRole() (throws, logs, returns the acting User_ID) and hasSecurityRole() (decision only, never enforcement). Gated at three independently-reachable layers: the tools layout, every feature server action, and every service method, reads included. Per-request memoization via React cache() keeps that to one MP read while ensuring a role revoked in MP takes effect on the very next request. Fails closed; infrastructure failures throw rather than reporting as a refusal, so "MP is down" can never be mistaken for "not allowed". Policy: any MP user may sign in and use the shell; the tools require a security role. Sign-in is deliberately NOT role-gated — a role-less user keeps a session, the header and a working sign-out, and is redirected to /no-access. Refusing at sign-in would strand them with no way out. Configured by MP_SECURITY_ROLES; blank means any MP role will do. Four carve-outs use a plain session check and justify it in-file. Write attribution now has exactly one source $userId comes from the gate's return value, applied in the service. No server action accepts a userId parameter. getSelectionRecordIds takes its @userid from the gate too — a selection belongs to a specific MP user, so accepting one from the payload let any caller read someone else's. F5 — PII must not reach logs Removed 21 logger.debug calls dumping $filter params, stored-procedure parameters, PUT bodies and full result sets (names, emails, phones), plus 11 console.log sites. Being gated on NODE_ENV was not enough: dev machines and non-production deployments still wrote member PII to aggregators with broader access and longer retention than MP itself. Also strips the response body from THROWN error messages, not just logs — a thrown message reaches error reporters and client-visible action results, so it leaked record content and $filter strings everywhere at once. The logger now has no debug channel at all. no-console is enforced by ESLint (warn/error only), verified to fire against a negative control. F9 — security headers and a nonce-based CSP Static headers in next.config.ts (reaching /api and the paths the proxy matcher skips); CSP in the proxy, because the nonce must be fresh per request. Anti-framing is expressed twice on purpose — two CSP headers on one response are enforced as an intersection, which is miserable to debug. style-src keeps 'unsafe-inline' with NO nonce: Radix's dialog injects a <style> ELEMENT at runtime whose content embeds the computed scrollbar width, so neither a nonce nor a stable hash can cover it, and CSP3 browsers ignore 'unsafe-inline' whenever a nonce sits beside it. form-action and img-src include the MP origin. CSP_ENFORCE enforces by default — only the exact string "false" drops to report-only, so a typo fails loud. Nonces force dynamic rendering, so /signin and /session-error are pinned force-dynamic. Route segment config is silently IGNORED in a "use client" module, so /signin's body moved to sign-in-content.tsx and its page.tsx stays a server component. Tests pin both halves. F3 — open redirect via ?callbackUrl= Sanitized at the source, not at each sink: the value feeds both a location.href assignment and signIn's callbackURL, so cleaning it once means a future third use cannot miss it. Rejects //host and /\host. Error boundaries The app had none. Three, because placement is the design: (web)/error.tsx renders inside the shell so sign-out survives, app/error.tsx covers the shell-less routes, global-error.tsx replaces a failed root layout. Next 16 renamed the prop to `retry` — a boundary wired to `reset` renders fine and its button silently does nothing, so that is pinned. Boundaries log identifiers and a digest, never the message. tool-params split src/lib/tool-params.ts is imported by client components, so importing a service from it drags next/headers into the client graph and fails the Turbopack build. Server-side parsing moved to tool-params.server.ts. A dynamic import() is NOT sufficient — it still creates a graph edge. Not applicable here: F4 (no Made_By/contact-log feature; attribution was already server-authoritative), F10/F11 service specifics (no ContactService). Tests 670 -> 783 passing. Build and lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ings
Two bodies of work that touch the same files: the remaining hardening
findings in the auth layer, and the 1.6.11 -> 1.7.4 upgrade. Every claim
below was read off the INSTALLED dist, not assumed, and verified end to end
against a real `next start` plus a live MP tenant.
F-UPDATE-USER (Critical) — session identity was reassignable
better-auth mounts /update-user unconditionally; it is NOT gated on having
email/password sign-in enabled. Its body schema is z.record(z.string(),
z.any()), it rejects only `email`, and every other key reaches
parseUserInput, which copies any additional field declared input !== false
VERBATIM AND WITH NO VALIDATOR, then re-mints the session cookie. Its only
gate is sessionMiddleware, satisfied by any valid session cookie.
Because userGuid must stay input: true for the OAuth profile path to work,
the two facts compose: any authenticated user could POST themselves another
MP User_GUID and inherit that user's roles on every authorization check and
their User_ID on every write. Stateless is not a mitigation — the handler
falls back to {...session.user, ...additionalFields}.
The in-file comment justified input: true on the grounds that there was "no
update-user endpoint". That premise was false, and a test locked the flag in
place on the strength of it. The flag is correct; the missing piece was the
endpoint-layer control. Comment rewritten to say where the control lives.
F7 — deny-by-default on the better-auth catch-all
toNextJsHandler mounts ~30 endpoints; this client calls three. The allowlist
returns a plain 404 without reaching better-auth for everything else,
including endpoints a FUTURE version adds. disabledPaths is defence in
depth. onAPIError.errorURL points at an owned /auth-error page, since
better-auth's default /api/auth/error is now itself 404'd; /auth-error is
allowlisted public in the proxy, or an unauthenticated visitor bounces to
/signin, which auto-starts OAuth again and loops forever.
F2 — a shared email merged two people onto one identity
MP enforces no uniqueness on email addresses; households routinely share
one. better-auth keys identity on email. Users are now keyed on the OIDC
sub via a synthetic <sub>@mp.invalid address (RFC 2606 reserved TLD), with
the real address kept as mpEmail for display; accountLinking disabled;
emailVerified reports the actual claim instead of asserting true.
1.7 narrowed this but did NOT close it: handleOAuthUserInfo now resolves the
account key first, then still falls back to findUserByEmail when no account
matches — which is every FIRST sign-in for a sub. The fix is load-bearing.
Side benefit: MP does not require a user to have an email, and better-auth
hard-fails the callback with email_is_missing when none is present. Those
users previously could not sign in at all.
better-auth 1.7 breaking changes
* genericOAuth no longer mounts its own endpoints. It registers providers as
first-class SOCIAL providers, so sign-in moved from POST /sign-in/oauth2
to POST /sign-in/social, and the callback from
GET /oauth2/callback/:providerId to GET /callback/:id. The allowlist
pinned the old paths, so this failed closed and loudly — which is the
point of an allowlist, but it is still an outage if missed.
* genericOAuthClient was removed along with signIn.oauth2. The client now
calls signIn.social({ provider }) — note `provider`, not `providerId`.
* id_token nonce binding is ON by default for any provider whose discovery
yields an id_token config. MP does not echo the nonce claim, so every
sign-in fails with unable_to_get_user_info. disableIdTokenNonceBinding is
now set. The failure is inverted from the obvious reading: sign-in works
only when the boot-time discovery fetch FAILED, because that skips
verification entirely. A working discovery means a broken sign-in.
* The provider account key moved. 1.7 derives it from accountSubject(...)
rather than profile.id — the user-info type now declares `id?: never` —
and genericOAuth's default reads profile.sub for an OIDC provider.
getUserInfo returned the 1.6 `id` shape, so sub was undefined and
resolveOAuthAccountKey threw OAUTH_ACCOUNT_SUBJECT_INVALID AFTER a
successful token exchange, surfacing as unable_to_get_user_info — which
reads like a userinfo fetch failure rather than an identity-mapping one.
getUserInfo now returns `sub`, mapProfileToUser reads it from the raw
profile, and accountSubject is declared EXPLICITLY so the account key
never depends on a boot-time discovery fetch inferring isOidc.
* OAuth error codes changed substantially (oAuth_code_missing -> no_code,
email_doesn't_match -> email_does_not_match, plus new
nonce_binding_missing and oauth_provider_not_found). /auth-error's map is
rebuilt from OAUTH_CALLBACK_ERROR_CODES in the installed dist. It is a Map,
not an object literal — the key is an arbitrary query value, and
?error=constructor against a plain object returns an inherited
Object.prototype member. error_description is never rendered.
F8 — PKCE: closed as WONTFIX, with evidence
MP's discovery document advertises code_challenge_methods_supported
["plain","S256"], but MP does not honour the verifier at the token endpoint.
With pkce: true the authorize leg succeeds and returns a code, then the
exchange fails with invalid_grant (400). Both signals you would naturally
check — the advertised support, and MP accepting the code_challenge on the
authorize URL — look like confirmation; the flow only breaks on the last
hop. pkce stays false and src/auth.test.ts pins it with this reasoning so it
is not re-opened from the discovery document alone.
Note: an earlier revision of the reference docs claimed PKCE was impossible
because "MP does not support PKCE". The conclusion was right and the stated
reason was wrong — MP advertises it. The docs now carry the actual trace.
Also: the provider config is lifted to an exported
ministryPlatformProviderConfig so pkce, disableIdTokenNonceBinding and
accountSubject are pinnable; getUserInfo's return is now `satisfies
GenericOAuthUserInfo` instead of an `as` cast, which is precisely why the
id/sub mismatch was not caught at compile time.
npm audit: 14 findings, none from better-auth — all pre-existing transitive
deps (next, postcss, sharp, vitest, cheerio, svgo, docx). Next was not
changed by this install.
Tests 783 -> 810 passing. The account-key guard is verified to FAIL against
the 1.6 shape, not merely to pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BREAKING CHANGE: the OAuth redirect URI registered on the Ministry Platform
client must be updated before this is deployed, or MP rejects the
authorization request outright.
old: <BETTER_AUTH_URL>/api/auth/oauth2/callback/ministry-platform
new: <BETTER_AUTH_URL>/api/auth/callback/ministryplatform
Note BOTH halves of that path changed. The preceding commit moved
/oauth2/callback/ to /callback/ (better-auth 1.7 routes genericOAuth through
the core social endpoints); this commit drops the hyphen from the provider id.
Because the URI already had to be re-registered for the upgrade, the rename
costs nothing extra operationally.
The provider id is load-bearing in three places that must agree or sign-in
breaks, and src/auth.test.ts pins that they do:
* ministryPlatformProviderConfig.providerId
* the deny-by-default allowlist entry GET /callback/ministryplatform
* signIn.social({ provider }) on the sign-in page
Care taken: "ministry-platform" is ALSO the name of the MP provider directory
(src/lib/providers/ministry-platform/), which appears in hundreds of import
paths. A blanket find-and-replace would have broken every one. The rewrite
protected that path and was verified afterwards — no import was touched, and
`grep "providers/ministryplatform" src/` returns nothing. The two concepts
merely shared a spelling.
No session or data migration is needed: the app is stateless (in-memory
adapter), so no persisted account.providerId rows carry the old value. That
would change if a database adapter is ever added.
This commit is a pure rename — 34 insertions, 34 deletions, and every changed
line mentions the provider id.
Verified against a live MP tenant:
GET /api/auth/callback/ministryplatform -> 302 (routes)
GET /api/auth/callback/ministry-platform -> 404 (old id closed)
authorize redirect_uri = .../api/auth/callback/ministryplatform
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The OAuth redirect URI registered on the Ministry Platform client must be updated before this is deployed, or MP rejects the authorization request outright and nobody can sign in.
Both halves of that path changed: better-auth 1.7 routes genericOAuth through the core social endpoints (
/oauth2/callback/→/callback/), and the provider id dropped its hyphen.Also consider rotating
BETTER_AUTH_SECRET— see Incident response below.What this is
Applies the downstream hardening playbook (upstream MPNext
436466d..5bc505a) to the findings that affect this repo, and upgrades better-auth1.6.11 → 1.7.4.getSession()in feature actionsconsole.log/debug/infoinsrc/Build, lint and typecheck clean at the tip.
Review order
Three commits, deliberately split by concern. The repo merges rather than squashes, so they survive.
6f23636fix(security):— authorization gate, log scrubbing, CSP + headers, error boundaries (57 files)e94e549fix(auth):— better-auth 1.7.4 upgrade + auth-layer findings (17 files)5a43d8dfix(auth)!:— the provider rename, isolated (13 files, 34 insertions / 34 deletions, every changed line mentions the provider id)Start with 3 — it's two minutes and it's the part with an operational consequence. Then 1. Commit 2 is the longest but its message carries the full reasoning per finding.
The findings
User_GUID$filterstrings and response bodies in logs and thrown messages?callbackUrl=on/signinNot applicable here: F4 (no
Made_By/contact-log feature — attribution was already server-authoritative, no server action ever accepted auserId), F10/F11 (noContactService).Why authentication wasn't enough
MP's OIDC endpoint authenticates any
dp_Usersrecord, and this app reads MP with its own client-credentials service account — so MP's per-user record security never applies to what it returns. A session proved only that some MP user signed in.The sharpest surface wasn't contacts:
updatePageFieldOrderrewritesdp_Page_Fieldsfor the entire MP domain, and was reachable by any MP login.Policy this establishes
Sign-in is deliberately not role-gated — a role-less user keeps a session, the header and a working sign-out, and is redirected to
/no-access. Refusing at sign-in would strand them with no way out. Configured viaMP_SECURITY_ROLES(blank = any MP role).Gated at three independently-reachable layers, because a server action is a callable POST endpoint whether or not its page ever rendered.
better-auth 1.7 breaking changes handled
Each was read off the installed
dist, not assumed:/sign-in/social+/callback/:id. The F7 allowlist pinned the old paths, so this failed closed and loudly.genericOAuthClientremoved along withsignIn.oauth2→signIn.social({ provider }).profile.idtoaccountSubject(the type now saysid?: never). OurgetUserInforeturned the 1.6 shape →OAUTH_ACCOUNT_SUBJECT_INVALIDafter a successful token exchange, surfacing asunable_to_get_user_info./auth-error's map rebuilt fromOAUTH_CALLBACK_ERROR_CODES.F8 / PKCE — closed as WONTFIX
MP's discovery document advertises
code_challenge_methods_supported: ["plain","S256"], but MP does not honour the verifier at the token endpoint. With PKCE on, the authorize leg succeeds and returns a code, then the exchange fails withinvalid_grant(400).Both signals you'd naturally check — the advertised support, and MP accepting the
code_challengeon the authorize URL — look like confirmation. The flow only breaks on the last hop.src/auth.test.tspinspkce: falsewith this reasoning so it isn't re-opened from the discovery document alone.Verified against a real server
Not only unit tests — against
next startand a live MP tenant:Negative controls were run too — the account-key guard is verified to fail against the 1.6 shape, and the
no-consolerule to fire on a probe file. Tests that only pass don't protect.Incident response
Patching does not revoke sessions already forged via F-UPDATE-USER. They survive in the JWT cookie cache for up to an hour, and with no database there's no session table to clear. Rotating
BETTER_AUTH_SECRETis the only immediate revocation — it signs everyone out. Checkdp_Audit_Logfor the window from whenever this repo picked upuserGuid: input: trueto deploy.New config
Also included
c8980b1 "Removed old Tools"(authored separately) deletes three unused slash-command definitions —.claude/commands/{audit-deps,branch-commit,pr}.md. Unrelated to the security work; noted here so the diff stat isn't surprising.Follow-ups (not in this PR)
16.2.6 → 16.3.3. Dependabot flags a middleware/proxy bypass (>=16.0.0 <16.2.11) that directly undermines the CSP and session redirect this PR adds, plus two criticals fixed in 16.3.3. Separate PR, separate risk surface.Full reasoning lives in
.claude/references/security/README.md.🤖 Generated with Claude Code