Compare commits
4 Commits
85577c9d26
...
db2e8e487d
| Author | SHA1 | Date | |
|---|---|---|---|
| db2e8e487d | |||
| f93aa331d9 | |||
| d73c841ab7 | |||
| fe4978dbea |
@@ -52,6 +52,7 @@ func main() {
|
||||
Duration: cfg.SessionDuration,
|
||||
Secure: cfg.SessionSecure,
|
||||
PublicBaseURL: cfg.PublicBaseURL,
|
||||
FrontendURL: cfg.FrontendURL,
|
||||
})
|
||||
|
||||
ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM)
|
||||
|
||||
@@ -28,6 +28,7 @@ var testSessionConfig = SessionConfig{
|
||||
Duration: time.Hour,
|
||||
Secure: false,
|
||||
PublicBaseURL: "https://geniusrun.example.com",
|
||||
FrontendURL: "https://app.geniusrun.example.com",
|
||||
}
|
||||
|
||||
func newTestServer(t *testing.T) (*Server, *store.DB, int64) {
|
||||
@@ -828,7 +829,7 @@ func TestSessionCallback_AuthorizedSetsSessionCookieAndRedirectsHome(t *testing.
|
||||
rec := httptest.NewRecorder()
|
||||
s.Router().ServeHTTP(rec, req)
|
||||
|
||||
if rec.Code != http.StatusFound || rec.Header().Get("Location") != "/" {
|
||||
if rec.Code != http.StatusFound || rec.Header().Get("Location") != "https://app.geniusrun.example.com/" {
|
||||
t.Fatalf("status = %d, Location = %q", rec.Code, rec.Header().Get("Location"))
|
||||
}
|
||||
var sessionCookie *http.Cookie
|
||||
@@ -864,7 +865,7 @@ func TestSessionCallback_UnauthorizedRedirectsWithoutSessionCookie(t *testing.T)
|
||||
rec := httptest.NewRecorder()
|
||||
s.Router().ServeHTTP(rec, req)
|
||||
|
||||
if rec.Code != http.StatusFound || rec.Header().Get("Location") != "/?auth_error=forbidden" {
|
||||
if rec.Code != http.StatusFound || rec.Header().Get("Location") != "https://app.geniusrun.example.com/?auth_error=forbidden" {
|
||||
t.Fatalf("status = %d, Location = %q", rec.Code, rec.Header().Get("Location"))
|
||||
}
|
||||
for _, c := range rec.Result().Cookies() {
|
||||
@@ -879,7 +880,7 @@ func TestSessionCallback_MissingTxnCookieRedirectsFailed(t *testing.T) {
|
||||
req := httptest.NewRequest(http.MethodGet, "/api/session/callback?code=abc&state=s1", nil)
|
||||
rec := httptest.NewRecorder()
|
||||
s.Router().ServeHTTP(rec, req)
|
||||
if rec.Code != http.StatusFound || rec.Header().Get("Location") != "/?auth_error=failed" {
|
||||
if rec.Code != http.StatusFound || rec.Header().Get("Location") != "https://app.geniusrun.example.com/?auth_error=failed" {
|
||||
t.Fatalf("status = %d, Location = %q", rec.Code, rec.Header().Get("Location"))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -21,6 +21,10 @@ type SessionConfig struct {
|
||||
// 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
|
||||
}
|
||||
|
||||
type sessionMeResponse struct {
|
||||
@@ -49,7 +53,7 @@ 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)
|
||||
http.Redirect(w, r, s.Session.FrontendURL+"/?auth_error=failed", http.StatusFound)
|
||||
return
|
||||
}
|
||||
http.SetCookie(w, auth.ClearCookie(auth.TxnCookieName, s.Session.Secure))
|
||||
@@ -57,18 +61,18 @@ func (s *Server) handleSessionCallback(w http.ResponseWriter, r *http.Request) {
|
||||
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)
|
||||
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, "/?auth_error=failed", http.StatusFound)
|
||||
http.Redirect(w, r, s.Session.FrontendURL+"/?auth_error=failed", http.StatusFound)
|
||||
return
|
||||
}
|
||||
if !result.Authorized {
|
||||
http.Redirect(w, r, "/?auth_error=forbidden", http.StatusFound)
|
||||
http.Redirect(w, r, s.Session.FrontendURL+"/?auth_error=forbidden", http.StatusFound)
|
||||
return
|
||||
}
|
||||
|
||||
@@ -78,7 +82,7 @@ func (s *Server) handleSessionCallback(w http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
http.SetCookie(w, sessionCookie)
|
||||
http.Redirect(w, r, "/", http.StatusFound)
|
||||
http.Redirect(w, r, s.Session.FrontendURL+"/", http.StatusFound)
|
||||
}
|
||||
|
||||
func (s *Server) handleSessionLogout(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
@@ -44,6 +44,17 @@ type Config struct {
|
||||
// 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
|
||||
OIDCClientID string
|
||||
OIDCClientSecret string
|
||||
@@ -80,6 +91,7 @@ func Load() (Config, error) {
|
||||
if cfg.PublicBaseURL == "" {
|
||||
return cfg, fmt.Errorf("GENIUSRUN_PUBLIC_BASE_URL is required (e.g. https://geniusrun.example.com)")
|
||||
}
|
||||
cfg.FrontendURL = strings.TrimRight(getEnvDefault("GENIUSRUN_FRONTEND_URL", cfg.PublicBaseURL), "/")
|
||||
if cfg.OIDCIssuerURL == "" {
|
||||
return cfg, fmt.Errorf("GENIUSRUN_OIDC_ISSUER_URL is required")
|
||||
}
|
||||
|
||||
@@ -134,6 +134,32 @@ func TestLoad_GarminPythonPathExplicitOverridesDefault(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
||||
func TestLoad_CustomRoleAndDuration(t *testing.T) {
|
||||
setRequiredEnv(t)
|
||||
t.Setenv("GENIUSRUN_OIDC_REQUIRED_ROLE", "admin")
|
||||
|
||||
@@ -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
|
||||
)"
|
||||
```
|
||||
@@ -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).
|
||||
Reference in New Issue
Block a user