Feat/add jwks command#27
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR updates the project's documentation to emphasize secure HTTP endpoint maintenance, adds new JWT/JWKS inspection and generation commands, refactors JWT handling to support context cancellation, introduces a new JWKS package with comprehensive tests, updates development environment test configurations, and refreshes multiple Go dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 24 minutes.Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/jwtinfo.go (1)
38-38:⚠️ Potential issue | 🟡 MinorFix typo in validation URL example.
The example says
jkws.json; this should bejwks.jsonto avoid copy/paste confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/jwtinfo.go` at line 38, Update the example value for the VALIDATION_URL environment variable to use the correct JWKS filename: replace the typo "jkws.json" with "jwks.json" in the export VALIDATION_URL line so the sample shows export VALIDATION_URL="https://url.to/jwks.json".
🧹 Nitpick comments (2)
devenv.nix (1)
618-625: Negative-path JWT test script should assert expected failure output.This script currently just runs the command. Consider asserting a known error substring (e.g., with
grep) so it acts as a deterministic test rather than a manual probe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devenv.nix` around lines 618 - 625, The test script scripts.run-jwtinfo-test-auth0-wrong-validation-url.exec currently just invokes ./dist/https-wrench jwtinfo and should instead assert a deterministic failure: capture the command output (or pipe through tee) and grep for a known error substring (e.g., "validation failed", "invalid signature", or the specific message your tool emits) and fail the script if the substring is not found (exit non-zero); update the script wrapper around the ./dist/https-wrench jwtinfo invocation to run the command, search its stdout/stderr for the expected error text, and return non-zero when the expected failure message is absent so the CI step becomes a deterministic negative test.internal/jwtinfo/jwtinfo_test.go (1)
284-290: Use deadline-backed contexts in these network-path tests.These APIs are context-aware now, but the updated tests still only pass
context.Background(). A shortcontext.WithTimeoutfor the negative URL/JWKS cases would both bound CI runtime and verify that cancellation/deadline propagation actually works.Also applies to: 402-408, 544-588
🤖 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 284 - 290, Replace the plain context.Background() used in RequestToken calls with a deadline-backed context created via context.WithTimeout (e.g., short timeout like 1-5s) and ensure you call the returned cancel function (defer cancel()) so the test both bounds CI runtime and verifies cancellation propagation; update the failing network-path test invocations that pass context.Background() (notably the calls to RequestToken with serverJwtEndpoint, reqValues, client, io.ReadAll) and make the same change in the other mentioned test blocks (around the 402-408 and 544-588 ranges).
🤖 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/jwks/jwks.go`:
- Around line 23-24: The comment and error text in GenerateJWKS incorrectly call
the SHA-256 of PKIX/SPKI bytes a "thumbprint" and also state that jwkset infers
a thumbprint-based kid while the code actually computes and sets kid; fix this
by either (A) implementing an RFC‑7638 JWK thumbprint: build the canonical JSON
representation of the JWK, compute its SHA‑256 thumbprint per RFC‑7638 and use
that as kid, and update comments/messages to say "RFC‑7638 thumbprint", or (B)
keep the current PKIX/SPKI SHA‑256 behavior but rename all mentions from
"thumbprint" to "SHA-256-derived kid" (or similar), update the top comment in
GenerateJWKS and the error/message strings emitted when creating the kid, and
remove the incorrect claim that jwkset infers a thumbprint-based kid since the
function sets kid explicitly; ensure references to the kid generation logic (the
kid variable and the code path that computes and sets it) are updated
consistently.
In `@internal/jwtinfo/jwtinfo.go`:
- Around line 213-215: The current isValidJSON uses json.Valid which accepts
non-object JSON; update validation so DecodeBase64 only accepts JSON objects for
JWT header/claims: modify isValidJSON to first check json.Valid(data) and then
ensure the top-level value is an object (e.g., verify the first non-whitespace
char is '{' and the last non-whitespace char is '}', or attempt to decode into
map[string]interface{}), and call this stricter isValidJSON from DecodeBase64 to
reject scalars/nulls early.
In `@README.md`:
- Around line 221-223: Update the README docs to fix typos: change the
validation example string VALIDATION_URL value from "https://url.to/jkws.json"
to "https://url.to/jwks.json", and correct any image alt text or captions that
say "jwtingo" to "jwtinfo" (search for occurrences around the example blocks and
lines referenced, and update the REQ_URL/REQ_VALUES example if any similar
misspellings exist).
---
Outside diff comments:
In `@cmd/jwtinfo.go`:
- Line 38: Update the example value for the VALIDATION_URL environment variable
to use the correct JWKS filename: replace the typo "jkws.json" with "jwks.json"
in the export VALIDATION_URL line so the sample shows export
VALIDATION_URL="https://url.to/jwks.json".
---
Nitpick comments:
In `@devenv.nix`:
- Around line 618-625: The test script
scripts.run-jwtinfo-test-auth0-wrong-validation-url.exec currently just invokes
./dist/https-wrench jwtinfo and should instead assert a deterministic failure:
capture the command output (or pipe through tee) and grep for a known error
substring (e.g., "validation failed", "invalid signature", or the specific
message your tool emits) and fail the script if the substring is not found (exit
non-zero); update the script wrapper around the ./dist/https-wrench jwtinfo
invocation to run the command, search its stdout/stderr for the expected error
text, and return non-zero when the expected failure message is absent so the CI
step becomes a deterministic negative test.
In `@internal/jwtinfo/jwtinfo_test.go`:
- Around line 284-290: Replace the plain context.Background() used in
RequestToken calls with a deadline-backed context created via
context.WithTimeout (e.g., short timeout like 1-5s) and ensure you call the
returned cancel function (defer cancel()) so the test both bounds CI runtime and
verifies cancellation propagation; update the failing network-path test
invocations that pass context.Background() (notably the calls to RequestToken
with serverJwtEndpoint, reqValues, client, io.ReadAll) and make the same change
in the other mentioned test blocks (around the 402-408 and 544-588 ranges).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a45af976-eb7c-4457-9756-79a88f897c15
⛔ Files ignored due to path filters (3)
assets/img/https-wrench_jwtinfo_read_validate_token.pngis excluded by!**/*.pngassets/img/https-wrench_jwtinfo_request_token.pngis excluded by!**/*.pnggo.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
README.mdcmd/certinfo.gocmd/jwks.gocmd/jwtinfo.gocmd/man.gocmd/requests.gocmd/root.godevenv.nixgo.modinternal/certinfo/certinfo.gointernal/jwks/jwks.gointernal/jwks/jwks_test.gointernal/jwtinfo/jwtinfo.gointernal/jwtinfo/jwtinfo_test.gointernal/style/style.go
Summary by CodeRabbit
New Features
Documentation