GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||
GeezerVison is showing github.com — test,crypto: adjust tests for BoringSSL by codebytere · Pull Request #61459 · nodejs/node Conversation
codebytere
marked this pull request as ready for review
codebytere
force-pushed
the
bssl-test-tweaks
branch
from
f659ab0 to
e21d796
Compare
|
There was an error while loading. Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## main #61459 +/- ## ========================================== - Coverage 89.67% 89.65% -0.02% ========================================== Files 676 676 Lines 206469 206469 Branches 39537 39542 +5 ========================================== - Hits 185157 185117 -40 - Misses 13448 13491 +43 + Partials 7864 7861 -3 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
e21d796 to
efdaebc
Compare
| assert.strictEqual(client.getCertificate().serialNumber, | ||
| '147D36C1C2F74206DE9FAB5F2226D78ADB00A426'); | ||
| assert.match(client.getCertificate().serialNumber, | ||
| /147D36C1C2F74206DE9FAB5F2226D78ADB00A426/i); |
There was a problem 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); |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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()...)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Given that'd be a slightly larger undertaking and not just a test change should i break that into a new PR?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
If you want to go with the diff above, I think that would be a smaller change overall 🙂
I personally don't have a strong preference for a new PR or keeping this one, I'm happy to review either way
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@codebytere Fyi, I've opened a separate PR with that suggestion: #61752
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
This hides a discrepancy between OpenSSL and BoringSSL, as the latter returns lowercase hex values. Refs: nodejs#61459 (comment)
This hides a discrepancy between OpenSSL and BoringSSL, as the latter returns lowercase hex values. Refs: #61459 (comment) PR-URL: #61752 Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
This hides a discrepancy between OpenSSL and BoringSSL, as the latter returns lowercase hex values. Refs: #61459 (comment) PR-URL: #61752 Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
b7d8764 to
b8ec581
Compare
b8ec581 to
d27e019
Compare
This hides a discrepancy between OpenSSL and BoringSSL, as the latter returns lowercase hex values. Refs: #61459 (comment) PR-URL: #61752 Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
This hides a discrepancy between OpenSSL and BoringSSL, as the latter returns lowercase hex values. Refs: #61459 (comment) PR-URL: #61752 Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
This hides a discrepancy between OpenSSL and BoringSSL, as the latter returns lowercase hex values. Refs: #61459 (comment) PR-URL: #61752 Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
This hides a discrepancy between OpenSSL and BoringSSL, as the latter returns lowercase hex values. Refs: #61459 (comment) PR-URL: #61752 Reviewed-By: Shelley Vohr <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
|
Superseded by #62883 using a recent BoringSSL snapshot. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.