Escape values in --dry-run curl preview to prevent shell injection - #1
Open
brodkin wants to merge 1 commit into
Open
Escape values in --dry-run curl preview to prevent shell injection#1brodkin wants to merge 1 commit into
brodkin wants to merge 1 commit into
Conversation
The --dry-run preview built its curl command by concatenating the URL and JSON body inside single quotes without escaping. A value containing a single quote, such as an Egnyte path with an apostrophe, terminated the quoted string and produced a syntactically broken command. A crafted path could also inject additional shell commands into the previewed output, which a user is invited to copy and run. Add shellQuote() and use it for the URL, JSON body, and multipart file argument so interpolated values are always safely quoted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--dry-runprints acurlcommand for the user to inspect and copy. That command is built by concatenating the request URL and JSON body inside single quotes without escaping (formatDryRuninsrc/lib/output.js). A value that contains a single quote ends the quoted string. Egnyte paths can contain apostrophes, semicolons, and other shell metacharacters, and path validation insrc/lib/validation.jsdoes not reject them, so a path can both break the previewed command and inject additional shell commands into output the user is likely to paste into a terminal.Reproduction
The single quote after
xcloses the quoted argument, so; touch /tmp/pwned ;runs as a separate command if the previewed line is pasted into a shell. This is reachable from every command that previews throughformatDryRun, includingfs delete,fs action,fs move,fs copy,fs rename,fs mkdir,fs upload,fs set-metadata, andrequest. This matters most for a tool driven by AI agents over folder names an outside party can set, where the--dry-runpreview is the human review step before execution.Fix
Add
shellQuote()and use it for the URL, the JSON body, and the multipart file argument. The same input is then emitted as a single safe argument:The
Authorization: ***masking is unchanged.Note
The chunked-upload preview in
src/commands/fs.jsbuilds its owncurllines with double-quoted interpolation and has the same class of issue. I scoped this PR to the sharedformatDryRunpath. I am happy to extend it to the chunked preview here or in a follow-up, whichever you prefer.