Merge pull request #6313 from cloudflare/yagiz/fix-x509-certificate

anonrig · web-flow · commit 15392d48ba6f · 2026-03-12T20:12:30.000-04:00
fix X509Certificate::parse() throwing internal error for invalid input
diff --git a/src/node/internal/crypto.d.ts b/src/node/internal/crypto.d.ts
@@ -31,7 +31,7 @@ export interface CheckOptions {
 }
 
 export class X509Certificate {
-  static parse(data: ArrayBuffer | ArrayBufferView): X509Certificate;
+  static parse(data: ArrayBuffer | ArrayBufferView): X509Certificate | null;
   get subject(): string | undefined;
   get subjectAltName(): string | undefined;
   get infoAccess(): string | undefined;
diff --git a/src/node/internal/crypto_x509.ts b/src/node/internal/crypto_x509.ts
@@ -114,7 +114,15 @@ export class X509Certificate {
         buffer
       );
     }
-    this.#handle = cryptoImpl.X509Certificate.parse(buffer);
+    const handle = cryptoImpl.X509Certificate.parse(buffer);
+    if (handle == null) {
+      throw new ERR_INVALID_ARG_VALUE(
+        'buffer',
+        buffer,
+        'is not a valid certificate'
+      );
+    }
+    this.#handle = handle;
   }
 
   get subject() {
diff --git a/src/workerd/api/crypto/x509.c++ b/src/workerd/api/crypto/x509.c++
@@ -514,15 +514,17 @@ constexpr StackOfXASN1Disposer stackOfXASN1Disposer;
 
 kj::Maybe<jsg::Ref<X509Certificate>> X509Certificate::parse(
     jsg::Lock& js, kj::Array<const kj::byte> raw) {
-  ClearErrorOnReturn ClearErrorOnReturn;
+  ClearErrorOnReturn clearErrorOnReturn;
   KJ_IF_SOME(bio, loadBio(raw)) {
     auto ptr = PEM_read_bio_X509_AUX(bio.get(), nullptr, NoPasswordCallback, nullptr);
     if (ptr == nullptr) {
       MarkPopErrorOnReturn mark_here;
       auto data = raw.begin();
       ptr = d2i_X509(nullptr, &data, raw.size());
       if (ptr == nullptr) {
-        throwOpensslError(__FILE__, __LINE__, "X509Certificate::parse()");
+        // Invalid certificate data is a user input error, not an internal error.
+        // Return kj::none and let the JS layer throw a user-facing error.
+        return kj::none;
       }
     }
     return js.alloc<X509Certificate>(ptr);
diff --git a/src/workerd/api/node/tests/crypto_x509-test.js b/src/workerd/api/node/tests/crypto_x509-test.js
@@ -347,6 +347,37 @@ export const test_ok = {
 //   }
 // };
 
+// Verify that invalid certificate data throws a user-facing error (not an internal error).
+export const test_invalid_cert = {
+  test() {
+    // Completely invalid data
+    throws(() => new X509Certificate('not a certificate'), {
+      code: 'ERR_INVALID_ARG_VALUE',
+    });
+
+    // Invalid base64 in PEM wrapper
+    throws(
+      () =>
+        new X509Certificate(
+          '-----BEGIN CERTIFICATE-----\n!!!invalid!!!\n-----END CERTIFICATE-----'
+        ),
+      {
+        code: 'ERR_INVALID_ARG_VALUE',
+      }
+    );
+
+    // Empty buffer
+    throws(() => new X509Certificate(Buffer.alloc(0)), {
+      code: 'ERR_INVALID_ARG_VALUE',
+    });
+
+    // Random bytes (not a valid DER or PEM certificate)
+    throws(() => new X509Certificate(Buffer.from([0x01, 0x02, 0x03])), {
+      code: 'ERR_INVALID_ARG_VALUE',
+    });
+  },
+};
+
 // Ref: https://github.com/unjs/unenv/pull/310
 export const shouldImportCertificate = {
   test() {

Original file line number	Diff line number	Diff line change
`@@ -31,7 +31,7 @@ export interface CheckOptions {`
`31`	`31`	`}`
`32`	`32`
`33`	`33`	`export class X509Certificate {`
`34`		`- static parse(data: ArrayBuffer \| ArrayBufferView): X509Certificate;`
	`34`	`+ static parse(data: ArrayBuffer \| ArrayBufferView): X509Certificate \| null;`
`35`	`35`	`get subject(): string \| undefined;`
`36`	`36`	`get subjectAltName(): string \| undefined;`
`37`	`37`	`get infoAccess(): string \| undefined;`