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 <noreply@anthropic.com>
This commit is contained in:
@@ -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 <noreply@anthropic.com>
|
||||
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 <noreply@anthropic.com>
|
||||
EOF
|
||||
)"
|
||||
```
|
||||
Reference in New Issue
Block a user