Skip to content

Comments

src: enable X509sToArrayOfStrings to skip errors#61784

Open
StefanStojanovic wants to merge 3 commits intonodejs:mainfrom
JaneaSystems:mefi-cert-fix
Open

src: enable X509sToArrayOfStrings to skip errors#61784
StefanStojanovic wants to merge 3 commits intonodejs:mainfrom
JaneaSystems:mefi-cert-fix

Conversation

@StefanStojanovic
Copy link
Contributor

As suggested in the linked issue, this PR relaxes the X509 to PEM conversion by skipping failures for system certs. If conversion fails, Node.js will try to give info about the failed certs if it can. This new behavior is used only on system certs, since for others, the user has control. AFAIK, there is no way to add a reliable test for this, but if someone has an idea, feel free to share it with me.

Fixes: #61636

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Feb 12, 2026
@codecov
Copy link

codecov bot commented Feb 12, 2026

Codecov Report

❌ Patch coverage is 23.33333% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.69%. Comparing base (9a237cd) to head (61e3e8e).
⚠️ Report is 62 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_context.cc 23.33% 21 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61784      +/-   ##
==========================================
- Coverage   89.73%   89.69%   -0.04%     
==========================================
  Files         675      672       -3     
  Lines      204674   204549     -125     
  Branches    39330    39274      -56     
==========================================
- Hits       183667   183480     -187     
- Misses      13283    13383     +100     
+ Partials     7724     7686      -38     
Files with missing lines Coverage Δ
src/crypto/crypto_context.cc 69.59% <23.33%> (-1.25%) ⬇️

... and 81 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

}

if (skipped > 0) {
ProcessEmitWarning(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should make this an option to the API instead of unconditionally emitting a warning? So if the user uses tls.getCACertifcate, they can choose warn, ignore, throw (or even get a list of errors on the side) when there's an error in parsing these certificates. For NODE_USE_SYSTEM_CA/--use-system-ca, warn is a sensible behavior.

StefanStojanovic and others added 2 commits February 24, 2026 12:50
Co-authored-by: Anna Henningsen <github@addaleax.net>
Co-authored-by: Anna Henningsen <github@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve ergonomics of tls.getCACertificates('system') errors on Windows

4 participants