Repository navigation
Conversation
…he ASN.1 time
The two getters printed the time as text ("Dec 31 23:59:59 30 GMT") and
parsed that text back with WTF::parseDate. The two-digit-year rule of the
parser moved the years 0000-0099 to 1950-2049, so a certificate that
expired in the year 30 reported validToDate = 2030-12-31.
Convert the ASN1_TIME to POSIX seconds with BoringSSL and build the Date
from that, as Node does (ncrypto X509View::getValidFromTime /
getValidToTime). ASN1_TIME_to_posix_nonstandard also applies a "+hhmm"
offset on a UTCTime. The X509 parser accepts that form and the printed
text for it is "Bad time value", so the getters returned Invalid Date
where Node returns the instant.
|
Warning Review limit reached
On-demand reviews are free for the next 9 days. After that, they cost $0.25 per reviewed file. Or wait 43 seconds for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 1:16 PM PT - Sep 11th, 2026
✅ @robobun, your commit 13fbb75898b2e36e0d032368a2a6b1e7c7b5a138 passed in 🧪 To try this PR locally: bunx bun-pr 42363That installs a local version of the PR into your bun-42363 --bun |
|
Status Closed as superseded. #47820 (commit 5e08ec1) landed the same change on main:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — replaces the lossy text round-trip with a direct ASN.1→POSIX conversion, matching Node's implementation.
What was reviewed:
- The two getters after simplification:
scopeis still used bythrowThisTypeError, andDateInstance::createcannot throw, so no missing exception check. int64_t * 1000.0range: year 0000 (~-6.2e13 ms) through 9999 (~2.5e14 ms) are exact in a double.asn1TimeToPosixnull-checks bothcert_and theASN1_TIME*; the_nonstandardvariant is the right choice given BoringSSL's X509 parser accepts the+hhmmUTCTime form.- Test fixture:
certForYearoverwrites the 4-byte year inside a fixed-length GeneralizedTime, so DER lengths stay valid; boundary years and the UTCTime-offset case are covered.
Extended reasoning...
Overview
This PR fixes X509Certificate.prototype.validFromDate / validToDate in src/jsc/bindings/JSX509CertificatePrototype.cpp. The old implementation printed the certificate's ASN1_TIME to a human-readable string via validFrom/validTo, then re-parsed it with WTF::parseDate. Because ASN1_TIME_print renders the year 30 as the two-character string 30, the date parser interpreted it as 2030, so a certificate that expired in year 0030 reported validToDate > new Date() as true. The fix adds X509View::getValidFromTime() / getValidToTime() in ncrypto.{h,cpp} that call BoringSSL's ASN1_TIME_to_posix_nonstandard to get seconds-since-epoch directly, and a small validityDate helper that constructs the DateInstance from seconds * 1000.0 (or PNaN → Invalid Date on failure). Tests are added to the existing test/js/node/crypto/x509.test.ts covering GeneralizedTime years across the full 0000–9999 range plus the UTCTime +hhmm offset case.
Security risks
The changed getters are read-only accessors on an already-parsed certificate and are not part of the TLS verification path (which the PR notes explicitly). The behavior change is strictly in the correct direction: a certificate that expired ~2000 years ago no longer reports as valid to callers doing validToDate > new Date(). There is no new parsing of untrusted bytes here — ASN1_TIME_to_posix_nonstandard operates on an ASN1_TIME that BoringSSL's X509 parser already validated, and the helper null-checks both cert_ and the returned ASN1_TIME*. No new allocation, no error-queue interaction, no user-JS re-entry.
Level of scrutiny
Moderate. It touches crypto-adjacent C++ bindings, but the change is a net simplification: ~20 lines of string-roundtrip code deleted per getter, replaced by a one-line call to a BoringSSL primitive, mirroring Node's crypto_x509.cc approach that the PR cites. The remaining ThrowScope in each getter is still used by the this-type error path, and the new tail call cannot throw, so exception-check discipline is intact. Both sibling getters are fixed symmetrically per the "fix the whole class" rule. CODEOWNERS only covers *.d.ts, so none of these files are owned.
Other factors
The tests follow repo conventions: added to the existing module test file, test.each for the year matrix, .toEqual on a combined object for the primary regression assertion, no sleeps/network/timeouts. The certForYear fixture technique (overwrite the 4 ASCII year bytes inside a fixed-length GeneralizedTime in the DER) keeps every DER length byte valid and lets one template cover twelve boundary years. The PR description shows the tests fail on bun 1.4.3 and pass on Node v26.3.0, satisfying the USE_SYSTEM_BUN=1 requirement. The bug hunt ran to a dry streak with no findings and no candidate concerns to rule out. No prior reviewer objections exist on the timeline.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix that mirrors Node's ASN1_TIME → POSIX-seconds path and removes the lossy string round-trip.
What was reviewed:
validityDate()andasn1TimeToPosix(): nullopt → PNaN → Invalid Date;int64_t seconds * 1000.0stays exact for years 0000-9999;secondsis only read on the success return.- ThrowScope hygiene after the string calls were dropped:
DateInstance::createdoesn't throw, so the remaining scope is only for the bad-receiver path — no missing RETURN_IF_EXCEPTION. - Confirmed
wtf/DateMath.hhas no remaining users in the file; both sibling getters go through the same helper. - Tests: hermetic, in the existing x509 file,
test.eachover the 0000/49/50/99/100/1601/1969/1970/2038/2049/2050/9999 boundaries plus the UTCTime+hhmmoffset case; the DER-rewrite fixture keeps lengths intact.
Extended reasoning...
Overview
The PR replaces the validFromDate/validToDate implementation in src/jsc/bindings/JSX509CertificatePrototype.cpp, which previously formatted the ASN.1 time to text via BIO and re-parsed it with WTF::parseDate. That round-trip mapped two-digit printed years into 1950-2049, so a GeneralizedTime year of 0030 came back as 2030. The new code adds X509View::getValidFromTime()/getValidToTime() in ncrypto.{h,cpp} returning std::optional<int64_t> POSIX seconds via BoringSSL's ASN1_TIME_to_posix_nonstandard, and a small validityDate() helper builds a DateInstance from seconds * 1000.0 (or PNaN on nullopt). The now-unused wtf/DateMath.h include is dropped. Tests are added to the existing test/js/node/crypto/x509.test.ts.
Security risks
This is crypto-adjacent but read-only and informational: these getters are not on Bun's TLS certificate-verification path (that runs inside BoringSSL). The behavioral change is strictly in the safe direction — a certificate that expired in year 30 previously reported validToDate in 2030 (so a naive cert.validToDate > new Date() check passed); it now correctly reports 0030. The _nonstandard variant is chosen deliberately, with a comment, to match what Node/OpenSSL do for the RFC-5280-forbidden UTCTime +hhmm offset that BoringSSL's X509 parser nonetheless accepts. No new allocation, no error-queue interaction, and the uninitialized-read hazard in Node's equivalent (which ignores the return code) is avoided by the std::optional.
Level of scrutiny
Medium. The production change is ~25 lines across three files, mechanically mirrors Node's crypto_x509.cc/ncrypto.cc reference implementation (cited in the PR), and both sibling getters are fixed symmetrically through one shared helper per REVIEW.md's dedup rule. I checked that the removed JS-throwing calls (validTo(), ->view()) leave no dangling RETURN_IF_EXCEPTION obligations — DateInstance::create and the BoringSSL calls do not throw JS exceptions, so the remaining ThrowScope is used only for throwThisTypeError. The int64_t → double widening keeps millisecond precision exact across the entire 0000-9999 GeneralizedTime range (well under 2^53).
Other factors
Test coverage is thorough and follows repo conventions: added to the existing module test file, describe/test.each over twelve boundary years including the old 0-49/50-99 windowing edges and the 1970/2038 epoch boundaries, a combined-object .toEqual assertion, and a separate UTCTime-offset fixture. The gate evidence in the PR shows 6 failures on main and all-pass on the branch under both debug+ASAN and release. No CODEOWNERS cover the changed paths. A prior COMMENTED review and github-actions formatting comments were followed by two commits; the current diff is clean.
… UTF-8 (#42798) ### Problem - A non-ASCII string of 2^30 Latin-1 characters or more aborts the process: `panic(main thread): abort() called`, exit 134, also inside `try` / `catch`. Each API that reads it through `Bun::UTF8View` does this, for example `x509.checkHost("\u00e9".repeat(2 ** 30))`. Fuzzing found it, with no user report. - `UTF8View` (`src/jsc/bindings/BunString.h:18`) called `utf8()`, which is `tryGetUTF8()` plus `RELEASE_ASSERT`. It had no failure channel. ### Fix - `UTF8View` has no public constructor. `UTF8View::tryCreate()` returns `std::nullopt` when `tryGetUTF8()` fails, and an overload throws `RangeError: Out of memory`, as #42202 and #42308 do. - An 8-bit ASCII string is still borrowed, never copied. Each user passes a pointer and a length. A `const char*` consumer always needs a copy, and that case is not in this PR (Notes). - These throw now: `new X509Certificate(string)`, `hkdf` / `hkdfSync` (`salt`, `info`), `bun:sqlite` SQL text and string parameters, `new Bun.CookieMap(string)`. `checkHost`, `checkEmail`, `require.resolve.paths` and `Module._resolveLookupPaths` only look the string up, so they answer "no match". Removed `keyFromString`, `passphraseFromBufferSource`, `keyFromPublicString` (no caller). - Verified: `test/js/bun/util/utf8-conversion-limit.test.ts`. The unfixed build aborts at the first case. One row shows that an ASCII string is not copied. ### Background - A JSC string is Latin-1 (8-bit) or UTF-16, with no NUL terminator. `UTF8View` borrows an 8-bit all-ASCII string and converts any other string with WTF. - WTF reserves 2 bytes per Latin-1 character and a buffer holds at most 2^31 - 1 bytes, so 2^30 Latin-1 characters fail. A 16-bit string fails at a UTF-8 form of 2^31 bytes. - JSC throws `RangeError: Out of memory` for a string that it cannot create. <details><summary>Notes</summary> **Repro, on release 1.4.3 canary (09bb546, linux x64).** Each line aborts on its own with exit 134, at 1.1 GB RSS in about 0.6 s. ```js const crypto = require("node:crypto"); const { Database } = require("bun:sqlite"); const long = "\u00e9".repeat(2 ** 30); // Latin-1, not ASCII const x509 = new crypto.X509Certificate(pem); const db = new Database(":memory:"); x509.checkHost(long); x509.checkEmail(long); new crypto.X509Certificate(long); crypto.hkdfSync("sha256", "key", long, "info", 8); // salt crypto.hkdfSync("sha256", "key", "salt", long, 8); // info crypto.hkdf("sha256", "key", long, "info", 8, () => {}); require.resolve.paths(long); require("node:module")._resolveLookupPaths(long); db.run(long); db.prepare(long); db.prepare("SELECT length(?)").get(long); new Bun.CookieMap("a=%41" + long); // the value is converted only when the header has a "%" ``` An ASCII string of the same length is clean in all of them, because `UTF8View` borrows it. A 16-bit string aborts too: 715,827,883 x U+0800 has a UTF-8 form of 2^31 + 1 bytes. With this PR all of these cases throw or answer "no match" for that string as well. One character below the limit (2^30 - 1 x U+00E9) still converts: `checkHost` returns `undefined` after 8 s on release. **Every `UTF8View` user, and what it does when the conversion fails** | User | Now | | --- | --- | | `new X509Certificate(string)` (`JSX509Certificate.cpp`) | throws | | `X509Certificate#checkHost`, `#checkEmail` (`JSX509CertificatePrototype.cpp`) | return `undefined` | | `hkdf`, `hkdfSync`: `salt`, `info` (`CryptoHkdf.cpp`) | throws | | `Database#run`, `#exec`, `#prepare`, `#query`: SQL text (`JSSQLStatement.cpp`) | throws | | `bun:sqlite` string parameter (`JSSQLStatement.cpp` `rebindValue`) | throws | | `new Bun.CookieMap(string)`, `request.cookies` (`CookieMap.cpp`) | `Exception { OutOfMemoryError }`, which both callers throw | | `require.resolve.paths` (`JSCommonJSModule.cpp`), `Module._resolveLookupPaths` (`NodeModuleModule.cpp`) | the request is not a builtin, continue | | `decodeURIComponentSIMD` (`bun:internal-for-testing`) | throws | | `X509Certificate#validToDate`, `#validFromDate` | return `undefined`. The string is the 24 characters that `ASN1_TIME_print` writes, so this cannot happen. #42363 removes both uses. | | native `WebSocket` `onMessage` (`WebSocket.cpp`, used by the Chrome backend of `Bun.WebView`) | drops the message. The client fails a message longer than `MAX_RECEIVE_MESSAGE_LENGTH` first (128 MiB, `src/http_jsc/websocket_client.rs:57`, checked at `:813`). A compressed message has the same bound (`MAX_DECOMPRESSED_SIZE`, `websocket_client/WebSocketDeflate.rs:64`). So this cannot happen. | | replay of queued Chrome commands over the pipe (`ChromeBackend.cpp`) | rejects every pending promise of the transport | | `keyFromString`, `passphraseFromBufferSource`, `keyFromPublicString` | removed, no caller | Two of these did not use `UTF8View` before. The `bun:sqlite` parameter binding had its own copy of the same dispatch (borrow ASCII, else `utf8()`). `Module._resolveLookupPaths` is the twin of `require.resolve.paths` and called `utf8()` for the same builtin check. **Not in this PR.** These call `String::utf8()` or `StringView::utf8()` directly, not `UTF8View`. They abort for a string of 2^30 characters, ASCII included. | Parameter | Site | Owner | | --- | --- | --- | | `X509Certificate#checkIP(ip)` | `jsX509CertificateProtoFuncCheckIP` | #42780 (throws `ERR_INVALID_ARG_VALUE`, as for the same string one character shorter) | | `hkdf(digest)` and the other digest, cipher and curve names | `ncrypto.cpp` | #42780 | | `hkdf(ikm)` as a string of 2^31 UTF-8 bytes | `CryptoHkdf.cpp` `prepareKey` | #42187 | | `process.execve` | `BunProcess.cpp` | #42308 | | `Set-Cookie` header write | `CookieMap.cpp:20` | #42237 | | `bun:sqlite` `new Database(filename)`, `serialize(name)`, `loadExtension(path, entryPoint)`, `fileControl(name)`, `setCustomSQLite(path)` | `JSSQLStatement.cpp:1208`, `1377`, `1428`, `1430`, `1803`, `1955` | follow-up, with the 20 sites of `node:sqlite` (`NodeSqlite.cpp`) | | `Bun.Cookie` `expires` string | `Cookie.cpp:124`, `JSCookie.cpp:62` | follow-up | | `WebSocket` close reason, protocols | `WebSocket.cpp` | follow-up | | Chrome backend pipe command | `ChromeBackend.h:211` | follow-up | Most of them pass a `const char*` to a C API (`sqlite3_open_v2`, `sqlite3_serialize`, `sqlite3_load_extension`, `X509_check_ip_asc`, `EVP_get_digestbyname`). A WTF string has no NUL terminator (`StringImpl::allocationSize` is `tailOffset + length * sizeof(T)`), so that case always needs a copy, and the borrowed branch of `UTF8View` cannot serve it. It stays on `tryGetUTF8()`. `src/jsc` and `src/runtime` have about 100 `utf8()` call sites. Each needs a look: some pass a pointer and a length and can move to `UTF8View`, which also saves the copy of an ASCII string. That is a separate change. **Related open PRs** - #42187 edits `copyBufferOrString` in `CryptoHkdf.cpp` and keeps the `UTF8View utf8(view);` line. The PR that merges second takes the `tryCreate` form, a 3 line rebase. - #42363 rewrites `validToDate` / `validFromDate` and drops their `UTF8View`. - #33312 adds 7 new `Bun::UTF8View(...)` constructor calls (clipboard). After this PR they do not compile until they use `tryCreate`. That is the purpose of the private constructor: a new user cannot skip the failure case. An unchecked `tryCreate(s)->span()` still compiles. **Test.** The length is what is under test, so the child holds a string of 1 GiB, and a second one while the cookie header is joined (2.5 GB peak). One child runs the 14 cases and takes 6 to 10 s in a debug ASAN build. About 5 s of that is the last row, where the unoptimized build scans 1 GiB for a non-ASCII byte. The test skips below 8 GiB of total memory and carries a 30 s ceiling, like `error-message-string-length-limit.test.ts`. A second test runs the same APIs with `"café"` and `"café 😀"` to check that a string that converts still gives the same result. **The ASCII row.** `tryGetUTF8()` refuses every 8-bit string of 2^30 characters before it reads one (`StringImpl.h` `tryGetUTF8ForCharacters`: `isValidCapacityForVector<char8_t>(characters.size() * 2)`). So a `UTF8View` that copied an ASCII string of that length reports `Out of memory`. The row binds `"q".repeat(2 ** 30)` as a `bun:sqlite` parameter. The bundled SQLite answers `string or blob too big` (its limit is 1e9 bytes). The system SQLite that macOS loads takes up to 2 GiB and returns the length. The row accepts these two answers and no other: each shows that SQLite got the string's buffer. The same mechanism is why `Module._resolveLookupPaths("q".repeat(2 ** 30))` aborts on `main`: it called `request.utf8()` to pass a pointer and a length. It borrows now. **Also ran (debug ASAN build):** `x509.test.ts`, `x509-subclass.test.ts`, the vendored `test-crypto-x509.js`, `test-crypto-hkdf.js`, `test-crypto-sign-verify.js`, `test-crypto-key-objects.js`, `test-require-resolve.js`, `hkdf-callback-null.test.ts`, `hkdf-zero-length.test.ts`, `sqlite.test.js`, the four cookie suites, `decodeURIComponentSIMD.test.ts`, `node-module-module.test.js`, `resolve.test.ts`, `webview-chrome-pipe.test.ts`, `webview-chrome-disconnect.test.ts`. The child script also ran with `BUN_JSC_validateExceptionChecks=1`, with no report. In `sqlite.test.js`, `#13082` times out on this machine's debug build (about 6 s against the 5 s limit), with and without this change. **Self-review: 6 concerns raised, 4 addressed.** - Addressed: `checkHost` / `checkEmail` threw. They return `undefined` now. The rule that they and `checkIP` in #42780 follow: a call reports what it reports for the same string one character shorter. That string is a name that matches no certificate. - Addressed: the same 4 line throw block appeared 7 times. It is the `tryCreate(globalObject, scope, view)` overload now. - Addressed: the `bun:sqlite` parameter binding still aborted, through its own copy of the dispatch. - Addressed: the description named API surfaces as done. It names parameters now, and the table above lists what is left, with an owner for each row. - Rejected: convert the six other `utf8()` calls of `JSSQLStatement.cpp` here, through a new `Bun::tryUTF8`. They are direct `utf8()` callers that need a NUL-terminated copy. They belong with the `node:sqlite` sites in one change for that mechanism. - Rejected: bound the conversion by `Bun::maxVectorSize` so that a test can reach the failure with a small string. It would test a branch that production never takes, and a crash fix needs the crashing input as its fixture. **Review.** The review asked that the two cases stay apart: a `const char*` consumer (always a copy) and a pointer plus length consumer (borrow an ASCII string). `UTF8View` is the second case only, before and after this PR. Since the review: the comment on `UTF8View` says so, the fields are `m_borrowed` and `m_converted`, and the ASCII row above guards the borrow. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 16 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/bun/util/utf8-conversion-limit.test.ts" bun test v1.4.3 (09bb546) test/js/bun/util/utf8-conversion-limit.test.ts: 69 | env: bunEnv, 70 | stdout: "pipe", 71 | stderr: "pipe", 72 | }); 73 | const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); 74 | expect({ stdout: stdout.trim().split("\n"), stderr, exitCode }).toEqual({ ^ error: expect(received).toEqual(expected) { - "exitCode": 0, - "stderr": "", + "exitCode": 134, + "stderr": + "ASSERTION FAILED: expectedString + vendor/WebKit/Source/WTF/wtf/text/StringView.cpp(121) : CString WTF::StringView::utf8(ConversionMode) const + no stacktrace available" + , "stdout": [ - "X509Certificate#checkHost: returned undefined", - "X509Certificate#checkEmail: returned undefined", - "new X509Certificate: RangeError: Out of memory", - "hkdfSync salt: RangeError: Out of memory", - "hkdfSync info: RangeEr ... (truncated) release without fix: 1 FAILED bun test v1.4.3-canary.1 (09bb546) test/js/bun/util/utf8-conversion-limit.test.ts: 69 | env: bunEnv, 70 | stdout: "pipe", 71 | stderr: "pipe", 72 | }); 73 | const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); 74 | expect({ stdout: stdout.trim().split("\n"), stderr, exitCode }).toEqual({ ^ error: expect(received).toEqual(expected) { - "exitCode": 0, - "stderr": "", + "exitCode": 134, + "stderr": + "============================================================ + Bun Canary v1.4.3-canary.1 (09bb546) Linux x64 + Linux Kernel v7.0.0 | glibc v2.41 + CPU: sse42 popcnt avx avx2 avx512 + Args: "/workspace/bun/build/release/bun" "-e" "\n import { decodeURIComponentSIMD } from \"bun:internal-for-testing\";\n import { Database } from \"bun:sqlite\";\n import crypto from \"node:crypto\";\n import Module"... + Features: Bun.stderr(2) bunfig jsc tsconfig + Builtins: "bun:internal-for-testing" "bun:main" "bun:sqlite" "node:crypto" "node:module" + + Elapsed: 740ms | User: 198ms | Sys: 543ms + RSS: 1.12 ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/bun/util/utf8-conversion-limit.test.ts" bun test v1.4.3 (09bb546) test/js/bun/util/utf8-conversion-limit.test.ts: (pass) a string whose UTF-8 form does not fit in a buffer is an error instead of an abort [3129.31ms] (pass) a short Latin-1 string converts as before [70.38ms] (pass) a short 16-bit string converts as before [10.22ms] 3 pass 0 fail 3 expect() calls Ran 3 tests across 1 file. [5.80s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 40af339 features lto, baseline 23 deps, 131 codegen, 1176 objects in 698ms ninja: Entering directory `/workspace/bun/build/release' [1/1254] mkdir codegen [2/1254] mkdir stamps [3/1254] mkdir pch [4/1254] mkdir obj [5/1254] install /workspace/bun bun install v1.4.3-canary.1 (09bb546) Checked 22 installs across 61 packages (no changes) [11.00ms] [6/1254] install /workspace/bun/packages/bun-error bun install v1.4.3-canary.1 (09bb546) Checked 1 install across 2 packages (no changes) [4.00ms] [7/1254] gen ErrorCode+*.h [8/1254] install /workspace/bun/src/node-fallbacks bun install v1.4.3-canary.1 (09bb546) Checked 111 installs across 104 packages (no changes) [6.00ms] [9/1254] gen .bind.ts → GeneratedBindings.cpp [10/1254] fetch picohttpparser [picohttpparser] up to date [11/1254] fetch zlib [zlib] up to date [12/1254] gen bindgenv2 [13/1254] gen node-fallbacks/react-refresh.js Bundled 1 module in 5ms react-refresh.js 4.81 KB (entry point) [14/1254] fetch libjpeg ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/bindings/BunString.cpp | 8 ++ src/jsc/bindings/BunString.h | 52 ++++----- src/jsc/bindings/CookieMap.cpp | 6 +- src/jsc/bindings/JSCommonJSModule.cpp | 6 +- src/jsc/bindings/JSX509Certificate.cpp | 9 +- src/jsc/bindings/JSX509CertificatePrototype.cpp | 26 +++-- src/jsc/bindings/decodeURIComponentSIMD.cpp | 5 +- src/jsc/bindings/node/crypto/CryptoHkdf.cpp | 5 +- src/jsc/bindings/node/crypto/CryptoUtil.cpp | 76 -------------- src/jsc/bindings/node/crypto/CryptoUtil.h | 2 - src/jsc/bindings/node/crypto/JSVerify.cpp | 36 ------- src/jsc/bindings/sqlite/JSSQLStatement.cpp | 22 ++-- src/jsc/bindings/webcore/WebSocket.cpp | 5 +- src/jsc/modules/NodeModuleModule.cpp | 10 +- src/runtime/webview/ChromeBackend.cpp | 8 +- test/js/bun/util/utf8-conversion-limit.test.ts | 134 ++++++++++++++++++++++++ 16 files changed, 233 insertions(+), 177 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/bindings/BunString.cpp 0 0 13 src/jsc/bindings/BunString.h 3 2 13 src/jsc/bindings/CookieMap.cpp 0 0 13 src/jsc/bindings/JSCommonJSModule.cpp 0 0 13 src/jsc/bindings/JSX509Certificate.cpp 0 0 13 src/jsc/bindings/JSX509CertificatePrototype.cpp 0 0 13 src/jsc/bindings/decodeURIComponentSIMD.cpp 0 0 13 src/jsc/bindings/node/crypto/CryptoHkdf.cpp 0 0 13 src/jsc/bindings/node/crypto/CryptoUtil.cpp 0 0 13 src/jsc/bindings/node/crypto/CryptoUtil.h 0 0 13 src/jsc/bindings/node/crypto/JSVerify.cpp 0 0 13 src/jsc/bindings/sqlite/JSSQLStatement.cpp 0 0 13 src/jsc/bindings/webcore/WebSocket.cpp 0 0 13 src/jsc/modules/NodeModuleModule.cpp 0 0 13 src/runtime/webview/ChromeBackend.cpp 0 0 13 test/js/bun/util/utf8-conversion-limit.test.ts 1 1 13 ``` </details> <!-- robobun:evidence:end -->
|
Closing: main already has this change. #47820 (commit 5e08ec1) builds I ran the test block of this PR against a debug build of main (cdfa19b). 13 of the 14 cases pass. The one case that fails is the UTCTime with a timezone offset ( |
Problem
X509CertificatevalidToDate/validFromDatemove a validity year 0000-0099 to 1950-2049. A certificate that expired in the year 30 reportsvalidToDate = 2030-12-31T23:59:59.000Z, socert.validToDate > new Date()istrue. Node v26.3.0 reports0030-12-31T23:59:59.000Z.src/jsc/bindings/JSX509CertificatePrototype.cpp:722-731(validFromDatehas the same code). The getter parses thevalidTotext (Dec 31 23:59:59 30 GMT) withWTF::parseDate, which reads the year30as 2030.Invalid Datefor a UTCTime with a+hhmmoffset (the text isBad time value). Node returns the instant.Fix
X509View::getValidFromTime/getValidToTimeconvert theASN1_TIMEto POSIX seconds. The getters build theDatefromseconds * 1000, as Node does (crypto_x509.cc, ncrypto.cc).ASN1_TIME_to_posix_nonstandard. BoringSSL's X509 parser accepts the+hhmmform and the strictASN1_TIME_to_posixrefuses it. OpenSSL'sASN1_TIME_to_tm, which Node uses, applies the offset.Invalid Date, as before.test/js/node/crypto/x509.test.ts(new block: bun 1.4.3 fails 6 of 14, Node v26.3.0 passes the same assertions). Alsotest-crypto-x509.js,test-tls-getcertificate-x509.js,test/regression/issue/21274.test.ts.Background
ASN1_TIME: a UTCTime (YYMMDDHHMMSSZ, years 1950-2049) or a GeneralizedTime (YYYYMMDDHHMMSSZ). RFC 5280 has no encoding for a year before 1950, but a GeneralizedTime year 0000-0099 parses in BoringSSL and in OpenSSL.ASN1_TIME_printdoes not pad the year, so the year 30 prints as30.validFrom/validTo,toLegacyObject()and TLS verification do not use the Date getters. This change does not touch them.Notes
Repro (mints a certificate that is valid from the year 20 to the year 30):
validTovalidToDatevalidToDate > new Date()Dec 31 23:59:59 30 GMT2030-12-31T23:59:59.000ZtrueDec 31 23:59:59 30 GMT0030-12-31T23:59:59.000ZfalseDec 31 23:59:59 30 GMT0030-12-31T23:59:59.000ZfalseThe old parser moved the years 0-49 to 2000-2049 and the years 50-99 to 1950-1999. A year of 100 or later was already exact. The new test writes the years 0000, 0049, 0050, 0099, 0100, 1601, 1969, 1970, 2038, 2049, 2050 and 9999 over one GeneralizedTime template. The 1949 / 1950 / 2049 / 2050 UTCTime boundary is already in
test-crypto-x509.js.UTCTime offset. The test certificate has notBefore
200101000000-0530and notAfter301231235959+0100.validFromvalidFromDatevalidTovalidToDateBad time valueInvalid DateBad time valueInvalid DateBad time value2020-01-01T05:30:00.000ZBad time value2030-12-31T22:59:59.000ZJan 1 05:30:00 2020 GMT2020-01-01T05:30:00.000ZDec 31 22:59:59 2030 GMT2030-12-31T22:59:59.000ZThe
validFrom/validTostrings for that form still differ from Node. BoringSSL'sASN1_TIME_printparses withallow_timezone_offset=0. That is a different getter with a different cause, and this PR leaves it alone.Why a failed conversion is not reachable after a successful parse:
x_x509.ccvalidates both times withallow_utc_timezone_offset=1, which is whatASN1_TIME_to_posix_nonstandardaccepts.OPENSSL_tm_to_posixcovers the years 0000-9999, which is all a GeneralizedTime can hold. Thestd::optionalis there because Node's version ignores the return code and reads an uninitializedint64_ton failure.The conversion does not touch the OpenSSL error queue, so the new functions have no
ClearErrorOnReturn.The old getters returned
undefinedwhen the printed text was empty.ASN1_TIME_printalways prints something, so that branch was not reachable. Node always returns aDate.[human-review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file