Skip to content

Conversation

@codebytere
Copy link
Member

Ongoing effort to make crypto tests work with BoringSSL.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 21, 2026
@codebytere codebytere marked this pull request as ready for review January 21, 2026 10:38
@codecov
Copy link

codecov bot commented Jan 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.52%. Comparing base (91a9978) to head (efdaebc).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61459      +/-   ##
==========================================
- Coverage   88.54%   88.52%   -0.02%     
==========================================
  Files         704      704              
  Lines      208971   208976       +5     
  Branches    40341    40347       +6     
==========================================
- Hits       185026   184998      -28     
- Misses      15919    15960      +41     
+ Partials     8026     8018       -8     

see 37 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.

assert.strictEqual(client.getCertificate().serialNumber,
'147D36C1C2F74206DE9FAB5F2226D78ADB00A426');
assert.match(client.getCertificate().serialNumber,
/147D36C1C2F74206DE9FAB5F2226D78ADB00A426/i);
Copy link
Member

Choose a reason for hiding this comment

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

^ Do you know what's causing these? This is a difference in the public API that I would not expect.

This seems like the kind of thing where we should consistently return an uppercase string from the API, because it would be reasonable for user code to rely on that particular behavior. This should do the trick:

diff --git a/src/crypto/crypto_x509.cc b/src/crypto/crypto_x509.cc
index 11622d823bbc..f40f6dd4f2bc 100644
--- a/src/crypto/crypto_x509.cc
+++ b/src/crypto/crypto_x509.cc
@@ -253,8 +253,8 @@ MaybeLocal<Value> GetSignatureAlgorithmOID(Environment* env,
 
 MaybeLocal<Value> GetSerialNumber(Environment* env, const X509View& view) {
   if (auto serial = view.getSerialNumber()) {
-    return OneByteString(env->isolate(),
-                         static_cast<unsigned char*>(serial.get()));
+    return ToV8Value(
+        env, ToUpper(std::string_view(static_cast<char*>(serial.get()))));
   }
   return Undefined(env->isolate());
 }

If there's more to this and we really, really cannot do that for some reason, these regexps should be anchored, i.e.

Suggested change
/147D36C1C2F74206DE9FAB5F2226D78ADB00A426/i);
/^147D36C1C2F74206DE9FAB5F2226D78ADB00A426$/i);

Copy link
Member

Choose a reason for hiding this comment

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

(and actually, maybe this is something we should handle at all sites where we call BN_bn2hex()...)

code: 'ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS'
});
} else {
common.skip('Skipping unsupported RSA-PSS key test');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
common.skip('Skipping unsupported RSA-PSS key test');
common.printSkipMessage('Skipping unsupported RSA-PSS key test');

I assume?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants