feat(auth): delegate OIDC loginPage to dj-site /login#1396
Merged
Conversation
`oidcProvider({ loginPage: '/sign-in' })` was redirecting unauthenticated authorize-endpoint visitors to a route that doesn't exist on the auth Express app. The canonical WXYC login UI lives in dj-site at `/login`; this change repoints loginPage there via an env-var-driven helper.
`buildLoginPage(env)` lives in its own file for the same testability reasons as `oidc-trusted-clients.ts` — pure function over a `NodeJS.ProcessEnv`, no `betterAuth({…})` instantiation needed to exercise the contract. Reuses `FRONTEND_SOURCE`, the existing dj-site base-URL env var already consumed by `trustedOrigins` and `rewriteUrlForFrontend`, so the canonical frontend origin is defined exactly once.
Documents `FRONTEND_SOURCE` in `.env.example` (previously only referenced in code).
6 tasks
Replace string-concat + trailing-slash regex with `new URL(raw).origin`, matching the parse-and-fallback pattern in url-rewrite.ts. The regex form silently accepted: missing scheme, embedded paths, trailing queries / fragments, multiple trailing slashes, and whitespace — several of which would produce a 302 to a malformed URL with no auth-service error. Two new failure modes: - Production fail-loud: throw at module load when NODE_ENV=production and FRONTEND_SOURCE is unset / empty / whitespace. The previous localhost fallback would silently redirect every unauthenticated OIDC user to a host their browser can't reach. - Invalid URL: throw when `new URL(raw)` rejects the value, so a typo surfaces at boot instead of via user reports. Tests grow from 4 to 17 (new groups: paste-from-browser hardening, production fail-loud, invalid URL). Existing happy-path + dev-fallback assertions kept verbatim. .env.example comment updated to document the new production semantics.
…e2e env
Three substantive bugs surfaced in iter-2 of code review.
1. `new URL('mailto:foo')` and other non-special schemes parse
successfully with `.origin === 'null'`. The previous helper would
silently return `'null/login'` — better-auth pipes that straight into
a 302 Location header, defeating the iter-1 fail-loud goal. Reject
anything that isn't `http:` or `https:` explicitly.
2. Fail-loud guard was `env.NODE_ENV === 'production'`. BS#1097
(documented inline at shared/authentication/src/auth.middleware.ts)
is the exact failure mode of NODE_ENV unset in real production:
under that polarity, missing NODE_ENV silently took the dev
fallback. Inverted to fail-loud on every NODE_ENV value except
`development` and `test`, so unset / `production` / `staging` all
throw. Tests pin NODE_ENV explicitly per case.
3. The `not a valid URL` error message echoed `JSON.stringify(raw)`
into Sentry / CloudWatch. Operators debugging OAuth sometimes paste
URLs that embed session tokens (e.g. `?session=...`). Don't echo
the value; the operator can re-check their config.
Cross-file blast radius caught by the same review pass:
- `apps/backend/app.ts:42` imports `requirePermissions` from
`@wxyc/authentication`, which evaluates `betterAuth({...})` at module
load (calling `buildLoginPage`). `Dockerfile.backend` pins
`NODE_ENV=production`, so the new throw fires inside the backend
container too if FRONTEND_SOURCE is missing. The `e2e-backend`
compose service inherits the pin but had no `NODE_ENV=test` override
AND no FRONTEND_SOURCE forwarded — `npm run e2e:env` would crash at
boot post-merge. Mirror the `backend` ci-profile service's
NODE_ENV=test + add FRONTEND_SOURCE=http://e2e-frontend:3000.
`.env.example` comment corrected: previous draft claimed
`rewriteUrlForFrontend` "falls back to localhost on parse error or
missing value" — actually parse errors leave the source URL untouched,
which is a different failure mode operators need to recognize.
Tests grow 17 → 27. New groups: scheme allowlist (4), production
polarity (unset NODE_ENV + staging + production cases), error-message
scrubbing assertion. Existing happy-path / paste-from-browser
assertions kept verbatim.
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.
Closes #1395. Companion PR: WXYC/dj-site#761.
Summary
oidcProviderplugin hadloginPage: '/sign-in'— a route that doesn't exist on the auth Express app. Existing reviewers of the flowsheet verifier (and any future OIDC client) were landing on a 404 instead of a sign-in form. This PR pointsloginPageat the canonical WXYC login UI in dj-site.shared/authentication/src/oidc-login-page.ts::buildLoginPage(env)builds the URL fromFRONTEND_SOURCE— the existing dj-site base-URL env var already consumed bytrustedOriginsandrewriteUrlForFrontend. Reusing it keeps the canonical frontend origin defined in exactly one env var. Falls back tohttp://localhost:3000/loginwhen unset, matchingrewriteUrlForFrontend's default.FRONTEND_SOURCEso a copy-from-browser URL doesn't produce a doubled-slashloginPage.oidc-trusted-clients.ts— pure function over aNodeJS.ProcessEnv, nobetterAuth({…})instantiation needed to exercise the contract..env.examplenow documentsFRONTEND_SOURCE(it was previously only referenced in code).Deployment
FRONTEND_SOURCEalready exists on the EC2 auth container — no new env var to set. Before merging, confirm the existing value points to the dj-site origin in each environment:https://dj.wxyc.org).Test plan
buildLoginPageunit tests cover:FRONTEND_SOURCEset, trailing slash, unset, empty string.prettier --checkclean on changed files.${BETTER_AUTH_URL}/oauth2/authorize?client_id=flowsheet&response_type=code&..., confirm bounce to dj-site/login?<query>, sign in, confirm the resume URL goes back to authorize and the final redirect to the flowsheet verifier client.