From 5b0e8ad0a23ec5dc910f3ece86c3d0bc6f77c0e0 Mon Sep 17 00:00:00 2001
From: Harshal Brahmbhatt <harshal@cloudflare.com>
Date: Tue, 27 Aug 2024 14:26:16 +0000
Subject: [PATCH] Fix R2 wildcard etag parsing

This commit addresses issue #2572
---
 src/workerd/api/r2-bucket.c++   | 17 ++++--
 src/workerd/api/r2-test.js      | 98 +++++++++++++++++++++++++++++++++
 src/workerd/api/r2-test.wd-test | 19 +++++++
 3 files changed, 129 insertions(+), 5 deletions(-)
 create mode 100644 src/workerd/api/r2-test.js
 create mode 100644 src/workerd/api/r2-test.wd-test

diff --git a/src/workerd/api/r2-bucket.c++ b/src/workerd/api/r2-bucket.c++
index 1428f5919b7f..5c659df0bcf0 100644
--- a/src/workerd/api/r2-bucket.c++
+++ b/src/workerd/api/r2-bucket.c++
@@ -935,10 +935,17 @@ kj::Array<R2Bucket::Etag> parseConditionalEtagHeader(kj::StringPtr condHeader,
   }
 }
 
-kj::Array<R2Bucket::Etag> buildSingleStrongEtagArray(kj::StringPtr etagValue) {
-  struct R2Bucket::StrongEtag etag = {.value = kj::str(etagValue)};
+kj::Array<R2Bucket::Etag> buildSingleEtagArray(kj::StringPtr etagValue) {
   kj::ArrayBuilder<R2Bucket::Etag> etagArrayBuilder = kj::heapArrayBuilder<R2Bucket::Etag>(1);
-  etagArrayBuilder.add(kj::mv(etag));
+
+  if (etagValue == "*") {
+    struct R2Bucket::WildcardEtag etag = {};
+    etagArrayBuilder.add(kj::mv(etag));
+  } else {
+    struct R2Bucket::StrongEtag etag = {.value = kj::str(etagValue)};
+    etagArrayBuilder.add(kj::mv(etag));
+  }
+
   return etagArrayBuilder.finish();
 }
 
@@ -967,12 +974,12 @@ R2Bucket::UnwrappedConditional::UnwrappedConditional(const Conditional& c)
   KJ_IF_SOME(e, c.etagMatches) {
     JSG_REQUIRE(!isQuotedEtag(e.value), TypeError,
         "Conditional ETag should not be wrapped in quotes (", e.value, ").");
-    etagMatches = buildSingleStrongEtagArray(e.value);
+    etagMatches = buildSingleEtagArray(e.value);
   }
   KJ_IF_SOME(e, c.etagDoesNotMatch) {
     JSG_REQUIRE(!isQuotedEtag(e.value), TypeError,
         "Conditional ETag should not be wrapped in quotes (", e.value, ").");
-    etagDoesNotMatch = buildSingleStrongEtagArray(e.value);
+    etagDoesNotMatch = buildSingleEtagArray(e.value);
   }
   KJ_IF_SOME(d, c.uploadedAfter) {
     uploadedAfter = d;
diff --git a/src/workerd/api/r2-test.js b/src/workerd/api/r2-test.js
new file mode 100644
index 000000000000..0e95f794541d
--- /dev/null
+++ b/src/workerd/api/r2-test.js
@@ -0,0 +1,98 @@
+// Copyright (c) 2023 Cloudflare, Inc.
+// Licensed under the Apache 2.0 license found in the LICENSE file or at:
+//     https://opensource.org/licenses/Apache-2.0
+
+import assert from 'node:assert'
+
+export default {
+  // Handler for HTTP request binding makes to R2
+  async fetch(request, env, ctx) {
+    // We only expect PUT/Get
+    assert(['GET', 'PUT'].includes(request.method))
+
+    // Each request should have a metadata size header indicating how much
+    // we should read to understand what type of request this is
+    const metadataSizeString = request.headers.get('cf-r2-metadata-size')
+    assert.notStrictEqual(metadataSizeString, null)
+
+    const metadataSize = parseInt(metadataSizeString)
+    assert(!Number.isNaN(metadataSize))
+
+    const reader = request.body.getReader({ mode: 'byob' })
+    const jsonArray = new Uint8Array(metadataSize)
+    const { value } = await reader.readAtLeast(metadataSize, jsonArray)
+    reader.releaseLock()
+
+    const jsonRequest = JSON.parse(new TextDecoder().decode(value))
+
+    // Currently not using the body in these test so I'm going to just discard
+    for await (const _ of request.body) {
+    }
+
+    // Assert it's the correct version
+    assert((jsonRequest.version = 1))
+
+    if (request.method === 'PUT') {
+      assert(jsonRequest.method === 'put')
+
+      if (jsonRequest.object === 'onlyIfStrongEtag') {
+        assert.deepStrictEqual(jsonRequest.onlyIf, {
+          etagMatches: [
+            {
+              value: 'strongEtag',
+              type: 'strong',
+            },
+          ],
+          etagDoesNotMatch: [
+            {
+              value: 'strongEtag',
+              type: 'strong',
+            },
+          ],
+        })
+      }
+
+      if (jsonRequest.object === 'onlyIfWildcard') {
+        assert.deepStrictEqual(jsonRequest.onlyIf, {
+          etagMatches: [
+            {
+              type: 'wildcard',
+            },
+          ],
+          etagDoesNotMatch: [
+            {
+              type: 'wildcard',
+            },
+          ],
+        })
+      }
+
+      return Response.json({
+        name: 'objectKey',
+        version: 'objectVersion',
+        size: '123',
+        etag: 'objectEtag',
+        uploaded: '1724767257918',
+        storageClass: 'Standard',
+      })
+    }
+
+    throw new Error('unexpected')
+  },
+
+  async test(ctrl, env, ctx) {
+    await env.BUCKET.put('basic', 'content')
+    await env.BUCKET.put('onlyIfStrongEtag', 'content', {
+      onlyIf: {
+        etagMatches: 'strongEtag',
+        etagDoesNotMatch: 'strongEtag',
+      },
+    })
+    await env.BUCKET.put('onlyIfWildcard', 'content', {
+      onlyIf: {
+        etagMatches: '*',
+        etagDoesNotMatch: '*',
+      },
+    })
+  },
+}
diff --git a/src/workerd/api/r2-test.wd-test b/src/workerd/api/r2-test.wd-test
new file mode 100644
index 000000000000..52703f7be0aa
--- /dev/null
+++ b/src/workerd/api/r2-test.wd-test
@@ -0,0 +1,19 @@
+using Workerd = import "/workerd/workerd.capnp";
+
+const unitTests :Workerd.Config = (
+  services = [
+    ( name = "r2-test",
+      worker = (
+        modules = [
+          ( name = "worker", esModule = embed "r2-test.js" )
+        ],
+        bindings = [
+          ( name = "BUCKET", r2Bucket = "r2-test" ),
+          ( name = "SERVICE", service = "r2-test" ),
+        ],
+        compatibilityDate = "2023-07-24",
+        compatibilityFlags = ["nodejs_compat", "service_binding_extra_handlers"],
+      )
+    ),
+  ],
+);