diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index bf924201a..0ce5a4ef7 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -503,26 +503,27 @@ var ( ) // mapMFAServiceError maps a service-layer MFA error to the right -// client-facing status + message. Service errors land here as plain -// fmt.Errorf strings (the auth package doesn't export every shape as -// a sentinel because the handler is the only consumer that needs -// status-code-level discrimination). Substring matching is OK because -// the auth package owns these strings and tests pin them. +// client-facing status + message. The auth package exports typed +// sentinel errors for each user-correctable condition; we match via +// errors.Is so a renamed error message in the service never silently +// drifts the HTTP status. See issue #512. func mapMFAServiceError(err error) error { if err == nil { return nil } - msg := err.Error() switch { - case strings.Contains(msg, "invalid password"), - strings.Contains(msg, "invalid MFA code"), - strings.Contains(msg, "MFA code or recovery code required"), - strings.Contains(msg, "no MFA enrollment in progress"), - strings.Contains(msg, "MFA enrollment expired"), - strings.Contains(msg, "MFA is not enabled"): - return NewClientError(400, msg) - case strings.Contains(msg, "authentication failed"): - return NewClientError(401, msg) + case errors.Is(err, auth.ErrMFAInvalidPassword), + errors.Is(err, auth.ErrMFAInvalidCode), + errors.Is(err, auth.ErrMFACodeRequired), + errors.Is(err, auth.ErrMFANoEnrollmentInProgress), + errors.Is(err, auth.ErrMFAEnrollmentExpired), + errors.Is(err, auth.ErrMFANotEnabled): + return NewClientError(400, err.Error()) + case errors.Is(err, auth.ErrMFAAuthFailed): + // Opaque 401 to prevent user enumeration: the service returns this + // for both "user not found" and "DB lookup failed" paths so callers + // cannot distinguish the two. + return NewClientError(401, err.Error()) } return err } diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index 1ebea1c26..70d170b31 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "errors" + "fmt" "testing" "github.com/LeanerCloud/CUDly/internal/auth" @@ -1139,6 +1140,7 @@ func TestHandler_login_MFARequired_ReturnsMFASentinel(t *testing.T) { require.True(t, ok) assert.Equal(t, 401, ce.code) assert.Equal(t, "mfa_required", ce.Error()) + mockAuth.AssertExpectations(t) } func TestHandler_login_InvalidMFACode_ReturnsCodedSentinel(t *testing.T) { @@ -1156,6 +1158,7 @@ func TestHandler_login_InvalidMFACode_ReturnsCodedSentinel(t *testing.T) { require.True(t, ok) assert.Equal(t, 401, ce.code) assert.Equal(t, "invalid_mfa_code", ce.Error()) + mockAuth.AssertExpectations(t) } func TestHandler_mfaSetup_HappyPath(t *testing.T) { @@ -1171,6 +1174,7 @@ func TestHandler_mfaSetup_HappyPath(t *testing.T) { require.NoError(t, err) assert.Equal(t, "SECRET123", resp.Secret) assert.Contains(t, resp.ProvisioningURI, "otpauth://") + mockAuth.AssertExpectations(t) } func TestHandler_mfaSetup_WrongPassword(t *testing.T) { @@ -1178,8 +1182,11 @@ func TestHandler_mfaSetup_WrongPassword(t *testing.T) { mockAuth := new(MockAuthService) session := &Session{UserID: "user-1", Email: "u@x.com"} mockAuth.On("ValidateSession", ctx, "tok").Return(session, nil) + // The real service returns a wrapped sentinel; the handler maps it to a + // 400 ClientError via errors.Is (issue #512). The mock must return the + // same sentinel so the errors.Is check in mapMFAServiceError fires. mockAuth.On("MFASetupAPI", ctx, "user-1", "wrong"). - Return("", "", errors.New("invalid password")) + Return("", "", fmt.Errorf("%w", auth.ErrMFAInvalidPassword)) handler := &Handler{auth: mockAuth} _, err := handler.mfaSetup(ctx, authedReq("tok", `{"password":"`+b64("wrong")+`"}`)) @@ -1188,6 +1195,7 @@ func TestHandler_mfaSetup_WrongPassword(t *testing.T) { require.True(t, ok) assert.Equal(t, 400, ce.code) assert.Contains(t, ce.Error(), "invalid password") + mockAuth.AssertExpectations(t) } func TestHandler_mfaEnable_HappyPath(t *testing.T) { @@ -1202,6 +1210,7 @@ func TestHandler_mfaEnable_HappyPath(t *testing.T) { resp, err := handler.mfaEnable(ctx, authedReq("tok", `{"code":"123456"}`)) require.NoError(t, err) assert.Len(t, resp.RecoveryCodes, 2) + mockAuth.AssertExpectations(t) } func TestHandler_mfaEnable_NoSession(t *testing.T) { @@ -1215,6 +1224,7 @@ func TestHandler_mfaEnable_NoSession(t *testing.T) { ce, ok := IsClientError(err) require.True(t, ok) assert.Equal(t, 401, ce.code) + mockAuth.AssertExpectations(t) } func TestHandler_mfaDisable_HappyPath(t *testing.T) { @@ -1227,6 +1237,7 @@ func TestHandler_mfaDisable_HappyPath(t *testing.T) { handler := &Handler{auth: mockAuth} _, err := handler.mfaDisable(ctx, authedReq("tok", `{"password":"`+b64("pw")+`","code":"123456"}`)) require.NoError(t, err) + mockAuth.AssertExpectations(t) } func TestHandler_mfaRegenerateRecoveryCodes_HappyPath(t *testing.T) { @@ -1241,6 +1252,7 @@ func TestHandler_mfaRegenerateRecoveryCodes_HappyPath(t *testing.T) { resp, err := handler.mfaRegenerateRecoveryCodes(ctx, authedReq("tok", `{"code":"123456"}`)) require.NoError(t, err) assert.Len(t, resp.RecoveryCodes, 1) + mockAuth.AssertExpectations(t) } // ErrMFARequired_test / ErrInvalidMFACode_test return the sentinel @@ -1483,3 +1495,69 @@ func TestHandler_login_ErrorEquivalence(t *testing.T) { assert.Equal(t, ceNotFound.Error(), ceWrongPass.Error(), "error body must be identical for unknown-user and wrong-password paths to prevent enumeration") } + +// --------------------------------------------------------------- +// mapMFAServiceError sentinel-to-HTTP-code mapping tests (issue #512). +// +// Each test verifies that a specific auth sentinel maps to the +// expected HTTP status code in mapMFAServiceError. These tests would +// fail if someone renames a sentinel value in the auth package +// without updating the switch in mapMFAServiceError. +// --------------------------------------------------------------- + +func TestMapMFAServiceError_Nil(t *testing.T) { + assert.Nil(t, mapMFAServiceError(nil)) +} + +func TestMapMFAServiceError_NonSentinelPassesThrough(t *testing.T) { + plain := errors.New("database connection lost") + got := mapMFAServiceError(plain) + _, isClient := IsClientError(got) + assert.False(t, isClient, "non-sentinel errors must not be wrapped as ClientError") + assert.Equal(t, plain, got) +} + +func testMFASentinel400(t *testing.T, sentinel error, name string) { + t.Helper() + wrapped := fmt.Errorf("some context: %w", sentinel) + got := mapMFAServiceError(wrapped) + ce, ok := IsClientError(got) + require.True(t, ok, "%s must map to a ClientError, got %T: %v", name, got, got) + assert.Equal(t, 400, ce.code, "%s must map to HTTP 400", name) + assert.Contains(t, ce.Error(), sentinel.Error()) +} + +func TestMapMFAServiceError_InvalidPassword_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFAInvalidPassword, "ErrMFAInvalidPassword") +} + +func TestMapMFAServiceError_InvalidCode_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFAInvalidCode, "ErrMFAInvalidCode") +} + +func TestMapMFAServiceError_CodeRequired_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFACodeRequired, "ErrMFACodeRequired") +} + +func TestMapMFAServiceError_NoEnrollmentInProgress_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFANoEnrollmentInProgress, "ErrMFANoEnrollmentInProgress") +} + +func TestMapMFAServiceError_EnrollmentExpired_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFAEnrollmentExpired, "ErrMFAEnrollmentExpired") +} + +func TestMapMFAServiceError_NotEnabled_Is400(t *testing.T) { + testMFASentinel400(t, auth.ErrMFANotEnabled, "ErrMFANotEnabled") +} + +func TestMapMFAServiceError_AuthFailed_Is401(t *testing.T) { + // ErrMFAAuthFailed must map to 401 (not 400) to prevent user enumeration: + // both "user not found" and "DB error" paths surface as opaque 401. + wrapped := fmt.Errorf("some context: %w", auth.ErrMFAAuthFailed) + got := mapMFAServiceError(wrapped) + ce, ok := IsClientError(got) + require.True(t, ok, "ErrMFAAuthFailed must map to a ClientError") + assert.Equal(t, 401, ce.code, "ErrMFAAuthFailed must map to HTTP 401") + assert.Contains(t, ce.Error(), auth.ErrMFAAuthFailed.Error()) +} diff --git a/internal/auth/errors.go b/internal/auth/errors.go index 3a70fea4b..ad896571b 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -46,4 +46,29 @@ var ( // without substring-matching the human message. See issue #497. ErrMFARequired = errors.New("mfa_required") ErrInvalidMFACode = errors.New("invalid_mfa_code") + + // MFA service-operation sentinels — returned (wrapped via fmt.Errorf + // "%w") by the MFA lifecycle methods in service_mfa.go so the API + // handler can map each error class to the right HTTP status code via + // errors.Is rather than brittle substring matching. See issue #512. + // + // ErrMFAInvalidPassword — wrong current password on setup/disable. + // ErrMFAInvalidCode — wrong TOTP or recovery code. + // ErrMFACodeRequired — MFA-enabled user supplied no code on disable. + // ErrMFANoEnrollmentInProgress — MFAEnable called before MFASetup. + // ErrMFAEnrollmentExpired — pending enrollment window elapsed. + // ErrMFANotEnabled — regenerate/disable called when MFA is off. + // ErrMFAAuthFailed — generic opaque auth failure (user not found or + // DB error; maps to 401 to prevent user enumeration). + // + // Message strings are intentionally identical to the pre-sentinel + // fmt.Errorf literals so that existing tests relying on err.Error() + // substrings continue to pass unchanged. See issue #512. + ErrMFAInvalidPassword = errors.New("invalid password") + ErrMFAInvalidCode = errors.New("invalid MFA code") + ErrMFACodeRequired = errors.New("MFA code or recovery code required") + ErrMFANoEnrollmentInProgress = errors.New("no MFA enrollment in progress") + ErrMFAEnrollmentExpired = errors.New("MFA enrollment expired") + ErrMFANotEnabled = errors.New("MFA is not enabled") + ErrMFAAuthFailed = errors.New("authentication failed") ) diff --git a/internal/auth/service_mfa.go b/internal/auth/service_mfa.go index 8505d8e37..2a42d57d3 100644 --- a/internal/auth/service_mfa.go +++ b/internal/auth/service_mfa.go @@ -262,13 +262,13 @@ func (s *Service) MFASetup(ctx context.Context, userID, password string) (*MFASe } user, err := s.store.GetUserByID(ctx, userID) if err != nil { - return nil, fmt.Errorf("authentication failed") + return nil, fmt.Errorf("%w", ErrMFAAuthFailed) } if user == nil { - return nil, fmt.Errorf("authentication failed") + return nil, fmt.Errorf("%w", ErrMFAAuthFailed) } if !s.verifyPassword(password, user.PasswordHash) { - return nil, fmt.Errorf("invalid password") + return nil, fmt.Errorf("%w", ErrMFAInvalidPassword) } secret, err := generateMFASecret() @@ -316,16 +316,16 @@ func (s *Service) generateAndHashRecoveryCodes() (plaintext, hashes []string, er // "no enrollment in progress" rather than "expired" forever. func (s *Service) validatePendingMFAEnrollment(ctx context.Context, user *User, code string) error { if user.MFAPendingSecret == "" || user.MFAPendingSecretExpiresAt == nil { - return fmt.Errorf("no MFA enrollment in progress") + return fmt.Errorf("%w", ErrMFANoEnrollmentInProgress) } if time.Now().After(*user.MFAPendingSecretExpiresAt) { user.MFAPendingSecret = "" user.MFAPendingSecretExpiresAt = nil _ = s.store.UpdateUser(ctx, user) - return fmt.Errorf("MFA enrollment expired") + return fmt.Errorf("%w", ErrMFAEnrollmentExpired) } if !verifyTOTP(user.MFAPendingSecret, code) { - return fmt.Errorf("invalid MFA code") + return fmt.Errorf("%w", ErrMFAInvalidCode) } return nil } @@ -350,7 +350,7 @@ func (s *Service) MFAEnable(ctx context.Context, userID, code string) ([]string, } user, err := s.store.GetUserByID(ctx, userID) if err != nil || user == nil { - return nil, fmt.Errorf("authentication failed") + return nil, fmt.Errorf("%w", ErrMFAAuthFailed) } if err := s.validatePendingMFAEnrollment(ctx, user, code); err != nil { return nil, err @@ -410,16 +410,16 @@ func (s *Service) MFADisable(ctx context.Context, userID, password, codeOrRecove } user, err := s.store.GetUserByID(ctx, userID) if err != nil || user == nil { - return fmt.Errorf("authentication failed") + return fmt.Errorf("%w", ErrMFAAuthFailed) } if !s.verifyPassword(password, user.PasswordHash) { - return fmt.Errorf("invalid password") + return fmt.Errorf("%w", ErrMFAInvalidPassword) } if !user.MFAEnabled { return s.disableMFAAlreadyOff(ctx, user) } if codeOrRecovery == "" { - return fmt.Errorf("MFA code or recovery code required") + return fmt.Errorf("%w", ErrMFACodeRequired) } // Try TOTP first (cheap), then fall back to recovery code (bcrypt @@ -427,7 +427,7 @@ func (s *Service) MFADisable(ctx context.Context, userID, password, codeOrRecove // fresh proof-of-possession. matched := verifyTOTP(user.MFASecret, codeOrRecovery) || s.consumeRecoveryCode(user, codeOrRecovery) if !matched { - return fmt.Errorf("invalid MFA code") + return fmt.Errorf("%w", ErrMFAInvalidCode) } clearMFAFromUser(user) @@ -448,13 +448,13 @@ func (s *Service) MFARegenerateRecoveryCodes(ctx context.Context, userID, code s } user, err := s.store.GetUserByID(ctx, userID) if err != nil || user == nil { - return nil, fmt.Errorf("authentication failed") + return nil, fmt.Errorf("%w", ErrMFAAuthFailed) } if !user.MFAEnabled || user.MFASecret == "" { - return nil, fmt.Errorf("MFA is not enabled") + return nil, fmt.Errorf("%w", ErrMFANotEnabled) } if !verifyTOTP(user.MFASecret, code) { - return nil, fmt.Errorf("invalid MFA code") + return nil, fmt.Errorf("%w", ErrMFAInvalidCode) } plaintext, hashes, err := s.generateAndHashRecoveryCodes() diff --git a/internal/auth/service_mfa_test.go b/internal/auth/service_mfa_test.go index 0033294dd..24f1778ce 100644 --- a/internal/auth/service_mfa_test.go +++ b/internal/auth/service_mfa_test.go @@ -2,6 +2,7 @@ package auth import ( "context" + "errors" "strings" "testing" "time" @@ -463,3 +464,167 @@ func TestLogin_WithMFA_RecoveryCode_ConsumedOnce(t *testing.T) { require.NotNil(t, resp) assert.Empty(t, user.MFARecoveryCodes, "consumed recovery code must be removed from the slice") } + +// --------------------------------------------------------------- +// Sentinel-identity tests (issue #512). +// +// Each test asserts that the real service returns the expected typed +// sentinel so that a future rename of the error message string in +// service_mfa.go causes a compile-time or test failure rather than a +// silent HTTP-status drift. errors.Is is the contract; the message +// text is NOT the contract (but is preserved for human readability). +// --------------------------------------------------------------- + +func TestMFASetup_WrongPassword_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFASetup(ctx, user.ID, "WrongPassword!@#") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAInvalidPassword), + "MFASetup wrong-password must return ErrMFAInvalidPassword, got: %v", err) + mockStore.AssertExpectations(t) +} + +func TestMFAEnable_NoPending_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFAEnable(ctx, user.ID, "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFANoEnrollmentInProgress), + "MFAEnable with no pending enrollment must return ErrMFANoEnrollmentInProgress, got: %v", err) + mockStore.AssertExpectations(t) +} + +func TestMFAEnable_ExpiredPending_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + secret := "JBSWY3DPEHPK3PXP" + expiresAt := time.Now().Add(-1 * time.Minute) + user := createTestUser(t, "SecurePass@123") + user.MFAPendingSecret = secret + user.MFAPendingSecretExpiresAt = &expiresAt + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Maybe() + + _, err := service.MFAEnable(ctx, user.ID, totpFor(secret)) + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAEnrollmentExpired), + "MFAEnable with expired enrollment must return ErrMFAEnrollmentExpired, got: %v", err) + mockStore.AssertExpectations(t) +} + +func TestMFAEnable_WrongCode_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + secret := "JBSWY3DPEHPK3PXP" + expiresAt := time.Now().Add(mfaPendingExpiry) + user := createTestUser(t, "SecurePass@123") + user.MFAPendingSecret = secret + user.MFAPendingSecretExpiresAt = &expiresAt + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFAEnable(ctx, user.ID, "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAInvalidCode), + "MFAEnable wrong code must return ErrMFAInvalidCode, got: %v", err) + mockStore.AssertExpectations(t) +} + +func TestMFADisable_WrongPassword_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + user.MFASecret = "JBSWY3DPEHPK3PXP" + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + err := service.MFADisable(ctx, user.ID, "wrong", "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAInvalidPassword), + "MFADisable wrong password must return ErrMFAInvalidPassword, got: %v", err) + mockStore.AssertExpectations(t) +} + +func TestMFADisable_NoCode_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + user.MFASecret = "JBSWY3DPEHPK3PXP" + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + err := service.MFADisable(ctx, user.ID, "SecurePass@123", "") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFACodeRequired), + "MFADisable empty code must return ErrMFACodeRequired, got: %v", err) + mockStore.AssertExpectations(t) +} + +func TestMFADisable_WrongCode_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + user.MFASecret = "JBSWY3DPEHPK3PXP" + user.MFARecoveryCodes = []string{"$2a$04$hashedstub"} // won't match any real code + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + err := service.MFADisable(ctx, user.ID, "SecurePass@123", "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAInvalidCode), + "MFADisable wrong code must return ErrMFAInvalidCode, got: %v", err) + mockStore.AssertExpectations(t) +} + +func TestMFARegenerateRecoveryCodes_NotEnabled_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = false + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFARegenerateRecoveryCodes(ctx, user.ID, "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFANotEnabled), + "MFARegenerateRecoveryCodes when disabled must return ErrMFANotEnabled, got: %v", err) + mockStore.AssertExpectations(t) +} + +func TestMFARegenerateRecoveryCodes_WrongCode_ReturnsSentinel(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + user.MFASecret = "JBSWY3DPEHPK3PXP" + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFARegenerateRecoveryCodes(ctx, user.ID, "000000") + require.Error(t, err) + assert.True(t, errors.Is(err, ErrMFAInvalidCode), + "MFARegenerateRecoveryCodes wrong TOTP must return ErrMFAInvalidCode, got: %v", err) + mockStore.AssertExpectations(t) +}