From a5b21d7c2328cc8b464bfc39906b327f2a511b9a Mon Sep 17 00:00:00 2001 From: "Brandon Waterloo [MSFT]" <36966225+bwateratmsft@users.noreply.github.com> Date: Tue, 29 Nov 2022 15:54:41 -0500 Subject: [PATCH] Don't escape `customOptions` or single-string `command` options (#3729) --- src/debugging/netcore/NetCoreDebugHelper.ts | 6 +-- .../DockerClientBase/DockerClientBase.ts | 9 ++-- .../DockerComposeClient.ts | 9 ++-- src/runtimes/docker/test/DockerClient.test.ts | 48 +++++++++++++++++++ .../docker/test/DockerComposeClient.test.ts | 22 +++++++++ .../docker/utils/commandLineBuilder.ts | 33 ++++++++++--- src/runtimes/docker/utils/spawnStreamAsync.ts | 14 ++++++ .../runners/TaskCommandRunnerFactory.ts | 4 +- 8 files changed, 125 insertions(+), 20 deletions(-) diff --git a/src/debugging/netcore/NetCoreDebugHelper.ts b/src/debugging/netcore/NetCoreDebugHelper.ts index 397c5c307a..4b8cb522a2 100644 --- a/src/debugging/netcore/NetCoreDebugHelper.ts +++ b/src/debugging/netcore/NetCoreDebugHelper.ts @@ -5,9 +5,9 @@ import * as fse from 'fs-extra'; import * as path from 'path'; -import { composeArgs, ContainerOS, VoidCommandResponse, withArg, withQuotedArg } from '../../runtimes/docker'; +import { CommandLineArgs, composeArgs, ContainerOS, VoidCommandResponse, withArg, withQuotedArg } from '../../runtimes/docker'; import { DialogResponses, IActionContext, UserCancelledError } from '@microsoft/vscode-azext-utils'; -import { DebugConfiguration, MessageItem, ProgressLocation, ShellQuotedString, window } from 'vscode'; +import { DebugConfiguration, MessageItem, ProgressLocation, window } from 'vscode'; import { ext } from '../../extensionVariables'; import { localize } from '../../localize'; import { NetCoreTaskHelper, NetCoreTaskOptions } from '../../tasks/netcore/NetCoreTaskHelper'; @@ -307,7 +307,7 @@ export class NetCoreDebugHelper implements DebugHelper { private async isDebuggerInstalled(containerName: string, debuggerPath: string, containerOS: ContainerOS): Promise { let containerCommand: string; - let containerCommandArgs: ShellQuotedString[]; + let containerCommandArgs: CommandLineArgs; if (containerOS === 'windows') { containerCommand = 'cmd'; containerCommandArgs = composeArgs( diff --git a/src/runtimes/docker/clients/DockerClientBase/DockerClientBase.ts b/src/runtimes/docker/clients/DockerClientBase/DockerClientBase.ts index 7e9bd59c2f..acf710f218 100644 --- a/src/runtimes/docker/clients/DockerClientBase/DockerClientBase.ts +++ b/src/runtimes/docker/clients/DockerClientBase/DockerClientBase.ts @@ -88,6 +88,7 @@ import { withFlagArg, withNamedArg, withQuotedArg, + withVerbatimArg, } from "../../utils/commandLineBuilder"; import { CommandNotSupportedError } from '../../utils/CommandNotSupportedError'; import { byteStreamToGenerator, stringStreamToGenerator } from '../../utils/streamToGenerator'; @@ -390,7 +391,7 @@ export abstract class DockerClientBase extends ConfigurableClient implements ICo withDockerLabelsArg(options.labels), withNamedArg('--iidfile', options.imageIdFile), withDockerBuildArg(options.args), - withArg(options.customOptions), + withVerbatimArg(options.customOptions), withQuotedArg(options.path), )(); } @@ -829,9 +830,9 @@ export abstract class DockerClientBase extends ConfigurableClient implements ICo withDockerEnvArg(options.environmentVariables), withNamedArg('--env-file', options.environmentFiles), withNamedArg('--entrypoint', options.entrypoint), - withArg(options.customOptions), + withVerbatimArg(options.customOptions), withArg(options.imageRef), - withArg(...(toArray(options.command || []))), + typeof options.command === 'string' ? withVerbatimArg(options.command) : withArg(...(toArray(options.command || []))), )(); } @@ -875,7 +876,7 @@ export abstract class DockerClientBase extends ConfigurableClient implements ICo withFlagArg('--tty', options.tty), withDockerEnvArg(options.environmentVariables), withArg(options.container), - withArg(...toArray(options.command)), + typeof options.command === 'string' ? withVerbatimArg(options.command) : withArg(...toArray(options.command)), )(); } diff --git a/src/runtimes/docker/clients/DockerComposeClient/DockerComposeClient.ts b/src/runtimes/docker/clients/DockerComposeClient/DockerComposeClient.ts index 5899b1ea01..fcde113cb6 100644 --- a/src/runtimes/docker/clients/DockerComposeClient/DockerComposeClient.ts +++ b/src/runtimes/docker/clients/DockerComposeClient/DockerComposeClient.ts @@ -14,7 +14,7 @@ import { RestartCommandOptions, StartCommandOptions, StopCommandOptions, - UpCommandOptions + UpCommandOptions, } from '../../contracts/ContainerOrchestratorClient'; import { CommandLineArgs, @@ -22,7 +22,8 @@ import { composeArgs, withArg, withFlagArg, - withNamedArg + withNamedArg, + withVerbatimArg, } from '../../utils/commandLineBuilder'; import { stringStreamToGenerator } from '../../utils/streamToGenerator'; import { ConfigurableClient } from '../ConfigurableClient'; @@ -96,7 +97,7 @@ export class DockerComposeClient extends ConfigurableClient implements IContaine withNamedArg('--scale', Object.entries(options.scale || {}).map(([service, scale]) => `${service}=${scale}`)), withNamedArg('--timeout', options.timeoutSeconds?.toString(10)), withFlagArg('--wait', options.wait), - withArg(options.customOptions), + withVerbatimArg(options.customOptions), withArg(...(options.services || [])), )(); } @@ -125,7 +126,7 @@ export class DockerComposeClient extends ConfigurableClient implements IContaine withNamedArg('--rmi', options.removeImages), withFlagArg('--volumes', options.removeVolumes), withNamedArg('--timeout', options.timeoutSeconds?.toString(10)), - withArg(options.customOptions), + withVerbatimArg(options.customOptions), )(); } diff --git a/src/runtimes/docker/test/DockerClient.test.ts b/src/runtimes/docker/test/DockerClient.test.ts index 9c136325fb..52d7bb9cf0 100644 --- a/src/runtimes/docker/test/DockerClient.test.ts +++ b/src/runtimes/docker/test/DockerClient.test.ts @@ -14,7 +14,9 @@ import { ShellQuoting } from 'vscode'; import { DockerClient, } from '../clients/DockerClient/DockerClient'; +import { BuildImageCommandOptions, RunContainerCommandOptions } from '../contracts/ContainerClient'; import { escaped } from '../utils/commandLineBuilder'; +import { Bash, Powershell } from '../utils/spawnStreamAsync'; dayjs.extend(customParseFormat); dayjs.extend(utc); @@ -91,3 +93,49 @@ describe('DockerClient', () => { }); }); }); + +describe('DockerClient (unit)', () => { + const client = new DockerClient(); + + it('Should produce the expected lack of quoting/escaping customOptions', async () => { + const options: BuildImageCommandOptions = { + path: '.', + customOptions: '--no-cache --progress plain' + }; + + const commandResponse = await client.buildImage(options); + const pwshQuoted = new Powershell().quote(commandResponse.args); + const bashQuoted = new Bash().quote(commandResponse.args); + + expect(pwshQuoted).to.deep.equal(['image', 'build', '--no-cache --progress plain', '\'.\'']); + expect(bashQuoted).to.deep.equal(['image', 'build', '--no-cache --progress plain', '\'.\'']); + }); + + it('Should produce the expected lack of quoting/escaping a single string command', async () => { + const options: RunContainerCommandOptions = { + imageRef: 'someimage', + command: 'sh -c "echo hello world"', + }; + + const commandResponse = await client.runContainer(options); + const pwshQuoted = new Powershell().quote(commandResponse.args); + const bashQuoted = new Bash().quote(commandResponse.args); + + expect(pwshQuoted).to.deep.equal(['container', 'run', 'someimage', 'sh -c "echo hello world"']); + expect(bashQuoted).to.deep.equal(['container', 'run', 'someimage', 'sh -c "echo hello world"']); + }); + + it('Should produce the expected quoting/escaping of an array command', async () => { + const options: RunContainerCommandOptions = { + imageRef: 'someimage', + command: ['sh', '-c', 'echo hello world'], + }; + + const commandResponse = await client.runContainer(options); + const pwshQuoted = new Powershell().quote(commandResponse.args); + const bashQuoted = new Bash().quote(commandResponse.args); + + expect(pwshQuoted).to.deep.equal(['container', 'run', 'someimage', 'sh', '-c', 'echo` hello` world']); + expect(bashQuoted).to.deep.equal(['container', 'run', 'someimage', 'sh', '-c', 'echo\\ hello\\ world']); + }); +}); diff --git a/src/runtimes/docker/test/DockerComposeClient.test.ts b/src/runtimes/docker/test/DockerComposeClient.test.ts index c2a26fc5fa..997e0f3205 100644 --- a/src/runtimes/docker/test/DockerComposeClient.test.ts +++ b/src/runtimes/docker/test/DockerComposeClient.test.ts @@ -18,6 +18,7 @@ import { UpCommandOptions } from '../contracts/ContainerOrchestratorClient'; import { AccumulatorStream } from '../utils/AccumulatorStream'; +import { Bash, Powershell } from '../utils/spawnStreamAsync'; const commonOptions: CommonOrchestratorCommandOptions = { files: ['docker-compose.yml'], @@ -102,3 +103,24 @@ xdescribe('DockerComposeClient', () => { expect(result).to.contain('registry'); }); }); + +describe('DockerComposeClient (unit)', () => { + const client = new DockerComposeClient(); + client.composeV2 = false; + + it('Should produce the expected lack of quoting/escaping customOptions', async () => { + const options: UpCommandOptions = { + ...commonOptions, + detached: true, + build: true, + customOptions: '--timeout 10 --wait' + }; + + const commandResponse = await client.up(options); + const pwshQuoted = new Powershell().quote(commandResponse.args); + const bashQuoted = new Bash().quote(commandResponse.args); + + expect(pwshQuoted).to.deep.equal(['--file', '\'docker-compose.yml\'', 'up', '--detach', '--build', '--timeout 10 --wait']); + expect(bashQuoted).to.deep.equal(['--file', '\'docker-compose.yml\'', 'up', '--detach', '--build', '--timeout 10 --wait']); + }); +}); diff --git a/src/runtimes/docker/utils/commandLineBuilder.ts b/src/runtimes/docker/utils/commandLineBuilder.ts index cf350bf4af..63bf1e1478 100644 --- a/src/runtimes/docker/utils/commandLineBuilder.ts +++ b/src/runtimes/docker/utils/commandLineBuilder.ts @@ -6,7 +6,7 @@ import { ShellQuotedString, ShellQuoting } from 'vscode'; import { toArray } from './toArray'; -export type CommandLineArgs = Array; +export type CommandLineArgs = Array; export type CommandLineCurryFn = (cmdLineArgs?: CommandLineArgs) => CommandLineArgs; @@ -95,14 +95,33 @@ export function withNamedArg( ): CommandLineCurryFn { return (cmdLineArgs: CommandLineArgs = []) => { return toArray(args) - .reduce((allArgs, arg) => { - if (arg) { - const normalizedArg = shouldQuote ? quoted(arg) : escaped(arg); - if (assignValue) { - return withArg(`${name}=${normalizedArg}`)(allArgs); + .reduce((allArgs, arg) => { + if (arg) { + const normalizedArg = shouldQuote ? quoted(arg) : escaped(arg); + if (assignValue) { + return withArg(`${name}=${normalizedArg}`)(allArgs); + } + + return withArg(name, normalizedArg)(allArgs); } - return withArg(name, normalizedArg)(allArgs); + return allArgs; + }, cmdLineArgs); + }; +} + +/** + * Functional method for adding additional verbatim arguments to an existing list of + * arguments. They will not be quoted nor escaped. + * @param args Raw arguments to add to the CommandLineArguments records + * @returns A function that takes an optional array of CommandLineArguments and appends the provided arguments + * @deprecated {@link withArg} should be used instead in almost all cases + */ +export function withVerbatimArg(...args: Array): CommandLineCurryFn { + return (cmdLineArgs: CommandLineArgs = []) => { + return args.reduce((allArgs, arg) => { + if (arg) { + return [...allArgs, arg]; } return allArgs; diff --git a/src/runtimes/docker/utils/spawnStreamAsync.ts b/src/runtimes/docker/utils/spawnStreamAsync.ts index e2fe3b168e..c1ff248ad4 100644 --- a/src/runtimes/docker/utils/spawnStreamAsync.ts +++ b/src/runtimes/docker/utils/spawnStreamAsync.ts @@ -62,6 +62,13 @@ export class Powershell extends Shell { const escape = (value: string) => `\`${value}`; return args.map((quotedArg) => { + // If it's a verbatim argument, return it as-is. + // The overwhelming majority of arguments are `ShellQuotedString`, so + // verbatim arguments will only show up if `withVerbatimArg` is used. + if (typeof quotedArg === 'string') { + return quotedArg; + } + switch (quotedArg.quoting) { case ShellQuoting.Escape: return quotedArg.value.replace(/[ "'()]/g, escape); @@ -103,6 +110,13 @@ export class Bash extends Shell { const escape = (value: string) => `\\${value}`; return args.map((quotedArg) => { + // If it's a verbatim argument, return it as-is. + // The overwhelming majority of arguments are `ShellQuotedString`, so + // verbatim arguments will only show up if `withVerbatimArg` is used. + if (typeof quotedArg === 'string') { + return quotedArg; + } + switch (quotedArg.quoting) { case ShellQuoting.Escape: return quotedArg.value.replace(/[ "']/g, escape); diff --git a/src/runtimes/runners/TaskCommandRunnerFactory.ts b/src/runtimes/runners/TaskCommandRunnerFactory.ts index 1f70be5229..98701fa241 100644 --- a/src/runtimes/runners/TaskCommandRunnerFactory.ts +++ b/src/runtimes/runners/TaskCommandRunnerFactory.ts @@ -5,7 +5,7 @@ import * as os from 'os'; import * as vscode from 'vscode'; -import { CommandNotSupportedError, CommandRunner, ICommandRunnerFactory, Like, normalizeCommandResponseLike, PromiseCommandResponse, StreamingCommandRunner, VoidCommandResponse } from '../docker'; +import { CommandLineArgs, CommandNotSupportedError, CommandRunner, ICommandRunnerFactory, Like, normalizeCommandResponseLike, PromiseCommandResponse, StreamingCommandRunner, VoidCommandResponse } from '../docker'; interface TaskCommandRunnerOptions { taskName: string; @@ -35,7 +35,7 @@ export class TaskCommandRunnerFactory implements ICommandRunnerFactory { } } -async function executeAsTask(options: TaskCommandRunnerOptions, command: string, args?: vscode.ShellQuotedString[]): Promise { +async function executeAsTask(options: TaskCommandRunnerOptions, command: string, args?: CommandLineArgs): Promise { const shellExecutionOptions = { cwd: options.cwd || options.workspaceFolder?.uri?.fsPath || os.homedir() }; const shellExecution = args ?