node:crypto: uppercase X509 serialNumber and modulus hex to match Node#31519
Conversation
|
@robobun adopt |
|
Updated 9:05 PM PT - May 28th, 2026
❌ @robobun, your commit 9bd3a86 has some failures in 🧪 To try this PR locally: bunx bun-pr 31519That installs a local version of the PR into your bun-31519 --bun |
|
✅ Adopted & addressed review. X509 Rebased onto |
|
Actionable comments posted: 0 |
|
Warning Review limit reached
More reviews will be available in 36 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR normalizes X509 certificate hexadecimal output to uppercase. The native C++ binding uppercases serial numbers and RSA public key moduli; a new regression test verifies uppercase formatting and several existing tests update expectations to match. ChangesX509 Certificate Hex Uppercase Normalization
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward Node compat fix uppercasing BoringSSL hex output for serialNumber/modulus.
Extended reasoning...
Overview
Three one-line changes in src/jsc/bindings/JSX509Certificate.cpp appending .convertToASCIIUppercase() to the hex strings produced for serialNumber (in computeSerialNumber) and modulus (in both toLegacyObject overloads). The remaining diff is test fixture updates flipping expected values from lowercase to uppercase, plus one new regression test asserting /^[0-9A-F]+$/.
Security risks
None. This is a presentation-only change to hex strings that already contain only [0-9a-f]; uppercasing cannot alter semantics or introduce injection. It actually improves compat for users doing literal serial-string cert pinning against Node-produced values.
Level of scrutiny
Low. The change is mechanical, the rationale (BoringSSL's BN_bn2hex/BN_print emit lowercase vs OpenSSL's uppercase) is well-documented and correct, and the PR explicitly justifies leaving exponent lowercase (Node uses %x printf there too) and not touching BignumPointer::toHex. The fingerprint path was already uppercased, so this brings serialNumber/modulus in line.
Other factors
No CODEOWNERS for these paths. The bug-hunting system found nothing. Existing tests that previously locked in the (incorrect) lowercase behavior are updated rather than deleted, and a new dedicated regression test is added. getPeerCertificate routes through the same toLegacyObject, so TLS socket output is covered without separate changes.
There was a problem hiding this comment.
LGTM — focused Node-compat string-casing fix with Jarred's review feedback addressed and CI green.
Extended reasoning...
Overview
This PR normalizes X509Certificate.serialNumber and the modulus field of toLegacyObject()/getPeerCertificate() to uppercase hex, matching Node.js/OpenSSL output (BoringSSL emits lowercase). The C++ change adds a small toUppercaseASCIIWTFString helper (single-allocation via createUninitialized + toASCIIUpper) and applies it at three call sites in JSX509Certificate.cpp. Four test files update lowercase→uppercase hex literals and a new regression test pins the uppercase behavior.
Security risks
None. While the file is in the crypto bindings, the change is pure presentation-layer string casing of already-computed hex values. No verification, key handling, parsing, or trust decisions are touched. toASCIIUpper only affects a-z, so non-hex bytes (none expected from BN_bn2hex/BN_print) would pass through unchanged.
Level of scrutiny
Low–medium. The diff is ~20 net lines of straightforward C++ plus mechanical test-literal updates. The helper follows an established WTF pattern (createUninitialized + fill) already used elsewhere in the bindings. The BIO overload mirrors the existing toWTFString(BIOPointer&) shape minus the UTF-8 branch, which is unnecessary since BN hex is always ASCII.
Other factors
- Jarred-Sumner reviewed the original revision and requested the single-allocation approach; that feedback was addressed in 083a7af and both inline comments are resolved.
- CI passed (Build #58670).
- The bug-hunting system found no issues.
- No CODEOWNERS coverage for the touched paths.
- Behavior change is intentional and Node-compat-driven; the updated tests now match what Node.js produces.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/tls/node-tls-connect.test.ts`:
- Around line 491-495: Remove the watchdog setTimeout block (the const timer =
setTimeout(...) that calls socket.end() and done(new Error(...))) from the test;
instead rely on the socket's 'timeout' event and the test framework's timeout to
fail if needed, and ensure any existing socket timeout handler calls
done()/rejects the test promise appropriately (adjust the socket timeout handler
used in this test so it produces the failure path formerly enforced by the
watchdog, then delete references to timer, socket.end() from that setTimeout).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0224c08c-9def-445e-a5df-a9bd5512c82a
📒 Files selected for processing (2)
src/jsc/bindings/JSX509Certificate.cpptest/js/node/tls/node-tls-connect.test.ts
The serialNumber/modulus hex is always ASCII, so build the uppercased string in one pass via createUninitialized + toASCIIUpper instead of creating the lowercase string and then allocating a second copy with convertToASCIIUppercase(). The modulus path also no longer creates the intermediate external string.
The node-tls-connect suite has several tests that dial the live bun.sh host. In offline/sandboxed environments these fail with DNSException/getaddrinfo errors unrelated to the code under test. Gate them behind a one-time reachability probe so the suite passes offline while still exercising the local self-signed-cert assertions (which cover the X509 serialNumber/modulus uppercase behavior).
Rely on the test's existing 10s timeout budget as the failsafe instead of a manual setTimeout watchdog, per the repo's no-setTimeout-in-tests rule.
The module-load probe waited only on secureConnect/error, so a stalled TLS handshake or black-holed connection could hang module evaluation indefinitely (per-test timeouts don't apply before any test registers). Bound it with a 5s timer that resolves false and always destroy the socket, so the live tests skip cleanly instead of hanging.
…tests The listen tests raced a 100ms setTimeout watchdog against the actual bind; under load (ASAN) a legitimate listen could exceed 100ms and trip mustNotCall, failing a passing test. Rely on the server 'error' handler (real bind failures still fail fast) and the per-test timeout instead, per the repo's no-setTimeout-in-tests rule.
robobun
force-pushed
the
ali/x509-serial-uppercase-hex
branch
from
a2d2ca2 to
9bd3a86
Compare
3 participants
X509Certificate.serialNumber,.toLegacyObject().serialNumber/.modulus, andtls.getPeerCertificate().serialNumber/.moduluswere lowercase hex in Bun, but Node/OpenSSL (andopenssl x509 -serial) emit uppercase — breaking literal serial-string cert pinning / allowlists.BoringSSL's
BN_bn2hex/BN_printemit lowercase; OpenSSL (which Node'sncryptotargets) emits uppercase. Bun already uppercases the fingerprint path but missed serialNumber/modulus. ASCII-uppercase them incomputeSerialNumberand the twotoLegacyObjectmodulus sites (getPeerCertificateroutes through the same C++toLegacyObject, so it's covered).exponentis left lowercase (Node emits it lowercase too), andBignumPointer::toHexis untouched (its other callers re-parse viaparseInt).Updates the existing tests that asserted the old lowercase (
node-tls-connect/node-tls-server, regression #21274) to the uppercase values Node produces, and adds an X509 uppercase-hex regression test. Not a port regression — 1.3.14 is also lowercase; Node is the reference.