fix(oidc): clamp post_login_redirect to CORS allow-list (open-redirect)
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.
This commit is contained in:
@@ -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 <url>] [--client-id <id>]
|
||||
[--client-secret <s>] [--callback-url <url>]
|
||||
[--post-login-redirect <url>]
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user