fix(ext/node): add tls.setDefaultCACertificates support#31522
fix(ext/node): add tls.setDefaultCACertificates support#31522bartlomieju merged 10 commits intodenoland:mainfrom
Conversation
… CA certificates
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (2)tests/unit_node/tls_test.ts (1)
ext/node/ops/tls.rs (1)
⏰ 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). (9)
🔇 Additional comments (1)
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.
Pull request overview
This PR implements the tls.setDefaultCACertificates API in Deno's Node.js compatibility layer, enabling applications to programmatically override the global CA certificate store for custom root CAs or testing environments.
Key Changes:
- Added native state management (
NodeTlsState) in Rust to store custom certificates per-isolate with proper worker isolation - Implemented
setDefaultCACertificatesfunction in the TypeScript polyfill with input validation and cache invalidation - Added comprehensive unit tests for the new API including validation checks
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/unit_node/tls_test.ts |
Added unit tests for setDefaultCACertificates API including existence check, input validation, and basic acceptance test; minor formatting changes to existing tests |
ext/node/polyfills/tls.ts |
Implemented setDefaultCACertificates function with input validation, integrated with native ops, and added cache invalidation for lazyRootCertificates; minor formatting changes to class declarations |
ext/node/ops/tls.rs |
Added NodeTlsState struct for storing custom CA certificates and implemented op_set_default_ca_certificates op; updated op_get_root_certificates to check for custom certificates before returning defaults |
ext/node/lib.rs |
Registered the new op_set_default_ca_certificates op in the deno_node extension |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit_node/tls_test.ts (1)
467-475: Consider adding a functional verification test.This test only verifies that
setDefaultCACertificatesdoesn't throw when given a valid input. It doesn't verify thatrootCertificatesactually reflects the updated certificates after calling the function. A more complete test would check that the change propagates correctly:Deno.test("tls.setDefaultCACertificates updates rootCertificates", () => { const testCert = `-----BEGIN CERTIFICATE----- MIIBkTCB+wIJAKHHCgVZU1FFMA0GCSqGSIb3DQEBCwUAMBExDzANBgNVBAMMBnRl c3RDQTAeFw0yMDAxMDEwMDAwMDBaFw0zMDAxMDEwMDAwMDBaMBExDzANBgNVBAMM BnRlc3RDQTCB -----END CERTIFICATE-----`; tls.setDefaultCACertificates([testCert]); assertEquals(tls.rootCertificates.length, 1); assertEquals(tls.rootCertificates[0], testCert); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ext/node/lib.rs(1 hunks)ext/node/ops/tls.rs(2 hunks)ext/node/polyfills/tls.ts(4 hunks)tests/unit_node/tls_test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
ext/**/lib.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Extensions should provide ops (operations) exposed to JavaScript in Rust code within
ext/<extension_name>/directories
Files:
ext/node/lib.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
ext/node/lib.rsext/node/ops/tls.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
ext/node/lib.rsext/node/ops/tls.rs
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
ext/node/polyfills/tls.tstests/unit_node/tls_test.ts
🧬 Code graph analysis (2)
ext/node/lib.rs (1)
ext/node/ops/tls.rs (1)
op_set_default_ca_certificates(86-97)
ext/node/polyfills/tls.ts (1)
ext/node/ops/tls.rs (1)
op_set_default_ca_certificates(86-97)
⏰ 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). (9)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
🔇 Additional comments (10)
ext/node/lib.rs (1)
389-394: LGTM!The new
op_set_default_ca_certificatesoperation is correctly registered alongside the existing TLS operations.tests/unit_node/tls_test.ts (2)
151-151: LGTM!Minor formatting change, no functional impact.
372-375: LGTM!Template literals now correctly form the file paths for reading TLS test data.
ext/node/ops/tls.rs (3)
49-52: LGTM!The
NodeTlsStatestruct is straightforward and appropriate for per-isolate TLS state management.
56-83: LGTM!The updated
op_get_root_certificatescorrectly prioritizes custom CA certificates when set, falling back to the default webpki root certificates otherwise.
85-97: Node.js intentionally lacks direct reset mechanism; this is expected parity, not a limitation.Node.js
tls.setDefaultCACertificates()similarly provides no direct way to reset to system defaults. The documented workaround istls.setDefaultCACertificates(tls.getCACertificates('system')). Since the current implementation mirrors Node.js behavior, consider whether Deno should support the same pattern or if this API surface is intentionally simplified. If supporting system CA retrieval is desired, that would require additional infrastructure (agetCACertificatesequivalent), not changes toop_set_default_ca_certificatesitself.ext/node/polyfills/tls.ts (4)
10-13: LGTM!Import added correctly for the new operation.
104-122: LGTM on validation logic.Input validation is solid - checks for array type and string elements before calling the native op. Cache invalidation by setting
lazyRootCertificates = nullcorrectly triggers a refresh on next access.
140-140: LGTM!Function correctly added to the default export.
41-48: This review comment appears to be based on a misunderstanding of the Node.js TLS API.The concern assumes that
setDefaultCACertificatesresetslazyRootCertificates = null, triggering a re-populate of a frozen array on subsequent access. However, in Node.js (and Deno's polyfill),tls.rootCertificatesis the immutable, bundled Mozilla CA store, andsetDefaultCACertificatesmanages a separate internal default CA list—it does not reset or modifytls.rootCertificatesitself.If
lazyRootCertificatesis only populated once fromop_get_root_certificates()and never reset to null, the freeze + re-populate issue described will not occur. The frozen array remains frozen, but it does not need to be repopulated becausetls.rootCertificatesis meant to be immutable and constant throughout the runtime's lifetime.Likely an incorrect or invalid review comment.
…dd lint ignores to TLS tests.
|
@bartlomieju could I get some review for this PR thank you |
feat(node): implement tls.setDefaultCACertificates
related issue:
closes # 31208
The
tls.setDefaultCACertificatesAPI was missing innode:tls, causingTypeErrorin applications that rely on programmatically overriding the global CA store (e.g., for custom root CAs or testing environments).How
• Added
NodeTlsStatetoOpStatein Rust to store custom certificates per-isolate, ensuring proper worker isolation and thread safety• Updated
op_get_root_certificatesto check for custom certificates inOpStatebefore falling back to system roots• Implemented
tls.setDefaultCACertificatesin the polyfill to update the native state• Added cache invalidation logic for
lazyRootCertificatesto ensure subsequent calls fetch the updated list• Implemented in-place array clearing (
target.length = 0) for therootCertificatesproxy to preserve object references while updating data