diff --git a/CubeOps/internal/auth/handler.go b/CubeOps/internal/auth/handler.go index 6da2c0891..99c431c39 100644 --- a/CubeOps/internal/auth/handler.go +++ b/CubeOps/internal/auth/handler.go @@ -64,7 +64,7 @@ func (h *Handler) Login(c *gin.Context) { return } logging.G(c.Request.Context()).Errorf("login failed: internal error: username=%q client_ip=%s error=%q", req.Username, c.ClientIP(), err.Error()) - httputil.WriteError(c, http.StatusInternalServerError, err.Error()) + httputil.WriteError(c, http.StatusInternalServerError, "internal server error") return } httputil.WriteJSON(c, http.StatusOK, model.LoginResponse{ @@ -120,7 +120,7 @@ func (h *Handler) ChangePassword(c *gin.Context) { logging.G(c.Request.Context()).Errorf("password change failed: operator=%q result=error error=%q", username, err.Error()) // Validation errors and DB errors share the 500 path here; finer // mapping can be added by wrapping with sentinel errors if needed. - httputil.WriteError(c, http.StatusInternalServerError, err.Error()) + httputil.WriteError(c, http.StatusInternalServerError, "internal server error") } } @@ -137,7 +137,8 @@ func (h *Handler) Refresh(c *gin.Context) { httputil.WriteError(c, http.StatusUnauthorized, "invalid or expired refresh token") return } - httputil.WriteError(c, http.StatusInternalServerError, err.Error()) + logging.G(c.Request.Context()).Errorf("token refresh failed: internal error: client_ip=%s error=%q", c.ClientIP(), err.Error()) + httputil.WriteError(c, http.StatusInternalServerError, "internal server error") return } httputil.WriteJSON(c, http.StatusOK, model.RefreshResponse{ diff --git a/CubeOps/internal/auth/handler_error_leak_test.go b/CubeOps/internal/auth/handler_error_leak_test.go new file mode 100644 index 000000000..954544f4f --- /dev/null +++ b/CubeOps/internal/auth/handler_error_leak_test.go @@ -0,0 +1,110 @@ +// Copyright (c) 2026 Tencent Inc. +// SPDX-License-Identifier: Apache-2.0 + +package auth_test + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/tencentcloud/CubeSandbox/CubeOps/internal/auth" + "github.com/tencentcloud/CubeSandbox/CubeOps/internal/service" +) + +const dbErrorDetail = "dial tcp 10.0.0.5:3306: connect: connection refused" + +type outageUserStore struct{ fakeUserStore } + +func (o *outageUserStore) GetUserPassword(_ context.Context, _ string) (string, error) { + return "", errors.New(dbErrorDetail) +} + +func (o *outageUserStore) IsRefreshTokenRevoked(_ context.Context, _ string) (bool, error) { + return false, errors.New(dbErrorDetail) +} + +func newOutageRouter(t *testing.T) *gin.Engine { + t.Helper() + jm := auth.NewJWTManager("test-secret-32-bytes-long-enough!", 15*time.Minute, 168*time.Hour) + svc := service.NewAuthService(&outageUserStore{}, jm) + h := auth.NewHandler(svc) + + r := gin.New() + h.RegisterPublic(r.Group("/api/v1")) + h.RegisterAuthed(r.Group("/api/v1", auth.Middleware(jm))) + return r +} + +func TestLoginOutageDoesNotLeakDatabaseDetailsToTheCaller(t *testing.T) { + r := newOutageRouter(t) + + w := doRequest(t, r, "POST", "/api/v1/auth/login", + `{"username":"admin","password":"s3cret"}`, "") + + if w.Code != 500 { + t.Fatalf("status = %d, want 500", w.Code) + } + body := w.Body.String() + for _, secret := range []string{dbErrorDetail, "10.0.0.5", "3306", "dial tcp", "connection refused"} { + if strings.Contains(body, secret) { + t.Errorf("500 body leaks %q to an unauthenticated caller: %s", secret, body) + } + } + if !strings.Contains(body, "internal server error") { + t.Errorf("500 body = %s, want a generic message", body) + } + if strings.Contains(body, "required") { + t.Errorf("the request never reached the database path: %s", body) + } +} + +func TestChangePasswordOutageDoesNotLeakDatabaseDetailsToTheCaller(t *testing.T) { + jm := auth.NewJWTManager("test-secret-32-bytes-long-enough!", 15*time.Minute, 168*time.Hour) + token, err := jm.GenerateAccessToken("admin") + if err != nil { + t.Fatalf("GenerateAccessToken: %v", err) + } + + r := newOutageRouter(t) + w := doRequest(t, r, "POST", "/api/v1/auth/change-password", + `{"old_password":"s3cret","new_password":"n3wpass"}`, token) + + if w.Code != 500 { + t.Fatalf("status = %d, want 500 (body: %s)", w.Code, w.Body.String()) + } + body := w.Body.String() + for _, secret := range []string{dbErrorDetail, "10.0.0.5", "3306", "dial tcp", "connection refused"} { + if strings.Contains(body, secret) { + t.Errorf("500 body leaks %q to the caller: %s", secret, body) + } + } +} + +func TestRefreshOutageDoesNotLeakDatabaseDetailsToTheCaller(t *testing.T) { + jm := auth.NewJWTManager("test-secret-32-bytes-long-enough!", 15*time.Minute, 168*time.Hour) + refresh, _, err := jm.GenerateRefreshToken("admin") + if err != nil { + t.Fatalf("GenerateRefreshToken: %v", err) + } + + r := newOutageRouter(t) + w := doRequest(t, r, "POST", "/api/v1/auth/refresh", + `{"refreshToken":"`+refresh+`"}`, "") + + if w.Code != 500 { + t.Fatalf("status = %d, want 500 (body: %s)", w.Code, w.Body.String()) + } + body := w.Body.String() + for _, secret := range []string{dbErrorDetail, "10.0.0.5", "3306", "dial tcp", "connection refused"} { + if strings.Contains(body, secret) { + t.Errorf("500 body leaks %q to the caller: %s", secret, body) + } + } + if !strings.Contains(body, "internal server error") { + t.Errorf("500 body = %s, want a generic message", body) + } +} diff --git a/CubeOps/internal/handler/agenthub.go b/CubeOps/internal/handler/agenthub.go index bd521aa91..35aecf076 100644 --- a/CubeOps/internal/handler/agenthub.go +++ b/CubeOps/internal/handler/agenthub.go @@ -64,7 +64,7 @@ func writeServiceError(c *gin.Context, err error, instanceID string) { ctx = updateTraceAndLogger(ctx, cubelog.GetTraceInfo(ctx), instanceID, "", 0) } logging.G(ctx).Errorf("agenthub: handler failed: %v", err) - httputil.WriteError(c, http.StatusInternalServerError, err.Error()) + httputil.WriteError(c, http.StatusInternalServerError, "internal server error") return } status := svcErr.Status diff --git a/CubeOps/internal/service/auth.go b/CubeOps/internal/service/auth.go index 7afb5f86a..9d078aec8 100644 --- a/CubeOps/internal/service/auth.go +++ b/CubeOps/internal/service/auth.go @@ -80,16 +80,9 @@ func (s *AuthService) Login(ctx context.Context, username, password string) (*Lo } stored, err := s.store.GetUserPassword(ctx, username) if err != nil { - // Distinguish "user not found" (→ ErrInvalidCredentials, safe to - // expose) from infrastructure errors (→ return verbatim). We do this - // by checking whether stored is empty — the store layer returns "" + - // a "not found" error when the row is missing. - if stored == "" { - return nil, ErrInvalidCredentials - } return nil, fmt.Errorf("failed to read user: %w", err) } - if !crypto.VerifyPassword(stored, password) { + if stored == "" || !crypto.VerifyPassword(stored, password) { return nil, ErrInvalidCredentials } accessToken, err := s.jm.GenerateAccessToken(username) @@ -180,7 +173,7 @@ func (s *AuthService) ChangePassword(ctx context.Context, username, oldPassword, if err != nil { return fmt.Errorf("failed to read user: %w", err) } - if !crypto.VerifyPassword(stored, oldPassword) { + if stored == "" || !crypto.VerifyPassword(stored, oldPassword) { return ErrInvalidOldPassword } newHash, err := crypto.HashPassword(newPassword) diff --git a/CubeOps/internal/service/auth_db_error_test.go b/CubeOps/internal/service/auth_db_error_test.go new file mode 100644 index 000000000..714acd885 --- /dev/null +++ b/CubeOps/internal/service/auth_db_error_test.go @@ -0,0 +1,81 @@ +// Copyright (c) 2026 Tencent Inc. +// SPDX-License-Identifier: Apache-2.0 + +package service + +import ( + "context" + "errors" + "testing" + "time" +) + +type dbErrorStore struct { + err error +} + +func (d dbErrorStore) GetUserPassword(context.Context, string) (string, error) { + return "", d.err +} +func (d dbErrorStore) SetUserPassword(context.Context, string, string) error { return nil } +func (d dbErrorStore) CreateRefreshToken(context.Context, string, string) error { return nil } +func (d dbErrorStore) IsRefreshTokenRevoked(context.Context, string) (bool, error) { return false, nil } +func (d dbErrorStore) RevokeRefreshToken(context.Context, string) error { return nil } +func (d dbErrorStore) RevokeAllRefreshTokensForUser(context.Context, string) error { return nil } + +type stubIssuer struct{} + +func (stubIssuer) GenerateAccessToken(string) (string, error) { return "a", nil } +func (stubIssuer) GenerateRefreshToken(string) (string, string, error) { return "r", "t", nil } +func (stubIssuer) VerifyRefreshToken(string) (*RefreshClaims, error) { return &RefreshClaims{}, nil } +func (stubIssuer) AccessTTL() time.Duration { return time.Minute } + +func TestLoginSurfacesInfrastructureError(t *testing.T) { + dbDown := errors.New("dial tcp 10.0.0.5:3306: connect: connection refused") + svc := NewAuthService(dbErrorStore{err: dbDown}, stubIssuer{}) + + _, err := svc.Login(context.Background(), "admin", "hunter2") + if err == nil { + t.Fatal("Login returned no error while the database was unreachable") + } + if errors.Is(err, ErrInvalidCredentials) { + t.Fatalf("Login masked a database outage as invalid credentials: %v", err) + } + if !errors.Is(err, dbDown) { + t.Fatalf("Login did not wrap the underlying error, got %v", err) + } +} + +func TestLoginUnknownUserStillReportsInvalidCredentials(t *testing.T) { + svc := NewAuthService(dbErrorStore{err: nil}, stubIssuer{}) + + _, err := svc.Login(context.Background(), "nobody", "hunter2") + if !errors.Is(err, ErrInvalidCredentials) { + t.Fatalf("unknown user should report invalid credentials, got %v", err) + } +} + +func TestChangePasswordSurfacesInfrastructureError(t *testing.T) { + dbDown := errors.New("dial tcp 10.0.0.5:3306: connect: connection refused") + svc := NewAuthService(dbErrorStore{err: dbDown}, stubIssuer{}) + + err := svc.ChangePassword(context.Background(), "admin", "old-pass", "new-pass") + if err == nil { + t.Fatal("ChangePassword returned no error while the database was unreachable") + } + if errors.Is(err, ErrInvalidOldPassword) { + t.Fatalf("ChangePassword masked a database outage as a bad old password: %v", err) + } + if !errors.Is(err, dbDown) { + t.Fatalf("ChangePassword discarded the underlying database error: %v", err) + } +} + +func TestChangePasswordUnknownUserReportsBadOldPassword(t *testing.T) { + svc := NewAuthService(dbErrorStore{err: nil}, stubIssuer{}) + + err := svc.ChangePassword(context.Background(), "nobody", "old-pass", "new-pass") + if !errors.Is(err, ErrInvalidOldPassword) { + t.Fatalf("unknown user should report a bad old password, got %v", err) + } +} diff --git a/CubeOps/internal/service/auth_test.go b/CubeOps/internal/service/auth_test.go index 8a6ba3138..4fb628eeb 100644 --- a/CubeOps/internal/service/auth_test.go +++ b/CubeOps/internal/service/auth_test.go @@ -23,7 +23,7 @@ type fakeUserStore struct { func (f *fakeUserStore) GetUserPassword(_ context.Context, username string) (string, error) { pw, ok := f.passwords[username] if !ok { - return "", errors.New("user not found") + return "", nil } return pw, nil } diff --git a/CubeOps/internal/store/db.go b/CubeOps/internal/store/db.go index 8178056ff..7ad561484 100644 --- a/CubeOps/internal/store/db.go +++ b/CubeOps/internal/store/db.go @@ -87,9 +87,12 @@ func (s *Store) bootstrapMasterKey(ctx context.Context) error { if b64 == "" { // 2. Fallback: read from the old agenthub settings table. - // This covers the upgrade window where the migration has run - // (key copied to t_system_setting) but also the case where - // CubeOps starts against a DB that hasn't been migrated yet. + // This covers the upgrade window where the migration has run but + // the key has not been copied into t_system_setting yet, plus the + // rollback case where only the old table holds it. An unmigrated DB + // no longer reaches here: the read above returns a real + // table-not-found error and startup fails, as it would anyway at + // seedDefaultAdmin. b64, err = s.GetSetting(ctx, "secret_master_key") if err != nil { return fmt.Errorf("read master key from t_agenthub_setting: %w", err) diff --git a/CubeOps/internal/store/setting.go b/CubeOps/internal/store/setting.go index bc0306705..dc35eea94 100644 --- a/CubeOps/internal/store/setting.go +++ b/CubeOps/internal/store/setting.go @@ -17,14 +17,17 @@ const settingMasterKey = "secret_master_key" // GetSystemSetting retrieves a system-level setting value by key. func (s *Store) GetSystemSetting(ctx context.Context, key string) (string, error) { - var val string + var val sql.NullString err := s.db.WithContext(ctx).Raw( "SELECT setting_value FROM t_system_setting WHERE setting_key = ? LIMIT 1", key, - ).Scan(&val).Error - if errors.Is(err, sql.ErrNoRows) || val == "" { - return "", nil + ).Row().Scan(&val) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return "", nil + } + return "", err } - return val, err + return val.String, nil } // GetOrCreateSystemSetting atomically gets an existing system setting or @@ -52,14 +55,17 @@ func (s *Store) SetSystemSetting(ctx context.Context, key, value string) error { // GetSetting retrieves an AgentHub-level setting value by key. func (s *Store) GetSetting(ctx context.Context, key string) (string, error) { - var val string + var val sql.NullString err := s.db.WithContext(ctx).Raw( "SELECT setting_value FROM t_agenthub_setting WHERE setting_key = ? LIMIT 1", key, - ).Scan(&val).Error - if errors.Is(err, sql.ErrNoRows) || val == "" { - return "", nil + ).Row().Scan(&val) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return "", nil + } + return "", err } - return val, err + return val.String, nil } // GetOrCreateSetting atomically gets an existing setting or creates it with the given value. @@ -104,14 +110,17 @@ func (s *Store) SetSettingsTx(ctx context.Context, kv map[string]string) error { // GetUserPassword retrieves the stored password hash for a user. func (s *Store) GetUserPassword(ctx context.Context, username string) (string, error) { - var pwd string + var pwd sql.NullString err := s.db.WithContext(ctx).Raw( "SELECT password FROM t_system_user WHERE username = ? LIMIT 1", username, - ).Scan(&pwd).Error - if errors.Is(err, sql.ErrNoRows) || pwd == "" { - return "", nil + ).Row().Scan(&pwd) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return "", nil + } + return "", err } - return pwd, err + return pwd.String, nil } // SetUserPassword updates the password hash for a user. diff --git a/CubeOps/internal/store/setting_contract_test.go b/CubeOps/internal/store/setting_contract_test.go new file mode 100644 index 000000000..d06f58633 --- /dev/null +++ b/CubeOps/internal/store/setting_contract_test.go @@ -0,0 +1,104 @@ +// Copyright (c) 2026 Tencent Inc. +// SPDX-License-Identifier: Apache-2.0 + +package store_test + +import ( + "context" + "testing" +) + +func TestGettersReturnEmptyAndNilWhenTheRowIsAbsent(t *testing.T) { + env := newTestStore(t) + defer env.teardown() + s := env.store + ctx := context.Background() + + cases := []struct { + name string + get func() (string, error) + }{ + {"GetSystemSetting", func() (string, error) { return s.GetSystemSetting(ctx, "no-such-system-setting") }}, + {"GetSetting", func() (string, error) { return s.GetSetting(ctx, "no-such-agenthub-setting") }}, + {"GetUserPassword", func() (string, error) { return s.GetUserPassword(ctx, "no-such-user") }}, + } + for _, c := range cases { + got, err := c.get() + if err != nil { + t.Errorf("%s on a missing row returned an error: %v", c.name, err) + } + if got != "" { + t.Errorf("%s on a missing row returned %q, want empty", c.name, got) + } + } +} + +func TestGettersPropagateDriverErrors(t *testing.T) { + env := newTestStore(t) + defer env.teardown() + s := env.store + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + cases := []struct { + name string + get func() (string, error) + }{ + {"GetSystemSetting", func() (string, error) { return s.GetSystemSetting(ctx, "secret_master_key") }}, + {"GetSetting", func() (string, error) { return s.GetSetting(ctx, "any") }}, + {"GetUserPassword", func() (string, error) { return s.GetUserPassword(ctx, "admin") }}, + } + for _, c := range cases { + got, err := c.get() + if err == nil { + t.Errorf("%s swallowed a driver error and returned %q with no error", c.name, got) + } + if got != "" { + t.Errorf("%s returned %q alongside an error, want empty", c.name, got) + } + } +} + +func TestGetUserPasswordReturnsTheStoredHashForTheSeededAdmin(t *testing.T) { + env := newTestStore(t) + defer env.teardown() + ctx := context.Background() + + got, err := env.store.GetUserPassword(ctx, "admin") + if err != nil { + t.Fatalf("GetUserPassword: %v", err) + } + if got == "" { + t.Fatal("GetUserPassword returned an empty hash for the seeded admin account") + } +} + +func TestGettersReportAnEmptyStoredValueAsAbsent(t *testing.T) { + env := newTestStore(t) + defer env.teardown() + s := env.store + ctx := context.Background() + + if err := s.SetSystemSetting(ctx, "empty-on-purpose", ""); err != nil { + t.Fatalf("SetSystemSetting: %v", err) + } + got, err := s.GetSystemSetting(ctx, "empty-on-purpose") + if err != nil { + t.Fatalf("GetSystemSetting on an empty stored value returned an error: %v", err) + } + if got != "" { + t.Fatalf("GetSystemSetting = %q, want empty", got) + } + + if err := s.SetSetting(ctx, "empty-on-purpose", ""); err != nil { + t.Fatalf("SetSetting: %v", err) + } + got, err = s.GetSetting(ctx, "empty-on-purpose") + if err != nil { + t.Fatalf("GetSetting on an empty stored value returned an error: %v", err) + } + if got != "" { + t.Fatalf("GetSetting = %q, want empty", got) + } +}