Skip to content

fix(cli): stop masking auth errors as token expiry#2945

Draft
waveywaves wants to merge 2 commits intochainloop-dev:mainfrom
waveywaves:fix/auth-error-masking
Draft

fix(cli): stop masking auth errors as token expiry#2945
waveywaves wants to merge 2 commits intochainloop-dev:mainfrom
waveywaves:fix/auth-error-masking

Conversation

@waveywaves
Copy link
Copy Markdown
Contributor

@waveywaves waveywaves commented Mar 27, 2026

Summary

  • Fix all 401/UNAUTHORIZED auth errors being incorrectly displayed as "your authentication token has expired"
  • Reorder switch cases so isUnmatchedAuthErr fallback does not shadow org-membership error cases
  • Add unit tests for isWrappedErr and isUnmatchedAuthErr helper functions

Root Cause Analysis

The Kratos errors.Is() implementation only compares Code and Reason fields:

func (e *Error) Is(err error) bool {
    if se := new(Error); errors.As(err, &se) {
        return se.Code == e.Code && se.Reason == e.Reason
    }
    return false
}

All JWT middleware errors (ErrTokenExpired, ErrTokenInvalid, ErrTokenParseFail, ErrMissingJwtToken) are created with errors.Unauthorized("UNAUTHORIZED", "<message>"), giving them identical Code: 401 and Reason: "UNAUTHORIZED" fields. Only the Message field differs. This caused errors.Is() to match any auth error against ErrTokenExpired first in the switch statement, masking the real error type.

Changes

  • Remove the errors.Is() short-circuit in isWrappedErr — keep only the Code + Message comparison, which correctly distinguishes between different JWT error types
  • Add explicit case branches for ErrTokenInvalid ("token is invalid") and ErrTokenParseFail ("failed to parse token") so users see accurate error messages
  • Add a fallback (isUnmatchedAuthErr) for unmatched 401 errors that surfaces the server's original message instead of silently dropping it
  • Reorder switch cases so org-membership checks (IsUserNotMemberOfOrgErrorNotInOrg, IsUserWithNoMembershipErrorNotInOrg) come BEFORE the generic isUnmatchedAuthErr fallback, preventing the fallback from shadowing them
  • Add explanatory comment on isWrappedErr documenting why we use Message comparison instead of Kratos errors.Is()
  • Add unit tests covering:
    • isWrappedErr correctly matches each JWT error sentinel against itself
    • isWrappedErr does NOT cross-match different JWT error types
    • isUnmatchedAuthErr catches generic 401/Unauthenticated errors
    • isUnmatchedAuthErr does NOT catch non-401 errors (PermissionDenied, Internal, NotFound, OK)
    • Regression test demonstrating the Kratos errors.Is() masking bug and proving our fix avoids it

Test Plan

  • go test ./app/cli/ -v — all 15 test cases pass
  • go vet ./app/cli/... passes
  • Test with an expired JWT token: confirms "token has expired" message
  • Test with an invalid/revoked JWT token: confirms "token is invalid" message (previously showed "token has expired")
  • Test with a malformed JWT token: confirms "failed to parse" message
  • Test with missing JWT token: confirms "authentication required" message
  • Test with an unrecognized 401 error: confirms server's original message is shown

Fixes #2922

@waveywaves waveywaves force-pushed the fix/auth-error-masking branch 2 times, most recently from 2127381 to 093b98b Compare March 27, 2026 19:22
waveywaves and others added 2 commits March 28, 2026 09:45
Add top-level permissions blocks following the two-tier permission
pattern recommended by OpenSSF Scorecard:

- stale.yml: add `permissions: {}` at workflow level (job already has
  issues: write + pull-requests: write)
- build_external_container_images.yaml: move `packages: write` from
  workflow level to job level; set workflow level to `permissions: read-all`

scm_configuration_check.yaml already had `permissions: read-all` at
workflow level so no change was needed.

Fixes chainloop-dev#2841

Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>
The Kratos errors.Is() function matches only on Code and Reason (not
Message). Since all JWT middleware errors share Code=401 and
Reason="UNAUTHORIZED", every authentication error was incorrectly
matching ErrTokenExpired and showing "your authentication token has
expired" regardless of the actual cause.

Fix by removing the errors.Is() short-circuit in isWrappedErr and
keeping only the Code+Message comparison, which carries the
distinguishing text for each JWT error type. Also add explicit case
branches for ErrTokenInvalid and ErrTokenParseFail, and a fallback
for unmatched 401 errors that surfaces the server's original message.

Closes chainloop-dev#2922

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>
@waveywaves waveywaves force-pushed the fix/auth-error-masking branch from 093b98b to 948c8f5 Compare March 28, 2026 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auth token expiry error masks other underlying errors (e.g. invalid credentials)

1 participant