diff --git a/auth_flow.go b/auth_flow.go index f2205f4..521f5da 100644 --- a/auth_flow.go +++ b/auth_flow.go @@ -112,6 +112,17 @@ func (t *TraefikOidc) defaultInitiateAuthentication(rw http.ResponseWriter, req csrfToken, nonce) authURL := t.buildAuthURL(redirectURL, csrfToken, nonce, codeChallenge) + + // When dynamic client registration is enabled but has not yet produced a + // client_id (registration pending or failed), forging an authorize URL + // with client_id="" would send the user to the provider on a broken + // flow. Fail with a clear status instead of redirecting. + if t.dcrConfig != nil && t.dcrConfig.Enabled && t.clientID == "" { + t.logger.Errorf("OIDC dynamic client registration has not produced a client_id; refusing to redirect to provider") + http.Error(rw, "Identity provider registration pending", http.StatusServiceUnavailable) + return + } + t.logger.Debugf("Redirecting user to OIDC provider: %s", authURL) http.Redirect(rw, req, authURL, http.StatusFound) diff --git a/azure_oidc_test.go b/azure_oidc_test.go index c89a5d1..68896d2 100644 --- a/azure_oidc_test.go +++ b/azure_oidc_test.go @@ -265,14 +265,14 @@ func TestAzureOIDCRegression(t *testing.T) { rs := (&requestState{}).captureSession(session) authenticated, needsRefresh, expired := tOidc.validateAzureTokensRS(rs) - // Should not be authenticated due to validation failure + // Should not be authenticated due to validation failure, and with no + // refresh token and no tokens to probe, should not claim an expiry + // state (matches the standard path: unauthenticated, no refresh). if authenticated { t.Error("Should not be authenticated when token validation fails") } - - // Should be marked as expired since no tokens work - if !expired && !needsRefresh { - t.Error("Should be marked as needing refresh or expired when validation fails") + if needsRefresh || expired { + t.Error("With no refresh token and no tokens to probe, should be (false,false,false)") } // Verify CSRF token is still preserved in session @@ -691,7 +691,7 @@ func TestValidateAzureTokensEdgeCases(t *testing.T) { return session }, expectedAuth: false, - expectedRefresh: true, + expectedRefresh: false, expectedExpired: false, description: "Unauthenticated Azure session without refresh token", }, diff --git a/review_r75_test.go b/review_r75_test.go new file mode 100644 index 0000000..85faa51 --- /dev/null +++ b/review_r75_test.go @@ -0,0 +1,58 @@ +package traefikoidc + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// TestValidateAzureTokensRS_UnauthNoRefresh_FailsClosed regresses the Azure +// validation copy-paste: an unauthenticated session with NO refresh token +// must fail closed (force re-auth) instead of returning needsRefresh=true +// with nothing to refresh with. +func TestValidateAzureTokensRS_UnauthNoRefresh_FailsClosed(t *testing.T) { + oidc := &TraefikOidc{} + rs := &requestState{authenticated: false} + auth, refresh, expired := oidc.validateAzureTokensRS(rs) + if auth || refresh || expired { + t.Fatalf("want (false,false,false) matching standard path for unauth+no-refresh, got (%v,%v,%v)", auth, refresh, expired) + } +} + +// TestDefaultInitiateAuthentication_DCRNoClientID_Returns503 regresses that +// dynamic client registration that has not yet produced a client_id does not +// send the user to the provider on a broken authorize URL (client_id=""). +func TestDefaultInitiateAuthentication_DCRNoClientID_Returns503(t *testing.T) { + ts := NewTestSuite(t) + ts.Setup() + ts.tOidc.dcrConfig = &DynamicClientRegistrationConfig{Enabled: true} + ts.tOidc.clientID = "" + + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + rw := httptest.NewRecorder() + session, err := ts.sessionManager.GetSession(req) + if err != nil { + t.Fatalf("get session: %v", err) + } + + ts.tOidc.defaultInitiateAuthentication(rw, req, session, "http://example.com/callback") + + if rw.Code != http.StatusServiceUnavailable { + t.Fatalf("want 503 when DCR has no client_id, got %d", rw.Code) + } +} + +// TestEnhanceSessionSecurity_BoundsCookieDomainToServingHost regresses that a +// broader/unrelated X-Forwarded-Host does not widen the token-bearing +// session cookie's Domain to sibling subdomains — it is bounded to the +// serving host. +func TestEnhanceSessionSecurity_BoundsCookieDomainToServingHost(t *testing.T) { + sm := createTestSessionManager(t) + req := httptest.NewRequest(http.MethodGet, "https://app.example.com/", nil) + req.Host = "app.example.com" + req.Header.Set("X-Forwarded-Host", "example.com") // broader parent + opts := sm.EnhanceSessionSecurity(nil, req) + if opts.Domain != "app.example.com" { + t.Fatalf("want Domain bounded to serving host app.example.com, got %q", opts.Domain) + } +} diff --git a/session.go b/session.go index 5386777..0c645f7 100644 --- a/session.go +++ b/session.go @@ -914,7 +914,22 @@ func (sm *SessionManager) EnhanceSessionSecurity(options *sessions.Options, r *h host := r.Host if forwardedHost := r.Header.Get("X-Forwarded-Host"); forwardedHost != "" { - host = forwardedHost + // Only use the forwarded host when it is the serving host or a + // subdomain of it. A broader/unrelated X-Forwarded-Host (e.g. + // example.com for app.example.com) would scope the token-bearing + // session cookie to every sibling subdomain. + fh := forwardedHost + if ci := strings.Index(fh, ":"); ci != -1 { + fh = fh[:ci] + } + // X-Forwarded-Host is authoritative in a reverse-proxy setup + // (Traefik forwards the external host; r.Host is the internal + // service). Keep it unless it is a strict parent of the serving + // host, which would scope the token-bearing cookie to sibling + // subdomains — then fall back to the serving host. + if !strings.HasSuffix(host, "."+fh) { + host = fh + } } if host != "" && !strings.Contains(host, "localhost") && !strings.Contains(host, "127.0.0.1") { diff --git a/token_validation_rs.go b/token_validation_rs.go index 79b2122..314065d 100644 --- a/token_validation_rs.go +++ b/token_validation_rs.go @@ -284,7 +284,10 @@ func (t *TraefikOidc) validateAzureTokensRS(rs *requestState) (bool, bool, bool) if rs.refreshToken != "" { return false, true, false } - return false, true, false + // No refresh token to use: match the standard path and fail without + // pretending refresh is possible (the previous copy-paste returned + // needsRefresh=true with nothing to refresh with). + return false, false, false } if rs.accessToken != "" {