Skip to content

crypto: surface OpenSSL errors in Certificate.exportPublicKey - #63272

Open
fru1tworld wants to merge 3 commits into
nodejs:mainfrom
fru1tworld:crypto-spkac-throw-on-failure-63264
Open

fru1tworld wants to merge 3 commits into
nodejs:mainfrom
fru1tworld:crypto-spkac-throw-on-failure-63264

Conversation

@fru1tworld

Copy link
Copy Markdown

Surface internal OpenSSL failures via ThrowCryptoError instead of returning an empty string, matching the convention used elsewhere in src/crypto.

Refs: #63264

Refs: nodejs#63264
Signed-off-by: fru1tworld <fruitworld.planet@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
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 May 13, 2026
@panva panva added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 13, 2026
@panva
panva requested a review from tniessen May 13, 2026 06:35
@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (131fd84) to head (adf8f83).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_spkac.cc 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63272      +/-   ##
==========================================
- Coverage   91.94%   90.02%   -1.92%     
==========================================
  Files         362      714     +352     
  Lines      155999   225245   +69246     
  Branches    24057    42581   +18524     
==========================================
+ Hits       143429   202781   +59352     
- Misses      12295    14243    +1948     
- Partials      275     8221    +7946     
Files with missing lines Coverage Δ
src/crypto/crypto_spkac.cc 63.46% <25.00%> (ø)

... and 476 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.

Comment on lines +44 to +50
MarkPopErrorOnReturn mark_pop_error_on_return;
BIOPointer bio = ncrypto::ExportPublicKey(input.data(), input.size());
if (!bio) return args.GetReturnValue().SetEmptyString();
if (!bio) {
return ThrowCryptoError(env,
mark_pop_error_on_return.peekError(),
"Failed to export public key from SPKAC");
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is only wrapping a single operation, should it not just be using ERR_get_error() instead?

Comment thread test/parallel/test-crypto-certificate.js
Co-authored-by: René <contact.9a5d6388@renegade334.me.uk>
@fru1tworld

fru1tworld commented May 13, 2026

Copy link
Copy Markdown
Author

Thanks For feedback!
updated !😄

@fru1tworld
fru1tworld force-pushed the crypto-spkac-throw-on-failure-63264 branch from e9b1f4b to e753ba5 Compare May 14, 2026 00:04
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actions github-actions Bot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Aug 12, 2026
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. semver-major PRs that contain breaking changes and should be released in the next major version. stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants