fix(cli): stop masking auth errors as token expiry#2945
Draft
waveywaves wants to merge 2 commits intochainloop-dev:mainfrom
Draft
fix(cli): stop masking auth errors as token expiry#2945waveywaves wants to merge 2 commits intochainloop-dev:mainfrom
waveywaves wants to merge 2 commits intochainloop-dev:mainfrom
Conversation
2127381 to
093b98b
Compare
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>
093b98b to
948c8f5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
isUnmatchedAuthErrfallback does not shadow org-membership error casesisWrappedErrandisUnmatchedAuthErrhelper functionsRoot Cause Analysis
The Kratos
errors.Is()implementation only comparesCodeandReasonfields:All JWT middleware errors (
ErrTokenExpired,ErrTokenInvalid,ErrTokenParseFail,ErrMissingJwtToken) are created witherrors.Unauthorized("UNAUTHORIZED", "<message>"), giving them identicalCode: 401andReason: "UNAUTHORIZED"fields. Only theMessagefield differs. This causederrors.Is()to match any auth error againstErrTokenExpiredfirst in the switch statement, masking the real error type.Changes
errors.Is()short-circuit inisWrappedErr— keep only theCode + Messagecomparison, which correctly distinguishes between different JWT error typesErrTokenInvalid("token is invalid") andErrTokenParseFail("failed to parse token") so users see accurate error messagesisUnmatchedAuthErr) for unmatched 401 errors that surfaces the server's original message instead of silently dropping itIsUserNotMemberOfOrgErrorNotInOrg,IsUserWithNoMembershipErrorNotInOrg) come BEFORE the genericisUnmatchedAuthErrfallback, preventing the fallback from shadowing themisWrappedErrdocumenting why we use Message comparison instead of Kratoserrors.Is()isWrappedErrcorrectly matches each JWT error sentinel against itselfisWrappedErrdoes NOT cross-match different JWT error typesisUnmatchedAuthErrcatches generic 401/Unauthenticated errorsisUnmatchedAuthErrdoes NOT catch non-401 errors (PermissionDenied, Internal, NotFound, OK)errors.Is()masking bug and proving our fix avoids itTest Plan
go test ./app/cli/ -v— all 15 test cases passgo vet ./app/cli/...passesFixes #2922