polish: validate payload for kv:bulk put on client side (#1024)

This adds client side validation for the paylod for `kv:bulk put`, importantly ensuring we're uploading only string key/value pairs (as well as validation for the other fields). Fixes #571
cloudflare · May 16, 2022 · 110f340 · 110f340
1 parent a6e2057
commit 110f340
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 14 deletions.
diff --git a/.changeset/violet-poems-wait.md b/.changeset/violet-poems-wait.md
@@ -0,0 +1,9 @@
+---
+"wrangler": patch
+---
+
+polish: validate payload for `kv:bulk put` on client side
+
+This adds client side validation for the paylod for `kv:bulk put`, importantly ensuring we're uploading only string key/value pairs (as well as validation for the other fields).
+
+Fixes https://github.com/cloudflare/wrangler2/issues/571
diff --git a/packages/wrangler/src/__tests__/kv.test.ts b/packages/wrangler/src/__tests__/kv.test.ts
@@ -1262,7 +1262,25 @@ describe("wrangler", () => {
           "a string",
           { key: "someKey" },
           { value: "someValue" },
+          // add a valid object here to make sure it's not included
+          { key: "someKey1", value: "someValue1" },
+          // this one will only add a warning
           { key: "someKey1", value: "someValue1", invalid: true },
+          // back to the invalid ones
+          { key: 123, value: "somevalue" },
+          { key: "somekey", value: 123 },
+          { key: "someKey1", value: "someValue1", expiration: "string" },
+          { key: "someKey1", value: "someValue1", expiration_ttl: "string" },
+          {
+            key: 123,
+            value: {
+              a: {
+                nested: "object",
+              },
+            },
+          },
+          { key: "someKey1", value: "someValue1", metadata: 123 },
+          { key: "someKey1", value: "someValue1", base64: "string" },
         ];
         writeFileSync("./keys.json", JSON.stringify(keyValues));
         await expect(
@@ -1280,10 +1298,17 @@ describe("wrangler", () => {
                   base64?: boolean;
                 }
 
-                The item at index 0 is type: \\"number\\" - 123
-                The item at index 1 is type: \\"string\\" - \\"a string\\"
+                The item at index 0 is 123
+                The item at index 1 is \\"a string\\"
                 The item at index 2 is {\\"key\\":\\"someKey\\"}
-                The item at index 3 is {\\"value\\":\\"someValue\\"}"
+                The item at index 3 is {\\"value\\":\\"someValue\\"}
+                The item at index 6 is {\\"key\\":123,\\"value\\":\\"somevalue\\"}
+                The item at index 7 is {\\"key\\":\\"somekey\\",\\"value\\":123}
+                The item at index 8 is {\\"key\\":\\"someKey1\\",\\"value\\":\\"someValue1\\",\\"expiration\\":\\"string\\"}
+                The item at index 9 is {\\"key\\":\\"someKey1\\",\\"value\\":\\"someValue1\\",\\"expiration_ttl\\":\\"string\\"}
+                The item at index 10 is {\\"key\\":123,\\"value\\":{\\"a\\":{\\"nested\\":\\"object\\"}}}
+                The item at index 11 is {\\"key\\":\\"someKey1\\",\\"value\\":\\"someValue1\\",\\"metadata\\":123}
+                The item at index 12 is {\\"key\\":\\"someKey1\\",\\"value\\":\\"someValue1\\",\\"base64\\":\\"string\\"}"
               `);
 
         expect(std.out).toMatchInlineSnapshot(`
@@ -1293,7 +1318,7 @@ describe("wrangler", () => {
         expect(std.warn).toMatchInlineSnapshot(`
           "[33m▲ [43;33m[[43;30mWARNING[43;33m][0m [1mUnexpected key-value properties in \\"keys.json\\".[0m
 
-            The item at index 4 contains unexpected properties: [\\"invalid\\"].
+            The item at index 5 contains unexpected properties: [\\"invalid\\"].
 
           "
         `);

diff --git a/packages/wrangler/src/index.tsx b/packages/wrangler/src/index.tsx
@@ -2334,13 +2334,7 @@ export async function main(argv: string[]): Promise<void> {
             const warnings: string[] = [];
             for (let i = 0; i < content.length; i++) {
               const keyValue = content[i];
-              if (typeof keyValue !== "object") {
-                errors.push(
-                  `The item at index ${i} is type: "${typeof keyValue}" - ${JSON.stringify(
-                    keyValue
-                  )}`
-                );
-              } else if (!isKVKeyValue(keyValue)) {
+              if (!isKVKeyValue(keyValue)) {
                 errors.push(
                   `The item at index ${i} is ${JSON.stringify(keyValue)}`
                 );

diff --git a/packages/wrangler/src/kv.ts b/packages/wrangler/src/kv.ts
@@ -129,9 +129,24 @@ const KeyValueKeys = new Set([
 /**
  * Is the given object a valid `KeyValue` type?
  */
-export function isKVKeyValue(keyValue: object): keyValue is KeyValue {
-  const props = Object.keys(keyValue);
-  if (!props.includes("key") || !props.includes("value")) {
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+export function isKVKeyValue(keyValue: any): keyValue is KeyValue {
+  // (keyValue could indeed be any-thing)
+  if (
+    typeof keyValue !== "object" ||
+    typeof keyValue.key !== "string" ||
+    typeof keyValue.value !== "string" ||
+    !(
+      keyValue.expiration === undefined ||
+      typeof keyValue.expiration === "number"
+    ) ||
+    !(
+      keyValue.expiration_ttl === undefined ||
+      typeof keyValue.expiration_ttl === "number"
+    ) ||
+    !(keyValue.base64 === undefined || typeof keyValue.base64 === "boolean") ||
+    !(keyValue.metadata === undefined || typeof keyValue.metadata === "object")
+  ) {
     return false;
   }
   return true;