From d73c841ab7a8809bf7538c5e7280dc671030d052 Mon Sep 17 00:00:00 2001 From: Christophe Vila Date: Sat, 25 Jul 2026 22:49:02 +0200 Subject: [PATCH] docs: add implementation plan for the OIDC callback redirect fix Two small tasks: add config.Config.FrontendURL (Task 1), then thread it through api.SessionConfig and handleSessionCallback's five redirects (Task 2). Co-Authored-By: Claude Sonnet 5 --- ...7-25-oidc-callback-redirect-origin-plan.md | 354 ++++++++++++++++++ 1 file changed, 354 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-25-oidc-callback-redirect-origin-plan.md diff --git a/docs/superpowers/plans/2026-07-25-oidc-callback-redirect-origin-plan.md b/docs/superpowers/plans/2026-07-25-oidc-callback-redirect-origin-plan.md new file mode 100644 index 0000000..4cfa23f --- /dev/null +++ b/docs/superpowers/plans/2026-07-25-oidc-callback-redirect-origin-plan.md @@ -0,0 +1,354 @@ +# Fix OIDC callback redirect origin — implementation plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix `handleSessionCallback`'s redirects so they resolve to the frontend's actual origin instead of a relative path resolved against the backend's own origin, which 404s in this project's own supported split-origin local dev setup. + +**Architecture:** Add one new optional config field, `FrontendURL` (env `GENIUSRUN_FRONTEND_URL`), defaulting to `PublicBaseURL`. Thread it through `config.Config` → `cmd/geniusrund/main.go` → `api.SessionConfig` → the five `http.Redirect` calls in `handleSessionCallback`. + +**Tech Stack:** Go, `net/http`, existing `internal/config`/`internal/api` test conventions. + +## Global Constraints + +- No behavior change when `GENIUSRUN_FRONTEND_URL` is unset — `FrontendURL` must default to exactly `PublicBaseURL` (already trimmed of trailing slash). +- `handleSessionLogin`'s `writeError` responses are explicitly out of scope — do not touch them (confirmed with the user). +- `handleSessionLogout`'s redirect (`s.Auth.EndSessionURL(s.Session.PublicBaseURL+"/")`) is explicitly out of scope — do not touch it. +- All 5 redirect targets in `handleSessionCallback` must use `s.Session.FrontendURL` as the origin: the 3 `"/?auth_error=failed"` branches, the 1 `"/?auth_error=forbidden"` branch, and the 1 success (`"/"`) branch. + +--- + +### Task 1: Add `FrontendURL` to `internal/config` + +**Files:** +- Modify: `backend/internal/config/config.go` +- Modify: `backend/internal/config/config_test.go` + +**Interfaces:** +- Produces: `Config.FrontendURL string` — set by `Load()`, defaults to `Config.PublicBaseURL` when `GENIUSRUN_FRONTEND_URL` is unset, trimmed of any trailing slash otherwise. Consumed by Task 2's `main.go` wiring. + +- [ ] **Step 1: Write the failing tests** + +Add to `backend/internal/config/config_test.go` (anywhere in the file, e.g. after `TestLoad_GarminPythonPathExplicitOverridesDefault`): + +```go +func TestLoad_FrontendURLDefaultsToPublicBaseURL(t *testing.T) { + setRequiredEnv(t) + t.Setenv("GENIUSRUN_FRONTEND_URL", "") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.FrontendURL != cfg.PublicBaseURL { + t.Errorf("FrontendURL = %q, want it to default to PublicBaseURL %q", cfg.FrontendURL, cfg.PublicBaseURL) + } +} + +func TestLoad_FrontendURLExplicitOverridesDefault(t *testing.T) { + setRequiredEnv(t) + t.Setenv("GENIUSRUN_FRONTEND_URL", "http://localhost:5173/") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.FrontendURL != "http://localhost:5173" { + t.Errorf("FrontendURL = %q, want http://localhost:5173 (trailing slash trimmed)", cfg.FrontendURL) + } +} +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cd backend && go test ./internal/config/... -run TestLoad_FrontendURL -v` +Expected: compile failure — `Config.FrontendURL` doesn't exist yet. + +- [ ] **Step 3: Add the field and default logic** + +In `backend/internal/config/config.go`, add a new field right after `PublicBaseURL` in the `Config` struct: + +```go + // OIDC login gate (Keycloak). PublicBaseURL is this app's own externally + // reachable origin (e.g. "https://geniusrun.example.com") -- it derives + // OIDCRedirectURL and whether session cookies can be marked Secure, + // instead of requiring both to be configured separately and risking them + // drifting out of sync. + PublicBaseURL string + // FrontendURL is the origin the browser should land on after the OIDC + // callback (both success and failure) -- e.g. "http://localhost:5173" in + // local dev, where the frontend and backend are different origins + // bridged by CORS (see internal/api's corsMiddleware and + // frontend/src/api/client.ts's BASE_URL). Defaults to PublicBaseURL when + // unset, which is correct for the common production topology where a + // reverse proxy unifies frontend and backend under one origin. + // PublicBaseURL itself must stay pointed at the backend's own origin + // regardless -- it derives OIDCRedirectURL and post_logout_redirect_uri, + // which must match wherever those routes are actually served. + FrontendURL string + OIDCIssuerURL string +``` + +(i.e. insert the `FrontendURL` field and its comment between `PublicBaseURL` and `OIDCIssuerURL` — the struct's other fields are unchanged.) + +In `Load()`, immediately after the existing `if cfg.PublicBaseURL == "" { return cfg, fmt.Errorf(...) }` check (so the default is only computed once `PublicBaseURL` is confirmed non-empty), add: + +```go + cfg.FrontendURL = strings.TrimRight(getEnvDefault("GENIUSRUN_FRONTEND_URL", cfg.PublicBaseURL), "/") +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cd backend && go test ./internal/config/... -v` +Expected: all tests PASS, including the two new ones. + +- [ ] **Step 5: Commit** + +```bash +git add backend/internal/config/config.go backend/internal/config/config_test.go +git commit -m "$(cat <<'EOF' +feat(config): add GENIUSRUN_FRONTEND_URL, defaulting to PublicBaseURL + +Lets the OIDC callback redirect to the frontend's real origin instead of +a relative path resolved against the backend's own origin -- needed for +this project's own supported split-origin local dev setup (frontend on +Vite, backend on geniusrund, bridged by CORS). + +Co-Authored-By: Claude Sonnet 5 +EOF +)" +``` + +--- + +### Task 2: Use `FrontendURL` in `handleSessionCallback`'s redirects + +**Files:** +- Modify: `backend/internal/api/session.go` +- Modify: `backend/cmd/geniusrund/main.go` +- Modify: `backend/internal/api/api_test.go` + +**Interfaces:** +- Consumes: `config.Config.FrontendURL` (Task 1). +- Produces: `api.SessionConfig.FrontendURL string` — new field, read by `handleSessionCallback`. + +- [ ] **Step 1: Update the shared test fixture and write the failing tests** + +In `backend/internal/api/api_test.go`, update `testSessionConfig` (currently): + +```go +var testSessionConfig = SessionConfig{ + Secret: []byte("test-session-secret-at-least-32-bytes-long"), + Duration: time.Hour, + Secure: false, + PublicBaseURL: "https://geniusrun.example.com", +} +``` + +to: + +```go +var testSessionConfig = SessionConfig{ + Secret: []byte("test-session-secret-at-least-32-bytes-long"), + Duration: time.Hour, + Secure: false, + PublicBaseURL: "https://geniusrun.example.com", + FrontendURL: "https://app.geniusrun.example.com", +} +``` + +(Deliberately a *different* value from `PublicBaseURL`, so the three tests below actually exercise the new field instead of accidentally passing against an empty-string prefix.) + +Update the three existing `Location`-asserting tests' expectations: + +`TestSessionCallback_AuthorizedSetsSessionCookieAndRedirectsHome` — change: +```go + if rec.Code != http.StatusFound || rec.Header().Get("Location") != "/" { +``` +to: +```go + if rec.Code != http.StatusFound || rec.Header().Get("Location") != "https://app.geniusrun.example.com/" { +``` + +`TestSessionCallback_UnauthorizedRedirectsWithoutSessionCookie` — change: +```go + if rec.Code != http.StatusFound || rec.Header().Get("Location") != "/?auth_error=forbidden" { +``` +to: +```go + if rec.Code != http.StatusFound || rec.Header().Get("Location") != "https://app.geniusrun.example.com/?auth_error=forbidden" { +``` + +`TestSessionCallback_MissingTxnCookieRedirectsFailed` — change: +```go + if rec.Code != http.StatusFound || rec.Header().Get("Location") != "/?auth_error=failed" { +``` +to: +```go + if rec.Code != http.StatusFound || rec.Header().Get("Location") != "https://app.geniusrun.example.com/?auth_error=failed" { +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `cd backend && go test ./internal/api/... -run TestSessionCallback -v` +Expected: all three tests FAIL (current code still redirects to the bare relative path; `SessionConfig.FrontendURL` doesn't exist yet, so this is actually a compile failure first — that's expected too). + +- [ ] **Step 3: Add `FrontendURL` to `SessionConfig` and use it in the five redirects** + +In `backend/internal/api/session.go`, add a field to `SessionConfig` (right after `PublicBaseURL`): + +```go +type SessionConfig struct { + Secret []byte + Duration time.Duration + Secure bool + // PublicBaseURL is this app's own externally reachable origin (e.g. + // "https://geniusrun.example.com", no trailing slash), used to build an + // absolute post_logout_redirect_uri for the identity provider -- some + // providers, including Keycloak, require this to be an absolute URL + // matching one registered on the client, not a bare relative path. + PublicBaseURL string + // FrontendURL is the origin the browser should land on after the OIDC + // callback (success or failure) -- see config.Config.FrontendURL for why + // this can differ from PublicBaseURL in a split-origin deployment. + FrontendURL string +} +``` + +Then replace `handleSessionCallback`'s body (all five `http.Redirect` targets) — the function currently is: + +```go +func (s *Server) handleSessionCallback(w http.ResponseWriter, r *http.Request) { + txnCookie, err := r.Cookie(auth.TxnCookieName) + if err != nil { + log.Printf("session callback: missing txn cookie: %v", err) + http.Redirect(w, r, "/?auth_error=failed", http.StatusFound) + return + } + http.SetCookie(w, auth.ClearCookie(auth.TxnCookieName, s.Session.Secure)) + + txn, err := auth.ParseTxnCookie(txnCookie, s.Session.Secret) + if err != nil { + log.Printf("session callback: failed to parse txn cookie: %v", err) + http.Redirect(w, r, "/?auth_error=failed", http.StatusFound) + return + } + + result, err := s.Auth.HandleCallback(r.Context(), txn, r.URL.Query()) + if err != nil { + log.Printf("session callback: HandleCallback failed (state mismatch, code exchange, or ID-token verification): %v", err) + http.Redirect(w, r, "/?auth_error=failed", http.StatusFound) + return + } + if !result.Authorized { + http.Redirect(w, r, "/?auth_error=forbidden", http.StatusFound) + return + } + + sessionCookie, err := auth.MintSessionCookie(result.Claims, s.Session.Secret, s.Session.Duration, s.Session.Secure) + if err != nil { + writeError(w, http.StatusInternalServerError, err.Error()) + return + } + http.SetCookie(w, sessionCookie) + http.Redirect(w, r, "/", http.StatusFound) +} +``` + +Replace it with (only the five redirect-target string literals change, nothing else): + +```go +func (s *Server) handleSessionCallback(w http.ResponseWriter, r *http.Request) { + txnCookie, err := r.Cookie(auth.TxnCookieName) + if err != nil { + log.Printf("session callback: missing txn cookie: %v", err) + http.Redirect(w, r, s.Session.FrontendURL+"/?auth_error=failed", http.StatusFound) + return + } + http.SetCookie(w, auth.ClearCookie(auth.TxnCookieName, s.Session.Secure)) + + txn, err := auth.ParseTxnCookie(txnCookie, s.Session.Secret) + if err != nil { + log.Printf("session callback: failed to parse txn cookie: %v", err) + http.Redirect(w, r, s.Session.FrontendURL+"/?auth_error=failed", http.StatusFound) + return + } + + result, err := s.Auth.HandleCallback(r.Context(), txn, r.URL.Query()) + if err != nil { + log.Printf("session callback: HandleCallback failed (state mismatch, code exchange, or ID-token verification): %v", err) + http.Redirect(w, r, s.Session.FrontendURL+"/?auth_error=failed", http.StatusFound) + return + } + if !result.Authorized { + http.Redirect(w, r, s.Session.FrontendURL+"/?auth_error=forbidden", http.StatusFound) + return + } + + sessionCookie, err := auth.MintSessionCookie(result.Claims, s.Session.Secret, s.Session.Duration, s.Session.Secure) + if err != nil { + writeError(w, http.StatusInternalServerError, err.Error()) + return + } + http.SetCookie(w, sessionCookie) + http.Redirect(w, r, s.Session.FrontendURL+"/", http.StatusFound) +} +``` + +In `backend/cmd/geniusrund/main.go`, change: + +```go + }, authVerifier, api.SessionConfig{ + Secret: cfg.SessionSecret, + Duration: cfg.SessionDuration, + Secure: cfg.SessionSecure, + PublicBaseURL: cfg.PublicBaseURL, + }) +``` + +to: + +```go + }, authVerifier, api.SessionConfig{ + Secret: cfg.SessionSecret, + Duration: cfg.SessionDuration, + Secure: cfg.SessionSecure, + PublicBaseURL: cfg.PublicBaseURL, + FrontendURL: cfg.FrontendURL, + }) +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cd backend && go test ./internal/api/... -v` +Expected: all tests in the package PASS, including the three updated ones. + +- [ ] **Step 5: Full-repo verification** + +Run: +```bash +cd backend +gofmt -l . +go build ./... +go vet ./... +go test ./... +``` +Expected: `gofmt -l .` prints nothing; `go build`/`go vet` produce no errors; `go test ./...` passes for every package. + +- [ ] **Step 6: Commit** + +```bash +git add backend/internal/api/session.go backend/internal/api/api_test.go backend/cmd/geniusrund/main.go +git commit -m "$(cat <<'EOF' +fix(api): redirect the OIDC callback to FrontendURL, not a relative path + +handleSessionCallback's redirects (success and all 4 failure branches) +were relative paths, which resolve against the backend's own origin -- +broken in this project's own supported split-origin local dev setup, +since the Go backend serves no "/" route at all. Now uses the new +config.Config.FrontendURL (defaults to PublicBaseURL, so no change for +single-origin production deployments). + +Co-Authored-By: Claude Sonnet 5 +EOF +)" +```