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.

Author: cristim

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

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

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

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.

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