Repository navigation
fix(auth): post-login redirect, plus deterministic branding and a test-database guard - #381
Merged
Merged
Conversation
Signing in again after a session expired dropped the user on a raw loader payload or a 404 instead of the page they were on. React Router's single fetch means a client-side navigation requests `/programs.data`, not `/programs`. That is the request that trips `verifySession`, and `extractRedirectTo` captured the pathname verbatim, so the guard built `/login?redirectTo=%2Fprograms.data` and the login action redirected the browser to the loader endpoint. `normalizeRedirectPath` strips the decoration, mirroring React Router's own `getNormalizedPath` (lib/server-runtime/urls.ts), which is internal and cannot be imported: the root route decorates as `/_.data` rather than `/.data`, and `_routes` is an implementation detail that must not survive into a user-visible URL. Applied in two places on purpose. `safeRedirectUrl` is the funnel every target passes through — including one supplied by the query string — so a stale bookmark or a hand-edited `?redirectTo=` cannot reintroduce the landing. `extractRedirectTo` normalises at the source too, so the `?redirectTo=` the user actually sees in the address bar is a page. The destination is preserved rather than dropped to `/`: someone whose session expires on the programme list still ends up there. Verified by reproducing in the browser first — login with `?redirectTo=%2Fprograms.data` landed on `/programs.data` with a 404 — then confirming the same flow now lands on `/programs`. The regression tests were watched failing against the previous implementation (10 of them) before the fix was restored.
…alone
Self-review of the previous commit. The helper's return value goes
straight into a Location header, so anything it rewrites is a URL the
user is actually sent to — and it was rewriting four things it had no
business touching:
- `url.split('?')` discarded everything after a SECOND '?', so
`/search?q=a?b` was truncated to `/search?q=a`. A literal '?' inside a
query value is common enough that browsers pass it through unencoded.
This was a regression: the previous implementation returned the target
unchanged.
- Round-tripping the query through URLSearchParams rewrote `%20` as `+`
and turned a bare `?flag` into `?flag=`.
- The trailing-slash strip ran on every path, so `/programs/` became
`/programs` even though nothing had been decorated.
All three came from transforming unconditionally. It now splits on the
first '?' only, rewrites the pathname only when it actually ends in
`.data`, and rebuilds the query only when `_routes` is actually present.
The four cases are pinned as tests, and the full route tree was
re-checked: all 197 paths still round-trip through the decoration.
…ically `getBrandingName` and the locale fallback both pick "the congregation" in single-tenant mode with an unordered `findFirst`, which asks Postgres for any row. Single-tenant assumes exactly one, but a stale import or a leftover test fixture makes that false, and the app then brands itself with whichever row the planner reached first — an answer that can differ between requests against unchanged data. Ordering by id makes the pick stable. It does not make two congregations in a single-tenant install correct; it makes the symptom legible instead of intermittent. The two existence checks that also use an unordered `findFirst` (setup-first-account, seed) are deliberately left alone: they ask "is there any congregation at all", where the row identity is irrelevant. Found while debugging a login page branded "Roles Other 1788220863373".
The integration suite writes into, and in places truncates, whatever database it is pointed at. Nothing stopped that being a development one, and that is how a working database ends up holding a pile of leftover `Roles Other …` congregations — which then surface in the app itself, because single-tenant mode assumes one congregation row. The control plane has carried this rail for a while; this is the main app catching up. CI already names its database `unitae_test`, so nothing changes there. Checking DB_URL alone would not have been enough, and finding that out is most of the value here. Most integration files build their client from `DB_RUNTIME_URL ?? DB_URL` so RLS is exercised as the non-superuser role, while the migration suites use DB_URL because a migration runs as the schema owner. DB_RUNTIME_URL is typically exported from a shell profile and left pointing at development, so `DB_URL=…/unitae_test` looks safe while nearly every write still lands in the dev database. Both are checked, and they must name the same database — two different test databases would split the writes and tear down only one. The check runs from `setupFiles`, not from a shared helper, because these suites each construct their own PrismaClient; there is no single module they all pass through. Verified by pointing DB_URL at a test database with DB_RUNTIME_URL still on the dev one and watching it refuse, then running all 70 files / 440 tests green against a properly configured unitae_test.
GHSA-3f6p-5ww8-9rcr was published while this branch was open and fails CI's `pnpm audit --audit-level=high` step, which had passed on the previous commit twenty minutes earlier. It blocks every PR in the repo, not just this one — nothing here touches dependencies. mysql2 arrives only as a transitive dependency of prisma, which ships drivers for every database it supports. This app talks to PostgreSQL through @prisma/adapter-pg and never loads the MySQL driver, so the advisory is not reachable here — but the audit gate reasons about the dependency tree, not about which branches execute. An override is the smaller move than holding the branch for a prisma release that bumps it, and follows the `pkg@<version>` convention the fourteen entries already there use. Resolves 3.15.3 -> 3.24.2; the audit step then exits 0 with only the pre-existing moderate finding, which is below the threshold. Typecheck, lint, unit, integration and a production build all pass on the new resolution. Drop this once prisma ships a version carrying the patched driver itself.
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
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.
Three fixes plus an unrelated CI unblock. The first is the reported bug; the next two were found while debugging it and are bundled here on request. They touch disjoint files, so they can still be split if that reads better.
1. Signing in landed you on a loader, not a page
Signing in after a session expired dropped the user on a raw loader payload or a 404 instead of the page they were on.
React Router's single fetch means a client-side navigation requests
/programs.data, not/programs. That is the request that tripsverifySession, andextractRedirectTocaptured the pathname verbatim:So the guard built
/login?redirectTo=%2Fcongregation%2Froles%2Forganigram.data,safeRedirectUrlaccepted it (it does start with/), and the login action redirected the browser to the loader endpoint.It only shows up when you are being sent back to a previous page — that is the only time
redirectTois populated — but it applies to every route in the app, not one of them.Fix
normalizeRedirectPathstrips the decoration, mirroring React Router's owngetNormalizedPath(lib/server-runtime/urls.ts), which is internal and cannot be imported:/_.data, not/.data, so stripping only.datawould leave/__routesis a single-fetch implementation detail and must not survive into a user-visible URLIt is a suffix strip — no route list, no allowlist, nothing path-specific.
Applied in two places on purpose.
safeRedirectUrlis the funnel every redirect target passes through, including one supplied by the query string, so a stale bookmark or a hand-edited?redirectTo=cannot reintroduce the landing.extractRedirectTonormalises at the source too, so the?redirectTo=the user actually sees in the address bar is a page.The destination is preserved rather than dropped to
/: someone whose session expires on the programme list still ends up there.Second commit — self-review
Reviewing the first commit found the helper rewriting four things it had no business touching. Its return value goes into a
Locationheader, so every rewrite is a URL a user is actually sent to./search?q=a?b/search?q=a— truncated/search?q=a%20b/search?q=a+b/search?flag/search?flag=/programs//programsThe first was a regression:
url.split('?')discards everything after a second?, and the previous implementation returned the target unchanged. The next two came from round-tripping the query throughURLSearchParams; the last from running the trailing-slash strip on paths that were never decorated.All three causes were the same — transforming unconditionally. It now splits on the first
?only, rewrites the pathname only when it actually ends in.data, and rebuilds the query only when_routesis actually present.Verification
Reproduced in the browser before touching anything: logging in with
?redirectTo=%2Fprograms.datalanded on/programs.datashowing "404 — Page introuvable". After the fix the same flow lands on/programs, and?redirectTo=%2Fcongregation%2Froles%2Forganigram.datalands on the Organigramme page.Tests were written first and watched failing (10 of them) before the fix was restored. As a one-off check, the full route tree was pulled from
react-router routesand all 197 reconstructed paths round-tripped through the decoration.2. The single-tenant congregation was picked non-deterministically
getBrandingNameand the locale fallback both resolve "the congregation" in single-tenant mode with an unorderedfindFirst, which asks Postgres for any row. Single-tenant assumes exactly one, but a stale import or a leftover test fixture makes that false — and the app then brands itself, and picks its language, from whichever row the planner reached first. The answer can differ between requests against unchanged data.This is what surfaced a development login page titled
Roles Other 1788220863373.Ordering by id makes the pick stable. It does not make two congregations in a single-tenant install correct; it makes the symptom legible instead of intermittent.
The two existence checks that also use an unordered
findFirst(setup-first-account,seed) are deliberately left alone — they ask "is there any congregation at all", where row identity is irrelevant.3. Integration tests would run against any database
The integration suite writes into, and in places truncates, whatever database it is pointed at. Nothing stopped that being a development one — which is how a working database ends up holding a pile of leftover fixtures, feeding directly into the bug above.
The control plane has carried this rail for a while; this is the main app catching up. CI already names its database
unitae_test, so nothing changes there.Checking
DB_URLalone would not have been enough, and finding that out is most of the value. Most integration files build their client fromDB_RUNTIME_URL ?? DB_URLso RLS is exercised as the non-superuser role, while the migration suites useDB_URLbecause a migration runs as the schema owner.DB_RUNTIME_URLis typically exported from a shell profile and left pointing at development, soDB_URL=…/unitae_testlooks entirely safe while nearly every write still lands in the dev database.Both are checked, and they must name the same database — two different test databases would split the writes and tear down only one.
The check runs from
setupFilesrather than a shared helper, because these suites each construct their ownPrismaClient; there is no single module they all pass through.Documented in
docs/development/testing.mdwith a local setup recipe, plus a row in the CLAUDE.md troubleshooting table.Gate
Full local gate green on the combined branch: typecheck, lint, boundaries, aggregate-boundaries, tenant-scoping, server-barrel-exports, service-test-coverage, file-sizes, permission-coverage, unit, integration — plus
biome check, which the pre-commit hook enforces and CI does not run.The integration suite was re-run end to end against a properly configured
unitae_test: 70 files, 440 tests, 1 skipped, confirming the new guard does not break a correctly configured run.4. Unrelated: a new advisory turned CI red
GHSA-3f6p-5ww8-9rcr(mysql2 credential leak, high) was published while this branch was open, and CI'spnpm audit --audit-level=highstep started failing. The same job passed on the previous commit twenty minutes earlier, and nothing here touches dependencies — it blocks every PR in the repo, not this one.mysql2 arrives only as a transitive dependency of prisma, which ships drivers for every database it supports. This app talks to PostgreSQL through
@prisma/adapter-pgand never loads the MySQL driver, so the advisory is not reachable here — but the audit gate reasons about the dependency tree, not about which branches execute.An override (
3.15.3→3.24.2) is the smaller move than holding the branch for a prisma release that bumps it, and follows thepkg@<version>convention the fourteen entries already there use. Drop this commit once prisma carries the patched driver itself — it is the one commit here with a shelf life.