Merged
Conversation
Add JwtInfo command for initial testing
📝 WalkthroughWalkthroughAdds a new jwtinfo CLI and internal package to request, decode, inspect, and optionally validate JWTs (JWKS support); includes extensive tests and testdata, Nix/dev scripts and CI workflow edits, dependency bumps, a small syntax-highlighting API addition, and a revive line-length lint change. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant HTTP as Token Endpoint
participant Parser as JWT Parser
participant JWKS as JWKS Fetcher
participant Output as Formatter
CLI->>HTTP: RequestToken(request-url, formValues)
HTTP-->>CLI: Token response (application/jwt or application/json)
CLI->>Parser: DecodeBase64(accessToken)
Parser-->>CLI: headerBytes, claimsBytes
CLI->>Parser: ParseTokenData(accessToken, validation-url?)
alt validation-url provided
Parser->>JWKS: Fetch JWKS (validation-url)
JWKS-->>Parser: JWK set / error
Parser->>JWKS: Resolve key & validate signature
JWKS-->>Parser: Valid / Invalid
end
Parser-->>CLI: parsed *jwt.Token or error
CLI->>Output: PrintTokenInfo(JwtTokenData)
Output-->>CLI: formatted token info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
internal/jwtinfo/jwtinfo.go (1)
261-310: Consider removing large commented-out legacy blocks.These blocks add noise and make the active JWT flow harder to maintain. Prefer deleting or moving to docs/history.
Also applies to: 428-583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 261 - 310, Remove the large commented-out legacy function block for DisplayTokenInfo along with any associated commented code (the block referencing DisplayTokenInfo, getTokenHeadersMap, getTokenClaimsMap, getUnregisteredClaimsMap and style/table usage); either delete it entirely or extract and save it to a documentation/history file or git patch so the repo stays clean and the active JWT flow remains easy to maintain.internal/jwtinfo/jwtinfo_test.go (1)
299-306: Movedeferstatement before reading the body for consistency and safety.While this code works correctly (the body is still closed), placing
defer Close()immediately after the HTTP call is the idiomatic pattern. This ensures the resource is closed even ifReadAllpanics.♻️ Suggested reordering
rootResp, err := client.Get(serverRoot) require.NoError(t, err) + defer rootResp.Body.Close() rootBody, err := io.ReadAll(rootResp.Body) require.NoError(t, err) require.Contains(t, string(rootBody), "root handler") - - defer rootResp.Body.Close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo_test.go` around lines 299 - 306, Move the defer Close() to immediately after the HTTP call so the response body is always closed even if reading panics: after calling client.Get(serverRoot) and assigning rootResp, call defer rootResp.Body.Close() before performing io.ReadAll(rootResp.Body) and the subsequent assertions (rootResp, client.Get, serverRoot, rootResp.Body, io.ReadAll).cmd/jwtinfo.go (2)
25-25:keyfuncDefOverrideis declared but never initialized.This variable remains a zero-value
keyfunc.Override{}and is passed toParseTokenData. If this is intentional (to use defaults), consider removing the package-level variable and passingkeyfunc.Override{}directly, or document the intent. If it should be configurable, wire it to a flag or configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/jwtinfo.go` at line 25, The package-level variable keyfuncDefOverride is declared but left as the zero-value and then passed into ParseTokenData; either remove this unused global and pass a literal keyfunc.Override{} where ParseTokenData is called (or document that zero-value is intentional), or make it configurable by wiring keyfuncDefOverride to a flag/configuration and initializing it before use; locate references to keyfuncDefOverride and the call site ParseTokenData in cmd/jwtinfo.go and implement one of these fixes so the override value is explicit and intentional.
87-106: Consider markingrequest-urlas a required flag.The
request-urlflag is essential for the command to function, but it's not marked as required. Users invokingjwtinfowithout this flag will get a less helpful error fromRequestTokenrather than a clear "required flag not provided" message from Cobra.💡 Proposed enhancement to mark required flag
jwtinfoCmd.Flags().StringVar( &requestURL, flagNameRequestURL, "", "HTTP address to use for the JWT token request", ) + _ = jwtinfoCmd.MarkFlagRequired(flagNameRequestURL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/jwtinfo.go` around lines 87 - 106, The request-url flag is required for jwtinfo to operate; after registering the flag on jwtinfoCmd (where requestURL and flagNameRequestURL are used) call jwtinfoCmd.MarkFlagRequired(flagNameRequestURL) to mark it required (do this immediately after the StringVar registration block); handle the returned error from MarkFlagRequired (e.g., return or log) so startup fails loudly if marking the flag fails.internal/jwtinfo/main_test.go (1)
139-145: Ignoringos.ReadFileerror may mask test setup failures.If the corrupted JWKS test file is missing or unreadable, this handler will silently serve an empty response instead of failing clearly. Consider logging or returning an error status.
🔧 Proposed fix
corruptedJwksFile := "testdata/jwkset-from-rsa-private-key-corrupted.json" - jwksContent, _ := os.ReadFile(corruptedJwksFile) + jwksContent, err := os.ReadFile(corruptedJwksFile) + if err != nil { + log.Printf("failed to read corrupted JWKS file: %s", err) + writer.WriteHeader(http.StatusInternalServerError) + return + } writer.Header().Set("Content-Type", "application/json") _, _ = writer.Write(jwksContent)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/main_test.go` around lines 139 - 145, The handler currently ignores the error from os.ReadFile when loading corruptedJwksFile, which can mask missing or unreadable test fixtures; change the code that calls os.ReadFile(corruptedJwksFile) to check the returned error and fail the test immediately (e.g., call t.Fatalf with a clear message and the error) so jwksContent isn't empty, or alternatively return an HTTP error from the handler (http.Error or writer.WriteHeader(500)) if you prefer runtime failure; update the code paths around jwksContent, os.ReadFile, and writer.Write to handle the error instead of discarding it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/jwtinfo.go`:
- Around line 62-65: The DecodeBase64() error is logged but not handled,
allowing execution to continue with invalid tokenData and causing ParseTokenData
to operate on bad input; update the handler after tokenData.DecodeBase64() to
return (or exit) immediately on error (e.g., return from the enclosing function
or propagate the error) so ParseTokenData is not called with corrupt data—refer
to tokenData.DecodeBase64() and the subsequent ParseTokenData call in jwtinfo.go
to implement the early return.
In `@devenv.nix`:
- Around line 599-600: The scripts currently pass sensitive auth payloads via
the CLI (--request-values-json "$JWTINFO_TEST_AUTH0"), which can leak secrets;
add support in cmd/jwtinfo.go to accept a file-based input flag (e.g.,
--request-values-file) or stdin (e.g., --request-values-stdin) and parse JSON
from that source instead of only accepting the --request-values-json string,
then update the callers in devenv.nix to write the env var to a temp file or
pipe it to stdin and pass the new flag (reference cmd/jwtinfo.go for flag
parsing and the jwtinfo command handler that reads request values, and update
the invocation sites in devenv.nix to use --request-values-file or stdin instead
of --request-values-json).
- Around line 681-682: The two test entries run-jwtinfo-test-auth0 and
run-jwtinfo-test-auth0-no-validation are executed unconditionally and should be
gated; update the enterTest function to check for the JWTINFO_TEST_AUTH0
environment variable (and optionally outbound/network availability) before
enqueueing or executing those tests, returning a skipped/short-circuit path when
the variable is missing or network checks fail so enterTest does not attempt to
run provider-dependent tests without the required env or connectivity.
In `@internal/jwtinfo/jwtinfo.go`:
- Around line 96-112: The code currently accesses resp.Header["Content-Type"][0]
and compares exact strings which can panic when the header is missing and will
mis-detect values with parameters; update the parsing around readAll/resp.Body
handling to use resp.Header.Get("Content-Type") (or fetch safely) and normalize
the media type via mime.ParseMediaType (or strings.Split at ';') into a
mediatype variable, then compare mediatype == "application/jwt" and mediatype ==
"application/json" before assigning t.AccessToken or decoding with
json.NewDecoder; keep the existing use of readAll, resp.Body reset, and the
JwtTokenData variable (t) and json.NewDecoder call but change how
respContentType is obtained and compared to avoid panics and handle parameters.
- Around line 352-389: The code prints token output directly to stdout using
fmt.Print/Println instead of writing to the provided writer w; replace all
direct stdout writes (the empty newline prints, the header/claims highlighted
JSON outputs produced by style.CodeSyntaxHighlightWithStyle, and the final
newline) with writes to w (e.g., fmt.Fprintln(w, ""), fmt.Fprint(w,
style.CodeSyntaxHighlightWithStyle(...)) or io.WriteString(w, ...)) so
header/claims output and newlines use the provided writer in the token rendering
path that manipulates prettyJSON, token.header, token.claims and uses
style.CodeSyntaxHighlightWithStyle and the table rendering.
- Line 40: Fields AccessToken and RefreshToken use OAuth-standard snake_case
JSON tags and need targeted lint suppression. Add a per-field nolint for
tagliatelle (e.g., a trailing "// nolint:tagliatelle" comment) immediately after
the AccessToken and RefreshToken struct field declarations in jwtinfo.go so the
linter is suppressed only for those fields and other linting remains unaffected;
reference the AccessToken and RefreshToken field names when making the change.
- Around line 183-195: Guard against malformed tokens by checking token.raw
after splitting: ensure tokenB64Elements has at least two (preferably three)
segments and return a clear error if not before indexing; and decode the JWT
segments using base64.RawURLEncoding (not RawStdEncoding) when assigning
tokenHeader and tokenClaims to correctly handle base64url characters. Update
references around token.raw, tokenB64Elements, tokenHeader, and tokenClaims to
perform the length check and swap to base64.RawURLEncoding for decoding.
In `@internal/jwtinfo/main_test.go`:
- Around line 111-129: The jwk creation and storage error handling is currently
logging errors but allowing execution to continue; update jwksHandler so that
after calling jwkset.NewJWKFromKey(signKey, options) and jwkSet.KeyWrite(ctx,
jwk) you check err and immediately return an HTTP 500 (or appropriate) response
with a log message instead of proceeding to call jwkSet.JSONPublic;
specifically, on error from jwkset.NewJWKFromKey or jwkSet.KeyWrite write a
descriptive error to the response logger and call
writer.WriteHeader(http.StatusInternalServerError) and return to avoid using an
uninitialized jwkSet when calling jwkSet.JSONPublic.
- Around line 177-191: The code constructs tokenJSONString from tokenString
before checking the error returned by createToken; move the error check
immediately after calling createToken (i.e., check err from createToken right
away) and only build tokenJSONString after err == nil, ensuring you return/write
the HTTP 500 and log the error when createToken fails; reference createToken,
tokenString, tokenJSONString and the error handling block to reorder the logic
accordingly.
---
Nitpick comments:
In `@cmd/jwtinfo.go`:
- Line 25: The package-level variable keyfuncDefOverride is declared but left as
the zero-value and then passed into ParseTokenData; either remove this unused
global and pass a literal keyfunc.Override{} where ParseTokenData is called (or
document that zero-value is intentional), or make it configurable by wiring
keyfuncDefOverride to a flag/configuration and initializing it before use;
locate references to keyfuncDefOverride and the call site ParseTokenData in
cmd/jwtinfo.go and implement one of these fixes so the override value is
explicit and intentional.
- Around line 87-106: The request-url flag is required for jwtinfo to operate;
after registering the flag on jwtinfoCmd (where requestURL and
flagNameRequestURL are used) call
jwtinfoCmd.MarkFlagRequired(flagNameRequestURL) to mark it required (do this
immediately after the StringVar registration block); handle the returned error
from MarkFlagRequired (e.g., return or log) so startup fails loudly if marking
the flag fails.
In `@internal/jwtinfo/jwtinfo_test.go`:
- Around line 299-306: Move the defer Close() to immediately after the HTTP call
so the response body is always closed even if reading panics: after calling
client.Get(serverRoot) and assigning rootResp, call defer rootResp.Body.Close()
before performing io.ReadAll(rootResp.Body) and the subsequent assertions
(rootResp, client.Get, serverRoot, rootResp.Body, io.ReadAll).
In `@internal/jwtinfo/jwtinfo.go`:
- Around line 261-310: Remove the large commented-out legacy function block for
DisplayTokenInfo along with any associated commented code (the block referencing
DisplayTokenInfo, getTokenHeadersMap, getTokenClaimsMap,
getUnregisteredClaimsMap and style/table usage); either delete it entirely or
extract and save it to a documentation/history file or git patch so the repo
stays clean and the active JWT flow remains easy to maintain.
In `@internal/jwtinfo/main_test.go`:
- Around line 139-145: The handler currently ignores the error from os.ReadFile
when loading corruptedJwksFile, which can mask missing or unreadable test
fixtures; change the code that calls os.ReadFile(corruptedJwksFile) to check the
returned error and fail the test immediately (e.g., call t.Fatalf with a clear
message and the error) so jwksContent isn't empty, or alternatively return an
HTTP error from the handler (http.Error or writer.WriteHeader(500)) if you
prefer runtime failure; update the code paths around jwksContent, os.ReadFile,
and writer.Write to handle the error instead of discarding it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 594bb61f-5303-41f3-acba-672d65aa4076
⛔ Files ignored due to path filters (9)
devenv.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.suminternal/jwtinfo/testdata/ecdsa-plaintext-private-key.pemis excluded by!**/*.peminternal/jwtinfo/testdata/ed25519-plaintext-private-key.pemis excluded by!**/*.peminternal/jwtinfo/testdata/rsa-pkcs8-crt.pemis excluded by!**/*.peminternal/jwtinfo/testdata/rsa-pkcs8-csr.pemis excluded by!**/*.peminternal/jwtinfo/testdata/rsa-pkcs8-encrypted-private-key.pemis excluded by!**/*.peminternal/jwtinfo/testdata/rsa-pkcs8-plaintext-private-key.pemis excluded by!**/*.peminternal/jwtinfo/testdata/rsa-pkcs8-public-key.pemis excluded by!**/*.pem
📒 Files selected for processing (11)
.golangci.ymlcmd/jwtinfo.godevenv.nixgo.modinternal/jwtinfo/jwtinfo.gointernal/jwtinfo/jwtinfo_test.gointernal/jwtinfo/main_test.gointernal/jwtinfo/testdata/README.mdinternal/jwtinfo/testdata/jwkset-from-rsa-private-key-corrupted.jsoninternal/jwtinfo/testdata/jwkset-from-rsa-private-key-valid.jsoninternal/style/style_handlers.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/codeChecks.yml (1)
69-70: Remove unused JWT secret wiring from this CI step.On Line 70,
JWTINFO_TEST_AUTH0appears unnecessary for current tests (internal/jwtinfo/jwtinfo_test.goLines 89-228 andinternal/jwtinfo/main_test.goLines 154-221 are local/hardcoded). Keeping it adds avoidable secret handling in CI.Suggested cleanup
- name: Build the devenv shell and run any pre-commit hooks - env: - JWTINFO_TEST_AUTH0: ${{ secrets.JWTINFO_TEST_AUTH0 }} run: devenv test timeout-minutes: 15🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/codeChecks.yml around lines 69 - 70, Remove the unused CI environment variable wiring for JWTINFO_TEST_AUTH0: delete the env entry that sets JWTINFO_TEST_AUTH0 in the codeChecks.yml workflow so the CI step no longer exposes or requires that secret (since tests in internal/jwtinfo/jwtinfo_test.go and internal/jwtinfo/main_test.go use local/hardcoded values); ensure no other workflow steps reference JWTINFO_TEST_AUTH0 before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/codeChecks.yml:
- Line 87: The workflow currently sets continue-on-error: true which disables
the test-coverage gate; change it to be conditional so non-main branches remain
non-blocking but the main branch enforces failures by setting continue-on-error
to an expression like continue-on-error: ${{ github.ref != 'refs/heads/main' }}
(referencing the continue-on-error key and the .testcoverage.yml thresholds) so
coverage failures fail the pipeline on main while remaining non-blocking on
other branches.
---
Nitpick comments:
In @.github/workflows/codeChecks.yml:
- Around line 69-70: Remove the unused CI environment variable wiring for
JWTINFO_TEST_AUTH0: delete the env entry that sets JWTINFO_TEST_AUTH0 in the
codeChecks.yml workflow so the CI step no longer exposes or requires that secret
(since tests in internal/jwtinfo/jwtinfo_test.go and
internal/jwtinfo/main_test.go use local/hardcoded values); ensure no other
workflow steps reference JWTINFO_TEST_AUTH0 before committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64e818d1-752e-4a49-81cf-8a242c83344e
⛔ Files ignored due to path filters (1)
devenv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/codeChecks.ymldevenv.nixdevenv.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- devenv.nix
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
cmd/jwtinfo.go (2)
32-49: Address whitespace linting issues.The static analyzer reports missing whitespace on lines 34 and 47. Adding blank lines improves readability and satisfies the linter.
♻️ Proposed fix for wsl whitespace
Run: func(cmd *cobra.Command, args []string) { var err error + client := &http.Client{} requestValuesMap := make(map[string]string) if requestJSONValues != "" { requestValuesMap, err = jwtinfo.ParseRequestJSONValues( requestJSONValues, requestValuesMap, ) if err != nil { fmt.Printf( "error while parsing request's values JSON string: %s", err, ) + return } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/jwtinfo.go` around lines 32 - 49, The block inside the Run function around the requestJSONValues handling is missing blank lines which triggers whitespace linting errors; update the Run implementation (the anonymous Run func in cmd/jwtinfo.go) to add a blank line before declaring requestValuesMap and another blank line after the requestJSONValues parsing/err-handling block (the code that calls jwtinfo.ParseRequestJSONValues and checks err) so spacing/readability conforms to the linter.
88-107: Consider markingrequest-urlas a required flag.The
requestURLflag is essential for the command to function—RequestTokenwill return an error if it's empty. Marking it as required provides better UX by failing fast with a clear message.♻️ Proposed fix
jwtinfoCmd.Flags().StringVar( &requestURL, flagNameRequestURL, "", "HTTP address to use for the JWT token request", ) + _ = jwtinfoCmd.MarkFlagRequired(flagNameRequestURL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/jwtinfo.go` around lines 88 - 107, The requestURL flag is required but currently optional; mark the flag on jwtinfoCmd as required by calling the Cobra required flag helper after it’s defined (e.g., jwtinfoCmd.MarkFlagRequired(flagNameRequestURL)) so the CLI fails early with a clear message when request-url is not provided; ensure this happens alongside the existing StringVar setup that binds requestURL and consider updating any command initialization that registers flags for jwtinfoCmd if needed.internal/jwtinfo/jwtinfo.go (4)
68-82: Minor:urlReqValues.Encode()called twice.The encoded form values are computed once for the request body (line 71) and again for the
Content-Lengthheader (line 82). Consider storing the encoded string to avoid redundant encoding.♻️ Proposed fix
+ encodedValues := urlReqValues.Encode() + req, err := http.NewRequest( "POST", reqURL, - strings.NewReader(urlReqValues.Encode()), + strings.NewReader(encodedValues), ) if err != nil { return JwtTokenData{}, fmt.Errorf( "HTTP error while defining token data request: %w", err, ) } req.Header.Add("User-Agent", userAgent) req.Header.Add("Content-Type", "application/x-www-form-urlencoded") - req.Header.Add("Content-Length", strconv.Itoa(len(urlReqValues.Encode()))) + req.Header.Add("Content-Length", strconv.Itoa(len(encodedValues)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 68 - 82, The code currently calls urlReqValues.Encode() twice when building the request in the function that creates the POST (used for the request body and Content-Length); to fix, compute a single encoded string variable (e.g., encoded := urlReqValues.Encode()) and use that for both strings.NewReader(encoded) and strconv.Itoa(len(encoded)) when setting the Content-Length header, leaving the existing headers (User-Agent, Content-Type) unchanged.
1-1: Test coverage below threshold (42% vs 70% required).The static analysis indicates test coverage is at 42%, below the 70% threshold. Key functions that may need additional test coverage include
ParseRequestJSONValues, edge cases inRequestToken(various Content-Type responses), andunmarshallTokenTimeClaims.Would you like me to help identify specific test cases or generate test scaffolding to improve coverage?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` at line 1, Test coverage is below threshold because key functions lack unit tests—add focused tests for ParseRequestJSONValues (valid JSON, missing keys, invalid JSON, nil body), RequestToken (mock HTTP responses for application/json, text/plain, unexpected Content-Type, HTTP errors and non-200 status), and unmarshallTokenTimeClaims (numeric timestamps, string timestamps, RFC3339 strings, missing/invalid time claims) using table-driven tests and HTTP test servers/mocks to exercise each branch and error path; ensure tests assert returned values and errors for functions ParseRequestJSONValues, RequestToken, and unmarshallTokenTimeClaims to drive coverage above 70%.
266-315: Remove or document commented-out code.Large blocks of commented-out code (lines 266-315 and 433-588) reduce readability. Consider removing this code and relying on version control history, or converting useful portions to documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 266 - 315, The file contains a large commented-out function DisplayTokenInfo (and other large commented blocks) that reduce readability; either delete these commented blocks or convert them to proper documentation/comments explaining why they’re preserved, or restore the code if it’s still needed (e.g., by re-enabling DisplayTokenInfo and adding tests). Locate the commented DisplayTokenInfo block and any other multi-line comment blocks, then remove them if obsolete, move essential snippets into a short doc comment above the related functions (or into markdown docs), or uncomment/restore the function and ensure it compiles and has tests.
31-38: Remove unusedCustomClaimsstruct or document its intended purpose.
CustomClaimsshadows embeddedjwt.RegisteredClaimsfields with incompatible types (e.g.,int64vs*jwt.NumericDate), which would cause unexpected behavior during JSON unmarshaling. However, the struct is not used anywhere in the codebase—ParseTokenDatausesjwt.RegisteredClaimsdirectly, and tests use the separateCustomClaimsExampletype. If this struct was intended as a template for custom JWT claims, add a comment explaining its purpose; otherwise, remove it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 31 - 38, CustomClaims currently shadows jwt.RegisteredClaims with incompatible types and is unused; either remove it or convert it into a documented example that correctly embeds jwt.RegisteredClaims types. Update or delete the CustomClaims declaration so it no longer conflicts with jwt.RegisteredClaims (or add a comment explaining it’s only an example/template), and ensure ParseTokenData and existing tests (which use CustomClaimsExample) continue to reference jwt.RegisteredClaims; if you keep it, change the fields to use jwt.NumericDate pointers consistent with jwt.RegisteredClaims and document its intended purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/jwtinfo/jwtinfo.go`:
- Around line 222-224: Fix the typo in the comment inside the ParseTokenData
function: change "Parsing the access token whitout validation" to "Parsing the
access token without validation" so the inline comment in the ParseTokenData
function (jwtinfo.go) reads correctly.
- Around line 400-431: unmarshallTokenTimeClaims currently verifies "iat" and
"exp" exist but only converts float64 values, which yields empty entries when
those claims are strings; update the function (unmarshallTokenTimeClaims) to
validate the types of genericClaims["iat"] and genericClaims["exp"] explicitly:
if a value is float64 convert as now, else if it's a string attempt to parse it
as either an integer UNIX timestamp (strconv.ParseInt) or an RFC3339 time
(time.Parse) and convert to UTC string, and if neither succeeds return an error
indicating the claim is not a valid timestamp; ensure the produced tokenClaims
map contains the string timestamp for both keys or the function errors out.
In `@internal/jwtinfo/main_test.go`:
- Around line 141-151: The handler jwksFaultyHandler currently ignores the error
returned by os.ReadFile; change it to capture the (jwksContent, err) result,
check err, and on error write an appropriate HTTP error response (e.g., set
status code 500 and a clear message or log the error) instead of writing empty
content, ensuring writer.Write is only called when jwksContent was successfully
read.
- Around line 215-222: The branch checking scope == "appJwtInvalid" currently
prints to stdout and returns without writing an HTTP status or body; update that
branch (the scope == "appJwtInvalid" handler) to write a proper HTTP response
using the provided http.ResponseWriter (w) — call w.WriteHeader(...) with the
intended status (e.g. http.StatusOK or http.StatusUnauthorized as tests expect)
and write an explicit body (even an empty body or a short message like "invalid
jwt") via w.Write or fmt.Fprintln(w, ...) instead of fmt.Println; keep the
existing Content-Type header logic.
---
Nitpick comments:
In `@cmd/jwtinfo.go`:
- Around line 32-49: The block inside the Run function around the
requestJSONValues handling is missing blank lines which triggers whitespace
linting errors; update the Run implementation (the anonymous Run func in
cmd/jwtinfo.go) to add a blank line before declaring requestValuesMap and
another blank line after the requestJSONValues parsing/err-handling block (the
code that calls jwtinfo.ParseRequestJSONValues and checks err) so
spacing/readability conforms to the linter.
- Around line 88-107: The requestURL flag is required but currently optional;
mark the flag on jwtinfoCmd as required by calling the Cobra required flag
helper after it’s defined (e.g.,
jwtinfoCmd.MarkFlagRequired(flagNameRequestURL)) so the CLI fails early with a
clear message when request-url is not provided; ensure this happens alongside
the existing StringVar setup that binds requestURL and consider updating any
command initialization that registers flags for jwtinfoCmd if needed.
In `@internal/jwtinfo/jwtinfo.go`:
- Around line 68-82: The code currently calls urlReqValues.Encode() twice when
building the request in the function that creates the POST (used for the request
body and Content-Length); to fix, compute a single encoded string variable
(e.g., encoded := urlReqValues.Encode()) and use that for both
strings.NewReader(encoded) and strconv.Itoa(len(encoded)) when setting the
Content-Length header, leaving the existing headers (User-Agent, Content-Type)
unchanged.
- Line 1: Test coverage is below threshold because key functions lack unit
tests—add focused tests for ParseRequestJSONValues (valid JSON, missing keys,
invalid JSON, nil body), RequestToken (mock HTTP responses for application/json,
text/plain, unexpected Content-Type, HTTP errors and non-200 status), and
unmarshallTokenTimeClaims (numeric timestamps, string timestamps, RFC3339
strings, missing/invalid time claims) using table-driven tests and HTTP test
servers/mocks to exercise each branch and error path; ensure tests assert
returned values and errors for functions ParseRequestJSONValues, RequestToken,
and unmarshallTokenTimeClaims to drive coverage above 70%.
- Around line 266-315: The file contains a large commented-out function
DisplayTokenInfo (and other large commented blocks) that reduce readability;
either delete these commented blocks or convert them to proper
documentation/comments explaining why they’re preserved, or restore the code if
it’s still needed (e.g., by re-enabling DisplayTokenInfo and adding tests).
Locate the commented DisplayTokenInfo block and any other multi-line comment
blocks, then remove them if obsolete, move essential snippets into a short doc
comment above the related functions (or into markdown docs), or
uncomment/restore the function and ensure it compiles and has tests.
- Around line 31-38: CustomClaims currently shadows jwt.RegisteredClaims with
incompatible types and is unused; either remove it or convert it into a
documented example that correctly embeds jwt.RegisteredClaims types. Update or
delete the CustomClaims declaration so it no longer conflicts with
jwt.RegisteredClaims (or add a comment explaining it’s only an
example/template), and ensure ParseTokenData and existing tests (which use
CustomClaimsExample) continue to reference jwt.RegisteredClaims; if you keep it,
change the fields to use jwt.NumericDate pointers consistent with
jwt.RegisteredClaims and document its intended purpose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71e05fd5-f384-441c-8f7a-27d35e28ad58
📒 Files selected for processing (4)
.github/workflows/codeChecks.ymlcmd/jwtinfo.gointernal/jwtinfo/jwtinfo.gointernal/jwtinfo/main_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/codeChecks.yml
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
internal/jwtinfo/jwtinfo.go (4)
68-82: Avoid callingurlReqValues.Encode()twice.
urlReqValues.Encode()is called on line 71 (for the request body) and again on line 82 (for Content-Length). Cache the encoded value to avoid redundant work.♻️ Proposed fix
+ encodedValues := urlReqValues.Encode() + req, err := http.NewRequest( "POST", reqURL, - strings.NewReader(urlReqValues.Encode()), + strings.NewReader(encodedValues), ) if err != nil { return JwtTokenData{}, fmt.Errorf( "HTTP error while defining token data request: %w", err, ) } req.Header.Add("User-Agent", userAgent) req.Header.Add("Content-Type", "application/x-www-form-urlencoded") - req.Header.Add("Content-Length", strconv.Itoa(len(urlReqValues.Encode()))) + req.Header.Add("Content-Length", strconv.Itoa(len(encodedValues)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 68 - 82, Cache the result of urlReqValues.Encode() into a local variable before calling http.NewRequest and reuse that variable when setting the Content-Length header to avoid encoding twice; update the code around the http.NewRequest call (the call that uses urlReqValues.Encode()), and the subsequent req.Header.Add("Content-Length", strconv.Itoa(...)) to reference the cached encoded string.
441-596: Remove commented-out code.Lines 441-596 contain multiple commented-out functions (
unmarshallTokenClaims,unmarshallTokenHeader,getTokenClaimsMap,getUnregisteredClaimsMap,getTokenHeadersMap). If these are no longer needed, remove them. Version control preserves history if you need to recover them later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 441 - 596, Remove the large commented-out blocks containing the unused functions unmarshallTokenClaims, unmarshallTokenHeader, getTokenClaimsMap, getUnregisteredClaimsMap, and getTokenHeadersMap; delete their commented code entirely so the file no longer contains those dead code comments, then run gofmt/organize imports and run tests or build to ensure no references remain to those function names.
266-315: Remove commented-out code before merging.The entire
DisplayTokenInfofunction (lines 266-315) is commented out. If this code is no longer needed, remove it to maintain code cleanliness. If it's work-in-progress, consider tracking it in a separate branch or issue instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 266 - 315, The commented-out DisplayTokenInfo function is dead code; remove the entire commented block for DisplayTokenInfo (the commented function starting with "func DisplayTokenInfo" and ending at its closing brace) to clean up the file, or if you intend to keep it, move the implementation into a dedicated branch or open an issue instead of leaving it commented inline; ensure no references to DisplayTokenInfo remain in other code before deleting.
319-331: Remove commented-out code withinPrintTokenInfo.There are commented-out lines related to validity display (lines 319-320, 326-331). Either complete this functionality or remove the dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 319 - 331, The PrintTokenInfo function contains commented-out code for rendering a "Valid" boolean that should be removed or implemented; update the PrintTokenInfo implementation in jwtinfo.go to either delete the commented lines (sTrue/sFalse, validString logic, and the fmt.Fprintln that prints "Valid") or restore them by using the existing style helpers (style.BoolTrue.Render, style.BoolFalse.Render) and the token's Valid field (t.Valid) to produce the CertKeyP3 "Valid" line; ensure you reference PrintTokenInfo, the style helpers (style.BoolTrue/BoolFalse or style.LgSprintf/style.CertKeyP3), and the token variable (t) when making the change.internal/jwtinfo/main_test.go (1)
235-248: Consider simplifyingNewJwtTestServerreturn signature.The function always returns
nilfor the error. If error handling isn't planned, simplify the signature to return only*httptest.Server.♻️ Proposed simplification
-func NewJwtTestServer() (*httptest.Server, error) { +func NewJwtTestServer() *httptest.Server { mux := http.NewServeMux() mux.HandleFunc("/", rootHandler) mux.HandleFunc("/jwt", authHandler) mux.HandleFunc("/jwks.json", jwksHandler) mux.HandleFunc("/jwksFaulty.json", jwksFaultyHandler) mux.HandleFunc("/jwksEmpty.json", jwksEmptyHandler) ts := httptest.NewUnstartedServer(mux) ts.EnableHTTP2 = true ts.StartTLS() - return ts, nil + return ts }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/main_test.go` around lines 235 - 248, The NewJwtTestServer function always returns a nil error, so change its signature to return only *httptest.Server (remove the error return), update the function body to return ts instead of (ts, nil), and update all callers of NewJwtTestServer to stop expecting an error (remove error variables and checks). Ensure referenced symbols are NewJwtTestServer, rootHandler, authHandler, jwksHandler, jwksFaultyHandler, and jwksEmptyHandler so callers continue to use the same test server instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/jwtinfo/jwtinfo.go`:
- Around line 31-38: Remove the unused and conflicting CustomClaims type
declaration: delete the entire type CustomClaims struct that embeds
jwt.RegisteredClaims (named symbols: CustomClaims and jwt.RegisteredClaims) to
avoid duplicate JSON field mappings; then run a quick repo-wide search to ensure
there are no remaining references to CustomClaims and remove any now-unused
imports introduced solely for that type.
- Line 1: Add unit tests to raise file coverage above 70% by exercising the
untested functions: write tests for PrintTokenInfo that validate output for
valid and malformed token strings (capture stdout/stderr), tests for the
JwtTokenData.DecodeBase64 method covering valid base64, invalid base64 (expect
error), and empty input, and tests for unmarshallTokenTimeClaims that exercise
both successful unmarshalling of numeric and string time claims and error paths
when claim values are of unexpected types; use the existing
ParseTokenData/RequestToken helpers where helpful and assert returned values and
errors from PrintTokenInfo, JwtTokenData.DecodeBase64, and
unmarshallTokenTimeClaims to cover their branches.
In `@internal/jwtinfo/main_test.go`:
- Around line 72-89: The created JWT in createToken sets ExpiresAt but omits
IssuedAt, causing unmarshallTokenTimeClaims (used by PrintTokenInfo) to fail;
update createToken to populate jwt.RegisteredClaims.IssuedAt (e.g.,
jwt.NewNumericDate(time.Now())) alongside ExpiresAt in the CustomClaimsExample,
and also fix the "Creat" typo to "Create" in the token string comment near
SignedString(signKey).
---
Nitpick comments:
In `@internal/jwtinfo/jwtinfo.go`:
- Around line 68-82: Cache the result of urlReqValues.Encode() into a local
variable before calling http.NewRequest and reuse that variable when setting the
Content-Length header to avoid encoding twice; update the code around the
http.NewRequest call (the call that uses urlReqValues.Encode()), and the
subsequent req.Header.Add("Content-Length", strconv.Itoa(...)) to reference the
cached encoded string.
- Around line 441-596: Remove the large commented-out blocks containing the
unused functions unmarshallTokenClaims, unmarshallTokenHeader,
getTokenClaimsMap, getUnregisteredClaimsMap, and getTokenHeadersMap; delete
their commented code entirely so the file no longer contains those dead code
comments, then run gofmt/organize imports and run tests or build to ensure no
references remain to those function names.
- Around line 266-315: The commented-out DisplayTokenInfo function is dead code;
remove the entire commented block for DisplayTokenInfo (the commented function
starting with "func DisplayTokenInfo" and ending at its closing brace) to clean
up the file, or if you intend to keep it, move the implementation into a
dedicated branch or open an issue instead of leaving it commented inline; ensure
no references to DisplayTokenInfo remain in other code before deleting.
- Around line 319-331: The PrintTokenInfo function contains commented-out code
for rendering a "Valid" boolean that should be removed or implemented; update
the PrintTokenInfo implementation in jwtinfo.go to either delete the commented
lines (sTrue/sFalse, validString logic, and the fmt.Fprintln that prints
"Valid") or restore them by using the existing style helpers
(style.BoolTrue.Render, style.BoolFalse.Render) and the token's Valid field
(t.Valid) to produce the CertKeyP3 "Valid" line; ensure you reference
PrintTokenInfo, the style helpers (style.BoolTrue/BoolFalse or
style.LgSprintf/style.CertKeyP3), and the token variable (t) when making the
change.
In `@internal/jwtinfo/main_test.go`:
- Around line 235-248: The NewJwtTestServer function always returns a nil error,
so change its signature to return only *httptest.Server (remove the error
return), update the function body to return ts instead of (ts, nil), and update
all callers of NewJwtTestServer to stop expecting an error (remove error
variables and checks). Ensure referenced symbols are NewJwtTestServer,
rootHandler, authHandler, jwksHandler, jwksFaultyHandler, and jwksEmptyHandler
so callers continue to use the same test server instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6c666e3-4e26-40f2-9287-42f3c889fe82
📒 Files selected for processing (2)
internal/jwtinfo/jwtinfo.gointernal/jwtinfo/main_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/jwtinfo/jwtinfo.go (4)
229-243: Consider accepting a context parameter for cancellation support.The function creates a new
context.Background()internally. For better control over cancellation and timeouts when fetching JWKS from external URLs, consider accepting acontext.Contextparameter from the caller.♻️ Proposed signature change
-func ParseTokenData(jtd JwtTokenData, jwksURL string, keyfuncOverride keyfunc.Override) (*jwt.Token, error) { +func ParseTokenData(ctx context.Context, jtd JwtTokenData, jwksURL string, keyfuncOverride keyfunc.Override) (*jwt.Token, error) { // Parsing the access token without validation if jwksURL == "" { token, _, err := jwt.NewParser().ParseUnverified( jtd.AccessToken, &jwt.RegisteredClaims{}, ) if err != nil { return nil, fmt.Errorf( "unable to parse unverified access token: %w", err, ) } return token, nil } // Parsing and validating the access token - ctx := context.Background() - jwks, err := keyfunc.NewDefaultOverrideCtx( ctx,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 229 - 243, The function currently creates ctx := context.Background() before calling keyfunc.NewDefaultOverrideCtx; change the function signature to accept a context.Context parameter (e.g., ctx context.Context) and use that context when calling keyfunc.NewDefaultOverrideCtx (replacing context.Background()), update any internal variable named ctx accordingly, and propagate the new parameter to all callers/tests that invoke this function so callers can cancel or set timeouts when fetching the JWKS from jwksURL; ensure imports and any documentation/comments reflect the new signature.
431-586: Remove large block of commented-out code.These ~155 lines of commented-out functions (
unmarshallTokenClaims,unmarshallTokenHeader,getTokenClaimsMap,getUnregisteredClaimsMap,getTokenHeadersMap) appear to be dead code from a previous implementation. Version control history preserves them if needed for reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 431 - 586, Remove the large commented-out block that defines unmarshallTokenClaims, unmarshallTokenHeader, getTokenClaimsMap, getUnregisteredClaimsMap, and getTokenHeadersMap; delete the entire commented region so the file no longer contains those dead functions, then run a build/lint to remove any now-unused imports (e.g., fmt, time, strings) and ensure there are no remaining references to those function names elsewhere.
415-426: Minor: Function converts all float64 claims to timestamps.The loop converts every
float64value in the claims to a UTC timestamp string, even though onlyiatandexpare used by the caller. This works correctly but could be simplified to only extract the specific time claims needed.♻️ Proposed simplification
- for k, v := range genericClaims { - var vi any = v - - if vf, ok := vi.(float64); ok { - vInt64 := int64(vf) - t := time.Unix(vInt64, 0) - dateUtc := t.UTC().String() - tokenClaims[k] = fmt.Sprintf("%v", dateUtc) - - continue - } - } + for _, key := range []string{"iat", "exp"} { + if vf, ok := genericClaims[key].(float64); ok { + t := time.Unix(int64(vf), 0) + tokenClaims[key] = t.UTC().String() + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 415 - 426, The loop over genericClaims in jwtinfo.go currently converts every float64 value to a UTC timestamp string and stores it in tokenClaims; restrict this behavior to only handle the known time-related claim keys (e.g., "iat" and "exp") instead of all float64s. Modify the for k, v := range genericClaims block to check if k == "iat" || k == "exp" (or any other explicit time claim names used by the caller) before converting the float64 to time.Unix and assigning into tokenClaims, leaving other claim types untouched.
256-305: Remove commented-outDisplayTokenInfofunction.This 50-line commented block appears to be superseded by
PrintTokenInfo. If no longer needed, remove it to reduce code clutter. If needed for reference, the version control history preserves it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 256 - 305, Remove the dead 50-line commented-out DisplayTokenInfo function block from jwtinfo.go: delete the entire commented function (the code starting with "// func DisplayTokenInfo(t *jwt.Token, w io.Writer) error {" through its closing "}" lines) since PrintTokenInfo supersedes it and VCS preserves history; ensure no other references to DisplayTokenInfo remain in the file after removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/jwtinfo/jwtinfo.go`:
- Around line 229-243: The function currently creates ctx :=
context.Background() before calling keyfunc.NewDefaultOverrideCtx; change the
function signature to accept a context.Context parameter (e.g., ctx
context.Context) and use that context when calling keyfunc.NewDefaultOverrideCtx
(replacing context.Background()), update any internal variable named ctx
accordingly, and propagate the new parameter to all callers/tests that invoke
this function so callers can cancel or set timeouts when fetching the JWKS from
jwksURL; ensure imports and any documentation/comments reflect the new
signature.
- Around line 431-586: Remove the large commented-out block that defines
unmarshallTokenClaims, unmarshallTokenHeader, getTokenClaimsMap,
getUnregisteredClaimsMap, and getTokenHeadersMap; delete the entire commented
region so the file no longer contains those dead functions, then run a
build/lint to remove any now-unused imports (e.g., fmt, time, strings) and
ensure there are no remaining references to those function names elsewhere.
- Around line 415-426: The loop over genericClaims in jwtinfo.go currently
converts every float64 value to a UTC timestamp string and stores it in
tokenClaims; restrict this behavior to only handle the known time-related claim
keys (e.g., "iat" and "exp") instead of all float64s. Modify the for k, v :=
range genericClaims block to check if k == "iat" || k == "exp" (or any other
explicit time claim names used by the caller) before converting the float64 to
time.Unix and assigning into tokenClaims, leaving other claim types untouched.
- Around line 256-305: Remove the dead 50-line commented-out DisplayTokenInfo
function block from jwtinfo.go: delete the entire commented function (the code
starting with "// func DisplayTokenInfo(t *jwt.Token, w io.Writer) error {"
through its closing "}" lines) since PrintTokenInfo supersedes it and VCS
preserves history; ensure no other references to DisplayTokenInfo remain in the
file after removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f27aba84-8bab-4028-947b-2f2a1c536eb4
📒 Files selected for processing (2)
internal/jwtinfo/jwtinfo.gointernal/jwtinfo/main_test.go
Owner
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/codeChecks.yml:
- Around line 49-75: The CI removed the devenv_test and goreleaser_test jobs,
which stops exercising the end-to-end path that relies on the new jwtinfo
scripts and artifacts; restore an equivalent CI job by either pinning a
repo-compatible devenv version or re-adding a job named devenv_test that
installs the pinned devenv, builds ./dist/https-wrench before running devenv
test, and sets JWTINFO_TEST_AUTH0 in the environment, and also reintroduce
goreleaser_test (or an equivalent step) to validate .goreleaser.yaml changes;
ensure this restored job does not run run-go-tests (since run-go-tests is
intentionally excluded from enterTest) and reference the existing job names
devenv_test, goreleaser_test, and the env var JWTINFO_TEST_AUTH0 when making
changes.
In `@internal/jwtinfo/jwtinfo_test.go`:
- Around line 189-190: Tests that set serverJwtEndpoint to external URLs (e.g.,
when tt.scope == "wrongReqUrl" and the other case at lines referenced) use
ambient DNS and are flaky; change these negative-path cases to use a local
httptest.Server instead: create an httptest.Server that intentionally returns
the desired failure (e.g., 404 or invalid JWKS body) and assign its URL to
serverJwtEndpoint in the test table entries for the "wrongReqUrl" and the other
failing scope, so the test controls the endpoint behavior reliably (update the
test setup where serverJwtEndpoint is read and ensure the server is closed after
the test).
- Around line 17-23: Tests currently build expected results from mutated
inputMap, letting ParseRequestJSONValues' in-place changes hide failures;
instead, for each subtest create an independent expected map (e.g., clone the
original base map and then add the two JSON keys "testKey2" and "testKey3") and
compare that expected map to the function's output; update the test cases that
use inputMap, mapToValidJSON, sourceMaps, and outputMap so you never derive
expected values from the post-call inputMap but instead construct the full
expected map before calling ParseRequestJSONValues and assert full equality
against the returned/mutated map.
In `@internal/jwtinfo/jwtinfo.go`:
- Around line 194-199: The code currently wraps a stale/previous `err` when
`isValidJSON(tokenHeader)` (and likewise for `tokenPayload`) fails, which can
misreport or wrap nil; change the error returns in the `isValidJSON` checks to
create a fresh error without `%w` and the old `err`—e.g. return
fmt.Errorf("invalid JSON found in header from %s", token.name) for the
`tokenHeader` case and a similar plain fmt.Errorf for the `tokenPayload` case;
update the return sites that reference `isValidJSON`, `tokenHeader`,
`tokenPayload`, and `token.name` accordingly.
- Around line 180-183: The current check allows tokens with extra dot segments
(e.g. "a.b.c.d"); update the validation that splits token.raw into
tokenB64Elements to require exactly three segments (change the condition from
len(tokenB64Elements) < 3 to len(tokenB64Elements) != 3) so any token with fewer
or more segments returns an error for token.name, rejecting malformed inputs;
also update the accompanying error message if desired to state "invalid
three-segment JWT format" for clarity.
- Around line 251-257: Replace the non-cancelable context used for
keyfunc.NewDefaultOverrideCtx with a cancellable one: create ctx, cancel :=
context.WithCancel(context.Background()) and immediately defer cancel() in the
surrounding function (e.g., ParseTokenData) so the JWKS refresh goroutine is
terminated after validation; keep passing the same ctx to
keyfunc.NewDefaultOverrideCtx (the call site shown uses jwksURL and
keyfuncOverride) to avoid leaking background goroutines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6655b247-0cdf-49d1-a329-4be46aa94004
📒 Files selected for processing (5)
.github/workflows/codeChecks.ymldevenv.nixinternal/jwtinfo/jwtinfo.gointernal/jwtinfo/jwtinfo_test.gointernal/jwtinfo/main_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- devenv.nix
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/jwtinfo/jwtinfo.go (3)
331-343: Remove commented-out code within function.The commented lines (331-332, 338-343) for
sTrue/sFalseand validation logic add clutter. Either integrate them or remove them entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 331 - 343, Remove the dead commented-out code inside the JwtInfo printing block: delete the commented variable declarations for sTrue/sFalse (style.BoolTrue, style.BoolFalse) and the commented Valid logic and its fmt.Fprintln lines so only the active fmt.Fprintln calls (style.LgSprintf(style.Cmd, "JwtInfo")) remain; if you intend to show token validity later, reintroduce this logic as real code that reads t.Valid and uses style.BoolTrue/BoolFalse and style.LgSprintf, otherwise purge the commented lines to reduce clutter.
453-608: Remove large commented-out code block.This ~155-line block of commented functions (
unmarshallTokenClaims,unmarshallTokenHeader,getTokenClaimsMap, etc.) significantly impacts readability. Rely on version control for historical reference instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 453 - 608, Delete the large commented-out block containing the unused functions unmarshallTokenClaims, unmarshallTokenHeader, getTokenClaimsMap, getUnregisteredClaimsMap, and getTokenHeadersMap from jwtinfo.go to improve readability; keep no remnants of these commented functions (comments or dead code) and rely on VCS history if needed, ensuring any references to these function names elsewhere in the file/package are not left dangling.
278-327: Remove commented-outDisplayTokenInfofunction.This 50-line commented block appears to be legacy code superseded by
PrintTokenInfo. Keeping it adds noise and maintenance burden—use version control history if you need to reference it later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/jwtinfo/jwtinfo.go` around lines 278 - 327, Remove the large commented-out DisplayTokenInfo function block (the entire legacy 50-line comment) from internal/jwtinfo/jwtinfo.go to eliminate noise now that PrintTokenInfo supersedes it; simply delete the commented function (including its inside comments and blank lines) so only the active PrintTokenInfo implementation remains, relying on VCS history if the old code is needed later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/jwtinfo/jwtinfo.go`:
- Around line 331-343: Remove the dead commented-out code inside the JwtInfo
printing block: delete the commented variable declarations for sTrue/sFalse
(style.BoolTrue, style.BoolFalse) and the commented Valid logic and its
fmt.Fprintln lines so only the active fmt.Fprintln calls
(style.LgSprintf(style.Cmd, "JwtInfo")) remain; if you intend to show token
validity later, reintroduce this logic as real code that reads t.Valid and uses
style.BoolTrue/BoolFalse and style.LgSprintf, otherwise purge the commented
lines to reduce clutter.
- Around line 453-608: Delete the large commented-out block containing the
unused functions unmarshallTokenClaims, unmarshallTokenHeader,
getTokenClaimsMap, getUnregisteredClaimsMap, and getTokenHeadersMap from
jwtinfo.go to improve readability; keep no remnants of these commented functions
(comments or dead code) and rely on VCS history if needed, ensuring any
references to these function names elsewhere in the file/package are not left
dangling.
- Around line 278-327: Remove the large commented-out DisplayTokenInfo function
block (the entire legacy 50-line comment) from internal/jwtinfo/jwtinfo.go to
eliminate noise now that PrintTokenInfo supersedes it; simply delete the
commented function (including its inside comments and blank lines) so only the
active PrintTokenInfo implementation remains, relying on VCS history if the old
code is needed later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af2ff9d3-dcec-4c09-8c53-27786c2fe7b7
📒 Files selected for processing (1)
internal/jwtinfo/jwtinfo.go
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 by CodeRabbit
New Features
Tests
Chores / CI
Style
Dependencies