sec(auth): stop leaking MFA enrollment via login error responses (closes #388)

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

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)")
+}

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")
 )

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.

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