fix: close code-review findings on keys, claims, and output - #32
Merged
Merged
Conversation
Six confirmed review findings:
- Pre-check exp/nbf/iat through claimTime before the golang-jwt validator.
parseNumericDate discards the Float64 error and casts to int64, so a value
outside int64 range wrapped and produced the opposite verdict ("nbf":1e400
reported Claims: VALID). An unrepresentable timestamp is now an explicit
invalid reason naming the claim, shared by the human and --json paths.
- Reject a null JWT header or payload with "expected JSON object", the guard
the JWE protected header already had. A null payload previously rendered as
{} with a VALID claim verdict.
- Resolve the key before printParsedJWT writes anything, so a bad --key emits
nothing on stdout. Verification splits into verifyLoadedKeySignature (the
core, taking a loaded key) and verifyJWTSignature (the key-argument wrapper
the --json path uses); the alg allowlist stays in the core.
- Report an empty or whitespace-only token argument as "no token provided"
instead of as a malformed token.
- Bound the stdin read with io.LimitReader at maxStdinTokenBytes (16 MiB) and
fail with a clear message instead of truncating.
- Remove the production-dead isJWT and verifyClaims; verifyClaims moves to
helpers_test.go beside verifySignature.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0125xMARDzdnbkbMTbNxSihH
Four confirmed review findings around key resolution:
- An "oct" JWK bypassed the empty-secret gate: parseJWK returned jwk.Key
directly, so {"kty":"oct","k":""} yielded an empty HMAC key and a token
forged with the empty secret verified. Every JWK path now unwraps through
jwkKey, which routes []byte through symmetricKey. errEmptyKey joins
errKIDNotFound as a final verdict (finalKeyError), so the rejection cannot
degrade into a base64 retry or the misleading "pass it as hmac:<file>" hint.
- headerKID collapsed a non-string "kid" to "", which parseJWK read as "no kid
named" and answered with the first JWK Set entry. RFC 7515 requires a string,
so a present non-string kid is now errNonStringKID; the JWT, --json, and JWE
call sites all propagate it.
- A directory passed as --key was misreported as base64 key material. It gets
its own keySource and a clear "is a directory, not a key file" error;
printKeyInterpretation still narrates only the readings that actually apply.
- Removed the production-dead loadKey wrapper (kept as a test helper in
helpers_test.go) and loadInlineKey's unreachable decode-failure branch.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0125xMARDzdnbkbMTbNxSihH
- thread the key through printDecryptedPayload into the nested JWE/JWT decode, so --key keeps applying one level down: a JWE wrapping another token to the same key now decrypts all the way, and a nested JWS gets a real Signature verdict whose errInvalidSignature drives the exit code. A nested token the key does not fit is retried keyless, keeping its previous output. - emit "decryptedPayload": null in --json for a JWE whose plaintext is the JSON literal null; it was a nil interface that omitempty dropped, leaving neither encrypted nor decryptedPayload in the object. - compute exp/nbf annotations from Unix seconds instead of time.Time.Sub, whose Duration saturates at ~292 years and rendered every distant claim as the same bogus 106751d. - measure a base64url part arithmetically instead of decoding a possibly huge ciphertext just to report its length. - AGENTS.md: describe the changed behaviour, correct the golang-jwt row (the segment work is parseUnverifiedJWT's, not jwt.ParseUnverified's), and add the test-only gopkg.in/yaml.v3 dependency. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125xMARDzdnbkbMTbNxSihH
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125xMARDzdnbkbMTbNxSihH
# Conflicts: # AGENTS.md
bsg62
force-pushed
the
fix/review-findings-2
branch
from
September 9, 2026 19:43
06ef804 to
f52fe27
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
1 participant
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
Closes the fourteen findings from the
/code-review mediumpass over the code base. Every finding was confirmed against the built binary before and after the fix; each has a test.Security and correctness
octJWK (keys.go): a JWK or JWK Set entry with"k":""became an empty HMAC key and verified a forged HS256 token. A newjwkKeyhelper routes every JWK unwrap throughsymmetricKey, and theerrEmptyKeysentinel stops the rejection from degrading into the base64 fallback and itshmac:<file>hint.kid(main.go,jwe.go,jsonout.go): a header with"kid":123or"kid":nullcollapsed to""and silently selected the first JWK Set key.headerKIDnow returns an error (token header "kid" must be a string (RFC 7515)), propagated at all call sites.claims.go): golang-jwt discards theFloat64error and casts toint64, so"exp":1e400reported expired and"nbf":1e400reported valid.exp/nbf/iatare pre-checked through the display's ownclaimTime, and an unrepresentable value is an explicitClaims: INVALIDreason.--key(output.go,jwe.go):printDecryptedPayloadrecursed with an empty key. The outer key is now threaded through, a nested JWE decrypts, a nested JWT gets a signature verdict, anderrInvalidSignaturefrom it reaches the exit code. A key that does not fit the inner token falls back to the previous keyless render.nullheader or payload (main.go): rendered as{}withClaims: VALID. Both are rejected withexpected JSON object, the guardjweProtectedHeaderMapalready had.CLI and output
--jsonpaths (printSignatureVerdicttakes a loaded key;verifyLoadedKeySignatureis the core).jwtd ""reportsno token provided; piped stdin is bounded bymaxStdinTokenBytes(16 MiB) with an explicit error instead of silent truncation.--keyerrors as such (keySourceDirectory) instead of being read as base64 text.--jsonemits"decryptedPayload": nullfor a null plaintext and omits the field only without a key (*anyinstead ofanywithomitempty).exp/nbfannotations are computed from Unix seconds (secondsBetween/humanizeSeconds), so far-future values no longer saturate attime.Duration's 106751d.base64URLLenmeasures arithmetically instead of decoding the ciphertext; a test pins it equal to a full decode over valid and invalid inputs.Cleanup
isJWT,verifyClaims, andloadKeyleft production code (the latter two moved tohelpers_test.go); the unreachable!okbranch inloadInlineKeyis gone.gopkg.in/yaml.v3row.Test plan
gofmt -l .,go vet ./...,go test ./...green🤖 Generated with Claude Code
https://claude.ai/code/session_0125xMARDzdnbkbMTbNxSihH