From 360e7449adee93b12cdc5336d3ac455a31a9629c Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Mon, 11 May 2026 15:44:39 +0000 Subject: [PATCH] fix(oidc/integration): pass fx.IssuerURL as callbackIss arg in 7 HandleCallback call sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase-10 live-IdP smoke (post-Enabled-true fix landing in 1b52998) surfaced the next layer: 5 of 6 testcontainers-Keycloak integration tests failed with 'oidc: provider advertises iss-parameter support but callback omitted it'. Root cause: Keycloak's discovery doc advertises authorization_response_iss_parameter_supported=true. The Audit 2026-05-10 MED-17 closure (RFC 9207) gates the callback path: when the IdP advertises iss-param support, HandleCallback requires a non-empty callbackIss arg that matches the provider's IssuerURL, else ErrIssParamMissing. The 7 HandleCallback call sites in the integration tests were passing '' for the callbackIss arg — the synthetic test code never simulated the real browser's '?iss=' query param. Fix: replace '' with fx.IssuerURL at all 7 sites: - integration_keycloak_test.go: 5 sites (TestKeycloakIntegration_AuthCodeFlow_HappyPath, TestKeycloakIntegration_LogoutRevokesSession, TestKeycloakIntegration_JWKSRotation_RefreshKeysPicksUpNewKey pre+post HandleCallback, TestKeycloakIntegration_UnmappedGroupsFailsClosed) - integration_keycloak_rotate_test.go: 2 sites (TestKeycloakIntegration_MED6_AutoRefreshOnKidMiss pre+post) Inline note on the first site explains the rationale so future test-writers don't drop back to ''. Verify (sandbox): go vet -tags=integration ./internal/auth/oidc/... clean; gofmt clean; grep for remaining empty-iss callsites returns 0 matches. Workstation re-runs 'make keycloak-integration-test' to confirm the 5 affected tests advance past the iss-param check against a real Keycloak 26.x. --- .../oidc/integration_keycloak_rotate_test.go | 4 ++-- internal/auth/oidc/integration_keycloak_test.go | 17 ++++++++++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/internal/auth/oidc/integration_keycloak_rotate_test.go b/internal/auth/oidc/integration_keycloak_rotate_test.go index 9a38047..a6ac265 100644 --- a/internal/auth/oidc/integration_keycloak_rotate_test.go +++ b/internal/auth/oidc/integration_keycloak_rotate_test.go @@ -73,7 +73,7 @@ func TestKeycloakIntegration_MED6_AutoRefreshOnKidMiss(t *testing.T) { t.Fatalf("pre-rotate HandleAuthRequest: %v", err) } preCode, preState := driveAuthCodeFlow(t, preAuthURL, testfixtures.EngineerUser, testfixtures.EngineerPassword) - if _, err := svc.HandleCallback(ctx, preCookie, preCode, preState, "", "ip", "ua"); err != nil { + if _, err := svc.HandleCallback(ctx, preCookie, preCode, preState, fx.IssuerURL, "ip", "ua"); err != nil { t.Fatalf("pre-rotate HandleCallback (priming): %v", err) } @@ -92,7 +92,7 @@ func TestKeycloakIntegration_MED6_AutoRefreshOnKidMiss(t *testing.T) { t.Fatalf("post-rotate HandleAuthRequest: %v", err) } postCode, postState := driveAuthCodeFlow(t, postAuthURL, testfixtures.EngineerUser, testfixtures.EngineerPassword) - res, err := svc.HandleCallback(ctx, postCookie, postCode, postState, "", "ip", "ua") + res, err := svc.HandleCallback(ctx, postCookie, postCode, postState, fx.IssuerURL, "ip", "ua") if err != nil { t.Fatalf("post-rotate HandleCallback (expected MED-6 auto-refresh): %v", err) } diff --git a/internal/auth/oidc/integration_keycloak_test.go b/internal/auth/oidc/integration_keycloak_test.go index e06cc24..11499d4 100644 --- a/internal/auth/oidc/integration_keycloak_test.go +++ b/internal/auth/oidc/integration_keycloak_test.go @@ -436,7 +436,13 @@ func TestKeycloakIntegration_AuthCodeFlow_HappyPath(t *testing.T) { code, state := driveAuthCodeFlow(t, authURL, testfixtures.EngineerUser, testfixtures.EngineerPassword) // Complete the OIDC handshake. - res, err := svc.HandleCallback(ctx, preLoginCookie, code, state, "", "10.0.0.1", "integration-test/1.0") + // Keycloak's discovery doc advertises authorization_response_iss_ + // parameter_supported=true (RFC 9207). The Audit 2026-05-10 MED-17 + // callback path requires a non-empty `iss` query param that matches + // the provider's IssuerURL; pass fx.IssuerURL here so the check + // passes (the synthetic callback simulates what a real browser + // would have received as the `iss=...` query param). + res, err := svc.HandleCallback(ctx, preLoginCookie, code, state, fx.IssuerURL, "10.0.0.1", "integration-test/1.0") if err != nil { t.Fatalf("HandleCallback: %v", err) } @@ -495,7 +501,8 @@ func TestKeycloakIntegration_LogoutRevokesSession(t *testing.T) { t.Fatalf("HandleAuthRequest: %v", err) } code, state := driveAuthCodeFlow(t, authURL, testfixtures.EngineerUser, testfixtures.EngineerPassword) - res, err := svc.HandleCallback(ctx, preLoginCookie, code, state, "", "ip", "ua") + // RFC 9207 iss param required per Keycloak's discovery doc (MED-17). + res, err := svc.HandleCallback(ctx, preLoginCookie, code, state, fx.IssuerURL, "ip", "ua") if err != nil { t.Fatalf("HandleCallback: %v", err) } @@ -538,7 +545,7 @@ func TestKeycloakIntegration_JWKSRotation_RefreshKeysPicksUpNewKey(t *testing.T) t.Fatalf("pre-rotate HandleAuthRequest: %v", err) } preCode, preState := driveAuthCodeFlow(t, preAuthURL, testfixtures.EngineerUser, testfixtures.EngineerPassword) - if _, err := svc.HandleCallback(ctx, preCookie, preCode, preState, "", "ip", "ua"); err != nil { + if _, err := svc.HandleCallback(ctx, preCookie, preCode, preState, fx.IssuerURL, "ip", "ua"); err != nil { t.Fatalf("pre-rotate HandleCallback: %v", err) } @@ -557,7 +564,7 @@ func TestKeycloakIntegration_JWKSRotation_RefreshKeysPicksUpNewKey(t *testing.T) t.Fatalf("post-rotate HandleAuthRequest: %v", err) } postCode, postState := driveAuthCodeFlow(t, postAuthURL, testfixtures.EngineerUser, testfixtures.EngineerPassword) - if _, err := svc.HandleCallback(ctx, postCookie, postCode, postState, "", "ip", "ua"); err != nil { + if _, err := svc.HandleCallback(ctx, postCookie, postCode, postState, fx.IssuerURL, "ip", "ua"); err != nil { t.Fatalf("post-rotate HandleCallback: %v (rotation broke validation?)", err) } } @@ -582,7 +589,7 @@ func TestKeycloakIntegration_UnmappedGroupsFailsClosed(t *testing.T) { t.Fatalf("HandleAuthRequest: %v", err) } code, state := driveAuthCodeFlow(t, authURL, testfixtures.ViewerUser, testfixtures.ViewerPassword) - _, err = svc.HandleCallback(ctx, preCookie, code, state, "", "ip", "ua") + _, err = svc.HandleCallback(ctx, preCookie, code, state, fx.IssuerURL, "ip", "ua") if !errors.Is(err, oidc.ErrGroupsUnmapped) { t.Errorf("HandleCallback err = %v, want ErrGroupsUnmapped (fail-closed for unmapped groups)", err) }