tls: use options in getCACertificates() with X509Certificate#59349
tls: use options in getCACertificates() with X509Certificate#59349haramj wants to merge 34 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
| trusted store. | ||
| * When [`NODE_EXTRA_CA_CERTS`][] is used, this would also include certificates loaded from the specified | ||
| file. | ||
|
|
There was a problem hiding this comment.
It seems the white spaces are still there? Can you remove them?
There was a problem hiding this comment.
Okay. I'll remove the white space
There was a problem hiding this comment.
@joyeecheung Removing that white space will cause a format error in the document..
|
@jasnell Thank you very much for the thorough and detailed reviews. Additionally, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59349 +/- ##
==========================================
+ Coverage 89.68% 89.70% +0.01%
==========================================
Files 676 694 +18
Lines 206710 214253 +7543
Branches 39584 41123 +1539
==========================================
+ Hits 185398 192188 +6790
- Misses 13441 14109 +668
- Partials 7871 7956 +85
🚀 New features to boost your workflow:
|
|
The CI is failing due to a missing documentation anchor: |
2e19981 to
d978079
Compare
|
Hi, feedback has been addressed. |
|
If you have some time, could you please review this? @nodejs/net, @joyeecheung |
jasnell
left a comment
There was a problem hiding this comment.
LGTM! Great job. Let's ask @joyeecheung to also take a look since it was her TODO :-)
Thank you so much for the review and the LGTM! I really appreciate your time. During my final testing, I discovered an unexpected failure related to caching, and also found a minor inconsistency with the documentation. I would like to push a fix for those before the PR is ready to merge. I'll push the updated changes shortly. |
|
@jasnell @joyeecheung So I added the caching logic, but now other tests are failing. |
|
I've successfully implemented support for new formats by clearly separating the logic for object input while maintaining the existing function's behavior. All tests have passed, and I believe the PR is ready for review. Thank you. |
Co-authored-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
e2879a5 to
e787797
Compare
|
Hi @joyeecheung, I've refactored getCACertificates() to follow a clean, linear flow: Normalize options to handle both string and object inputs. Single retrieval using getCACertificatesAsStrings(). Process results based on the requested format. This eliminates redundant calls and preserves caching for the default format. I also updated the tests to strictly verify data integrity using tlsCommon.assertEqualCerts() |
This PR implements the TODO(joyeecheung) note to support X509Certificate output in tls.getCACertificates(). It introduces a new format option, enhancing the function's flexibility and aligning its API with Node.js's broader crypto module.
The function's behavior is now extended as follows:
API Enhancement: Adds an optional format parameter to tls.getCACertificates() to specify the output format.
Output Options: