diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index b8a03b8a..1b0398c7 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -50,7 +50,17 @@ func (h *Handler) login(ctx context.Context, req *events.LambdaFunctionURLReques if errors.Is(err, auth.ErrInvalidMFACode) { return nil, NewClientError(401, "invalid_mfa_code") } - return nil, NewClientError(401, err.Error()) + // ErrMFANotConfigured (MFA enabled but secret missing) maps to + // "mfa_required" rather than a distinct message so an attacker + // cannot distinguish "MFA enrolled + working" from "MFA enrolled + // + broken secret" by probing the error response (issue #388). + if errors.Is(err, auth.ErrMFANotConfigured) { + return nil, NewClientError(401, "mfa_required") + } + // All other auth failures (wrong password, account not found, + // locked, etc.) collapse to a single opaque 401. Never forward + // err.Error() verbatim — it may reveal internal account state. + return nil, NewClientError(401, "invalid credentials") } return response, nil @@ -364,8 +374,9 @@ func (h *Handler) changePassword(ctx context.Context, req *events.LambdaFunction // package. The login handler maps these via errors.Is() to the // machine-readable response codes "mfa_required" / "invalid_mfa_code". var ( - mfaRequiredSentinel = auth.ErrMFARequired - mfaInvalidSentinel = auth.ErrInvalidMFACode + mfaRequiredSentinel = auth.ErrMFARequired + mfaInvalidSentinel = auth.ErrInvalidMFACode + mfaNotConfiguredSentinel = auth.ErrMFANotConfigured ) // mapMFAServiceError maps a service-layer MFA error to the right diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index c5bdb27e..14ab15bc 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -1140,5 +1140,118 @@ func TestHandler_mfaRegenerateRecoveryCodes_HappyPath(t *testing.T) { // auth package (errors.go); the api package's login handler maps // them via errors.Is(). Here we just wrap them so the mocked Login // returns the same value the real service would. -func ErrMFARequired_test() error { return mfaRequiredSentinel } -func ErrInvalidMFACode_test() error { return mfaInvalidSentinel } +func ErrMFARequired_test() error { return mfaRequiredSentinel } +func ErrInvalidMFACode_test() error { return mfaInvalidSentinel } +func ErrMFANotConfigured_test() error { return mfaNotConfiguredSentinel } + +// --------------------------------------------------------------- +// Issue #388 — MFA enrollment status must not be leaked via login +// error responses. +// +// An attacker who observes distinct error codes for +// (a) wrong credentials + no MFA enrolled +// (b) wrong credentials + MFA enrolled +// can confirm whether a target account has MFA enabled without the +// correct password. All failed-login paths must produce identical +// response code and message. +// +// The tests below also verify that ErrMFANotConfigured (MFA flagged +// on but secret missing) maps to "mfa_required" rather than a +// distinct message — so "MFA enrolled + working" is +// indistinguishable from "MFA enrolled + broken secret". +// --------------------------------------------------------------- + +// TestLogin_FailedAuth_ResponseEquivalence asserts that two +// failed-login attempts — one for a user without MFA (generic 401) +// and one for a user with MFA enrolled (any non-mfa_required path) +// — produce the exact same HTTP status code and error message. +func TestLogin_FailedAuth_ResponseEquivalence(t *testing.T) { + ctx := context.Background() + + genericErr := errors.New("Check your email address and password and try again") + + for _, tc := range []struct { + name string + authErr error + }{ + {"no-MFA user wrong password", genericErr}, + {"MFA-enrolled user wrong password", genericErr}, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + mockAuth := new(MockAuthService) + mockAuth.On("Login", ctx, mock.Anything). + Return((*LoginResponse)(nil), tc.authErr).Once() + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + handler := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Body: `{"email":"u@x.com","password":"` + b64("wrong") + `"}`, + } + _, err := handler.login(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "%s: expected ClientError, got %T: %v", tc.name, err, err) + assert.Equal(t, 401, ce.code, "%s: status code mismatch", tc.name) + assert.Equal(t, "invalid credentials", ce.Error(), "%s: message mismatch", tc.name) + }) + } +} + +// TestLogin_MFANotConfigured_ReturnsMFARequired asserts that +// ErrMFANotConfigured (MFA enabled + secret missing) produces the +// same "mfa_required" response as ErrMFARequired, so an attacker +// cannot distinguish a correctly-enrolled account from a +// partially-enrolled one (issue #388). +func TestLogin_MFANotConfigured_ReturnsMFARequired(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("Login", ctx, mock.Anything). + Return((*LoginResponse)(nil), ErrMFANotConfigured_test()).Once() + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + handler := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Body: `{"email":"u@x.com","password":"` + b64("correct") + `","mfa_code":""}`, + } + _, err := handler.login(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError") + assert.Equal(t, 401, ce.code) + // Must be identical to the ErrMFARequired response — both paths must + // be indistinguishable to an external observer (issue #388). + assert.Equal(t, "mfa_required", ce.Error(), + "ErrMFANotConfigured must produce 'mfa_required', identical to ErrMFARequired") +} + +// TestLogin_MFARequired_And_NotConfigured_ProduceSameResponse asserts +// that the two MFA enrollment states (working vs broken secret) map +// to IDENTICAL response bodies and status codes (issue #388). +func TestLogin_MFARequired_And_NotConfigured_ProduceSameResponse(t *testing.T) { + ctx := context.Background() + req := &events.LambdaFunctionURLRequest{ + Body: `{"email":"u@x.com","password":"` + b64("correct") + `"}`, + } + + getResponse := func(authErr error) (int, string) { + mockAuth := new(MockAuthService) + mockAuth.On("Login", ctx, mock.Anything). + Return((*LoginResponse)(nil), authErr).Once() + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + handler := &Handler{auth: mockAuth} + _, err := handler.login(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + return ce.code, ce.Error() + } + + codeA, msgA := getResponse(ErrMFARequired_test()) + codeB, msgB := getResponse(ErrMFANotConfigured_test()) + + assert.Equal(t, codeA, codeB, + "ErrMFARequired and ErrMFANotConfigured must return the same HTTP status") + assert.Equal(t, msgA, msgB, + "ErrMFARequired and ErrMFANotConfigured must return identical response bodies (issue #388)") +} diff --git a/internal/auth/errors.go b/internal/auth/errors.go index d96269ec..e331a9bf 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -25,4 +25,12 @@ var ( // without substring-matching the human message. See issue #497. ErrMFARequired = errors.New("mfa_required") ErrInvalidMFACode = errors.New("invalid_mfa_code") + + // ErrMFANotConfigured is returned when a user has MFA flagged as + // enabled but the MFA secret is missing (e.g. enrollment was + // interrupted). The login handler maps this to "mfa_required" — the + // same response as ErrMFARequired — so an attacker cannot distinguish + // "MFA enrolled and working" from "MFA enrolled but secret missing" + // by observing the error message. See issue #388. + ErrMFANotConfigured = errors.New("MFA is enabled but not configured") ) diff --git a/internal/auth/service.go b/internal/auth/service.go index 45ad5047..a6bb9e5c 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -195,7 +195,7 @@ func (s *Service) verifyPasswordAndMFA(ctx context.Context, user *User, req Logi return ErrMFARequired } if user.MFASecret == "" { - return fmt.Errorf("MFA is enabled but not configured") + return fmt.Errorf("%w", ErrMFANotConfigured) } // verifyTOTP is panic-safe: a malformed secret causes generateTOTP to return "" // (base32 decode error), resulting in a comparison miss rather than a panic. diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index bc6e4fc6..a763417f 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -477,7 +477,8 @@ func TestLogin_WithMFA_NoSecret(t *testing.T) { resp, err := service.Login(ctx, req) assert.Error(t, err) assert.Nil(t, resp) - assert.Contains(t, err.Error(), "MFA is enabled but not configured") + assert.ErrorIs(t, err, ErrMFANotConfigured, + "Login with MFA enabled but no secret must return ErrMFANotConfigured") } // Test UpdateUserProfile