fix(auth): allowlist the better-auth route (deny-by-default) and own the OAuth error page - #82
Merged
Merged
Conversation
…the OAuth error page
Security review finding F7: better-auth 1.7.4 mounts ~30 HTTP endpoints under
/api/auth/* via the catch-all route (`export const { GET, POST } =
toNextJsHandler(auth)`), but this app's browser client uses exactly three:
| Method | Path | Caller |
|--------|-----------------------------|---------------------------------------------------------------------|
| GET | /get-session | authClient.useSession() (src/contexts/*), authClient.getSession() (src/app/signin/page.tsx) |
| POST | /sign-in/social | src/app/signin/page.tsx |
| GET | /callback/ministry-platform | Ministry Platform's redirect after login |
Everything else (`/get-access-token`, `/refresh-token`, `/list-accounts`,
`/link-social`, `/unlink-account`, `/account-info`, `/list-sessions`,
`/revoke-*`, `/sign-up/email`, `/sign-in/email`, `/update-session`, `/ok`,
`/sign-out`, `/error`, etc.) was previously reachable and is now closed.
`src/app/api/auth/[...all]/route.ts` now exports `allowedAuthRoutes` (a
deny-by-default allowlist) and wraps `toNextJsHandler(auth)`: `GET`/`POST`
compute the request path relative to `/api/auth` (prefix stripped, trailing
slashes stripped, exact string match — no regex/prefix matching) and return a
plain 404 without ever touching better-auth for anything not on the list.
`/sign-out` is deliberately excluded: sign-out runs server-side via
`auth.api.signOut` in `src/components/user-menu/actions.ts`, so no HTTP
sign-out route is needed today; adding `authClient.signOut()` client-side
would require adding it here first, and the 404 makes that omission loud.
`/error` is deliberately excluded: OAuth callback failures now redirect to our
own `/auth-error` page instead (`onAPIError.errorURL` in `src/lib/auth.ts`),
verified against `node_modules/better-auth/dist/api/routes/callback.mjs` and
`@better-auth/core`'s `appendQueryParams` — a root-relative errorURL like
`/auth-error` is left untouched (no baseURL prefixing needed), and the
redirect always carries `?error=<code>` plus, when available,
`&error_description=<text>`.
`src/app/auth-error/page.tsx` (mirrors `src/app/session-error/page.tsx`,
outside the `(web)` route group) maps known failure codes
(`unable_to_get_user_info`, `account_not_linked`, `email_not_found`,
`invalid_code`/`state_not_found`/`nonce_binding_missing`, ...) to plain-English
messages, never renders `error_description`, and always offers a "Try signing
in again" link to `/signin` with no auto-redirect (so a failing OAuth loop
lands somewhere stable). `src/proxy.ts` allowlists `/auth-error` as a public
path — without it, an unauthenticated visit here would bounce straight back
to `/signin`, which auto-starts OAuth again, looping forever.
`disabledAuthPaths` in `src/lib/auth.ts` is unchanged in behavior; its doc
comment now explains that the route allowlist is the primary, deny-by-default
control and this list is defense in depth, still verified by
`src/auth.test.ts`, which drives `auth.handler` directly and intentionally
bypasses the route.
Tests added/updated:
- `src/app/api/auth/[...all]/route.test.ts` — rewritten to drive the real
exported GET/POST with real NextRequest objects (previously it mocked both
`@/lib/auth` and `better-auth/next-js` and only asserted `toNextJsHandler`
wiring). This file already existed on `main` prior to this branch (not from
the parallel dependency-audit work stream, despite the task brief's
assumption) — its tests are replaced to match the new allowlist behavior,
keeping their original intent (GET/POST both exported, built from the
shared `auth` instance, no extra exports) as new assertions. Covers: the
allowlist's exact value; GET /get-session and POST /sign-in/social are not
404; GET /list-accounts, POST /get-access-token, POST /sign-out, GET
/error, GET /ok, POST /update-user, and POST /callback/ministry-platform
(wrong method) all 404; trailing-slash/`..`/doubled-slash tricks do not
bypass exact matching. Only `@/lib/providers/ministry-platform` is mocked
(MPHelper class mock), plus a hoisted stub of MP's OIDC discovery fetch so
the real `@/lib/auth` module constructs its provider without any network
call.
- `src/app/auth-error/page.test.tsx` — known code → mapped message, unknown
code → generic message, sign-in link always present, `error_description`
never rendered.
- `src/proxy.test.ts` — `/auth-error` passes through without a session
cookie.
- `src/auth.test.ts` — `auth.options.onAPIError?.errorURL === '/auth-error'`.
Verification: `npx vitest run` (807 tests, 51 files, all passing) and
`npx vitest run --coverage` (all thresholds pass, including `src/proxy.ts`
100/100/100/100 and the `src/app/**` glob), `npx tsc --noEmit` (clean),
`npm run lint` (clean). No changes to `package-lock.json` or any file from the
parallel dependency-audit work stream.
Co-Authored-By: Claude Fable 5.1 <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.
Summary
Security review finding F7: better-auth 1.7.4 mounts ~30 HTTP endpoints under
/api/auth/*via the catch-all route, but this app's browser client only calls three of them. This PR closes everything else with a deny-by-default allowlist, and replaces better-auth's own OAuth-error page with one this app controls.Allowlist
/get-sessionauthClient.useSession()(src/contexts/*),authClient.getSession()(src/app/signin/page.tsx)/sign-in/socialsrc/app/signin/page.tsx/callback/ministry-platformEverything else (
/get-access-token,/refresh-token,/list-accounts,/link-social,/unlink-account,/account-info,/list-sessions,/revoke-*,/sign-up/email,/sign-in/email,/update-session,/ok,/sign-out,/error, ...) now 404s at the route, before ever reaching better-auth./sign-outexcluded on purpose: sign-out runs server-side viaauth.api.signOutinsrc/components/user-menu/actions.ts. No HTTP sign-out route is needed today./errorexcluded on purpose: OAuth callback failures now redirect to this app's own/auth-errorpage (onAPIError.errorURLinsrc/lib/auth.ts) instead of better-auth's built-in error page.disabledAuthPathsinsrc/lib/auth.tsstays as defense in depth (still verified directly againstauth.handlerbysrc/auth.test.ts); the route allowlist is now the primary control.Behavior change
A failed Ministry Platform OAuth callback now lands on
/auth-error?error=<code>(a new page,src/app/auth-error/page.tsx) instead of better-auth's built-in/api/auth/errorpage. Known codes (unable_to_get_user_info,account_not_linked,email_not_found,invalid_code/state_not_found/nonce_binding_missing) map to plain-English messages; anything else falls back to a generic message.error_descriptionis never rendered. The page always offers a "Try signing in again" link to/signin, with no auto-redirect.src/proxy.tsallowlists/auth-erroras a public path so an unauthenticated visit doesn't bounce back to/signinand restart the loop.Verified directly against library source:
onAPIError.errorURL(@better-auth/core'sinit-options.d.mts) is used as-is by the callback'sredirectOnError/appendQueryParams, which leaves a root-relative URL like/auth-erroruntouched (no baseURL prefixing) and always appendserror(pluserror_descriptionwhen available).Tests
src/app/api/auth/[...all]/route.test.ts— rewritten to drive the real exportedGET/POSTwith realNextRequestobjects (previously it mocked@/lib/authandbetter-auth/next-jsand only asserted wiring). Note: this file already existed onmainbefore this branch — it did not originate in the parallel dependency-audit work stream the task brief assumed it might. Covers the exact allowlist value, both allowed endpoints reaching better-auth, seven disallowed endpoints 404ing, and trailing-slash/../doubled-slash tricks not bypassing exact matching.src/app/auth-error/page.test.tsx— known/unknown code mapping, sign-in link always present,error_descriptionnever rendered.src/proxy.test.ts—/auth-errorpasses through without a session cookie.src/auth.test.ts—onAPIError.errorURL === '/auth-error'.Verification
npx vitest run— 807 tests, 51 files, all passingnpx vitest run --coverage— all thresholds pass (src/proxy.ts100/100/100/100,src/app/**95/90/95/95 aggregate)npx tsc --noEmit -p tsconfig.json— cleannpm run lint— cleanNo changes to
package-lock.jsonor any file from the parallel dependency-audit work stream (.claude/references/deps-known-issues.md,.claude/references/testing.md,src/components/contact-logs/contact-logs.test.tsx,vitest.config.mts, untracked*.test.tsxfiles).🤖 Generated with Claude Code