use `shell-quote` package #4080

RamIdeas · 2023-10-01T12:47:35Z

use shell-quote package instead of approximating behaviour with .split(" ") and .join(" ")

Tests
Changeset (Changeset guidelines)

Reviewer is to perform the following, as applicable:

Checked for inclusion of relevant tests
Checked for inclusion of a relevant changeset
Checked for creation of associated docs updates
Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

changeset-bot · 2023-10-01T12:47:38Z

⚠️ No Changeset found

Latest commit: ea0e5e7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2023-10-01T12:50:00Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6384551922/npm-package-wrangler-4080

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6384551922/npm-package-wrangler-4080

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6384551922/npm-package-wrangler-4080 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6384551922/npm-package-cloudflare-pages-shared-4080

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.10.1 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	3.20230922.0	3.20230922.0
`workerd`	1.20230922.0	1.20230922.0
`workerd --version`	1.20230922.0	2023-09-22

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

codecov · 2023-10-01T12:54:00Z

Codecov Report

Merging #4080 (647440a) into main (17ced68) will decrease coverage by 0.08%.
The diff coverage is 90.90%.

❗ Current head 647440a differs from pull request most recent head ea0e5e7. Consider uploading reports for the commit ea0e5e7 to get more accurate results

@@            Coverage Diff             @@
##             main    #4080      +/-   ##
==========================================
- Coverage   75.07%   74.99%   -0.08%     
==========================================
  Files         216      217       +1     
  Lines       12054    12065      +11     
  Branches     3123     3122       -1     
==========================================
- Hits         9049     9048       -1     
- Misses       3005     3017      +12

Files	Coverage Δ
...ges/wrangler/src/__tests__/helpers/run-wrangler.ts	`100.00% <100.00%> (ø)`
packages/wrangler/src/init.ts	`92.01% <100.00%> (+0.01%)`	⬆️
packages/wrangler/src/utils/shell-quote.ts	`87.50% <87.50%> (ø)`

... and 10 files with indirect coverage changes

RamIdeas · 2023-10-01T12:54:11Z

packages/wrangler/src/__tests__/d1/execute.test.ts

-			runWrangler("d1 execute --command 'select 1;'")
+			runWrangler("d1 execute db --command 'select 1;'")


This test forgot to specify the database positional argument. It was passing before because the command argument has a space (and previously we split the whole command by space), resulting in command parsed as 'select and database parsed as 1;'

RamIdeas · 2023-10-01T12:57:47Z

packages/wrangler/src/__tests__/deploy.test.ts

-				"deploy --dry-run --outdir dist --define abc:'https://www.abc.net.au/news/'"
+				`deploy --dry-run --outdir dist --define "abc:'https://www.abc.net.au/news/'"`


This wasn't a correct example of how a user would specify the define arg with this value.

Before (incorrect, note the missing quotes by the time node.js sees the value):

▶ node -e 'console.log(process.argv.slice(1))' -- --define abc:'123' [ '--define', 'abc:123' ]

After (correct, note the quotes in the value as expected):

▶ node -e 'console.log(process.argv.slice(1))' -- --define "abc:'123'" [ '--define', "abc:'123'" ]

RamIdeas · 2023-10-01T13:03:05Z

packages/wrangler/src/__tests__/kv.test.ts

-					`kv:key put dKey dVal --namespace-id some-namespace-id --metadata {"mKey":"mValue"}`
+					`kv:key put dKey dVal --namespace-id some-namespace-id --metadata '{"mKey":"mValue"}'`


This wasn't a correct example of how a user would specify the metadata arg with JSON.

Before (incorrect, note the missing quotes which would fail JSON.parse)

▶ node -e 'console.log(process.argv.slice(1))' -- --metadata {"mKey":"mValue"} [ '--metadata', '{mKey:mValue}' ]

After (correct, note the valid JSON as expected):

▶ node -e 'console.log(process.argv.slice(1))' -- --metadata '{"mKey":"mValue"}' [ '--metadata', '{"mKey":"mValue"}' ]

RamIdeas · 2023-10-01T13:05:26Z

packages/wrangler/src/init.ts

-			...getC3CommandFromEnv().split(" "),
+			...shellquote.parse(getC3CommandFromEnv()),


This would have failed in scenarios where the c3 command was pointing to a local executable, for example, in a directory with spaces

RamIdeas · 2023-10-01T13:05:32Z

packages/wrangler/src/init.ts

-			c3Arguments.unshift(...getC3CommandFromEnv().split(" "));
+			c3Arguments.unshift(...shellquote.parse(getC3CommandFromEnv()));


This would have failed in scenarios where the c3 command was pointing to a local executable, for example, in a directory with spaces

RamIdeas · 2023-10-01T13:07:01Z

packages/wrangler/src/utils/shell-quote.ts

+import shellquote from "shell-quote";
+
+export const quote = shellquote.quote;
+
+export function parse(cmd: string, env?: Record<string, string>): string[] {
+	const entries = shellquote.parse(cmd, env);
+	const argv: string[] = [];
+
+	for (const entry of entries) {
+		// use string entries, as is
+		if (typeof entry === "string") {
+			argv.push(entry);
+			continue;
+		}
+
+		// ignore comments
+		if ("comment" in entry) {
+			continue;
+		}
+
+		// we don't want to resolve globs, passthrough the pattern unexpanded
+		if (entry.op === "glob") {
+			argv.push(entry.pattern);
+			continue;
+		}
+
+		// any other entry.op is a ControlOperator (e.g. && or ||) we don't want to support
+		throw new Error(
+			`Only simple commands are supported, please don't use the "${entry.op}" operator in "${cmd}".`
+		);
+	}
+
+	return argv;
+}


This is duplicated across C3 and wrangler. I will extract to a new shared package when #4038 is merged

instead of approximating behaviour with .split(" ") and .join(" ")

- return string[], not including objects - passthrough glob patterns - throw when using non-simple commands

RamIdeas requested review from a team as code owners October 1, 2023 12:47

RamIdeas force-pushed the command-parse-no-split branch from e7e5b6f to ac29253 Compare October 1, 2023 12:50

RamIdeas commented Oct 1, 2023

View reviewed changes

jculvey approved these changes Oct 2, 2023

View reviewed changes

RamIdeas added 5 commits October 2, 2023 20:24

use shell-quote package

eca7f30

instead of approximating behaviour with .split(" ") and .join(" ")

wrap shell-quote.parse to match our needs:

a99f872

- return string[], not including objects - passthrough glob patterns - throw when using non-simple commands

fix tests with incorrect CLI args formatting

d79639c

fix linting

4bafbac

fix linting in c3

ea0e5e7

RamIdeas force-pushed the command-parse-no-split branch from ef392be to ea0e5e7 Compare October 2, 2023 19:24

mrbbot approved these changes Oct 3, 2023

View reviewed changes

mrbbot merged commit b54512b into main Oct 3, 2023
22 of 24 checks passed

mrbbot deleted the command-parse-no-split branch October 3, 2023 11:40

RamIdeas mentioned this pull request Oct 18, 2023

use shell-quote.quote(args) instead of args.join(" ") #4215

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use `shell-quote` package #4080

use `shell-quote` package #4080

RamIdeas commented Oct 1, 2023 •

edited

Loading

changeset-bot bot commented Oct 1, 2023 •

edited

Loading

github-actions bot commented Oct 1, 2023 •

edited

Loading

codecov bot commented Oct 1, 2023 •

edited

Loading

RamIdeas Oct 1, 2023 •

edited

Loading

RamIdeas Oct 1, 2023 •

edited

Loading

RamIdeas Oct 1, 2023

RamIdeas Oct 1, 2023

RamIdeas Oct 1, 2023

RamIdeas Oct 1, 2023

		runWrangler("d1 execute --command 'select 1;'")
		runWrangler("d1 execute db --command 'select 1;'")

		"deploy --dry-run --outdir dist --define abc:'https://www.abc.net.au/news/'"
		`deploy --dry-run --outdir dist --define "abc:'https://www.abc.net.au/news/'"`

		`kv:key put dKey dVal --namespace-id some-namespace-id --metadata {"mKey":"mValue"}`
		`kv:key put dKey dVal --namespace-id some-namespace-id --metadata '{"mKey":"mValue"}'`

		...getC3CommandFromEnv().split(" "),
		...shellquote.parse(getC3CommandFromEnv()),

		c3Arguments.unshift(...getC3CommandFromEnv().split(" "));
		c3Arguments.unshift(...shellquote.parse(getC3CommandFromEnv()));

use shell-quote package #4080

use shell-quote package #4080

Conversation

RamIdeas commented Oct 1, 2023 • edited Loading

changeset-bot bot commented Oct 1, 2023 • edited Loading

⚠️ No Changeset found

github-actions bot commented Oct 1, 2023 • edited Loading

codecov bot commented Oct 1, 2023 • edited Loading

Codecov Report

RamIdeas Oct 1, 2023 • edited Loading

Choose a reason for hiding this comment

RamIdeas Oct 1, 2023 • edited Loading

Choose a reason for hiding this comment

RamIdeas Oct 1, 2023

Choose a reason for hiding this comment

RamIdeas Oct 1, 2023

Choose a reason for hiding this comment

RamIdeas Oct 1, 2023

Choose a reason for hiding this comment

RamIdeas Oct 1, 2023

Choose a reason for hiding this comment

use `shell-quote` package #4080

use `shell-quote` package #4080

RamIdeas commented Oct 1, 2023 •

edited

Loading

changeset-bot bot commented Oct 1, 2023 •

edited

Loading

github-actions bot commented Oct 1, 2023 •

edited

Loading

codecov bot commented Oct 1, 2023 •

edited

Loading

RamIdeas Oct 1, 2023 •

edited

Loading

RamIdeas Oct 1, 2023 •

edited

Loading