wrangler: Try to URL decode hyperdrive connection string components b…

…efore sending them to API (#5258) * wrangler: Try to URL decode hyperdrive connection string components before sending them to API Also do the same in miniflare plugin * fixup! wrangler: Try to URL decode hyperdrive connection string components before sending them to API
cloudflare · Mar 27, 2024 · fbdca7d · fbdca7d
1 parent 47b325a
commit fbdca7d
Show file tree

Hide file tree

Showing 6 changed files with 135 additions and 36 deletions.
diff --git a/.changeset/twenty-laws-mix.md b/.changeset/twenty-laws-mix.md
@@ -0,0 +1,6 @@
+---
+"miniflare": minor
+"wrangler": minor
+---
+
+feature: URL decode components of the Hyperdrive config connection string
diff --git a/packages/miniflare/src/plugins/hyperdrive/index.ts b/packages/miniflare/src/plugins/hyperdrive/index.ts
@@ -81,9 +81,9 @@ export const HYPERDRIVE_PLUGIN: Plugin<typeof HyperdriveInputOptionsSchema> = {
 						designator: {
 							name: `${HYPERDRIVE_PLUGIN_NAME}:${name}`,
 						},
-						database,
-						user: url.username,
-						password: url.password,
+						database: decodeURIComponent(database),
+						user: decodeURIComponent(url.username),
+						password: decodeURIComponent(url.password),
 						scheme,
 					},
 				};

diff --git a/packages/wrangler/e2e/dev.test.ts b/packages/wrangler/e2e/dev.test.ts
@@ -484,7 +484,7 @@ describe("hyperdrive dev tests", () => {
 					[[hyperdrive]]
 					binding = "HYPERDRIVE"
 					id = "hyperdrive_id"
-					localConnectionString = "postgresql://user:pass@127.0.0.1:${port}/some_db"
+					localConnectionString = "postgresql://user%3Aname:%21pass@127.0.0.1:${port}/some_db"
 			`,
 			"src/index.ts": dedent`
 					export default {
@@ -516,8 +516,8 @@ describe("hyperdrive dev tests", () => {
 			);
 			const url = new URL(text);
 			expect(url.pathname).toBe("/some_db");
-			expect(url.username).toBe("user");
-			expect(url.password).toBe("pass");
+			expect(url.username).toBe("user:name");
+			expect(url.password).toBe("!pass");
 			expect(url.host).not.toBe("localhost");
 		});
 	});

diff --git a/packages/wrangler/src/__tests__/hyperdrive.test.ts b/packages/wrangler/src/__tests__/hyperdrive.test.ts
@@ -178,6 +178,78 @@ describe("hyperdrive commands", () => {
 	`);
 	});
 
+	it("should handle creating a hyperdrive config if the user is URL encoded", async () => {
+		mockHyperdriveRequest();
+		await runWrangler(
+			"hyperdrive create test123 --connection-string='postgresql://user%3Aname:password@example.com/neondb'"
+		);
+		expect(std.out).toMatchInlineSnapshot(`
+		"🚧 Creating 'test123'
+		✅ Created new Hyperdrive config
+		 {
+		  \\"id\\": \\"xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx\\",
+		  \\"name\\": \\"test123\\",
+		  \\"origin\\": {
+		    \\"host\\": \\"example.com\\",
+		    \\"port\\": 5432,
+		    \\"database\\": \\"neondb\\",
+		    \\"user\\": \\"user:name\\"
+		  },
+		  \\"caching\\": {
+		    \\"disabled\\": false
+		  }
+		}"
+	`);
+	});
+
+	it("should handle creating a hyperdrive config if the password is URL encoded", async () => {
+		mockHyperdriveRequest();
+		await runWrangler(
+			"hyperdrive create test123 --connection-string='postgresql://test:a%23%3F81n%287@example.com/neondb'"
+		);
+		expect(std.out).toMatchInlineSnapshot(`
+		"🚧 Creating 'test123'
+		✅ Created new Hyperdrive config
+		 {
+		  \\"id\\": \\"xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx\\",
+		  \\"name\\": \\"test123\\",
+		  \\"origin\\": {
+		    \\"host\\": \\"example.com\\",
+		    \\"port\\": 5432,
+		    \\"database\\": \\"neondb\\",
+		    \\"user\\": \\"test\\"
+		  },
+		  \\"caching\\": {
+		    \\"disabled\\": false
+		  }
+		}"
+	`);
+	});
+
+	it("should handle creating a hyperdrive config if the database name is URL encoded", async () => {
+		mockHyperdriveRequest();
+		await runWrangler(
+			"hyperdrive create test123 --connection-string='postgresql://test:password@example.com/%22weird%22%20dbname'"
+		);
+		expect(std.out).toMatchInlineSnapshot(`
+		"🚧 Creating 'test123'
+		✅ Created new Hyperdrive config
+		 {
+		  \\"id\\": \\"xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx\\",
+		  \\"name\\": \\"test123\\",
+		  \\"origin\\": {
+		    \\"host\\": \\"example.com\\",
+		    \\"port\\": 5432,
+		    \\"database\\": \\"/\\"weird/\\" dbname\\",
+		    \\"user\\": \\"test\\"
+		  },
+		  \\"caching\\": {
+		    \\"disabled\\": false
+		  }
+		}"
+	`);
+	});
+
 	it("should handle listing configs", async () => {
 		mockHyperdriveRequest();
 		await runWrangler("hyperdrive list");
@@ -258,10 +330,7 @@ describe("hyperdrive commands", () => {
 
 		"
 	`);
-		expect(std.out).toMatchInlineSnapshot(`
-		"
-		[32mIf you think this is a bug then please create an issue at https://github.com/cloudflare/workers-sdk/issues/new/choose[0m"
-	`);
+		expect(std.out).toMatchInlineSnapshot(`""`);
 	});
 
 	it("should handle updating a hyperdrive config's caching settings", async () => {
@@ -290,6 +359,30 @@ describe("hyperdrive commands", () => {
 	`);
 	});
 
+	it("should handle disabling caching for a hyperdrive config", async () => {
+		mockHyperdriveRequest();
+		await runWrangler(
+			"hyperdrive update xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx --caching-disabled=true"
+		);
+		expect(std.out).toMatchInlineSnapshot(`
+		"🚧 Updating 'xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx'
+		✅ Updated xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx Hyperdrive config
+		 {
+		  \\"id\\": \\"xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx\\",
+		  \\"name\\": \\"test123\\",
+		  \\"origin\\": {
+		    \\"host\\": \\"example.com\\",
+		    \\"port\\": 5432,
+		    \\"database\\": \\"neondb\\",
+		    \\"user\\": \\"test\\"
+		  },
+		  \\"caching\\": {
+		    \\"disabled\\": true
+		  }
+		}"
+	`);
+	});
+
 	it("should handle updating a hyperdrive config's name", async () => {
 		mockHyperdriveRequest();
 		await runWrangler(

diff --git a/packages/wrangler/src/hyperdrive/create.ts b/packages/wrangler/src/hyperdrive/create.ts
@@ -88,9 +88,9 @@ export async function handler(
 				host: url.hostname,
 				port: parseInt(url.port),
 				scheme: url.protocol.replace(":", ""),
-				database: url.pathname.replace("/", ""),
-				user: url.username,
-				password: url.password,
+				database: decodeURIComponent(url.pathname.replace("/", "")),
+				user: decodeURIComponent(url.username),
+				password: decodeURIComponent(url.password),
 			},
 			caching: {
 				disabled: args.cachingDisabled,

diff --git a/packages/wrangler/src/hyperdrive/update.ts b/packages/wrangler/src/hyperdrive/update.ts
@@ -1,4 +1,5 @@
 import { readConfig } from "../config";
+import { UserError } from "../errors";
 import { logger } from "../logger";
 import { patchConfig } from "./client";
 import { hyperdriveBetaWarning } from "./utils";
@@ -62,12 +63,17 @@ export function options(yargs: CommonYargsArgv) {
 }
 
 const requiredOriginOptions = [
-	"origin-host",
-	"origin-port",
+	"originHost",
+	"originPort",
 	"database",
-	"origin-user",
-	"origin-password",
-];
+	"originUser",
+	"originPassword",
+] as const;
+
+// utility for displaying the yargs options to the user when displaying the "all or nothing" error message
+function camelToKebab(str: string): string {
+	return str.replace(/([a-z0-9])([A-Z])/g, "$1-$2").toLowerCase();
+}
 
 function isOptionSet<T extends object>(args: T, key: keyof T): boolean {
 	return key in args && args[key] !== undefined;
@@ -78,17 +84,17 @@ export async function handler(
 ) {
 	// check if all or none of the required origin fields are set, since we don't allow partial updates of the origin
 	const allOriginFieldsSet = requiredOriginOptions.every((field) =>
-		isOptionSet(args, field as keyof typeof options)
+		isOptionSet(args, field)
 	);
 	const noOriginFieldSet = requiredOriginOptions.every(
-		(field) => !isOptionSet(args, field as keyof typeof options)
+		(field) => !isOptionSet(args, field)
 	);
 
 	if (!allOriginFieldsSet && !noOriginFieldSet) {
-		throw new Error(
-			`When updating the origin, all of the following must be set: ${requiredOriginOptions.join(
-				", "
-			)}`
+		throw new UserError(
+			`When updating the origin, all of the following must be set: ${requiredOriginOptions
+				.map((option) => camelToKebab(option))
+				.join(", ")}`
 		);
 	}
 
@@ -104,26 +110,20 @@ export async function handler(
 
 	if (allOriginFieldsSet) {
 		database.origin = {
+			scheme: args.originScheme ?? "postgresql",
 			host: args.originHost,
 			port: args.originPort,
 			database: args.database,
 			user: args.originUser,
 			password: args.originPassword,
 		};
-		if (args.originScheme !== undefined) {
-			database.origin.scheme = args.originScheme;
-		} else {
-			database.origin.scheme = "postgresql"; // setting default if not passed
-		}
 	}
 
-	if (args.cachingDisabled || args.maxAge || args.swr) {
-		database.caching = {
-			disabled: args.cachingDisabled,
-			maxAge: args.maxAge,
-			staleWhileRevalidate: args.swr,
-		};
-	}
+	database.caching = {
+		disabled: args.cachingDisabled,
+		maxAge: args.maxAge,
+		staleWhileRevalidate: args.swr,
+	};
 
 	const updated = await patchConfig(config, args.id, database);
 	logger.log(