Skip to content

sec(api/auth): stop forwarding err.Error() in login catch-all (opaque 401) #937

Description

@cristim

Problem

The login handler's catch-all error path forwards raw service errors verbatim to the client:

// internal/api/handler_auth.go (login)
return nil, NewClientError(401, err.Error())

Forwarding err.Error() means any current or future service-layer error message (account state, internal failures, etc.) is exposed in the 401 response body. This is a defense-in-depth / information-disclosure hygiene gap: the specific MFA-enrollment leak was already fixed by #830 (#391) collapsing the missing-secret case into the generic message, but the catch-all itself still forwards whatever string the service returns.

Fix

Replace the catch-all with an opaque, hardcoded response so raw service errors are never forwarded:

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

Keep the existing typed-sentinel arms above it (ErrMFARequired -> mfa_required, ErrInvalidMFACode -> invalid_mfa_code) unchanged - only the final fallthrough changes.

Acceptance criteria

  • The login catch-all returns a fixed opaque message, not err.Error().
  • A regression test asserts that a service error with a distinctive internal message does NOT appear in the login 401 response body (the response is the opaque string).
  • Existing MFA-required / invalid-code response codes are unchanged.

Provenance

Salvaged from closed PR #886 (the rest of which was superseded by the merged #830/#391). This is the one non-conflicting hardening from that PR worth keeping on its own.

No activity

Activity on this issue will appear here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    effort/xsTrivial / one-linerimpact/fewLimited audiencepr-createdA PR has been opened for this issue (dedup guard for the auto-PR loop)pr-mergedThe PR for this issue has been mergedpriority/p3Polish / idea / may never shipseverity/lowMinor harmtriagedItem has been triagedtype/securitySecurity findingurgency/this-quarterWithin the quarter

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions