From ce0322dabce9adb90762f4155cc155ad1889fb4d Mon Sep 17 00:00:00 2001
From: James M Snell <jasnell@gmail.com>
Date: Thu, 11 Jul 2024 17:28:40 -0700
Subject: [PATCH] Make deriveBits length argument optional per spec change

Fixes: https://github.com/cloudflare/workerd/issues/2357
---
 src/workerd/api/crypto/crypto.c++           | 14 ++---
 src/workerd/api/crypto/crypto.h             |  7 ++-
 src/workerd/api/tests/crypto-extras-test.js | 57 +++++++++++++++++++++
 3 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/src/workerd/api/crypto/crypto.c++ b/src/workerd/api/crypto/crypto.c++
index 9f96e0f9c83..bcdab7ba840 100644
--- a/src/workerd/api/crypto/crypto.c++
+++ b/src/workerd/api/crypto/crypto.c++
@@ -447,16 +447,18 @@ jsg::Promise<kj::Array<kj::byte>> SubtleCrypto::deriveBits(
     jsg::Lock& js,
     kj::OneOf<kj::String, DeriveKeyAlgorithm> algorithmParam,
     const CryptoKey& baseKey,
-    kj::Maybe<int> lengthParam) {
+    jsg::Optional<kj::Maybe<int>> lengthParam) {
   auto algorithm = interpretAlgorithmParam(kj::mv(algorithmParam));
 
   auto checkErrorsOnFinish = webCryptoOperationBegin(__func__, algorithm);
 
-  auto length = lengthParam.map([&](int l) {
-    // TODO(cleanup): Use the type jsg wrapper for uint32_t?
-    JSG_REQUIRE(l >= 0, TypeError, "deriveBits length must be an unsigned long integer.");
-    return uint32_t(l);
-  });
+  kj::Maybe<uint32_t> length = kj::none;
+  KJ_IF_SOME(maybeLength, lengthParam) {
+    KJ_IF_SOME(l, maybeLength) {
+      JSG_REQUIRE(l >= 0, TypeError, "deriveBits length must be an unsigned long integer.");
+      length = uint32_t(l);
+    }
+  }
 
   return js.evalNow([&] {
     validateOperation(baseKey, algorithm.name, CryptoKeyUsageSet::deriveBits());
diff --git a/src/workerd/api/crypto/crypto.h b/src/workerd/api/crypto/crypto.h
index 3500a08f376..6c209b3fb1d 100644
--- a/src/workerd/api/crypto/crypto.h
+++ b/src/workerd/api/crypto/crypto.h
@@ -540,7 +540,12 @@ class SubtleCrypto: public jsg::Object {
       jsg::Lock& js,
       kj::OneOf<kj::String, DeriveKeyAlgorithm> algorithm,
       const CryptoKey& baseKey,
-      kj::Maybe<int> length);
+      // The operation needs to be able to take both undefined and null
+      // and handle them equivalently... if we just used jsg::Optional<int>
+      // here, null would be coerced to 0. If we just used kj::Maybe<int>
+      // here, undefined would be an error. So we need to use an optional
+      // maybe int in order to treat undefined and null as being equivalent.
+      jsg::Optional<kj::Maybe<int>> length);
 
   jsg::Promise<jsg::Ref<CryptoKey>> importKey(
       jsg::Lock& js,
diff --git a/src/workerd/api/tests/crypto-extras-test.js b/src/workerd/api/tests/crypto-extras-test.js
index 82751b2b49c..6a1fa218e0b 100644
--- a/src/workerd/api/tests/crypto-extras-test.js
+++ b/src/workerd/api/tests/crypto-extras-test.js
@@ -137,3 +137,60 @@ export const cryptoZeroLength = {
   }
 };
 
+export const deriveBitsNullLength = {
+  async test() {
+
+    // Tests that deriveBits can take a null or undefined length
+    // argument and still return the correct number of bits if
+    // the algorithm supports it. This is a recent spec change.
+
+    const pair = await crypto.subtle.generateKey(
+      {
+        name: "ECDH",
+        namedCurve: "P-384",
+      },
+      false,
+      ["deriveBits"],
+    );
+
+    {
+      const bits = await crypto.subtle.deriveBits(
+        {
+          name: 'ECDH',
+          namedCurve: 'P-384',
+          public: pair.publicKey,
+        },
+        pair.privateKey, undefined
+      );
+
+      strictEqual(bits.byteLength, 48);
+    }
+
+    {
+      const bits = await crypto.subtle.deriveBits(
+        {
+          name: 'ECDH',
+          namedCurve: 'P-384',
+          public: pair.publicKey,
+        },
+        pair.privateKey, null
+      );
+
+      strictEqual(bits.byteLength, 48);
+    }
+
+    {
+      const bits = await crypto.subtle.deriveBits(
+        {
+          name: 'ECDH',
+          namedCurve: 'P-384',
+          public: pair.publicKey,
+        },
+        pair.privateKey
+      );
+
+      strictEqual(bits.byteLength, 48);
+    }
+
+  }
+};