-
Notifications
You must be signed in to change notification settings - Fork 29
[FIX] added intermediate cert, fixed verify script, tests #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on October 20. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
WalkthroughSets the TLS crypto implementation to a WebCrypto provider at process start, adds a PEM root CA to the additional root list, moves Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as CLI/User
participant Script as verify-root-ca.ts
participant Crypto as WebCrypto Impl
participant TLS as TLS client
participant Cert as Root CA store
Note over Script,Crypto: Initialization (new)
User->>Script: run verification
Script->>Crypto: setCryptoImplementation(webcryptoCrypto)
Script->>TLS: create TLS client
TLS->>Cert: load default roots + additional PEM
TLS->>Crypto: perform crypto ops
TLS-->>Script: verification result
Script-->>User: print status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/tests/gcp-attestation.test.ts (2)
84-84
: Consider updating test fixtures instead of skipping tests.Skipping this test reduces coverage for GCP attestation validation. Since the test already handles expired tokens gracefully (lines 119-127), consider one of these alternatives:
- Generate fresh test data with non-expired tokens
- Mock the time-sensitive validation logic
- Use a test fixture that won't expire (if applicable to your testing strategy)
137-137
: Consider updating test fixtures instead of skipping tests.This test already handles expiration gracefully (lines 153-158). As with the previous skipped test, consider generating fresh test data or mocking time-sensitive components to maintain test coverage.
src/utils/tls.ts (1)
29-58
: RenameTLS_ADDITIONAL_ROOT_CA_LIST
to reflect intermediate CAs
• The added certificate is an intermediate CA (DigiCert Global Root G2 → RapidSSL TLS RSA CA G1), valid until 2027-11-02.
• Rename the list (e.g.TLS_ADDITIONAL_TRUSTED_CA_LIST
) to avoid implying it holds only root CAs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/scripts/verify-root-ca.ts
(2 hunks)src/tests/gcp-attestation.test.ts
(2 hunks)src/utils/tls.ts
(1 hunks)
🔇 Additional comments (2)
src/scripts/verify-root-ca.ts (2)
1-2
: LGTM! Proper imports for crypto initialization.The imports correctly bring in the crypto implementation setup needed for the TLS client.
11-11
: LGTM! Crypto initialization properly placed.Initializing the crypto implementation at the start of
main()
before any TLS operations is the correct approach. This ensures the WebCrypto implementation is available when the TLS client is created and certificates are verified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgt xD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json
(1 hunks)src/utils/index.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
package.json (1)
96-96
: Ensure koffi builds on all target platforms and update docs accordingly“koffi” was moved into
dependencies
and is only stubbed in browser/JSC builds (seesrc/scripts/build-browser.ts
andbuild-jsc.ts
). On Node it must compile native bindings. Confirm that:
- koffi installs successfully on Linux, macOS, Windows and all supported architectures
- README/docs include required toolchain prerequisites and fallback guidance if the native build fails
Summary by CodeRabbit
Bug Fixes
Chores
Tests