-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
test,crypto: adjust tests for BoringSSL #61459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
f659ab0 to
e21d796
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 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 🚀 New features to boost your workflow:
|
e21d796 to
efdaebc
Compare
| assert.strictEqual(client.getCertificate().serialNumber, | ||
| '147D36C1C2F74206DE9FAB5F2226D78ADB00A426'); | ||
| assert.match(client.getCertificate().serialNumber, | ||
| /147D36C1C2F74206DE9FAB5F2226D78ADB00A426/i); |
There was a problem hiding this comment.
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.
| /147D36C1C2F74206DE9FAB5F2226D78ADB00A426/i); | |
| /^147D36C1C2F74206DE9FAB5F2226D78ADB00A426$/i); |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| common.skip('Skipping unsupported RSA-PSS key test'); | |
| common.printSkipMessage('Skipping unsupported RSA-PSS key test'); |
I assume?
Ongoing effort to make crypto tests work with BoringSSL.