docs: add design spec for fixing OIDC callback redirect origin

handleSessionCallback's relative redirects resolve against the backend's
own origin, which 404s in this project's own supported split-origin local
dev setup (frontend on Vite, backend on geniusrund, bridged by CORS). Adds
GENIUSRUN_FRONTEND_URL, defaulting to PublicBaseURL for the common
single-origin production case, as the fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-25 22:42:50 +02:00
parent 85577c9d26
commit fe4978dbea

View File

@@ -0,0 +1,56 @@
# Fix cross-origin redirect targets in the OIDC callback — design
Date: 2026-07-25
## Overview
`internal/api/session.go`'s `handleSessionCallback` redirects the browser back into the app using bare relative paths (`"/"` on success, `"/?auth_error=failed"`/`"/?auth_error=forbidden"` on failure). A relative redirect resolves against the origin of the request that received it — and since Keycloak redirects the browser straight to `OIDCRedirectURL` (the backend's own `/api/session/callback`), that origin is always the **backend's**, never the frontend's.
This is silently correct in a deployment where a reverse proxy unifies the frontend and backend under one origin (the assumed production topology, per `GENIUSRUN_PUBLIC_BASE_URL`'s doc comment: "this app's own externally reachable origin"). It is broken in any split-origin deployment — including this project's own supported local dev setup, where the frontend (Vite, port 5173) and backend (`geniusrund`, port 8080) are deliberately different origins, bridged by `corsMiddleware` and `VITE_API_BASE_URL` (see `internal/api/server.go`'s `corsMiddleware` comment and `frontend/src/api/client.ts`'s `BASE_URL`). In that setup, the Go backend has no `/` route at all — only `/api/*` — so every one of these five redirects 404s.
## Goals
- All 5 redirects in `handleSessionCallback` (4 error cases + 1 success case) resolve to the frontend's actual origin, in both the single-origin (production, reverse-proxy) and split-origin (local dev) topologies, with no manual per-environment code branching.
- No behavior change for an existing single-origin deployment: if the new config value is left unset, behavior is identical to today.
## Non-goals
- `handleSessionLogin`'s `writeError` responses (on `BeginLogin`/txn-cookie-mint failure) are explicitly out of scope — confirmed with the user. These render a raw JSON error directly on the backend's own origin today; they are not redirects, so they don't 404, just look untailored. Left as-is.
- No change to `handleSessionLogout`'s redirect target, which already uses `s.Session.PublicBaseURL+"/"` (an absolute URL) rather than a relative path — it doesn't have this bug. (Whether `PublicBaseURL` is even the *correct* absolute origin for that redirect in a split-origin setup is a separate, pre-existing question not raised by this task; not touched here.)
- No change to the OIDC redirect_uri / post_logout_redirect_uri themselves (`PublicBaseURL`-derived) — those correctly must stay pointed at the backend's own origin, since that's where those routes are actually served.
## Design
Add one new optional config field, `FrontendURL` (env var `GENIUSRUN_FRONTEND_URL`), read in `config.Load()` and defaulted to `PublicBaseURL` when unset. Thread it into `api.SessionConfig` alongside the existing `PublicBaseURL` field. In `handleSessionCallback`, replace every relative redirect target with `s.Session.FrontendURL + "/"` (success and the two path-only cases) or `s.Session.FrontendURL + "/?auth_error=..."` (the error cases) instead of the bare `"/"` / `"/?auth_error=..."` strings.
### Config
`internal/config/config.go`:
- New `Config.FrontendURL string` field, doc comment explaining the split-origin-dev rationale above.
- In `Load()`: `FrontendURL: getEnvDefault("GENIUSRUN_FRONTEND_URL", cfg.PublicBaseURL)` — note this must be set *after* `PublicBaseURL` is computed (trimmed of trailing slash) in the same struct literal, or as a follow-up assignment; since Go struct literals can't reference sibling fields being built in the same literal, this needs to be assigned as a statement after the literal, mirroring how `OIDCRedirectURL`/`SessionSecure` are already derived post-literal today.
- Trim any trailing slash from the explicit env var value the same way `PublicBaseURL` already is, so `FrontendURL + "/"` never produces a double slash.
### Wiring
`cmd/geniusrund/main.go`: pass `cfg.FrontendURL` into the new `api.SessionConfig.FrontendURL` field alongside the existing `PublicBaseURL` field.
`internal/api/session.go`'s `SessionConfig` struct: add `FrontendURL string` field with a doc comment cross-referencing `PublicBaseURL`'s and explaining why they can differ.
### Redirect targets
In `handleSessionCallback`, each of the five `http.Redirect` calls' second-to-last argument changes from a relative path to `s.Session.FrontendURL + "<same relative path as today>"`:
- `"/?auth_error=failed"` (×3, one per failure branch) → `s.Session.FrontendURL+"/?auth_error=failed"`
- `"/?auth_error=forbidden"``s.Session.FrontendURL+"/?auth_error=forbidden"`
- `"/"` (success) → `s.Session.FrontendURL+"/"`
No change to any other logic in the handler (the log lines, the role check, cookie minting — all untouched).
## Testing
- `internal/config`: new test(s) asserting `FrontendURL` defaults to `PublicBaseURL` when `GENIUSRUN_FRONTEND_URL` is unset, and that an explicit value overrides the default (mirroring the existing `TestLoad_GarminTokenStoreRoot*`-style pair).
- `internal/api`: `testSessionConfig` (`api_test.go:26-31`) currently has no `FrontendURL` field set; since it's distinct from `PublicBaseURL` ("https://geniusrun.example.com"), it must be given its own distinct value (e.g. "https://app.geniusrun.example.com") so the three existing `Location`-asserting tests actually exercise the new field instead of accidentally passing against a zero-value empty-string prefix:
- `TestSessionCallback_AuthorizedSetsSessionCookieAndRedirectsHome` (`api_test.go:813-841`) — currently asserts `Location != "/"`; update to assert the `FrontendURL`-prefixed value.
- `TestSessionCallback_UnauthorizedRedirectsWithoutSessionCookie` (`api_test.go:852-873`) — currently asserts `Location != "/?auth_error=forbidden"`; update similarly.
- `TestSessionCallback_MissingTxnCookieRedirectsFailed` (`api_test.go:875-882`) — currently asserts `Location != "/?auth_error=failed"`; update similarly.
- No existing test appears to cover the other two `"/?auth_error=failed"` branches (txn-cookie-parse failure, `HandleCallback` error) — not introduced by this fix, so not required here, but worth noting as pre-existing gaps.
- No change needed to `internal/auth` (untouched by this fix).