From 2e5a90731bebf875b60c6f00ef3fec9af3181317 Mon Sep 17 00:00:00 2001 From: chengcheng Date: Wed, 29 Jul 2026 10:40:33 +0800 Subject: [PATCH] =?UTF-8?q?fix(oidc):=20=E4=BF=AE=E5=A4=8D=E7=99=BB?= =?UTF-8?q?=E5=BD=95=E4=BC=9A=E8=AF=9D=E8=90=BD=E5=BA=93=E5=85=BC=E5=AE=B9?= =?UTF-8?q?=E9=97=AE=E9=A2=98?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 部分已配对环境在 0090 首次执行后缺少 oidc_client_id，导致统一认证完成投影后无法写入 Gateway 服务端会话。新增幂等前向迁移补齐该列，并为 Token 处理及会话创建增加不泄露凭据的稳定失败分类和关联诊断。\n\n验证：OIDC Session 全量单测、HTTP 回调定向测试、迁移升级集成测试、隔离 PostgreSQL 跨仓库 OIDC E2E 和本地 Chrome 真实登录均通过。 --- apps/api/cmd/migrate/main_test.go | 28 ++++++ apps/api/internal/httpapi/oidc_session.go | 44 ++++++++- .../api/internal/httpapi/oidc_session_test.go | 97 ++++++++++++++++++- apps/api/internal/oidcsession/service.go | 51 ++++++++-- apps/api/internal/oidcsession/service_test.go | 97 ++++++++++++++++++- .../0091_oidc_session_client_identity.sql | 5 + 6 files changed, 305 insertions(+), 17 deletions(-) create mode 100644 apps/api/migrations/0091_oidc_session_client_identity.sql diff --git a/apps/api/cmd/migrate/main_test.go b/apps/api/cmd/migrate/main_test.go index 63249b3..b64ee2f 100644 --- a/apps/api/cmd/migrate/main_test.go +++ b/apps/api/cmd/migrate/main_test.go @@ -317,6 +317,34 @@ SET last_error_category='still-invalid' WHERE id=$1::uuid`, pairingID) } } +func TestOIDCSessionClientIdentityRepairMigrationUpgradesApplied0090Schema(t *testing.T) { + pool := newIdentityMigrationPostgresTestSchema(t) + ctx := context.Background() + if _, err := pool.Exec(ctx, ` +CREATE TABLE gateway_oidc_sessions ( + id uuid PRIMARY KEY DEFAULT gen_random_uuid(), + oidc_user_binding_id uuid +)`); err != nil { + t.Fatalf("create pre-repair OIDC session schema: %v", err) + } + + applyIdentityMigrationTestFile(t, ctx, pool, "../../migrations/0091_oidc_session_client_identity.sql") + applyIdentityMigrationTestFile(t, ctx, pool, "../../migrations/0091_oidc_session_client_identity.sql") + + var dataType string + if err := pool.QueryRow(ctx, ` +SELECT data_type +FROM information_schema.columns +WHERE table_schema=current_schema() + AND table_name='gateway_oidc_sessions' + AND column_name='oidc_client_id'`).Scan(&dataType); err != nil { + t.Fatalf("read repaired OIDC session client identity column: %v", err) + } + if dataType != "text" { + t.Fatalf("oidc_client_id data type=%q, want text", dataType) + } +} + func applyIdentityMigrationTestFile(t *testing.T, ctx context.Context, pool *pgxpool.Pool, path string) { t.Helper() payload, err := os.ReadFile(path) diff --git a/apps/api/internal/httpapi/oidc_session.go b/apps/api/internal/httpapi/oidc_session.go index f31ef02..d0f8c39 100644 --- a/apps/api/internal/httpapi/oidc_session.go +++ b/apps/api/internal/httpapi/oidc_session.go @@ -118,8 +118,8 @@ func (s *Server) completeOIDCLogin(w http.ResponseWriter, r *http.Request) { return } tokens, err := runtime.PublicClient.ExchangeCode(r.Context(), r.URL.Query().Get("code"), transaction.PKCEVerifier) - if err != nil || tokens.AccessToken == "" || tokens.RefreshToken == "" || tokens.IDToken == "" { - s.writeOIDCTokenFailure(w, r, "TOKEN_ENDPOINT_REJECTED", "TOKEN_RESPONSE_INVALID", "认证中心登录结果无效，请重新登录") + if stage, category, failed := classifyOIDCTokenExchangeFailure(tokens, err); failed { + s.writeOIDCTokenFailure(w, r, stage, category, "认证中心登录结果无效，请重新登录") return } identity, err := runtime.Verifier.Verify(r.Context(), tokens.AccessToken) @@ -145,7 +145,7 @@ func (s *Server) completeOIDCLogin(w http.ResponseWriter, r *http.Request) { AccessToken: tokens.AccessToken, RefreshToken: tokens.RefreshToken, IDToken: tokens.IDToken, }, projection.User) if err != nil { - s.writeOIDCCallbackError(w, r, http.StatusServiceUnavailable, "登录会话保存失败，请稍后重试", errorCodeOIDCSessionStoreUnavailable) + s.writeOIDCSessionFailure(w, r, err) return } if err := s.replaceOIDCBrowserSession(r.Context(), r, runtime, rawSession); err != nil { @@ -312,11 +312,47 @@ func (s *Server) writeOIDCTokenFailure(w http.ResponseWriter, r *http.Request, s http.StatusUnauthorized, message, errorCodeOIDCTokenExchangeFailed, - "", + validationCategory, diagnosticID, ) } +func (s *Server) writeOIDCSessionFailure(w http.ResponseWriter, r *http.Request, err error) { + category := oidcsession.SessionCreationCategory(err) + diagnosticID := newOIDCDiagnosticID() + if s.logger != nil { + s.logger.WarnContext(r.Context(), "OIDC session creation rejected", + "event", "oidc_session_creation_rejected", + "category", category, + "diagnosticId", diagnosticID, + ) + } + s.writeOIDCCallbackErrorWithDiagnostics( + w, + r, + http.StatusServiceUnavailable, + "登录会话保存失败，请稍后重试", + errorCodeOIDCSessionStoreUnavailable, + category, + diagnosticID, + ) +} + +func classifyOIDCTokenExchangeFailure(tokens auth.OIDCTokenResponse, err error) (stage, category string, failed bool) { + switch { + case err != nil: + return "TOKEN_ENDPOINT_REJECTED", "TOKEN_ENDPOINT_REQUEST_FAILED", true + case strings.TrimSpace(tokens.AccessToken) == "": + return "TOKEN_RESPONSE_INVALID", "ACCESS_TOKEN_MISSING", true + case strings.TrimSpace(tokens.RefreshToken) == "": + return "TOKEN_RESPONSE_INVALID", "REFRESH_TOKEN_MISSING", true + case strings.TrimSpace(tokens.IDToken) == "": + return "TOKEN_RESPONSE_INVALID", "ID_TOKEN_MISSING", true + default: + return "", "", false + } +} + func (s *Server) writeOIDCLoginTransactionError(w http.ResponseWriter, r *http.Request, message, reason string) { diagnosticID := newOIDCDiagnosticID() if s.logger != nil { diff --git a/apps/api/internal/httpapi/oidc_session_test.go b/apps/api/internal/httpapi/oidc_session_test.go index f123e21..c823f5a 100644 --- a/apps/api/internal/httpapi/oidc_session_test.go +++ b/apps/api/internal/httpapi/oidc_session_test.go @@ -3,6 +3,7 @@ package httpapi import ( "bytes" "context" + "errors" "io" "log/slog" "net/http" @@ -211,7 +212,7 @@ func TestOIDCTokenFailureReportsOnlyCorrelatedSafeStage(t *testing.T) { } diagnosticID := location.Query().Get("oidcDiagnosticId") if location.Query().Get("oidcError") != errorCodeOIDCTokenExchangeFailed || - location.Query().Get("oidcErrorReason") != "" || + location.Query().Get("oidcErrorReason") != "REQUIRED_SCOPE_MISSING" || diagnosticID == "" { t.Fatalf("unsafe callback diagnostic: %q", location.RawQuery) } @@ -224,6 +225,100 @@ func TestOIDCTokenFailureReportsOnlyCorrelatedSafeStage(t *testing.T) { } } +func TestOIDCSessionFailureReportsOnlyCorrelatedSafeCategory(t *testing.T) { + var logs bytes.Buffer + server := &Server{ + identityTestRevision: identity.Revision{ + ID: "diagnostic-revision", WebBaseURL: "http://localhost:5178", + }, + logger: slog.New(slog.NewJSONHandler(&logs, nil)), + } + request := httptest.NewRequest( + http.MethodGet, + "/api/v1/auth/oidc/callback?code=sensitive-code-marker", + nil, + ) + recorder := httptest.NewRecorder() + + server.writeOIDCSessionFailure( + recorder, + request, + errors.New("SESSION_REPOSITORY_FAILED"), + ) + + if recorder.Code != http.StatusSeeOther { + t.Fatalf("callback status=%d, want 303", recorder.Code) + } + location, err := url.Parse(recorder.Header().Get("Location")) + if err != nil { + t.Fatal(err) + } + diagnosticID := location.Query().Get("oidcDiagnosticId") + if location.Query().Get("oidcError") != errorCodeOIDCSessionStoreUnavailable || + location.Query().Get("oidcErrorReason") != "SESSION_CREATE_FAILED" || + diagnosticID == "" { + t.Fatalf("unsafe callback diagnostic: %q", location.RawQuery) + } + if !strings.Contains(logs.String(), `"diagnosticId":"`+diagnosticID+`"`) || + !strings.Contains(logs.String(), `"category":"SESSION_CREATE_FAILED"`) || + strings.Contains(logs.String(), "sensitive-code-marker") || + strings.Contains(location.RawQuery, "sensitive-code-marker") { + t.Fatalf("OIDC session diagnostic was missing or leaked callback material") + } +} + +func TestClassifyOIDCTokenExchangeFailure(t *testing.T) { + for _, test := range []struct { + name string + tokens auth.OIDCTokenResponse + err error + wantStage string + wantCategory string + wantFailed bool + }{ + { + name: "endpoint request failed", + err: errors.New("sensitive provider detail"), + wantStage: "TOKEN_ENDPOINT_REJECTED", + wantCategory: "TOKEN_ENDPOINT_REQUEST_FAILED", + wantFailed: true, + }, + { + name: "access token missing", + tokens: auth.OIDCTokenResponse{RefreshToken: "refresh", IDToken: "id"}, + wantStage: "TOKEN_RESPONSE_INVALID", + wantCategory: "ACCESS_TOKEN_MISSING", + wantFailed: true, + }, + { + name: "refresh token missing", + tokens: auth.OIDCTokenResponse{AccessToken: "access", IDToken: "id"}, + wantStage: "TOKEN_RESPONSE_INVALID", + wantCategory: "REFRESH_TOKEN_MISSING", + wantFailed: true, + }, + { + name: "id token missing", + tokens: auth.OIDCTokenResponse{AccessToken: "access", RefreshToken: "refresh"}, + wantStage: "TOKEN_RESPONSE_INVALID", + wantCategory: "ID_TOKEN_MISSING", + wantFailed: true, + }, + { + name: "complete response", + tokens: auth.OIDCTokenResponse{AccessToken: "access", RefreshToken: "refresh", IDToken: "id"}, + wantFailed: false, + }, + } { + t.Run(test.name, func(t *testing.T) { + stage, category, failed := classifyOIDCTokenExchangeFailure(test.tokens, test.err) + if stage != test.wantStage || category != test.wantCategory || failed != test.wantFailed { + t.Fatalf("classification=(%q, %q, %v), want (%q, %q, %v)", stage, category, failed, test.wantStage, test.wantCategory, test.wantFailed) + } + }) + } +} + func TestDeleteOIDCBrowserSessionIsIdempotentAndExpiresCookie(t *testing.T) { sessions := &fakeOIDCSessions{} server := &Server{oidcSessions: sessions, identityTestCookieSecure: true} diff --git a/apps/api/internal/oidcsession/service.go b/apps/api/internal/oidcsession/service.go index 4975fb4..1568e32 100644 --- a/apps/api/internal/oidcsession/service.go +++ b/apps/api/internal/oidcsession/service.go @@ -23,6 +23,38 @@ var ( ErrSecurityStateUnavailable = errors.New("OIDC security event state is unavailable") ) +const ( + sessionCreationPrerequisitesInvalid = "SESSION_PREREQUISITES_INVALID" + sessionCreationAccessTokenInvalid = "SESSION_ACCESS_TOKEN_INVALID" + sessionCreationIdentityMismatch = "SESSION_IDENTITY_MISMATCH" + sessionCreationTokenExpired = "SESSION_TOKEN_EXPIRED" + sessionCreationTokenGeneration = "SESSION_TOKEN_GENERATION_FAILED" + sessionCreationTokenEncryption = "SESSION_TOKEN_ENCRYPTION_FAILED" + sessionCreationRepositoryFailed = "SESSION_REPOSITORY_FAILED" +) + +type sessionCreationError struct { + category string + root error +} + +func (e *sessionCreationError) Error() string { return e.category } +func (e *sessionCreationError) Unwrap() error { return e.root } + +func newSessionCreationError(category string, root error) error { + return &sessionCreationError{category: category, root: root} +} + +// SessionCreationCategory returns a stable, non-sensitive failure category. +// It intentionally does not expose verifier, encryption, or repository errors. +func SessionCreationCategory(err error) string { + var creationError *sessionCreationError + if errors.As(err, &creationError) { + return creationError.category + } + return "SESSION_CREATE_FAILED" +} + type Repository interface { CreateOIDCSession(context.Context, store.CreateOIDCSessionInput) (store.OIDCSession, error) FindOIDCSessionByHash(context.Context, []byte) (store.OIDCSession, error) @@ -79,30 +111,33 @@ func NewService(repository Repository, cipher *Cipher, verifier TokenVerifier, c func (s *Service) Create(ctx context.Context, bundle TokenBundle, localUser *auth.User) (string, error) { if localUser == nil || localUser.GatewayUserID == "" || localUser.GatewayTenantID == "" || bundle.RefreshToken == "" { - return "", ErrSessionInvalid + return "", newSessionCreationError(sessionCreationPrerequisitesInvalid, ErrSessionInvalid) } verified, err := s.verifier.Verify(ctx, bundle.AccessToken) - if err != nil || verified == nil || verified.Source != "oidc" || verified.ID == "" || verified.ID != localUser.ID { - return "", ErrSessionInvalid + if err != nil || verified == nil || verified.Source != "oidc" || verified.ID == "" { + return "", newSessionCreationError(sessionCreationAccessTokenInvalid, ErrSessionInvalid) + } + if verified.ID != localUser.ID { + return "", newSessionCreationError(sessionCreationIdentityMismatch, ErrSessionInvalid) } if localUser.OIDCUserBindingID != "" && (localUser.Issuer == "" || localUser.ApplicationID == "" || localUser.TenantID == "" || localUser.OIDCClientID == "" || verified.Issuer != localUser.Issuer || verified.ApplicationID != localUser.ApplicationID || verified.TenantID != localUser.TenantID || verified.OIDCClientID != localUser.OIDCClientID) { - return "", ErrSessionInvalid + return "", newSessionCreationError(sessionCreationIdentityMismatch, ErrSessionInvalid) } now := s.now() if !verified.TokenExpiresAt.After(now) { - return "", ErrSessionExpired + return "", newSessionCreationError(sessionCreationTokenExpired, ErrSessionExpired) } raw, hash, err := newSessionToken() if err != nil { - return "", ErrSessionStoreUnavailable + return "", newSessionCreationError(sessionCreationTokenGeneration, ErrSessionStoreUnavailable) } aadSessionID := hex.EncodeToString(hash) ciphertext, err := s.cipher.EncryptBundle(bundle, aadSessionID, localUser.GatewayUserID) if err != nil { - return "", ErrSessionStoreUnavailable + return "", newSessionCreationError(sessionCreationTokenEncryption, ErrSessionStoreUnavailable) } _, err = s.repository.CreateOIDCSession(ctx, store.CreateOIDCSessionInput{ SessionTokenHash: hash, GatewayUserID: localUser.GatewayUserID, GatewayTenantID: localUser.GatewayTenantID, @@ -112,7 +147,7 @@ func (s *Service) Create(ctx context.Context, bundle TokenBundle, localUser *aut LastSeenAt: now, IdleExpiresAt: now.Add(s.config.IdleTTL), AbsoluteExpiresAt: now.Add(s.config.AbsoluteTTL), }) if err != nil { - return "", ErrSessionStoreUnavailable + return "", newSessionCreationError(sessionCreationRepositoryFailed, ErrSessionStoreUnavailable) } return raw, nil } diff --git a/apps/api/internal/oidcsession/service_test.go b/apps/api/internal/oidcsession/service_test.go index 93bd981..431a9e0 100644 --- a/apps/api/internal/oidcsession/service_test.go +++ b/apps/api/internal/oidcsession/service_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "strings" "sync" "sync/atomic" "testing" @@ -291,6 +292,90 @@ func TestServiceBindsMultiTenantSessionToIssuerApplicationTenantAndClient(t *tes } } +func TestServiceCreateReportsSafeFailureCategory(t *testing.T) { + now := time.Date(2026, 7, 29, 9, 0, 0, 0, time.UTC) + validVerified := &auth.User{ + ID: "subject-1", Source: "oidc", Issuer: "http://localhost:18003", + ApplicationID: "application-1", TenantID: "tenant-1", OIDCClientID: "gateway-browser", + TokenExpiresAt: now.Add(5 * time.Minute), + } + validLocal := &auth.User{ + ID: "subject-1", GatewayUserID: "11111111-1111-4111-8111-111111111111", + GatewayTenantID: "22222222-2222-4222-8222-222222222222", + OIDCUserBindingID: "44444444-4444-4444-8444-444444444444", + Issuer: validVerified.Issuer, ApplicationID: validVerified.ApplicationID, + TenantID: validVerified.TenantID, OIDCClientID: validVerified.OIDCClientID, + } + validBundle := TokenBundle{AccessToken: "access", RefreshToken: "refresh", IDToken: "id"} + + for _, test := range []struct { + name string + repository *fakeRepository + verifier fakeVerifier + bundle TokenBundle + localUser *auth.User + wantCategory string + wantRoot error + }{ + { + name: "prerequisites invalid", repository: newFakeRepository("subject-1"), + verifier: fakeVerifier{users: map[string]*auth.User{"access": validVerified}}, + bundle: validBundle, wantCategory: "SESSION_PREREQUISITES_INVALID", wantRoot: ErrSessionInvalid, + }, + { + name: "access token invalid", repository: newFakeRepository("subject-1"), + verifier: fakeVerifier{users: map[string]*auth.User{}}, + bundle: validBundle, localUser: validLocal, + wantCategory: "SESSION_ACCESS_TOKEN_INVALID", wantRoot: ErrSessionInvalid, + }, + { + name: "identity mismatch", repository: newFakeRepository("subject-1"), + verifier: fakeVerifier{users: map[string]*auth.User{"access": validVerified}}, + bundle: validBundle, localUser: func() *auth.User { + copy := *validLocal + copy.OIDCClientID = "different-client" + return © + }(), + wantCategory: "SESSION_IDENTITY_MISMATCH", wantRoot: ErrSessionInvalid, + }, + { + name: "token expired", repository: newFakeRepository("subject-1"), + verifier: fakeVerifier{users: map[string]*auth.User{"access": func() *auth.User { + copy := *validVerified + copy.TokenExpiresAt = now + return © + }()}}, + bundle: validBundle, localUser: validLocal, + wantCategory: "SESSION_TOKEN_EXPIRED", wantRoot: ErrSessionExpired, + }, + { + name: "repository failed", repository: &fakeRepository{ + external: "subject-1", createError: errors.New("sensitive database detail"), + }, + verifier: fakeVerifier{users: map[string]*auth.User{"access": validVerified}}, + bundle: validBundle, localUser: validLocal, + wantCategory: "SESSION_REPOSITORY_FAILED", wantRoot: ErrSessionStoreUnavailable, + }, + } { + t.Run(test.name, func(t *testing.T) { + service := newTestService(t, test.repository, test.verifier, &fakePublicClient{}) + service.now = func() time.Time { return now } + + _, err := service.Create(context.Background(), test.bundle, test.localUser) + + if !errors.Is(err, test.wantRoot) { + t.Fatalf("Create() error = %v, want root %v", err, test.wantRoot) + } + if category := SessionCreationCategory(err); category != test.wantCategory { + t.Fatalf("SessionCreationCategory() = %q, want %q", category, test.wantCategory) + } + if strings.Contains(err.Error(), "sensitive database detail") { + t.Fatal("session creation error exposed repository detail") + } + }) + } +} + func testLocalUser() *auth.User { return &auth.User{ ID: "subject-1", GatewayUserID: "11111111-1111-4111-8111-111111111111", @@ -345,10 +430,11 @@ func (f *fakePublicClient) EndSessionURL(context.Context, string) (string, error } type fakeRepository struct { - mu sync.Mutex - record store.OIDCSession - deleted bool - external string + mu sync.Mutex + record store.OIDCSession + deleted bool + external string + createError error } func newFakeRepository(external string) *fakeRepository { return &fakeRepository{external: external} } @@ -356,6 +442,9 @@ func newFakeRepository(external string) *fakeRepository { return &fakeRepository func (f *fakeRepository) CreateOIDCSession(_ context.Context, input store.CreateOIDCSessionInput) (store.OIDCSession, error) { f.mu.Lock() defer f.mu.Unlock() + if f.createError != nil { + return store.OIDCSession{}, f.createError + } f.record = store.OIDCSession{ ID: "33333333-3333-4333-8333-333333333333", SessionTokenHash: append([]byte(nil), input.SessionTokenHash...), GatewayUserID: input.GatewayUserID, GatewayTenantID: input.GatewayTenantID, ExternalUserID: f.external, diff --git a/apps/api/migrations/0091_oidc_session_client_identity.sql b/apps/api/migrations/0091_oidc_session_client_identity.sql new file mode 100644 index 0000000..021a1a8 --- /dev/null +++ b/apps/api/migrations/0091_oidc_session_client_identity.sql @@ -0,0 +1,5 @@ +-- 0090 was already applied by some local installations before oidc_client_id +-- was added to that migration. Keep migrations immutable and repair those +-- installations with a forward-only, idempotent migration. +ALTER TABLE gateway_oidc_sessions + ADD COLUMN IF NOT EXISTS oidc_client_id text;