Skip to content

Commit

Permalink
Fix some "shell" quoting bugs (#3759)
Browse files Browse the repository at this point in the history
  • Loading branch information
bwateratmsft authored Dec 9, 2022
1 parent 0cb8564 commit 15ce82e
Show file tree
Hide file tree
Showing 7 changed files with 49 additions and 47 deletions.
6 changes: 3 additions & 3 deletions src/debugging/netcore/NetCoreDebugHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import * as fse from 'fs-extra';
import * as path from 'path';
import { CommandLineArgs, composeArgs, ContainerOS, VoidCommandResponse, withArg, withQuotedArg } from '../../runtimes/docker';
import { CommandLineArgs, composeArgs, ContainerOS, innerQuoted, VoidCommandResponse, withArg } from '../../runtimes/docker';
import { DialogResponses, IActionContext, UserCancelledError } from '@microsoft/vscode-azext-utils';
import { DebugConfiguration, MessageItem, ProgressLocation, window } from 'vscode';
import { ext } from '../../extensionVariables';
Expand Down Expand Up @@ -312,13 +312,13 @@ export class NetCoreDebugHelper implements DebugHelper {
containerCommand = 'cmd';
containerCommandArgs = composeArgs(
withArg('/C'),
withQuotedArg(`IF EXIST "${debuggerPath}" (exit 0) else (exit 1)`)
withArg(innerQuoted(`IF EXIST "${debuggerPath}" (exit 0) else (exit 1)`))
)();
} else {
containerCommand = '/bin/sh';
containerCommandArgs = composeArgs(
withArg('-c'),
withQuotedArg(`if [ -f ${debuggerPath} ]; then exit 0; else exit 1; fi;`)
withArg(innerQuoted(`if [ -f ${debuggerPath} ]; then exit 0; else exit 1; fi;`))
)();
}

Expand Down
48 changes: 19 additions & 29 deletions src/runtimes/docker/clients/DockerClientBase/DockerClientBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import * as path from 'path';
import * as readline from 'readline';
import { ShellQuotedString, ShellQuoting } from 'vscode';
import type { ShellQuotedString } from 'vscode';
import { GeneratorCommandResponse, PromiseCommandResponse, VoidCommandResponse } from '../../contracts/CommandRunner';
import {
BuildImageCommandOptions,
Expand Down Expand Up @@ -76,6 +76,7 @@ import { CancellationError } from '../../utils/CancellationError';
import {
CommandLineArgs,
composeArgs,
innerQuoted,
withArg,
withFlagArg,
withNamedArg,
Expand Down Expand Up @@ -105,6 +106,7 @@ import { withContainerPathArg } from './withContainerPathArg';
import { withDockerAddHostArg } from './withDockerAddHostArg';
import { withDockerBuildArg } from './withDockerBuildArg';
import { withDockerEnvArg } from './withDockerEnvArg';
import { withDockerBooleanFilterArg, withDockerFilterArg } from './withDockerFilterArg';
import { withDockerJsonFormatArg } from "./withDockerJsonFormatArg";
import { withDockerLabelFilterArgs } from "./withDockerLabelFilterArgs";
import { withDockerLabelsArg } from "./withDockerLabelsArg";
Expand Down Expand Up @@ -232,8 +234,8 @@ export abstract class DockerClientBase extends ConfigurableClient implements ICo
withNamedArg('--since', options.since?.toString(), { shouldQuote: !(typeof options.since === 'number') }), // If it's numeric it should not be quoted
withNamedArg('--until', options.until?.toString(), { shouldQuote: !(typeof options.until === 'number') }), // If it's numeric it should not be quoted
withDockerLabelFilterArgs(options.labels),
withNamedArg('--filter', options.types?.map((type) => `type=${type}`)),
withNamedArg('--filter', options.events?.map((event) => `event=${event}`)),
withDockerFilterArg(options.types?.map((type) => `type=${type}`)),
withDockerFilterArg(options.events?.map((event) => `event=${event}`)),
withDockerJsonFormatArg,
)();
}
Expand Down Expand Up @@ -377,12 +379,8 @@ export abstract class DockerClientBase extends ConfigurableClient implements ICo
return composeArgs(
withArg('image', 'ls'),
withFlagArg('--all', options.all),
withNamedArg(
'--filter',
typeof options.dangling === 'boolean'
? `dangling=${options.dangling}`
: undefined),
withNamedArg('--filter', options.references?.map((reference) => `reference=${reference}`)),
withDockerBooleanFilterArg('dangling', options.dangling),
withDockerFilterArg(options.references?.map((reference) => `reference=${reference}`)),
withDockerLabelFilterArgs(options.labels),
withDockerNoTruncArg,
withDockerJsonFormatArg,
Expand Down Expand Up @@ -748,12 +746,12 @@ export abstract class DockerClientBase extends ConfigurableClient implements ICo
withArg('container', 'ls'),
withFlagArg('--all', options.all),
withDockerLabelFilterArgs(options.labels),
withNamedArg('--filter', options.running ? 'status=running' : undefined),
withNamedArg('--filter', options.exited ? 'status=exited' : undefined),
withNamedArg('--filter', options.names?.map((name) => `name=${name}`)),
withNamedArg('--filter', options.imageAncestors?.map((id) => `ancestor=${id}`)),
withNamedArg('--filter', options.volumes?.map((volume) => `volume=${volume}`)),
withNamedArg('--filter', options.networks?.map((network) => `network=${network}`)),
withDockerFilterArg(options.running ? 'status=running' : undefined),
withDockerFilterArg(options.exited ? 'status=exited' : undefined),
withDockerFilterArg(options.names?.map((name) => `name=${name}`)),
withDockerFilterArg(options.imageAncestors?.map((id) => `ancestor=${id}`)),
withDockerFilterArg(options.volumes?.map((volume) => `volume=${volume}`)),
withDockerFilterArg(options.networks?.map((network) => `network=${network}`)),
withDockerNoTruncArg,
withDockerJsonFormatArg,
)();
Expand Down Expand Up @@ -1113,16 +1111,8 @@ export abstract class DockerClientBase extends ConfigurableClient implements ICo
protected getListVolumesCommandArgs(options: ListVolumesCommandOptions): CommandLineArgs {
return composeArgs(
withArg('volume', 'ls'),
withNamedArg(
'--filter',
typeof options.dangling === 'boolean'
? `dangling=${options.dangling}`
: undefined),
withNamedArg(
'--filter',
options.driver
? `driver=${options.driver}`
: undefined),
withDockerBooleanFilterArg('dangling', options.dangling),
withDockerFilterArg(options.driver ? `driver=${options.driver}` : undefined),
withDockerLabelFilterArgs(options.labels),
withDockerJsonFormatArg,
)();
Expand Down Expand Up @@ -1353,7 +1343,7 @@ export abstract class DockerClientBase extends ConfigurableClient implements ICo
options: ListNetworksCommandOptions,
): CommandLineArgs {
return composeArgs(
withNamedArg('network', 'ls'),
withArg('network', 'ls'),
withDockerLabelFilterArgs(options.labels),
withDockerNoTruncArg,
withDockerJsonFormatArg,
Expand Down Expand Up @@ -1569,13 +1559,13 @@ export abstract class DockerClientBase extends ConfigurableClient implements ICo
'/C',
// Path is intentionally *not* quoted--no good quoting options work, but
// `cd` doesn't seem to care, so cd to the path and then do dir
{ value: `cd ${options.path} & dir /A-S /-C`, quoting: ShellQuoting.Strong }
innerQuoted(`cd ${options.path} & dir /A-S /-C`) as ShellQuotedString,
];
} else {
command = [
'/bin/sh',
'-c',
{ value: `ls -lA "${options.path}"`, quoting: ShellQuoting.Strong }
innerQuoted(`ls -lA '${options.path}'`) as ShellQuotedString,
];
}

Expand Down Expand Up @@ -1621,7 +1611,7 @@ export abstract class DockerClientBase extends ConfigurableClient implements ICo
const command = [
'cmd',
'/C',
{ value: `cd ${folder} & type ${file}`, quoting: ShellQuoting.Strong }
innerQuoted(`cd ${folder} & type ${file}`) as ShellQuotedString,
];

return this.getExecContainerCommandArgs(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See LICENSE in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { ShellQuotedString } from 'vscode';
import { CommandLineCurryFn, innerQuoted, withNamedArg } from '../../utils/commandLineBuilder';

// The Docker CLI requires weak quoting of the --filter argument
export function withDockerFilterArg(filter: string | ShellQuotedString | (string | ShellQuotedString | null | undefined)[] | null | undefined): CommandLineCurryFn {
return withNamedArg('--filter', Array.isArray(filter) ? filter.map(innerQuoted) : innerQuoted(filter));
}

export function withDockerBooleanFilterArg(filter: string, value: boolean | null | undefined): CommandLineCurryFn {
return withDockerFilterArg(typeof value === 'boolean' ? `${filter}=${value}` : undefined);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,7 @@
* Licensed under the MIT License. See LICENSE in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { composeArgs, withArg, withVerbatimArg } from '../../utils/commandLineBuilder';
import { innerQuoted, withNamedArg } from '../../utils/commandLineBuilder';

// Because Docker's wrapper script would split up `{{json .}}` into two arguments, we need to
// pre-quote it to prevent that, for cases where we're executing without a shell.
// Making it a verbatim argument also prevents it from being requoted later.
export const withDockerJsonFormatArg = composeArgs(
withArg('--format'),
withVerbatimArg('"{{json .}}"'),
);
// The Docker CLI requires weak quoting of the --format argument
export const withDockerJsonFormatArg = withNamedArg('--format', innerQuoted('{{json .}}'));
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
*--------------------------------------------------------------------------------------------*/

import { LabelFilters } from "../../contracts/ContainerClient";
import { withNamedArg } from "../../utils/commandLineBuilder";
import { conditional } from "../../utils/conditional";
import { withDockerFilterArg } from "./withDockerFilterArg";

export function formatDockerLabelFilter(name: string, value: boolean | string): string | undefined {
if (typeof value === 'boolean' && value) {
Expand All @@ -18,8 +18,7 @@ export function formatDockerLabelFilter(name: string, value: boolean | string):
}

export function withDockerLabelFilterArgs(labels?: LabelFilters) {
return withNamedArg(
'--filter',
return withDockerFilterArg(
Object.entries(labels || {}).map(([label, value]) => formatDockerLabelFilter(label, value)),
);
}
6 changes: 4 additions & 2 deletions src/runtimes/docker/utils/spawnStreamAsync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,10 @@ export async function spawnStreamAsync(
// *nix
const shell = options.shellProvider?.getShellOrDefault(options.shell) ?? options.shell;

// If there is a shell provider, apply its quoting, otherwise just flatten arguments into strings
const normalizedArgs: string[] = options.shellProvider?.quote(args) ?? args.map(arg => typeof arg === 'string' ? arg : arg.value);
// Apply quoting using the given shell provider or default
// Because Docker is reparsing arguments containing spaces, the arguments *must* be quoted,
// even though they are being supplied as whole strings without shell execution
const normalizedArgs: string[] = Shell.getShellOrDefault(options.shellProvider).quote(args);

if (cancellationToken.isCancellationRequested) {
throw new CancellationError('Command cancelled', cancellationToken);
Expand Down
4 changes: 2 additions & 2 deletions src/tree/containers/ContainerTreeItem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ const containerTooltipTemplate = `
---
#### Image
{{ imageName }} ({{ substr imageId 7 12 }})
{{ image.originalName }} ({{ substr imageId 7 12 }})
---
Expand All @@ -190,7 +190,7 @@ _None_
{{#if (eq this.type 'bind')}}
- {{ friendlyBindHost this.source }} ➔ {{ this.destination }} (Bind mount, {{#if this.readOnly}}RO{{else}}RW{{/if}})
{{/if}}
{{#if (eq this.Type 'volume')}}
{{#if (eq this.type 'volume')}}
- {{ this.name }} ➔ {{ this.destination }} (Named volume, {{#if this.readOnly}}RO{{else}}RW{{/if}})
{{/if}}
{{/each}}
Expand Down

0 comments on commit 15ce82e

Please sign in to comment.