Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 16, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/violet-poems-wait.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
33 changes: 29 additions & 4 deletions packages/wrangler/src/__tests__/kv.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(`
Expand All @@ -1293,7 +1318,7 @@ describe("wrangler", () => {
expect(std.warn).toMatchInlineSnapshot(`
"▲ [WARNING] Unexpected key-value properties in \\"keys.json\\".

The item at index 4 contains unexpected properties: [\\"invalid\\"].
The item at index 5 contains unexpected properties: [\\"invalid\\"].

"
`);
Expand Down
8 changes: 1 addition & 7 deletions packages/wrangler/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to add this check into the validation helper function! 👍🏻

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)}`
);
Expand Down
21 changes: 18 additions & 3 deletions packages/wrangler/src/kv.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down