fix: close code-review findings on keys, claims, and output

claims.go

@@ -1,9 +1,11 @@
 package main
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
+	"strconv"
 	"strings"
 
 	"github.com/golang-jwt/jwt/v5"
@@ -45,24 +47,17 @@ func validateClaimsSet(claims jwt.MapClaims, c claimChecks) (bool, error) {
 	if c.issuer != "" {
 		opts = append(opts, jwt.WithIssuer(c.issuer))
 	}
-	if err := jwt.NewValidator(opts...).Validate(claims); err != nil {
+	// The validator has to be shielded from timestamps it cannot convert: see
+	// unrepresentableTimeClaim, which would otherwise silently invert the
+	// verdict.
+	if err := unrepresentableTimeClaim(claims); err != nil {
 		return false, err
 	}
-	return true, nil
-}
 
-// verifyClaims parses the token, runs the requested claim validations, and
-// prints "Claims: VALID" or "Claims: INVALID" with the reason. It returns the
-// errInvalidClaims sentinel (wrapping the reason) when a check fails so the CLI
-// exits nonzero, mirroring signature verification; an unparseable token returns
-// a hard error instead. Claim validation is independent of the signature: it
-// runs with or without a key.
-func verifyClaims(w io.Writer, tokenStr string, c claimChecks) error {
-	p, err := parseUnverifiedJWT(tokenStr)
-	if err != nil {
-		return err
+	if err := jwt.NewValidator(opts...).Validate(claims); err != nil {
+		return false, err
 	}
-	return printClaimsVerdict(w, p.claims, c)
+	return true, nil
 }
 
 // printClaimsVerdict runs the requested checks against already-parsed claims
@@ -87,3 +82,57 @@ func printClaimsVerdict(w io.Writer, claims jwt.MapClaims, c claimChecks) error
 func claimReason(err error) string {
 	return strings.ReplaceAll(err.Error(), "\n", "; ")
 }
+
+// temporalClaimKeys are the numeric date claims the validator consults, in a
+// fixed order so the reported reason does not depend on map iteration.
+var temporalClaimKeys = [...]string{"exp", "nbf", "iat"}
+
+// unrepresentableTimeClaim reports the first temporal claim whose numeric value
+// does not name a time jwtd can represent.
+//
+// It has to run before the validator. golang-jwt converts these claims with a
+// Float64 whose error it discards and then casts to int64 (map_claims.go), so a
+// value outside int64 range wraps - typically to math.MinInt64 - and the
+// validator returns the opposite verdict: "exp":1e400 reads as long expired,
+// "nbf":1e400 as long since valid. Neither is visible in the output, because
+// claimTime declines to annotate a timestamp it cannot represent. Rejecting the
+// value up front makes the verdict say what is actually wrong and keeps it
+// agreeing with the displayed claims; the representability rule is claimTime's
+// own, so the two cannot drift.
+//
+// Only numeric values are examined: a non-numeric temporal claim is not a
+// timestamp at all, and the validator rejects it with its own message.
+func unrepresentableTimeClaim(claims jwt.MapClaims) error {
+	for _, key := range temporalClaimKeys {
+		val, ok := claims[key]
+		if !ok {
+			continue
+		}
+
+		var text string
+		switch num := val.(type) {
+		case json.Number:
+			text = num.String()
+		case float64:
+			text = strconv.FormatFloat(num, 'f', -1, 64)
+		default:
+			continue
+		}
+
+		if _, ok := claimTime(text); !ok {
+			return fmt.Errorf("%s claim %s is not a representable timestamp", key, truncateClaimValue(text))
+		}
+	}
+	return nil
+}
+
+// truncateClaimValue shortens a claim literal for an error message: a numeric
+// claim comes from the token and can be arbitrarily long, while the reason is
+// rendered on one line.
+func truncateClaimValue(text string) string {
+	const max = 32
+	if len(text) <= max {
+		return text
+	}
+	return text[:max] + "..."
+}

claims_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"bytes"
+	"encoding/json"
 	"errors"
 	"strings"
 	"testing"
@@ -301,3 +302,164 @@ func TestDecodeJWTJSON_SignatureTakesPrecedenceOverClaims(t *testing.T) {
 		t.Errorf("expected both verdicts false in JSON, got %q", out)
 	}
 }
+
+// golang-jwt converts the temporal claims with a Float64 whose error it
+// discards and then casts to int64, so a value outside int64 range wraps and
+// the validator returns the opposite verdict: an absurd "exp" reads as expired
+// and an absurd "nbf" as long since valid. Such a value must be reported as
+// what it is instead, and the verdict must not depend on which way it wrapped.
+func TestValidateClaimsSet_RejectsUnrepresentableTemporalClaims(t *testing.T) {
+	pinTime(t, 1000)
+
+	tests := []struct {
+		name   string
+		claims jwt.MapClaims
+		want   string
+	}{
+		{
+			name:   "exp past int64 range",
+			claims: jwt.MapClaims{"exp": json.Number("10000000000000000000")},
+			want:   "exp claim 10000000000000000000 is not a representable timestamp",
+		},
+		{
+			name:   "exp with an absurd exponent",
+			claims: jwt.MapClaims{"exp": json.Number("1e400")},
+			want:   "exp claim 1e400 is not a representable timestamp",
+		},
+		{
+			name:   "nbf with an absurd exponent",
+			claims: jwt.MapClaims{"nbf": json.Number("1e400")},
+			want:   "nbf claim 1e400 is not a representable timestamp",
+		},
+		{
+			name:   "nbf below int64 range",
+			claims: jwt.MapClaims{"nbf": json.Number("-10000000000000000000")},
+			want:   "nbf claim -10000000000000000000 is not a representable timestamp",
+		},
+		{
+			name:   "iat unrepresentable alongside a live exp",
+			claims: jwt.MapClaims{"iat": json.Number("1e400"), "exp": json.Number("2000")},
+			want:   "iat claim 1e400 is not a representable timestamp",
+		},
+		{
+			name:   "float64 claim past the representable range",
+			claims: jwt.MapClaims{"exp": 1e300},
+			want:   "exp claim",
+		},
+		{
+			name: "exp reported before nbf",
+			claims: jwt.MapClaims{
+				"exp": json.Number("1e400"),
+				"nbf": json.Number("1e400"),
+			},
+			want: "exp claim",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			valid, reason := validateClaimsSet(tt.claims, claimChecks{verify: true})
+			if valid {
+				t.Fatalf("expected an invalid verdict for %v", tt.claims)
+			}
+			if !strings.Contains(reason.Error(), tt.want) {
+				t.Errorf("reason %q missing %q", reason.Error(), tt.want)
+			}
+		})
+	}
+}
+
+// The pre-check must not change the verdict for values it can represent, and
+// must leave a non-numeric temporal claim to the validator's own message.
+func TestValidateClaimsSet_RepresentableAndNonNumericClaimsUnaffected(t *testing.T) {
+	pinTime(t, 1000)
+
+	tests := []struct {
+		name      string
+		claims    jwt.MapClaims
+		wantValid bool
+		wantAbout string
+	}{
+		{
+			name:      "ordinary integer seconds",
+			claims:    jwt.MapClaims{"exp": json.Number("2000"), "iat": json.Number("900")},
+			wantValid: true,
+		},
+		{
+			name:      "fractional seconds",
+			claims:    jwt.MapClaims{"exp": json.Number("2000.5")},
+			wantValid: true,
+		},
+		{
+			name:      "exponent form within range",
+			claims:    jwt.MapClaims{"exp": json.Number("2e3")},
+			wantValid: true,
+		},
+		{
+			name:      "string exp is the validator's to reject",
+			claims:    jwt.MapClaims{"exp": "tomorrow"},
+			wantValid: false,
+			wantAbout: "exp",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			valid, reason := validateClaimsSet(tt.claims, claimChecks{verify: true})
+			if valid != tt.wantValid {
+				t.Fatalf("valid = %v, want %v (reason: %v)", valid, tt.wantValid, reason)
+			}
+			if tt.wantValid {
+				return
+			}
+			if strings.Contains(reason.Error(), "representable") {
+				t.Errorf("a non-numeric claim must not be reported as unrepresentable: %v", reason)
+			}
+			if !strings.Contains(reason.Error(), tt.wantAbout) {
+				t.Errorf("reason %q missing %q", reason.Error(), tt.wantAbout)
+			}
+		})
+	}
+}
+
+// An unrepresentable claim reaches both output paths through the shared core,
+// so neither can report the wrapped verdict.
+func TestUnrepresentableClaimFailsBothOutputPaths(t *testing.T) {
+	pinTime(t, 1000)
+	token := makeJWT(`{"alg":"HS256"}`, `{"nbf":1e400}`, "sig")
+
+	t.Run("human", func(t *testing.T) {
+		var buf bytes.Buffer
+		err := decodeJWTHuman(&buf, token, "", claimChecks{verify: true})
+		if !errors.Is(err, errInvalidClaims) {
+			t.Fatalf("expected errInvalidClaims, got %v", err)
+		}
+		out := stripANSI(buf.String())
+		if !strings.Contains(out, "Claims: INVALID") || !strings.Contains(out, "nbf claim") {
+			t.Errorf("expected an INVALID verdict naming nbf, got %q", out)
+		}
+	})
+
+	t.Run("json", func(t *testing.T) {
+		var buf bytes.Buffer
+		err := decodeJWTJSON(&buf, token, "", claimChecks{verify: true})
+		if !errors.Is(err, errInvalidClaims) {
+			t.Fatalf("expected errInvalidClaims, got %v", err)
+		}
+		if !strings.Contains(buf.String(), `"claimsValid": false`) {
+			t.Errorf("expected claimsValid false, got %q", buf.String())
+		}
+	})
+}
+
+// The reason is rendered on one line, so a very long claim literal is truncated
+// rather than pasted into it whole.
+func TestTruncateClaimValue(t *testing.T) {
+	got := truncateClaimValue(strings.Repeat("9", 200))
+	if len(got) != 35 || !strings.HasSuffix(got, "...") {
+		t.Errorf("expected a 32-character prefix plus an ellipsis, got %q", got)
+	}
+	if short := truncateClaimValue("1758000000"); short != "1758000000" {
+		t.Errorf("a short value must be kept verbatim, got %q", short)
+	}
+}

helpers_test.go

@@ -365,15 +365,43 @@ func randomSymmetricKey(t *testing.T, size int) []byte {
 	return key
 }
 
-// verifySignature parses a token and renders its signature verdict. Production
-// code always has a parsedJWT in hand and calls printSignatureVerdict directly;
-// this wrapper exists so the signature tests can work from a compact string.
+// verifySignature parses a token, loads the key, and renders the signature
+// verdict. Production code always has a parsedJWT and a loaded key in hand and
+// calls printSignatureVerdict directly; this wrapper exists so the signature
+// tests can work from a compact string and a key argument.
 func verifySignature(w io.Writer, tokenStr, keyStr string) error {
 	p, err := parseUnverifiedJWT(tokenStr)
 	if err != nil {
 		return fmt.Errorf("signature verification: %w", err)
 	}
-	return printSignatureVerdict(w, p, keyStr)
+	kid, err := headerKID(p.header)
+	if err != nil {
+		return fmt.Errorf("signature verification: %w", err)
+	}
+	key, err := loadKeyForKID(keyStr, kid)
+	if err != nil {
+		return fmt.Errorf("signature verification: error loading key: %w", err)
+	}
+	return printSignatureVerdict(w, p, key)
+}
+
+// verifyClaims parses a token and renders its claim verdict. Production code
+// holds a parsedJWT at this point and calls printClaimsVerdict directly; this
+// wrapper exists so the claim tests can work from a compact string.
+func verifyClaims(w io.Writer, tokenStr string, c claimChecks) error {
+	p, err := parseUnverifiedJWT(tokenStr)
+	if err != nil {
+		return err
+	}
+	return printClaimsVerdict(w, p.claims, c)
+}
+
+// loadKey resolves a key argument without a kid, the shape most key tests
+// exercise. Production code always has a token header at this point and so
+// always knows the kid (possibly ""), which is why this wrapper lives with the
+// test helpers rather than in keys.go.
+func loadKey(keyStr string) (any, error) {
+	return loadKeyForKID(keyStr, "")
 }
 
 // --- JWS signature verification -----------------------------------------------
@@ -403,6 +431,22 @@ func signJWTWithHMAC(t *testing.T, key []byte, claims jwt.MapClaims) string {
 	return signed
 }
 
+// signJWTWithHMACHeader signs with HMAC-SHA256 like signJWTWithHMAC, but lets
+// the caller set extra header members — including ones RFC 7515 does not
+// allow, such as a non-string "kid".
+func signJWTWithHMACHeader(t *testing.T, key []byte, header map[string]any, claims jwt.MapClaims) string {
+	t.Helper()
+	token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims)
+	for k, v := range header {
+		token.Header[k] = v
+	}
+	signed, err := token.SignedString(key)
+	if err != nil {
+		t.Fatalf("signing JWT: %v", err)
+	}
+	return signed
+}
+
 type failOnWriteWriter struct {
 	failedWrite int
 	writes      int