From 804ff301d23f2154940d59287c87098ad8684405 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 30 May 2026 21:24:56 +0200 Subject: [PATCH] sec(auth): stop leaking MFA enrollment via login error responses (closes #388) The login handler previously forwarded err.Error() verbatim for any auth failure not matching ErrMFARequired or ErrInvalidMFACode. This included "MFA is enabled but not configured", which revealed to an attacker that credentials were correct and MFA was enrolled (but broken). Two attack vectors were closed: 1. Add ErrMFANotConfigured sentinel to internal/auth/errors.go and return it (wrapped via %w) from verifyPasswordAndMFA. Map it to "mfa_required" in the login handler -- identical to ErrMFARequired -- so "MFA enrolled + working" is indistinguishable from "MFA enrolled + broken secret" in the HTTP response. 2. Replace the bare err.Error() fallthrough in the login handler with a fixed "invalid credentials" string so no internal error message ever reaches the client. Tests positively assert response equivalence: ErrMFARequired and ErrMFANotConfigured produce the same 401 + "mfa_required" body; all wrong-password paths produce the same 401 + "invalid credentials" body. --- internal/api/handler_auth.go | 17 ++++- internal/api/handler_auth_test.go | 117 +++++++++++++++++++++++++++++- internal/auth/errors.go | 8 ++ internal/auth/service.go | 2 +- internal/auth/service_test.go | 3 +- 5 files changed, 140 insertions(+), 7 deletions(-) diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index a039c59b4..6ba8ba465 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 @@ -498,8 +508,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 1ebea1c26..ed800014a 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -1250,8 +1250,9 @@ 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 } // Tests for GET /api/auth/me/permissions (issue #917). @@ -1483,3 +1484,115 @@ 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") } + +// --------------------------------------------------------------- +// 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 3a70fea4b..eea0d28cc 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -46,4 +46,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 785dd6c3e..97d167d60 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -203,7 +203,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 eab8ac248..6950e6ee9 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -472,7 +472,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