From c093f8777941139bbd8b75f714d574c525486cce Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Mon, 20 Nov 2023 15:09:36 +0100 Subject: [PATCH] Docker and K8s: Fix shell arguments when split by the runner (#115) * Docker: Fix shell arguments when split by the runner * Add shlex to k8s hook as well --- packages/docker/package-lock.json | 11 +++++++ packages/docker/package.json | 1 + .../docker/src/dockerCommands/container.ts | 2 +- packages/docker/src/utils.ts | 6 ++++ packages/docker/tests/run-script-step-test.ts | 21 ++++++++++-- packages/docker/tests/utils-test.ts | 33 ++++++++++++++++++- packages/k8s/package-lock.json | 13 +++++++- packages/k8s/package.json | 3 +- packages/k8s/src/hooks/prepare-job.ts | 5 +-- packages/k8s/src/hooks/run-container-step.ts | 5 +-- packages/k8s/src/k8s/index.ts | 2 +- packages/k8s/src/k8s/utils.ts | 5 +++ packages/k8s/tests/run-container-step-test.ts | 2 +- 13 files changed, 96 insertions(+), 13 deletions(-) diff --git a/packages/docker/package-lock.json b/packages/docker/package-lock.json index 837ed96..d557b23 100644 --- a/packages/docker/package-lock.json +++ b/packages/docker/package-lock.json @@ -12,6 +12,7 @@ "@actions/core": "^1.9.1", "@actions/exec": "^1.1.1", "hooklib": "file:../hooklib", + "shlex": "^2.1.2", "uuid": "^8.3.2" }, "devDependencies": { @@ -4629,6 +4630,11 @@ "node": ">=8" } }, + "node_modules/shlex": { + "version": "2.1.2", + "resolved": "https://registry.npmjs.org/shlex/-/shlex-2.1.2.tgz", + "integrity": "sha512-Nz6gtibMVgYeMEhUjp2KuwAgqaJA1K155dU/HuDaEJUGgnmYfVtVZah+uerVWdH8UGnyahhDCgABbYTbs254+w==" + }, "node_modules/signal-exit": { "version": "3.0.7", "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-3.0.7.tgz", @@ -8930,6 +8936,11 @@ "integrity": "sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A==", "dev": true }, + "shlex": { + "version": "2.1.2", + "resolved": "https://registry.npmjs.org/shlex/-/shlex-2.1.2.tgz", + "integrity": "sha512-Nz6gtibMVgYeMEhUjp2KuwAgqaJA1K155dU/HuDaEJUGgnmYfVtVZah+uerVWdH8UGnyahhDCgABbYTbs254+w==" + }, "signal-exit": { "version": "3.0.7", "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-3.0.7.tgz", diff --git a/packages/docker/package.json b/packages/docker/package.json index 087fd94..707e891 100644 --- a/packages/docker/package.json +++ b/packages/docker/package.json @@ -13,6 +13,7 @@ "@actions/core": "^1.9.1", "@actions/exec": "^1.1.1", "hooklib": "file:../hooklib", + "shlex": "^2.1.2", "uuid": "^8.3.2" }, "devDependencies": { diff --git a/packages/docker/src/dockerCommands/container.ts b/packages/docker/src/dockerCommands/container.ts index 83214fe..91637a9 100644 --- a/packages/docker/src/dockerCommands/container.ts +++ b/packages/docker/src/dockerCommands/container.ts @@ -444,7 +444,7 @@ export async function isContainerAlpine(containerId: string): Promise { containerId, 'sh', '-c', - "[ $(cat /etc/*release* | grep -i -e '^ID=*alpine*' -c) != 0 ] || exit 1" + `'[ $(cat /etc/*release* | grep -i -e "^ID=*alpine*" -c) != 0 ] || exit 1'` ] try { await runDockerCommand(dockerArgs) diff --git a/packages/docker/src/utils.ts b/packages/docker/src/utils.ts index 4f2bdf7..bdfe5c0 100644 --- a/packages/docker/src/utils.ts +++ b/packages/docker/src/utils.ts @@ -5,6 +5,7 @@ import * as core from '@actions/core' import { env } from 'process' // Import this way otherwise typescript has errors const exec = require('@actions/exec') +const shlex = require('shlex') export interface RunDockerCommandOptions { workingDir?: string @@ -17,6 +18,7 @@ export async function runDockerCommand( options?: RunDockerCommandOptions ): Promise { options = optionsWithDockerEnvs(options) + args = fixArgs(args) const pipes = await exec.getExecOutput('docker', args, options) if (pipes.exitCode !== 0) { core.error(`Docker failed with exit code ${pipes.exitCode}`) @@ -84,6 +86,10 @@ export function sanitize(val: string): string { return newNameBuilder.join('') } +export function fixArgs(args: string[]): string[] { + return shlex.split(args.join(' ')) +} + export function checkEnvironment(): void { if (!env.GITHUB_WORKSPACE) { throw new Error('GITHUB_WORKSPACE is not set') diff --git a/packages/docker/tests/run-script-step-test.ts b/packages/docker/tests/run-script-step-test.ts index bf7c137..5fdd33c 100644 --- a/packages/docker/tests/run-script-step-test.ts +++ b/packages/docker/tests/run-script-step-test.ts @@ -40,21 +40,36 @@ describe('run script step', () => { definitions.runScriptStep.args.entryPoint = '/bin/bash' definitions.runScriptStep.args.entryPointArgs = [ '-c', - `if [[ ! $(env | grep "^PATH=") = "PATH=${definitions.runScriptStep.args.prependPath}:"* ]]; then exit 1; fi` + `'if [[ ! $(env | grep "^PATH=") = "PATH=${definitions.runScriptStep.args.prependPath}:"* ]]; then exit 1; fi'` ] await expect( runScriptStep(definitions.runScriptStep.args, prepareJobResponse.state) ).resolves.not.toThrow() }) + it("Should fix expansion and print correctly in container's stdout", async () => { + const spy = jest.spyOn(process.stdout, 'write').mockImplementation() + + definitions.runScriptStep.args.entryPoint = 'echo' + definitions.runScriptStep.args.entryPointArgs = ['"Mona', 'the', `Octocat"`] + await expect( + runScriptStep(definitions.runScriptStep.args, prepareJobResponse.state) + ).resolves.not.toThrow() + expect(spy).toHaveBeenCalledWith( + expect.stringContaining('Mona the Octocat') + ) + + spy.mockRestore() + }) + it('Should have path variable changed in container with prepend path string array', async () => { definitions.runScriptStep.args.prependPath = ['/some/other/path'] definitions.runScriptStep.args.entryPoint = '/bin/bash' definitions.runScriptStep.args.entryPointArgs = [ '-c', - `if [[ ! $(env | grep "^PATH=") = "PATH=${definitions.runScriptStep.args.prependPath.join( + `'if [[ ! $(env | grep "^PATH=") = "PATH=${definitions.runScriptStep.args.prependPath.join( ':' - )}:"* ]]; then exit 1; fi` + )}:"* ]]; then exit 1; fi'` ] await expect( runScriptStep(definitions.runScriptStep.args, prepareJobResponse.state) diff --git a/packages/docker/tests/utils-test.ts b/packages/docker/tests/utils-test.ts index 6211b62..4d40132 100644 --- a/packages/docker/tests/utils-test.ts +++ b/packages/docker/tests/utils-test.ts @@ -1,4 +1,4 @@ -import { optionsWithDockerEnvs, sanitize } from '../src/utils' +import { optionsWithDockerEnvs, sanitize, fixArgs } from '../src/utils' describe('Utilities', () => { it('should return sanitized image name', () => { @@ -10,6 +10,37 @@ describe('Utilities', () => { expect(sanitize(validStr)).toBe(validStr) }) + test.each([ + [['"Hello', 'World"'], ['Hello World']], + [ + [ + 'sh', + '-c', + `'[ $(cat /etc/*release* | grep -i -e "^ID=*alpine*" -c) != 0 ] || exit 1'` + ], + [ + 'sh', + '-c', + `[ $(cat /etc/*release* | grep -i -e "^ID=*alpine*" -c) != 0 ] || exit 1` + ] + ], + [ + [ + 'sh', + '-c', + `'[ $(cat /etc/*release* | grep -i -e '\\''^ID=*alpine*'\\'' -c) != 0 ] || exit 1'` + ], + [ + 'sh', + '-c', + `[ $(cat /etc/*release* | grep -i -e '^ID=*alpine*' -c) != 0 ] || exit 1` + ] + ] + ])('should fix split arguments(%p, %p)', (args, expected) => { + const got = fixArgs(args) + expect(got).toStrictEqual(expected) + }) + describe('with docker options', () => { it('should augment options with docker environment variables', () => { process.env.DOCKER_HOST = 'unix:///run/user/1001/docker.sock' diff --git a/packages/k8s/package-lock.json b/packages/k8s/package-lock.json index 112e5fc..e152303 100644 --- a/packages/k8s/package-lock.json +++ b/packages/k8s/package-lock.json @@ -14,7 +14,8 @@ "@actions/io": "^1.1.2", "@kubernetes/client-node": "^0.18.1", "hooklib": "file:../hooklib", - "js-yaml": "^4.1.0" + "js-yaml": "^4.1.0", + "shlex": "^2.1.2" }, "devDependencies": { "@types/jest": "^27.4.1", @@ -4163,6 +4164,11 @@ "node": ">=8" } }, + "node_modules/shlex": { + "version": "2.1.2", + "resolved": "https://registry.npmjs.org/shlex/-/shlex-2.1.2.tgz", + "integrity": "sha512-Nz6gtibMVgYeMEhUjp2KuwAgqaJA1K155dU/HuDaEJUGgnmYfVtVZah+uerVWdH8UGnyahhDCgABbYTbs254+w==" + }, "node_modules/signal-exit": { "version": "3.0.7", "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-3.0.7.tgz", @@ -8117,6 +8123,11 @@ "integrity": "sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A==", "dev": true }, + "shlex": { + "version": "2.1.2", + "resolved": "https://registry.npmjs.org/shlex/-/shlex-2.1.2.tgz", + "integrity": "sha512-Nz6gtibMVgYeMEhUjp2KuwAgqaJA1K155dU/HuDaEJUGgnmYfVtVZah+uerVWdH8UGnyahhDCgABbYTbs254+w==" + }, "signal-exit": { "version": "3.0.7", "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-3.0.7.tgz", diff --git a/packages/k8s/package.json b/packages/k8s/package.json index 0e31b5d..07a73c2 100644 --- a/packages/k8s/package.json +++ b/packages/k8s/package.json @@ -18,7 +18,8 @@ "@actions/io": "^1.1.2", "@kubernetes/client-node": "^0.18.1", "hooklib": "file:../hooklib", - "js-yaml": "^4.1.0" + "js-yaml": "^4.1.0", + "shlex": "^2.1.2" }, "devDependencies": { "@types/jest": "^27.4.1", diff --git a/packages/k8s/src/hooks/prepare-job.ts b/packages/k8s/src/hooks/prepare-job.ts index 05153a4..d289983 100644 --- a/packages/k8s/src/hooks/prepare-job.ts +++ b/packages/k8s/src/hooks/prepare-job.ts @@ -23,7 +23,8 @@ import { generateContainerName, mergeContainerWithOptions, readExtensionFromFile, - PodPhase + PodPhase, + fixArgs } from '../k8s/utils' import { JOB_CONTAINER_EXTENSION_NAME, JOB_CONTAINER_NAME } from './constants' @@ -206,7 +207,7 @@ export function createContainerSpec( } if (container.entryPointArgs?.length > 0) { - podContainer.args = container.entryPointArgs + podContainer.args = fixArgs(container.entryPointArgs) } podContainer.env = [] diff --git a/packages/k8s/src/hooks/run-container-step.ts b/packages/k8s/src/hooks/run-container-step.ts index 3979b3f..8125c43 100644 --- a/packages/k8s/src/hooks/run-container-step.ts +++ b/packages/k8s/src/hooks/run-container-step.ts @@ -14,7 +14,8 @@ import { containerVolumes, PodPhase, mergeContainerWithOptions, - readExtensionFromFile + readExtensionFromFile, + fixArgs } from '../k8s/utils' import { JOB_CONTAINER_EXTENSION_NAME, JOB_CONTAINER_NAME } from './constants' @@ -89,7 +90,7 @@ function createContainerSpec( ? [container.entryPoint] : undefined podContainer.args = container.entryPointArgs?.length - ? container.entryPointArgs + ? fixArgs(container.entryPointArgs) : undefined if (secretName) { diff --git a/packages/k8s/src/k8s/index.ts b/packages/k8s/src/k8s/index.ts index 5e45954..aac8440 100644 --- a/packages/k8s/src/k8s/index.ts +++ b/packages/k8s/src/k8s/index.ts @@ -511,7 +511,7 @@ export async function isPodContainerAlpine( [ 'sh', '-c', - "[ $(cat /etc/*release* | grep -i -e '^ID=*alpine*' -c) != 0 ] || exit 1" + `'[ $(cat /etc/*release* | grep -i -e "^ID=*alpine*" -c) != 0 ] || exit 1'` ], podName, containerName diff --git a/packages/k8s/src/k8s/utils.ts b/packages/k8s/src/k8s/utils.ts index 860b412..53da888 100644 --- a/packages/k8s/src/k8s/utils.ts +++ b/packages/k8s/src/k8s/utils.ts @@ -7,6 +7,7 @@ import * as path from 'path' import { v1 as uuidv4 } from 'uuid' import { POD_VOLUME_NAME } from './index' import { JOB_CONTAINER_EXTENSION_NAME } from '../hooks/constants' +import * as shlex from 'shlex' export const DEFAULT_CONTAINER_ENTRY_POINT_ARGS = [`-f`, `/dev/null`] export const DEFAULT_CONTAINER_ENTRY_POINT = 'tail' @@ -282,3 +283,7 @@ function mergeLists(base?: T[], from?: T[]): T[] { b.push(...from) return b } + +export function fixArgs(args: string[]): string[] { + return shlex.split(args.join(' ')) +} diff --git a/packages/k8s/tests/run-container-step-test.ts b/packages/k8s/tests/run-container-step-test.ts index 91b9f7d..7689c80 100644 --- a/packages/k8s/tests/run-container-step-test.ts +++ b/packages/k8s/tests/run-container-step-test.ts @@ -72,7 +72,7 @@ describe('Run container step', () => { runContainerStepData.args.entryPoint = 'bash' runContainerStepData.args.entryPointArgs = [ '-c', - 'if [[ -z $NODE_ENV ]]; then exit 1; fi' + "'if [[ -z $NODE_ENV ]]; then exit 1; fi'" ] await expect( runContainerStep(runContainerStepData.args)