Skip to content

Commit 948c8f5

Browse files
waveywavesclaude
waveywaves
and
claude
committed
fix(cli): stop masking auth errors as token expiry
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 #2922 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>
1 parent 56c5a3f commit 948c8f5

2 files changed

Lines changed: 186 additions & 6 deletions

File tree

‎app/cli/main.go‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
package main
1717

1818
import (
19+
"fmt"
1920
"os"
2021

2122
"github.com/chainloop-dev/chainloop/app/cli/cmd"
@@ -87,13 +88,22 @@ func errorInfo(err error, logger zerolog.Logger) (string, int) {
8788
case v1.IsCasBackendErrorReasonInvalid(err):
8889
msg = "the CAS backend you provided is invalid. Refer to `chainloop cas-backend update` command or contact your administrator."
8990
case isWrappedErr(st, jwtMiddleware.ErrTokenExpired):
90-
msg = "your authentication token has expired, please run chainloop auth login again"
91+
msg = "your authentication token has expired, please run \"chainloop auth login\" again"
92+
case isWrappedErr(st, jwtMiddleware.ErrTokenInvalid):
93+
msg = "your authentication token is invalid, please run \"chainloop auth login\" again"
94+
case isWrappedErr(st, jwtMiddleware.ErrTokenParseFail):
95+
msg = "failed to parse authentication token, please run \"chainloop auth login\" again"
9196
case isWrappedErr(st, jwtMiddleware.ErrMissingJwtToken):
9297
msg = "authentication required, please run \"chainloop auth login\""
9398
case v1.IsUserNotMemberOfOrgErrorNotInOrg(err):
9499
msg = "the organization you are trying to access does not exist or you are not part of it, please run \"chainloop auth login\""
95100
case v1.IsUserWithNoMembershipErrorNotInOrg(err):
96101
msg = "you are not part of any organization, please run \"chainloop organization create --name ORG_NAME\" to create one"
102+
case isUnmatchedAuthErr(st):
103+
// Fallback for any other 401 errors not matched above.
104+
// Must remain AFTER all specific Unauthenticated/org-membership cases
105+
// to avoid shadowing them.
106+
msg = fmt.Sprintf("authentication error: %s", st.Message())
97107
case errors.As(err, &cmd.GracefulError{}):
98108
// Graceful recovery if the flag is set and the received error is marked as recoverable
99109
if cmd.GracefulExit {
@@ -110,12 +120,21 @@ func errorInfo(err error, logger zerolog.Logger) (string, int) {
110120

111121
// target is the expected error
112122
// grpcStatus is the actual error that might be wrapped in both the status and the error
123+
//
124+
// NOTE: We intentionally do NOT use kratos errors.Is() here because it only
125+
// compares Code and Reason. Since all JWT errors share the same Code (401) and
126+
// Reason ("UNAUTHORIZED"), errors.Is() would match any 401 error against any
127+
// JWT sentinel — causing e.g. "token invalid" to be reported as "token expired".
128+
// Instead we compare Code and Message, which carry the distinguishing text.
113129
func isWrappedErr(grpcStatus *status.Status, target *errors.Error) bool {
114130
err := errors.FromError(grpcStatus.Err())
115-
// The error might be wrapped since the CLI sometimes returns a wrapped error
116-
if errors.Is(err, target) {
117-
return true
118-
}
119-
120131
return target.Code == err.Code && err.Message == target.Message
121132
}
133+
134+
// isUnmatchedAuthErr returns true when the gRPC status represents an
135+
// Unauthenticated (401-equivalent) error that was not matched by any of the
136+
// specific JWT sentinel checks above. This lets us surface the server's
137+
// original message instead of silently dropping it.
138+
func isUnmatchedAuthErr(grpcStatus *status.Status) bool {
139+
return grpcStatus.Code() == codes.Unauthenticated
140+
}

‎app/cli/main_test.go‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
//
2+
// Copyright 2023-2026 The Chainloop Authors.
3+
//
4+
// Licensed under the Apache License, Version 2.0 (the "License");
5+
// you may not use this file except in compliance with the License.
6+
// You may obtain a copy of the License at
7+
//
8+
// http://www.apache.org/licenses/LICENSE-2.0
9+
//
10+
// Unless required by applicable law or agreed to in writing, software
11+
// distributed under the License is distributed on an "AS IS" BASIS,
12+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
// See the License for the specific language governing permissions and
14+
// limitations under the License.
15+
16+
package main
17+
18+
import (
19+
"testing"
20+
21+
kratosErrors "github.com/go-kratos/kratos/v2/errors"
22+
jwtMiddleware "github.com/go-kratos/kratos/v2/middleware/auth/jwt"
23+
"github.com/stretchr/testify/assert"
24+
"google.golang.org/grpc/codes"
25+
"google.golang.org/grpc/status"
26+
)
27+
28+
// toGRPCStatus converts a Kratos *errors.Error into a *status.Status the same
29+
// way it would travel over the wire: Kratos Error -> GRPCStatus() -> gRPC Status.
30+
func toGRPCStatus(err *kratosErrors.Error) *status.Status {
31+
return err.GRPCStatus()
32+
}
33+
34+
func TestIsWrappedErr(t *testing.T) {
35+
tests := []struct {
36+
name string
37+
actual *kratosErrors.Error // the error that "came over the wire"
38+
target *kratosErrors.Error // the sentinel we are checking against
39+
want bool
40+
}{
41+
{
42+
name: "ErrTokenExpired matches itself",
43+
actual: jwtMiddleware.ErrTokenExpired,
44+
target: jwtMiddleware.ErrTokenExpired,
45+
want: true,
46+
},
47+
{
48+
name: "ErrTokenInvalid matches itself",
49+
actual: jwtMiddleware.ErrTokenInvalid,
50+
target: jwtMiddleware.ErrTokenInvalid,
51+
want: true,
52+
},
53+
{
54+
name: "ErrTokenParseFail matches itself",
55+
actual: jwtMiddleware.ErrTokenParseFail,
56+
target: jwtMiddleware.ErrTokenParseFail,
57+
want: true,
58+
},
59+
{
60+
name: "ErrMissingJwtToken matches itself",
61+
actual: jwtMiddleware.ErrMissingJwtToken,
62+
target: jwtMiddleware.ErrMissingJwtToken,
63+
want: true,
64+
},
65+
{
66+
name: "ErrTokenExpired does NOT match ErrTokenInvalid",
67+
actual: jwtMiddleware.ErrTokenExpired,
68+
target: jwtMiddleware.ErrTokenInvalid,
69+
want: false,
70+
},
71+
{
72+
name: "ErrTokenInvalid does NOT match ErrTokenExpired",
73+
actual: jwtMiddleware.ErrTokenInvalid,
74+
target: jwtMiddleware.ErrTokenExpired,
75+
want: false,
76+
},
77+
{
78+
name: "ErrTokenParseFail does NOT match ErrTokenExpired",
79+
actual: jwtMiddleware.ErrTokenParseFail,
80+
target: jwtMiddleware.ErrTokenExpired,
81+
want: false,
82+
},
83+
{
84+
name: "ErrTokenExpired does NOT match ErrMissingJwtToken",
85+
actual: jwtMiddleware.ErrTokenExpired,
86+
target: jwtMiddleware.ErrMissingJwtToken,
87+
want: false,
88+
},
89+
}
90+
91+
for _, tt := range tests {
92+
t.Run(tt.name, func(t *testing.T) {
93+
st := toGRPCStatus(tt.actual)
94+
got := isWrappedErr(st, tt.target)
95+
assert.Equal(t, tt.want, got, "isWrappedErr(%v, %v)", tt.actual.Message, tt.target.Message)
96+
})
97+
}
98+
}
99+
100+
func TestIsUnmatchedAuthErr(t *testing.T) {
101+
tests := []struct {
102+
name string
103+
st *status.Status
104+
want bool
105+
}{
106+
{
107+
name: "generic 401/Unauthenticated error is caught",
108+
st: status.New(codes.Unauthenticated, "some auth error"),
109+
want: true,
110+
},
111+
{
112+
name: "JWT token expired (Unauthenticated) is caught",
113+
st: toGRPCStatus(jwtMiddleware.ErrTokenExpired),
114+
want: true,
115+
},
116+
{
117+
name: "PermissionDenied is NOT caught",
118+
st: status.New(codes.PermissionDenied, "forbidden"),
119+
want: false,
120+
},
121+
{
122+
name: "OK status is NOT caught",
123+
st: status.New(codes.OK, ""),
124+
want: false,
125+
},
126+
{
127+
name: "Internal error is NOT caught",
128+
st: status.New(codes.Internal, "internal server error"),
129+
want: false,
130+
},
131+
{
132+
name: "NotFound is NOT caught",
133+
st: status.New(codes.NotFound, "not found"),
134+
want: false,
135+
},
136+
}
137+
138+
for _, tt := range tests {
139+
t.Run(tt.name, func(t *testing.T) {
140+
got := isUnmatchedAuthErr(tt.st)
141+
assert.Equal(t, tt.want, got, "isUnmatchedAuthErr()")
142+
})
143+
}
144+
}
145+
146+
// TestKratosErrorsIsMasksJWTErrors demonstrates the bug that motivates our
147+
// Message-based comparison: Kratos errors.Is() only compares Code and Reason.
148+
// Since all JWT errors share Code=401 and Reason="UNAUTHORIZED", errors.Is()
149+
// incorrectly matches ANY JWT error against ANY other JWT sentinel.
150+
func TestKratosErrorsIsMasksJWTErrors(t *testing.T) {
151+
// This test proves that the naive errors.Is approach is broken:
152+
// ErrTokenExpired would incorrectly match ErrTokenInvalid via Kratos errors.Is.
153+
if !kratosErrors.Is(jwtMiddleware.ErrTokenExpired, jwtMiddleware.ErrTokenInvalid) {
154+
t.Skip("Kratos errors.Is behavior has changed; this test documents the original masking bug")
155+
}
156+
157+
// Now verify that our isWrappedErr correctly distinguishes them
158+
st := toGRPCStatus(jwtMiddleware.ErrTokenExpired)
159+
assert.False(t, isWrappedErr(st, jwtMiddleware.ErrTokenInvalid), "isWrappedErr should NOT match ErrTokenExpired against ErrTokenInvalid")
160+
assert.True(t, isWrappedErr(st, jwtMiddleware.ErrTokenExpired), "isWrappedErr should match ErrTokenExpired against itself")
161+
}

0 commit comments

Comments
 (0)