fix(oidcgate): security hardening — sanitize XFU, guardrails, validations

cmd/oidcgate/config.go

@@ -82,6 +82,28 @@ func Load(path string) (*Config, error) {
 		return nil, fmt.Errorf("config: missing required 'listen' (or OIDCGATE_LISTEN env var)")
 	}
 
+	if !strings.HasPrefix(cfg.OIDC.CallbackURL, "/") {
+		return nil, fmt.Errorf("config: callbackURL must be a path starting with '/', got %q", cfg.OIDC.CallbackURL)
+	}
+	if !strings.HasPrefix(cfg.OIDC.LogoutURL, "/") {
+		return nil, fmt.Errorf("config: logoutURL must be a path starting with '/', got %q", cfg.OIDC.LogoutURL)
+	}
+
+	reserved := []string{
+		sentinelPath,
+		cfg.AuthPath,
+		cfg.StartPath,
+		cfg.OIDC.CallbackURL,
+		cfg.OIDC.LogoutURL,
+	}
+	for _, ex := range cfg.OIDC.ExcludedURLs {
+		for _, r := range reserved {
+			if r != "" && strings.HasPrefix(r, ex) {
+				return nil, fmt.Errorf("config: excludedURL %q would bypass reserved oidcgate path %q", ex, r)
+			}
+		}
+	}
+
 	// Force standalone semantics: trust X-Forwarded-Uri.
 	cfg.OIDC.TrustForwardedURI = true
 	return cfg, nil

cmd/oidcgate/config_test.go

@@ -6,6 +6,27 @@ import (
 	"testing"
 )
 
+// minimalYAML is a base config accepted by Load with no surprises.
+const minimalYAML = `
+listen: ":8080"
+providerURL: "https://idp.example"
+clientID: "abc"
+clientSecret: "secret"
+sessionEncryptionKey: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+callbackURL: "/oauth2/callback"
+logoutURL: "/oauth2/logout"
+`
+
+func writeConfig(t *testing.T, content string) string {
+	t.Helper()
+	dir := t.TempDir()
+	path := filepath.Join(dir, "config.yaml")
+	if err := os.WriteFile(path, []byte(content), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	return path
+}
+
 func TestLoad_YAMLRoundTrip(t *testing.T) {
 	dir := t.TempDir()
 	path := filepath.Join(dir, "config.yaml")
@@ -135,3 +156,148 @@ redis:
 		t.Errorf("redis address: want redis:6379, got %q", cfg.OIDC.Redis.Address)
 	}
 }
+
+// Fix 5: callbackURL / logoutURL must start with "/"
+func TestLoad_RejectsAbsoluteCallbackURL(t *testing.T) {
+	path := writeConfig(t, `
+listen: ":8080"
+providerURL: "https://idp.example"
+clientID: "abc"
+clientSecret: "secret"
+sessionEncryptionKey: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+callbackURL: "https://app.example.com/oauth2/callback"
+logoutURL: "/oauth2/logout"
+`)
+	if _, err := Load(path); err == nil {
+		t.Fatal("callbackURL with absolute URL must be rejected")
+	}
+}
+
+func TestLoad_RejectsAbsoluteLogoutURL(t *testing.T) {
+	path := writeConfig(t, `
+listen: ":8080"
+providerURL: "https://idp.example"
+clientID: "abc"
+clientSecret: "secret"
+sessionEncryptionKey: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+callbackURL: "/oauth2/callback"
+logoutURL: "https://app.example.com/oauth2/logout"
+`)
+	if _, err := Load(path); err == nil {
+		t.Fatal("logoutURL with absolute URL must be rejected")
+	}
+}
+
+// Fix 2: excludedURLs must not prefix reserved paths
+func TestLoad_RejectsExcludedURLPrefixingReservedPath(t *testing.T) {
+	path := writeConfig(t, `
+listen: ":8080"
+providerURL: "https://idp.example"
+clientID: "abc"
+clientSecret: "secret"
+sessionEncryptionKey: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+callbackURL: "/oauth2/callback"
+logoutURL: "/oauth2/logout"
+excludedURLs: ["/"]
+`)
+	if _, err := Load(path); err == nil {
+		t.Fatal("excludedURLs: ['/'] must be rejected (bypasses all reserved paths)")
+	}
+}
+
+func TestLoad_AllowsNonOverlappingExcludedURL(t *testing.T) {
+	path := writeConfig(t, minimalYAML+`excludedURLs: ["/public"]
+`)
+	if _, err := Load(path); err != nil {
+		t.Fatalf("non-overlapping excludedURL must be accepted: %v", err)
+	}
+}
+
+// Fix 3: env override coverage — every envScalarFields entry must have a
+// matching case in setScalarField. isAllZeroForField detects drift.
+func TestEnvOverrideCoverage(t *testing.T) {
+	for _, field := range envScalarFields {
+		field := field
+		t.Run(field, func(t *testing.T) {
+			probe := "/safe/probe-" + field
+			if field == "SessionEncryptionKey" {
+				probe = "fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210"
+			}
+			if field == "LogLevel" {
+				probe = "debug"
+			}
+			if field == "ClientAuthMethod" {
+				probe = "client_secret_post"
+			}
+			if field == "ClientAssertionAlg" {
+				probe = "RS256"
+			}
+
+			var fresh Config
+			setScalarField(&fresh, field, probe)
+			if isAllZeroForField(&fresh, field, probe) {
+				t.Fatalf("envScalarFields includes %q but setScalarField has no matching case (drift)", field)
+			}
+		})
+	}
+}
+
+// isAllZeroForField returns true when setScalarField did NOT set the expected
+// field — i.e., the switch is missing a case for `field`.
+func isAllZeroForField(cfg *Config, field, probe string) bool {
+	switch field {
+	case "Listen":
+		return cfg.Listen != probe
+	case "AuthPath":
+		return cfg.AuthPath != probe
+	case "StartPath":
+		return cfg.StartPath != probe
+	case "ProviderURL":
+		return cfg.OIDC.ProviderURL != probe
+	case "ClientID":
+		return cfg.OIDC.ClientID != probe
+	case "ClientSecret":
+		return cfg.OIDC.ClientSecret != probe
+	case "Audience":
+		return cfg.OIDC.Audience != probe
+	case "CallbackURL":
+		return cfg.OIDC.CallbackURL != probe
+	case "LogoutURL":
+		return cfg.OIDC.LogoutURL != probe
+	case "PostLogoutRedirectURI":
+		return cfg.OIDC.PostLogoutRedirectURI != probe
+	case "SessionEncryptionKey":
+		return cfg.OIDC.SessionEncryptionKey != probe
+	case "CookiePrefix":
+		return cfg.OIDC.CookiePrefix != probe
+	case "CookieDomain":
+		return cfg.OIDC.CookieDomain != probe
+	case "LogLevel":
+		return cfg.OIDC.LogLevel != probe
+	case "RevocationURL":
+		return cfg.OIDC.RevocationURL != probe
+	case "OIDCEndSessionURL":
+		return cfg.OIDC.OIDCEndSessionURL != probe
+	case "UserIdentifierClaim":
+		return cfg.OIDC.UserIdentifierClaim != probe
+	case "GroupClaimName":
+		return cfg.OIDC.GroupClaimName != probe
+	case "RoleClaimName":
+		return cfg.OIDC.RoleClaimName != probe
+	case "ClientAuthMethod":
+		return cfg.OIDC.ClientAuthMethod != probe
+	case "ClientAssertionPrivateKey":
+		return cfg.OIDC.ClientAssertionPrivateKey != probe
+	case "ClientAssertionKeyPath":
+		return cfg.OIDC.ClientAssertionKeyPath != probe
+	case "ClientAssertionKeyID":
+		return cfg.OIDC.ClientAssertionKeyID != probe
+	case "ClientAssertionAlg":
+		return cfg.OIDC.ClientAssertionAlg != probe
+	case "CACertPath":
+		return cfg.OIDC.CACertPath != probe
+	case "CACertPEM":
+		return cfg.OIDC.CACertPEM != probe
+	}
+	return true // unknown field → drift
+}

cmd/oidcgate/main.go

@@ -23,6 +23,7 @@ func main() {
 	}
 
 	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
 
 	success := newSuccessHandler()
 	middleware, err := traefikoidc.NewWithContext(ctx, &cfg.OIDC, success, "oidcgate")
@@ -42,11 +43,11 @@ func main() {
 		if err := shutdown(srv); err != nil {
 			log.Printf("oidcgate: shutdown error: %v", err)
 		}
-		cancel()
 	}()
 
 	log.Printf("oidcgate: listening on %s", cfg.Listen)
 	if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
+		cancel()
 		log.Fatalf("oidcgate: serve: %v", err)
 	}
 	log.Println("oidcgate: stopped")

forwarded_uri.go

@@ -1,17 +1,37 @@
 package traefikoidc
 
-import "net/http"
+import (
+	"net/http"
+	"net/url"
+	"strings"
+)
 
 // originalRequestURI returns the request URI that should be used as the
 // post-login redirect target. When TrustForwardedURI is enabled and the
-// X-Forwarded-Uri header is present, that header wins — this lets the
+// X-Forwarded-Uri header carries a safe same-origin path, that header
-// oidcgate forward-auth daemon recover the real URL when nginx/Caddy/etc
+// wins. Otherwise (or if the header is missing/unsafe), falls back to
-// proxied a stripped subrequest to /oauth2/auth or /oauth2/start.
+// req.URL.RequestURI() — the path the request reached the proxy with.
+//
+// "Safe" means: starts with "/", does NOT start with "//" (protocol-relative
+// URLs can change host), and has no scheme or host after parsing. This
+// prevents an attacker-controllable header from triggering an open redirect
+// via http.Redirect later in the auth flow.
 func (t *TraefikOidc) originalRequestURI(req *http.Request) string {
 	if t.trustForwardedURI {
-		if v := req.Header.Get("X-Forwarded-Uri"); v != "" {
+		if v := req.Header.Get("X-Forwarded-Uri"); v != "" && isSafeRedirectTarget(v) {
 			return v
 		}
 	}
 	return req.URL.RequestURI()
 }
+
+func isSafeRedirectTarget(v string) bool {
+	if !strings.HasPrefix(v, "/") || strings.HasPrefix(v, "//") {
+		return false
+	}
+	u, err := url.Parse(v)
+	if err != nil {
+		return false
+	}
+	return u.Host == "" && u.Scheme == ""
+}

forwarded_uri_test.go

@@ -31,3 +31,39 @@ func TestOriginalRequestURI_TrustEnabledNoHeader(t *testing.T) {
 		t.Fatalf("trust-on no header: want /protected, got %q", got)
 	}
 }
+func TestOriginalRequestURI_RejectsAbsoluteURL(t *testing.T) {
+	tr := &TraefikOidc{trustForwardedURI: true}
+	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
+	req.Header.Set("X-Forwarded-Uri", "https://evil.example/phish")
+	if got := tr.originalRequestURI(req); got != "/protected" {
+		t.Fatalf("absolute URL must be rejected, want /protected fallback, got %q", got)
+	}
+}
+
+func TestOriginalRequestURI_RejectsProtocolRelative(t *testing.T) {
+	tr := &TraefikOidc{trustForwardedURI: true}
+	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
+	req.Header.Set("X-Forwarded-Uri", "//evil.example/phish")
+	if got := tr.originalRequestURI(req); got != "/protected" {
+		t.Fatalf("protocol-relative URL must be rejected, want /protected fallback, got %q", got)
+	}
+}
+
+func TestOriginalRequestURI_AcceptsSafePathWithQuery(t *testing.T) {
+	tr := &TraefikOidc{trustForwardedURI: true}
+	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
+	req.Header.Set("X-Forwarded-Uri", "/safe?x=1&y=2")
+	if got := tr.originalRequestURI(req); got != "/safe?x=1&y=2" {
+		t.Fatalf("safe path with query must be accepted, got %q", got)
+	}
+}
+
+func TestOriginalRequestURI_RejectsBareHostnameNoSlash(t *testing.T) {
+	tr := &TraefikOidc{trustForwardedURI: true}
+	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
+	req.Header.Set("X-Forwarded-Uri", "evil.example/phish")
+	if got := tr.originalRequestURI(req); got != "/protected" {
+		t.Fatalf("non-/ prefix must be rejected, got %q", got)
+	}
+}
+