From b02b1706b6d0a7164cd686461e69447250584f6b Mon Sep 17 00:00:00 2001 From: hzhang Date: Sun, 24 May 2026 10:03:53 +0100 Subject: [PATCH] fix(oidc): clamp post_login_redirect to CORS allow-list (open-redirect) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the open-redirect risk surfaced by the v0.3.x security audit: the OIDC callback handler was using oidc_config.post_login_redirect verbatim as the redirect base on both error and success paths. An attacker with admin-key compromise (or a misconfigured operator) could set that field to an external domain and turn /api/auth/oidc/callback into a phishing redirector ('dialectic login failed → re-enter password at evil.com/login'). Fix: - New AuthHandler.safeRedirectBase(raw) validator: * empty → '/' * relative path starting with '/' (but not '//') → keep as-is * absolute URL whose host is in the allow-list → keep as-is * everything else → '/' - allow-list sourced from cfg.CORSAllowOrigins (the same set we already trust for browser CORS), threaded through NewAuthHandler. - Applied on BOTH the error branch and success branch of Callback. Also: drop redundant newline on cmd/dialectic-cli usage Fprintln so go test ./... passes. No behavior change for happy path: prod's PostLoginRedirect is 'https://dialectic.hangman-lab.top/oidc/callback', which matches the CORS allow-list ('https://dialectic.hangman-lab.top'), so the validator returns it unmodified. --- cmd/dialectic-cli/main.go | 2 +- internal/httpapi/handlers/auth.go | 62 ++++++++++++++++++++++++++----- internal/httpapi/routes.go | 8 +++- 3 files changed, 61 insertions(+), 11 deletions(-) diff --git a/cmd/dialectic-cli/main.go b/cmd/dialectic-cli/main.go index d65e861..e6f09c0 100644 --- a/cmd/dialectic-cli/main.go +++ b/cmd/dialectic-cli/main.go @@ -127,7 +127,7 @@ func check(label string, err error) { } func usage() { - fmt.Fprintln(os.Stderr, `Usage: + fmt.Fprint(os.Stderr, `Usage: dialectic-cli config oidc [--issuer ] [--client-id ] [--client-secret ] [--callback-url ] [--post-login-redirect ] diff --git a/internal/httpapi/handlers/auth.go b/internal/httpapi/handlers/auth.go index 7503e32..61b2609 100644 --- a/internal/httpapi/handlers/auth.go +++ b/internal/httpapi/handlers/auth.go @@ -21,16 +21,56 @@ import ( // GET /api/auth/me — current session user (401 if anon) // POST /api/auth/logout — clears the session cookie type AuthHandler struct { - oidc *oidc.Service - cookieName string - secure bool + oidc *oidc.Service + cookieName string + secure bool + allowedHosts []string // for open-redirect protection on post_login_redirect } -func NewAuthHandler(svc *oidc.Service, cookieName string, secure bool) *AuthHandler { +// NewAuthHandler wires the OIDC HTTP surface. `allowedHosts` is the +// allow-list of hostnames the callback may 302 the browser to (sourced +// from cfg.CORSAllowOrigins — the same origins we already trust for +// CORS, by definition. A relative path always passes regardless. +func NewAuthHandler(svc *oidc.Service, cookieName string, secure bool, allowedHosts []string) *AuthHandler { if cookieName == "" { cookieName = "dialectic_session" } - return &AuthHandler{oidc: svc, cookieName: cookieName, secure: secure} + return &AuthHandler{oidc: svc, cookieName: cookieName, secure: secure, allowedHosts: allowedHosts} +} + +// safeRedirectBase returns `raw` if it's a relative path OR an absolute +// URL whose host appears in the allow-list. Otherwise it returns "/" so +// the open-redirect attack (attacker-set PostLoginRedirect points to +// evil.com) is neutered — the worst the SPA sees is its own root with +// an error fragment, never an external bounce. +// +// Why we need this: oidc_config.post_login_redirect is set via the +// admin cli. An admin-key compromise OR a misconfiguration that points +// it at an external domain would otherwise let any /oidc/callback +// error path redirect the user there with `?oidc_error=...` attached, +// usable for phishing ("dialectic login failed: please re-enter your +// password at evil.com/login"). +func (h *AuthHandler) safeRedirectBase(raw string) string { + if raw == "" { + return "/" + } + // Relative paths are always safe (same-origin by definition). + if strings.HasPrefix(raw, "/") && !strings.HasPrefix(raw, "//") { + return raw + } + u, err := url.Parse(raw) + if err != nil || u.Host == "" { + return "/" + } + // Walk the allow-list. cors.AllowedOrigins entries look like + // "https://dialectic.hangman-lab.top" — parse host out + compare. + for _, origin := range h.allowedHosts { + o, err := url.Parse(origin) + if err == nil && o.Host != "" && strings.EqualFold(o.Host, u.Host) { + return raw + } + } + return "/" } func (h *AuthHandler) Status(w http.ResponseWriter, r *http.Request) { @@ -60,8 +100,8 @@ func (h *AuthHandler) Callback(w http.ResponseWriter, r *http.Request) { // something useful instead of a 500 page mid-login. c, _ := h.oidc.GetConfig(r.Context()) base := "/" - if c != nil && c.PostLoginRedirect != "" { - base = c.PostLoginRedirect + if c != nil { + base = h.safeRedirectBase(c.PostLoginRedirect) } sep := "#" if strings.Contains(base, "#") { @@ -70,11 +110,15 @@ func (h *AuthHandler) Callback(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, base+sep+"oidc_error="+url.QueryEscape(err.Error()), http.StatusFound) return } + // Validate the success-path redirect too — HandleCallback returned + // PostLoginRedirect from the same DB row, so the same open-redirect + // risk applies on the happy path. + safe := h.safeRedirectBase(redirect) sep := "#" - if strings.Contains(redirect, "#") { + if strings.Contains(safe, "#") { sep = "&" } - http.Redirect(w, r, redirect+sep+"oidc_ticket="+url.QueryEscape(ticket), http.StatusFound) + http.Redirect(w, r, safe+sep+"oidc_ticket="+url.QueryEscape(ticket), http.StatusFound) } type exchangeBody struct { diff --git a/internal/httpapi/routes.go b/internal/httpapi/routes.go index f840fb4..8fd5aff 100644 --- a/internal/httpapi/routes.go +++ b/internal/httpapi/routes.go @@ -87,7 +87,13 @@ func Mount(cfg *config.Config, db *sqlx.DB, oidcSvc *oidc.Service, version strin // cookie would prevent the browser from sending it back. The // SameSite=Lax + HttpOnly defenses still apply; CF/origin TLS // covers the wire. - authH := handlers.NewAuthHandler(oidcSvc, "dialectic_session", false) + // Pass the configured CORS allow-list as the open-redirect allowlist + // for the OIDC callback — the SPA hosts we already trust for CORS + // are by definition the same hosts a legitimate PostLoginRedirect + // can target. Anything else (incl. attacker-set values written via + // admin cli compromise) gets clamped back to "/" inside the + // handler. + authH := handlers.NewAuthHandler(oidcSvc, "dialectic_session", false, cfg.CORSAllowOrigins) // Routes. r.Route("/api", func(r chi.Router) {