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

waveywaves · claude · waveywaves · commit 6dd81dd59e2f · 2026-03-31T18:52:09.000+05:30
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>
diff --git a/app/cli/main.go b/app/cli/main.go
@@ -16,6 +16,7 @@
 package main
 
 import (
+	"fmt"
 	"os"
 
 	"github.com/chainloop-dev/chainloop/app/cli/cmd"
@@ -87,13 +88,24 @@ func errorInfo(err error, logger zerolog.Logger) (string, int) {
 	case v1.IsCasBackendErrorReasonInvalid(err):
 		msg = "the CAS backend you provided is invalid. Refer to `chainloop cas-backend update` command or contact your administrator."
 	case isWrappedErr(st, jwtMiddleware.ErrTokenExpired):
-		msg = "your authentication token has expired, please run chainloop auth login again"
+		msg = "your authentication token has expired, please run \"chainloop auth login\" again"
+	case isWrappedErr(st, jwtMiddleware.ErrTokenInvalid):
+		msg = "your authentication token is invalid, please run \"chainloop auth login\" again"
+	case isWrappedErr(st, jwtMiddleware.ErrTokenParseFail):
+		msg = "failed to parse authentication token, please run \"chainloop auth login\" again"
 	case isWrappedErr(st, jwtMiddleware.ErrMissingJwtToken):
 		msg = "authentication required, please run \"chainloop auth login\""
 	case v1.IsUserNotMemberOfOrgErrorNotInOrg(err):
 		msg = "the organization you are trying to access does not exist or you are not part of it, please run \"chainloop auth login\""
 	case v1.IsUserWithNoMembershipErrorNotInOrg(err):
 		msg = "you are not part of any organization, please run \"chainloop organization create --name ORG_NAME\" to create one"
+	case isUnmatchedAuthErr(st):
+		// Fallback for any other 401/Unauthenticated errors not matched above.
+		// Org-membership errors (IsUserNotMemberOfOrgErrorNotInOrg,
+		// IsUserWithNoMembershipErrorNotInOrg) use gRPC code 7 (PermissionDenied),
+		// not code 16 (Unauthenticated), so shadowing is structurally impossible.
+		// We keep this case ordered after them purely for readability/clarity.
+		msg = fmt.Sprintf("authentication error: %s", st.Message())
 	case errors.As(err, &cmd.GracefulError{}):
 		// Graceful recovery if the flag is set and the received error is marked as recoverable
 		if cmd.GracefulExit {
@@ -110,12 +122,21 @@ func errorInfo(err error, logger zerolog.Logger) (string, int) {
 
 // target is the expected error
 // grpcStatus is the actual error that might be wrapped in both the status and the error
+//
+// NOTE: We intentionally do NOT use kratos errors.Is() here because it only
+// compares Code and Reason. Since all JWT errors share the same Code (401) and
+// Reason ("UNAUTHORIZED"), errors.Is() would match any 401 error against any
+// JWT sentinel — causing e.g. "token invalid" to be reported as "token expired".
+// Instead we compare Code and Message, which carry the distinguishing text.
 func isWrappedErr(grpcStatus *status.Status, target *errors.Error) bool {
 	err := errors.FromError(grpcStatus.Err())
-	// The error might be wrapped since the CLI sometimes returns a wrapped error
-	if errors.Is(err, target) {
-		return true
-	}
-
 	return target.Code == err.Code && err.Message == target.Message
 }
+
+// isUnmatchedAuthErr returns true when the gRPC status represents an
+// Unauthenticated (401-equivalent) error that was not matched by any of the
+// specific JWT sentinel checks above. This lets us surface the server's
+// original message instead of silently dropping it.
+func isUnmatchedAuthErr(grpcStatus *status.Status) bool {
+	return grpcStatus.Code() == codes.Unauthenticated
+}
diff --git a/app/cli/main_test.go b/app/cli/main_test.go
@@ -0,0 +1,246 @@
+//
+// Copyright 2023-2026 The Chainloop Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main
+
+import (
+	"testing"
+
+	kratosErrors "github.com/go-kratos/kratos/v2/errors"
+	jwtMiddleware "github.com/go-kratos/kratos/v2/middleware/auth/jwt"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+)
+
+// toGRPCStatus converts a Kratos *errors.Error into a *status.Status the same
+// way it would travel over the wire: Kratos Error -> GRPCStatus() -> gRPC Status.
+func toGRPCStatus(err *kratosErrors.Error) *status.Status {
+	return err.GRPCStatus()
+}
+
+func TestIsWrappedErr(t *testing.T) {
+	tests := []struct {
+		name   string
+		actual *kratosErrors.Error // the error that "came over the wire"
+		target *kratosErrors.Error // the sentinel we are checking against
+		want   bool
+	}{
+		{
+			name:   "ErrTokenExpired matches itself",
+			actual: jwtMiddleware.ErrTokenExpired,
+			target: jwtMiddleware.ErrTokenExpired,
+			want:   true,
+		},
+		{
+			name:   "ErrTokenInvalid matches itself",
+			actual: jwtMiddleware.ErrTokenInvalid,
+			target: jwtMiddleware.ErrTokenInvalid,
+			want:   true,
+		},
+		{
+			name:   "ErrTokenParseFail matches itself",
+			actual: jwtMiddleware.ErrTokenParseFail,
+			target: jwtMiddleware.ErrTokenParseFail,
+			want:   true,
+		},
+		{
+			name:   "ErrMissingJwtToken matches itself",
+			actual: jwtMiddleware.ErrMissingJwtToken,
+			target: jwtMiddleware.ErrMissingJwtToken,
+			want:   true,
+		},
+		{
+			name:   "ErrTokenExpired does NOT match ErrTokenInvalid",
+			actual: jwtMiddleware.ErrTokenExpired,
+			target: jwtMiddleware.ErrTokenInvalid,
+			want:   false,
+		},
+		{
+			name:   "ErrTokenInvalid does NOT match ErrTokenExpired",
+			actual: jwtMiddleware.ErrTokenInvalid,
+			target: jwtMiddleware.ErrTokenExpired,
+			want:   false,
+		},
+		{
+			name:   "ErrTokenParseFail does NOT match ErrTokenExpired",
+			actual: jwtMiddleware.ErrTokenParseFail,
+			target: jwtMiddleware.ErrTokenExpired,
+			want:   false,
+		},
+		{
+			name:   "ErrTokenExpired does NOT match ErrMissingJwtToken",
+			actual: jwtMiddleware.ErrTokenExpired,
+			target: jwtMiddleware.ErrMissingJwtToken,
+			want:   false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			st := toGRPCStatus(tt.actual)
+			got := isWrappedErr(st, tt.target)
+			assert.Equal(t, tt.want, got, "isWrappedErr(%v, %v)", tt.actual.Message, tt.target.Message)
+		})
+	}
+}
+
+func TestIsUnmatchedAuthErr(t *testing.T) {
+	tests := []struct {
+		name string
+		st   *status.Status
+		want bool
+	}{
+		{
+			name: "generic 401/Unauthenticated error is caught",
+			st:   status.New(codes.Unauthenticated, "some auth error"),
+			want: true,
+		},
+		{
+			name: "JWT token expired (Unauthenticated) is caught",
+			st:   toGRPCStatus(jwtMiddleware.ErrTokenExpired),
+			want: true,
+		},
+		{
+			name: "PermissionDenied is NOT caught",
+			st:   status.New(codes.PermissionDenied, "forbidden"),
+			want: false,
+		},
+		{
+			name: "OK status is NOT caught",
+			st:   status.New(codes.OK, ""),
+			want: false,
+		},
+		{
+			name: "Internal error is NOT caught",
+			st:   status.New(codes.Internal, "internal server error"),
+			want: false,
+		},
+		{
+			name: "NotFound is NOT caught",
+			st:   status.New(codes.NotFound, "not found"),
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := isUnmatchedAuthErr(tt.st)
+			assert.Equal(t, tt.want, got, "isUnmatchedAuthErr()")
+		})
+	}
+}
+
+// TestKratosErrorsIsMasksJWTErrors demonstrates the bug that motivates our
+// Message-based comparison: Kratos errors.Is() only compares Code and Reason.
+// Since all JWT errors share Code=401 and Reason="UNAUTHORIZED", errors.Is()
+// incorrectly matches ANY JWT error against ANY other JWT sentinel.
+func TestKratosErrorsIsMasksJWTErrors(t *testing.T) {
+	// This test proves that the naive errors.Is approach is broken:
+	// ErrTokenExpired would incorrectly match ErrTokenInvalid via Kratos errors.Is.
+	if !kratosErrors.Is(jwtMiddleware.ErrTokenExpired, jwtMiddleware.ErrTokenInvalid) {
+		t.Skip("Kratos errors.Is behavior has changed; this test documents the original masking bug")
+	}
+
+	// Now verify that our isWrappedErr correctly distinguishes them
+	st := toGRPCStatus(jwtMiddleware.ErrTokenExpired)
+	assert.False(t, isWrappedErr(st, jwtMiddleware.ErrTokenInvalid),
+		"isWrappedErr should NOT match ErrTokenExpired against ErrTokenInvalid")
+	assert.True(t, isWrappedErr(st, jwtMiddleware.ErrTokenExpired),
+		"isWrappedErr should match ErrTokenExpired against itself")
+}
+
+// TestIsWrappedErrGRPCWireRoundTrip verifies that isWrappedErr works after a
+// full gRPC wire round-trip: KratosError -> GRPCStatus -> proto bytes -> gRPC
+// status.FromError -> isWrappedErr. This simulates the actual path an error
+// takes from the server through a gRPC transport to the CLI client.
+func TestIsWrappedErrGRPCWireRoundTrip(t *testing.T) {
+	sentinels := []*kratosErrors.Error{
+		jwtMiddleware.ErrTokenExpired,
+		jwtMiddleware.ErrTokenInvalid,
+		jwtMiddleware.ErrTokenParseFail,
+		jwtMiddleware.ErrMissingJwtToken,
+	}
+
+	for _, sentinel := range sentinels {
+		t.Run(sentinel.Message, func(t *testing.T) {
+			// Step 1: Convert Kratos error to gRPC status (server side)
+			grpcSt := sentinel.GRPCStatus()
+
+			// Step 2: Serialize to the wire format (proto bytes)
+			proto := grpcSt.Proto()
+			require.NotNil(t, proto, "gRPC status proto should not be nil")
+
+			// Step 3: Deserialize from proto back into a gRPC status (client side)
+			roundTripped := status.FromProto(proto)
+			require.NotNil(t, roundTripped, "round-tripped status should not be nil")
+
+			// Step 4: Verify isWrappedErr still matches after the full round-trip
+			assert.True(t, isWrappedErr(roundTripped, sentinel),
+				"isWrappedErr should match %q after gRPC wire round-trip", sentinel.Message)
+
+			// Step 5: Verify it does NOT match a different sentinel after round-trip
+			for _, other := range sentinels {
+				if other == sentinel {
+					continue
+				}
+				assert.False(t, isWrappedErr(roundTripped, other),
+					"isWrappedErr should NOT match %q against %q after round-trip",
+					sentinel.Message, other.Message)
+			}
+		})
+	}
+}
+
+// TestJWTSentinelMessageCanary asserts the exact Message strings of the JWT
+// sentinel errors we depend on. If a Kratos update changes these strings
+// (e.g. ErrTokenParseFail's trailing space), this test will fail and alert us
+// that our Message-based matching in isWrappedErr needs updating.
+func TestJWTSentinelMessageCanary(t *testing.T) {
+	tests := []struct {
+		name    string
+		err     *kratosErrors.Error
+		wantMsg string
+	}{
+		{
+			name:    "ErrTokenExpired",
+			err:     jwtMiddleware.ErrTokenExpired,
+			wantMsg: "JWT token has expired",
+		},
+		{
+			name:    "ErrTokenInvalid",
+			err:     jwtMiddleware.ErrTokenInvalid,
+			wantMsg: "Token is invalid",
+		},
+		{
+			name:    "ErrTokenParseFail (note trailing space)",
+			err:     jwtMiddleware.ErrTokenParseFail,
+			wantMsg: "Fail to parse JWT token ", // trailing space is intentional
+		},
+		{
+			name:    "ErrMissingJwtToken",
+			err:     jwtMiddleware.ErrMissingJwtToken,
+			wantMsg: "JWT token is missing",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			require.Equal(t, tt.wantMsg, tt.err.Message,
+				"Kratos sentinel %s Message has changed — update isWrappedErr matching and case branches in errorInfo", tt.name)
+		})
+	}
+}
