fix(metadata): consistent revocation policy and client timeouts - #740
Conversation
|
Warning Review limit reached
Next review available in: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR centralizes certificate revocation handling and adds tests for revoked and indeterminate certificates. It also defines a 30-second timeout for internally created metadata HTTP clients and strengthens response status and body handling. ChangesMetadata validation and HTTP controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Decoder
participant ChainValidation
participant checkRevocation
participant CRL
Decoder->>ChainValidation: validate certificate chain
ChainValidation->>checkRevocation: check intermediate and leaf
checkRevocation->>CRL: determine certificate status
CRL-->>checkRevocation: revoked, valid, or indeterminate
checkRevocation-->>ChainValidation: return status or revocation error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metadata/metadata_test.go`:
- Around line 99-103: Update the response handling around decoder.Decode in the
conformance fetch test so res.Body is closed before require.Equalf can terminate
execution on a non-200 status. Use scoped cleanup with defer or explicitly close
the body before the assertion, while preserving the existing decode behavior.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 67c8619f-8e58-4ccb-95ae-a08fc0791d62
📒 Files selected for processing (6)
metadata/const.gometadata/decode.gometadata/decode_test.gometadata/metadata.gometadata/metadata_test.gometadata/providers/cached/provider.go
6cf3c5a to
9195d5c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #740 +/- ##
==========================================
+ Coverage 86.76% 86.87% +0.10%
==========================================
Files 49 49
Lines 4345 4342 -3
==========================================
+ Hits 3770 3772 +2
+ Misses 359 357 -2
+ Partials 216 213 -3
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metadata/decode.go`:
- Around line 198-205: Update checkRevocation to stop using
cert.IssuingCertificateURL when handling indeterminate revocation results. Pass
the revocation-tolerance setting into checkRevocation and apply that policy
consistently to every !revoke.VerifyCertificate(cert) outcome, preserving the
existing revoked-certificate error handling.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f1648ced-e666-4f76-8e1c-749e8535b32f
📒 Files selected for processing (2)
metadata/decode.gometadata/decode_test.go
672efb5 to
28a7495
Compare
Revocation of an intermediate was gated on the certificate carrying an issuing certificate URL, while a leaf in the same position was rejected outright. Both now share one check: a certificate is rejected when it is known to be revoked. Fetch and the cached provider created clients with no timeout, so an unresponsive service stalled the caller indefinitely; both now use DefaultMDSTimeout. The decoder held a client that was never read, which is removed, and Decode no longer claims to close the reader it is given. The metadata service rate limits with a short plain text body, which was handed to the JWT parser and surfaced as an unintelligible base64 failure. The network tests now check the response status before decoding, and the conformance test no longer leaks a response body per endpoint.
28a7495 to
c2d2d9d
Compare
1 participant
Revocation of an intermediate was gated on the certificate carrying an issuing certificate URL, while a leaf in the same position was rejected outright. Both now share one check: a certificate is rejected when it is known to be revoked.
Fetch and the cached provider created clients with no timeout, so an unresponsive service stalled the caller indefinitely; both now use DefaultMDSTimeout. The decoder held a client that was never read, which is removed, and Decode no longer claims to close the reader it is given.
The metadata service rate limits with a short plain text body, which was handed to the JWT parser and surfaced as an unintelligible base64 failure. The network tests now check the response status before decoding, and the conformance test no longer leaks a response body per endpoint.