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.

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

fix usage of shell-quote.parse on windows #4105

Merged
merged 4 commits into from
Oct 4, 2023
Merged

Conversation

RamIdeas
Copy link
Contributor

@RamIdeas RamIdeas commented Oct 4, 2023

On Windows, paths contain \ characters as delimiters. Our command parser was interpreting these as backslash escape characters. This is essentially a double encoding bug.

This fixes the issue on Windows by replacing the backslash characters with a double-backslash which will be interpreted as an escaped backslash (i.e. a single backslash).

The double and quadruple backslashes in the cmd.replace(...) call are a double-encoding of themselves but I'm sure everyone reviewing this is aware.

Author has included the following, where applicable:

  • Tests (adding the e2e label to this PR so the Windows e2e tests pass to prove this works)
  • Changeset (no need for a changeset as the bug this introduced has blocked the release)

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.

@RamIdeas RamIdeas requested review from a team as code owners October 4, 2023 19:42
@changeset-bot
Copy link

changeset-bot bot commented Oct 4, 2023

⚠️ No Changeset found

Latest commit: 2887c7c

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

@RamIdeas RamIdeas added the e2e Run e2e tests on a PR label Oct 4, 2023
packages/create-cloudflare/src/helpers/shell-quote.ts Outdated Show resolved Hide resolved
packages/wrangler/src/utils/shell-quote.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #4105 (2887c7c) into main (866c783) will increase coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4105      +/-   ##
==========================================
+ Coverage   75.09%   75.12%   +0.02%     
==========================================
  Files         217      217              
  Lines       12075    12077       +2     
  Branches     3127     3128       +1     
==========================================
+ Hits         9068     9073       +5     
+ Misses       3007     3004       -3     
Files Coverage Δ
packages/wrangler/src/utils/shell-quote.ts 88.88% <100.00%> (+1.38%) ⬆️

... and 2 files with indirect coverage changes

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

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/6411961053/npm-package-wrangler-4105

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6411961053/npm-package-wrangler-4105 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6411961053/npm-package-cloudflare-pages-shared-4105

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.20231002.0 3.20231002.0
workerd 1.20231002.0 1.20231002.0
workerd --version 1.20231002.0 2023-10-02

|

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

@RamIdeas RamIdeas merged commit 5fc7a88 into main Oct 4, 2023
32 of 33 checks passed
@RamIdeas RamIdeas deleted the fix-shell-quote-windows branch October 4, 2023 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants