diff --git a/src/server/lib/__tests__/shellQuote.test.ts b/src/server/lib/__tests__/shellQuote.test.ts new file mode 100644 index 00000000..0fdfc261 --- /dev/null +++ b/src/server/lib/__tests__/shellQuote.test.ts @@ -0,0 +1,72 @@ +/** + * Copyright 2025 GoodRx, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { spawnSync } from 'child_process'; +import { escapeK8sDollar, quoteForJobScript, quoteIfNeeded, shellQuote } from '../shellQuote'; + +const HOSTILE = [ + 'a&x=', + 'a;touch PWNED;b', + 'a$(touch PWNED)b', + 'a`touch PWNED`b', + 'it\'s "both"', + 'line1\nline2;touch PWNED', + '$HOME ${PATH} $USER', + 'a|b||c&&d', + '*', + 'two spaces', + 'a\\,b\\\\c', + '', +]; + +describe('shellQuote', () => { + it.each(HOSTILE)('round-trips %j through sh', (value) => { + const result = spawnSync('/bin/sh', ['-c', `printf %s ${shellQuote(value)}`], { encoding: 'utf8' }); + expect(result.stdout).toBe(value); + }); +}); + +describe('quoteIfNeeded', () => { + it.each(['my-release', './values/prod.yaml', 'oci://registry.example.com/chart', '1.2.3', 'ns_1:a=b,c@d%e+f~'])( + 'leaves safe word %s unquoted', + (value) => { + expect(quoteIfNeeded(value)).toBe(value); + } + ); + + it.each(HOSTILE)('quotes %j when it is not a safe word', (value) => { + const result = spawnSync('/bin/sh', ['-c', `printf %s ${quoteIfNeeded(value)}`], { encoding: 'utf8' }); + expect(result.stdout).toBe(value); + }); + + it('quotes the empty string so it stays an argument', () => { + expect(quoteIfNeeded('')).toBe("''"); + }); +}); + +describe('escapeK8sDollar', () => { + it('doubles every dollar sign', () => { + expect(escapeK8sDollar('a$b')).toBe('a$$b'); + expect(escapeK8sDollar('$$$')).toBe('$$$$$$'); + expect(escapeK8sDollar('$(NODE_ENV) ${X}')).toBe('$$(NODE_ENV) $${X}'); + expect(escapeK8sDollar('no dollars')).toBe('no dollars'); + }); + + it('composes with shell quoting for job scripts', () => { + expect(quoteForJobScript('a$$b')).toBe("'a$$$$b'"); + expect(quoteForJobScript('plain')).toBe('plain'); + }); +}); diff --git a/src/server/lib/nativeBuild/__tests__/buildkit.test.ts b/src/server/lib/nativeBuild/__tests__/buildkit.test.ts index d7d27b44..51447d6b 100644 --- a/src/server/lib/nativeBuild/__tests__/buildkit.test.ts +++ b/src/server/lib/nativeBuild/__tests__/buildkit.test.ts @@ -1414,11 +1414,32 @@ const HOSTILE_VALUES: Record = { 'single quote': "it's", 'double quote': 'say "hi"', newline: 'line1\nline2; touch INJECTED', + 'dollar pairs and var refs': 'a$$b $(NODE_ENV) $$(X) $$$ end', 'dollar and backslash': '$HOME\\n\\$', pipe: 'a|b||c', empty: '', }; +const simulateK8sExpand = (input: string, env: Record): string => { + let out = ''; + for (let i = 0; i < input.length; ) { + if (input.startsWith('$$', i)) { + out += '$'; + i += 2; + continue; + } + const ref = /^\$\(([A-Za-z_][A-Za-z0-9_.-]*)\)/.exec(input.slice(i)); + if (ref && ref[1] in env) { + out += env[ref[1]]; + i += ref[0].length; + continue; + } + out += input[i]; + i += 1; + } + return out; +}; + describe('shellQuote', () => { it.each(Object.entries(HOSTILE_VALUES))('round-trips %s through sh unchanged', (_name, value) => { const result = spawnSync('/bin/sh', ['-c', `printf %s ${shellQuote(value)}; ls INJECTED 2>/dev/null`], { @@ -1429,6 +1450,8 @@ describe('shellQuote', () => { }); }); +const escapeForExpectation = (value: string): string => value.replace(/\$/g, '$$$$'); + describe('buildkit build args with shell metacharacters', () => { const mockDeploy = { deployable: { name: 'my-app' }, @@ -1453,7 +1476,16 @@ describe('buildkit build args with shell metacharacters', () => { const applyCall = (shellPromise as jest.Mock).mock.calls.find((call) => call[0].includes('kubectl apply')); const manifest = String(applyCall[0]).split("cat <<'EOF' | kubectl apply -f -\n")[1].split('\nEOF')[0]; const job: any = yaml.load(manifest); - return job.spec.template.spec.containers[0].args[0]; + const container = job.spec.template.spec.containers[0]; + const env = Object.fromEntries(container.env.map((e: any) => [e.name, e.value])); + return simulateK8sExpand(container.args[0], env); + }; + + const getContainerEnv = (): Record => { + const applyCall = (shellPromise as jest.Mock).mock.calls.find((call) => call[0].includes('kubectl apply')); + const manifest = String(applyCall[0]).split("cat <<'EOF' | kubectl apply -f -\n")[1].split('\nEOF')[0]; + const job: any = yaml.load(manifest); + return Object.fromEntries(job.spec.template.spec.containers[0].env.map((e: any) => [e.name, e.value])); }; it.each(Object.entries(HOSTILE_VALUES))('passes %s to buildctl as one literal argument', async (_name, value) => { @@ -1462,7 +1494,7 @@ describe('buildkit build args with shell metacharacters', () => { { ecrRepo: 'test-repo', ecrDomain: '123456789.dkr.ecr.us-east-1.amazonaws.com', - envVars: { PR_VALUE: value }, + envVars: { PR_VALUE: value, NODE_ENV: 'production' }, secretEnvKeys: ['PR_VALUE'], dockerfilePath: 'Dockerfile', tag: 'v1.0.0', @@ -1489,7 +1521,9 @@ describe('buildkit build args with shell metacharacters', () => { const argv = result.stdout.split('\0').slice(0, -1); expect(result.status).toBe(0); expect(argv).toContain(`build-arg:PR_VALUE=${value}`); + expect(argv.filter((a) => a === `build-arg:PR_VALUE=${value}`)).toHaveLength(value ? 2 : 1); expect(argv.filter((a) => a.startsWith('build-arg:PR_VALUE='))).toHaveLength(value ? 2 : 1); + expect(getContainerEnv().PR_VALUE).toBe(escapeForExpectation(value)); expect(argv.includes('id=PR_VALUE,env=PR_VALUE')).toBe(value !== ''); expect(fs.readdirSync(dir)).toEqual([]); } finally { @@ -1517,6 +1551,6 @@ describe('buildkit build args with shell metacharacters', () => { 'buildkit' ); const applyCall = (shellPromise as jest.Mock).mock.calls.find((call) => call[0].includes('kubectl apply')); - expect(applyCall[0]).toContain("REGISTRY_DOMAIN='reg.example.com$(touch INJECTED)'"); + expect(applyCall[0]).toContain("REGISTRY_DOMAIN='reg.example.com$$(touch INJECTED)'"); }); }); diff --git a/src/server/lib/nativeBuild/engines.ts b/src/server/lib/nativeBuild/engines.ts index d162a535..73029c22 100644 --- a/src/server/lib/nativeBuild/engines.ts +++ b/src/server/lib/nativeBuild/engines.ts @@ -15,6 +15,7 @@ */ import { Deploy } from '../../models'; +import { shellQuote, escapeK8sDollar } from '../shellQuote'; import { shellPromise } from '../shell'; import { getLogger } from '../logger'; import GlobalConfigService from '../../services/globalConfig'; @@ -99,9 +100,7 @@ interface BuildArgOptions { // Each secret goes to buildctl twice: as a build arg, for Dockerfiles that read it with ARG, and as a // BuildKit secret, for Dockerfiles that read it with `RUN --mount=type=secret,id=`. The secret // form never reaches the image history; a build arg only does if the Dockerfile declares its ARG. -export function shellQuote(value: string): string { - return `'${value.replace(/'/g, `'\\''`)}'`; -} +export { shellQuote }; export function generateSecretArgsScript(secretEnvKeys?: string[]): string { if (!secretEnvKeys || secretEnvKeys.length === 0) { @@ -163,7 +162,7 @@ const ENGINES: Record = { set -e # Detect registry type and perform appropriate login -REGISTRY_DOMAIN=${shellQuote(ecrDomain)} +REGISTRY_DOMAIN=${escapeK8sDollar(shellQuote(ecrDomain))} # AWS ECR Detection (format: .dkr.ecr..amazonaws.com) if echo "\${REGISTRY_DOMAIN}" | grep -qE "^[0-9]+\\.dkr\\.ecr\\.([a-z0-9-]+)\\.amazonaws\\.com$"; then @@ -207,7 +206,7 @@ set -- ${generateSecretArgsScript(secretEnvKeys)} echo "Running buildctl..." -buildctl ${buildctlArgs.map(shellQuote).join(' \\\n ')} "$@" +buildctl ${buildctlArgs.map((arg) => escapeK8sDollar(shellQuote(arg))).join(' \\\n ')} "$@" `; return [script.trim()]; @@ -239,7 +238,7 @@ buildctl ${buildctlArgs.map(shellQuote).join(' \\\n ')} "$@" args.push(`--build-arg=${key}=${value}`); }); - return args; + return args.map(escapeK8sDollar); }, getCacheRef: (cacheRegistry, ecrRepo) => `${cacheRegistry}/${ecrRepo}/cache`, }, @@ -311,7 +310,10 @@ function createBuildContainer( image: engine.image, command: engine.command, args, - env: Object.entries(containerEnvVars).map(([envName, value]) => ({ name: envName, value: String(value) })), + env: Object.entries(containerEnvVars).map(([envName, value]) => ({ + name: envName, + value: escapeK8sDollar(String(value)), + })), volumeMounts, resources, }; diff --git a/src/server/lib/nativeBuild/utils.ts b/src/server/lib/nativeBuild/utils.ts index 94edcb5a..69345e50 100644 --- a/src/server/lib/nativeBuild/utils.ts +++ b/src/server/lib/nativeBuild/utils.ts @@ -16,6 +16,7 @@ import { V1Job } from '@kubernetes/client-node'; import GlobalConfigService from '../../services/globalConfig'; +import { quoteForJobScript } from '../shellQuote'; import { createBuildJob } from '../kubernetes/jobFactory'; import { JobMonitor } from '../kubernetes/JobMonitor'; @@ -163,9 +164,13 @@ export const GIT_USERNAME = 'x-access-token'; export const MANIFEST_PATH = '/tmp/manifests'; export function createCloneScript(repo: string, branch: string, sha?: string): string { - const cloneCmd = `git clone --depth 1 --single-branch --progress -b ${branch} https://\${GIT_USERNAME}:\${GIT_PASSWORD}@github.com/${repo}.git /workspace`; + const cloneCmd = `git clone --depth 1 --single-branch --progress -b ${quoteForJobScript( + branch + )} https://\${GIT_USERNAME}:\${GIT_PASSWORD}@github.com/${repo}.git /workspace`; const checkoutCmd = sha - ? ` && cd /workspace && git fetch --depth 1 --progress origin ${sha} && git checkout ${sha}` + ? ` && cd /workspace && git fetch --depth 1 --progress origin ${quoteForJobScript( + sha + )} && git checkout ${quoteForJobScript(sha)}` : ''; return `${cloneCmd}${checkoutCmd}`; } @@ -179,7 +184,7 @@ export function createGitCloneContainer(repo: string, revision: string, gitUsern `git config --global --add safe.directory /workspace && \ git init /workspace && cd /workspace && \ git remote add origin https://\${GIT_USERNAME}:\${GIT_PASSWORD}@github.com/${repo}.git && \ - git fetch --depth 1 --progress origin ${revision} && \ + git fetch --depth 1 --progress origin ${quoteForJobScript(revision)} && \ git checkout FETCH_HEAD`, ], env: [ @@ -213,10 +218,10 @@ export function createRepoSpecificGitCloneContainer( image: 'alpine/git:latest', command: ['sh', '-c'], args: [ - `git config --global --add safe.directory ${targetDir} && \ - git init ${targetDir} && cd ${targetDir} && \ + `git config --global --add safe.directory ${quoteForJobScript(targetDir)} && \ + git init ${quoteForJobScript(targetDir)} && cd ${quoteForJobScript(targetDir)} && \ git remote add origin https://\${GIT_USERNAME}:\${GIT_PASSWORD}@github.com/${repo}.git && \ - git fetch --depth 1 --progress origin ${revision} && \ + git fetch --depth 1 --progress origin ${quoteForJobScript(revision)} && \ git checkout FETCH_HEAD`, ], env: [ diff --git a/src/server/lib/nativeHelm/__tests__/helm.test.ts b/src/server/lib/nativeHelm/__tests__/helm.test.ts index 8973e93a..329dc569 100644 --- a/src/server/lib/nativeHelm/__tests__/helm.test.ts +++ b/src/server/lib/nativeHelm/__tests__/helm.test.ts @@ -229,7 +229,7 @@ describe('Native Helm', () => { expect(result).toContain('helm upgrade --install my-release my-chart'); expect(result).toContain('--namespace my-namespace'); - expect(result).toContain('--set "key=value"'); + expect(result).toContain("--set 'key=value'"); expect(result).toContain('-f values.yaml'); // Should not have any default args when none provided expect(result).not.toContain('--wait'); @@ -304,8 +304,8 @@ describe('Native Helm', () => { // no defaultArgs ); - expect(result).toContain('--set "key1=value1"'); - expect(result).toContain('--set "key2=value2"'); + expect(result).toContain("--set 'key1=value1'"); + expect(result).toContain("--set 'key2=value2'"); expect(result).toContain('-f values1.yaml'); expect(result).toContain('-f values2.yaml'); }); @@ -335,9 +335,9 @@ describe('Native Helm', () => { ] ); - expect(result).toContain('--set "auth.database=app_db"'); + expect(result).toContain("--set 'auth.database=app_db'"); expect(result).toContain( - '--set-file "auth.password=/var/run/lifecycle/helm-secrets/example-db-aws-secrets/helm.auth.password.abc123"' + "--set-file 'auth.password=/var/run/lifecycle/helm-secrets/example-db-aws-secrets/helm.auth.password.abc123'" ); expect(result).not.toContain('{{aws:'); }); @@ -358,7 +358,7 @@ describe('Native Helm', () => { expect(result).toContain('helm upgrade --install my-release my-chart'); expect(result).toContain('--namespace my-namespace'); - expect(result).toContain('--set "key=value"'); + expect(result).toContain("--set 'key=value'"); expect(result).toContain('-f values.yaml'); expect(result).toContain('--force --timeout 60m0s --wait'); expect(result).not.toContain('--wait --timeout 30m'); @@ -496,7 +496,7 @@ describe('Native Helm', () => { 'helm upgrade --install my-release oci://ghcr.io/prometheus-community/helm-charts/prometheus' ); expect(result).toContain('--namespace my-namespace'); - expect(result).toContain('--set "key=value"'); + expect(result).toContain("--set 'key=value'"); expect(result).toContain('-f values.yaml'); }); @@ -518,8 +518,8 @@ describe('Native Helm', () => { expect(result).toContain('helm upgrade --install my-release oci://ghcr.io/myorg/charts/postgresql'); expect(result).toContain('--namespace my-namespace'); expect(result).toContain('--version 12.9.0'); - expect(result).toContain('--set "auth.username=admin"'); - expect(result).toContain('--set "auth.password=secret"'); + expect(result).toContain("--set 'auth.username=admin'"); + expect(result).toContain("--set 'auth.password=secret'"); expect(result).toContain('--wait'); }); @@ -541,7 +541,7 @@ describe('Native Helm', () => { expect(result).toContain('helm upgrade --install my-release prometheus-community/prometheus'); expect(result).toContain('--namespace my-namespace'); expect(result).toContain('--version 25.11.0'); - expect(result).toContain('--set "server.replicaCount=2"'); + expect(result).toContain("--set 'server.replicaCount=2'"); }); it('should not add chart version for LOCAL charts even when version is specified', () => { @@ -600,7 +600,7 @@ describe('Native Helm', () => { expect(result).toContain('helm upgrade --install my-release helm-app'); expect(result).toContain('--namespace my-namespace'); expect(result).toContain('--version 2.0.9'); - expect(result).toContain('--set "deployment.appImage=myapp:latest"'); + expect(result).toContain("--set 'deployment.appImage=myapp:latest'"); }); it('should not add chart version for ORG_CHART when version is not specified', () => { @@ -725,7 +725,7 @@ describe('Native Helm', () => { }, ]) ); - expect(result.args[0]).toContain('--set-file "auth.password='); + expect(result.args[0]).toContain("--set-file 'auth.password="); }); }); diff --git a/src/server/lib/nativeHelm/__tests__/shellInjection.test.ts b/src/server/lib/nativeHelm/__tests__/shellInjection.test.ts new file mode 100644 index 00000000..5500c78b --- /dev/null +++ b/src/server/lib/nativeHelm/__tests__/shellInjection.test.ts @@ -0,0 +1,278 @@ +/** + * Copyright 2025 GoodRx, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +jest.mock('@kubernetes/client-node', () => ({ + KubeConfig: jest.fn().mockImplementation(() => ({ loadFromDefault: jest.fn(), makeApiClient: jest.fn() })), + RbacAuthorizationV1Api: jest.fn(), +})); +jest.mock('server/lib/logger', () => ({ + getLogger: jest.fn(() => ({ debug: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn() })), +})); +jest.mock('server/services/globalConfig'); + +import { ChartType, escapeHelmValue, generateHelmInstallScript } from '../utils'; +import { createHelmContainer } from '../helm'; +import { serializeHelmEnvMap } from '../../helm/utils'; +import { createCloneScript, createGitCloneContainer } from '../../nativeBuild/utils'; + +const simulateK8sExpand = (input: string, env: Record = {}): string => { + let out = ''; + for (let i = 0; i < input.length; ) { + if (input.startsWith('$$', i)) { + out += '$'; + i += 2; + continue; + } + const ref = /^\$\(([A-Za-z_][A-Za-z0-9_.-]*)\)/.exec(input.slice(i)); + if (ref && ref[1] in env) { + out += env[ref[1]]; + i += ref[0].length; + continue; + } + out += input[i]; + i += 1; + } + return out; +}; + +const HOSTILE_VALUES: Record = { + ampersand: 'pr-12&x=', + redirects: 'a>b &1', + semicolon: 'a;touch PWNED;b', + 'command substitution': 'a$(touch PWNED)b', + backticks: 'a`touch PWNED`b', + quotes: "it's \"quoted\" 'both'", + newline: 'line1\nline2;touch PWNED', + 'variable refs': '$HOME ${PATH} $USER', + 'dollar pairs and env refs': 'a$$b $(NODE_ENV) $$(X) $$$ end', + 'helm separators': 'a,b/c', + spaces: 'two words', + glob: '*', + pipes: 'a|b||c&&d', + 'backslash comma': 'a\\,b', +}; + +describe('native helm deploy script with shell metacharacters', () => { + let workDir: string; + let binDir: string; + let argvFile: string; + + beforeEach(() => { + workDir = fs.mkdtempSync(path.join(os.tmpdir(), 'helm-script-')); + binDir = fs.mkdtempSync(path.join(os.tmpdir(), 'helm-bin-')); + argvFile = path.join(binDir, 'argv'); + fs.writeFileSync( + path.join(binDir, 'helm'), + `#!/bin/sh\nfor a in "$@"; do printf '%s\\0' "$a" >> "${argvFile}"; done\n`, + { mode: 0o755 } + ); + }); + + afterEach(() => { + fs.rmSync(workDir, { recursive: true, force: true }); + fs.rmSync(binDir, { recursive: true, force: true }); + }); + + const runScript = (script: string, env: Record = {}) => { + const result = spawnSync('/bin/sh', ['-c', simulateK8sExpand(script, env)], { + cwd: workDir, + encoding: 'utf8', + env: { ...process.env, ...env, PATH: `${binDir}:${process.env.PATH}` }, + }); + const argv = fs.existsSync(argvFile) ? fs.readFileSync(argvFile, 'utf8').split('\0').slice(0, -1) : []; + return { result, argv }; + }; + + it.each(Object.entries(HOSTILE_VALUES))('passes %s to helm as one literal --set argument', (_name, value) => { + const script = generateHelmInstallScript( + 'no-repo', + 'chart', + 'my-release', + 'env-test', + [`deployment.env.KEY=${value}`, 'plain.key=plain'], + ['values.yaml'], + ChartType.ORG_CHART, + undefined, + 'oci://registry.example.com/chart' + ); + + const { result, argv } = runScript(script, { NODE_ENV: 'production' }); + + expect(result.status).toBe(0); + expect(argv).toContain(`deployment.env.KEY=${escapeHelmValue(value).replace(/\\([\\"$`])/g, '$1')}`); + expect(argv).toContain('plain.key=plain'); + expect(fs.readdirSync(workDir)).toEqual([]); + }); + + it('does not evaluate hostile release names or repo URLs in the log and repo-add lines', () => { + const script = generateHelmInstallScript( + 'no-repo', + 'chart', + 'rel$(touch PWNED)ease', + 'env-test', + [], + [], + ChartType.PUBLIC, + undefined, + 'https://charts.example.com/$(touch PWNED)' + ); + + const { argv } = runScript(script); + expect(fs.readdirSync(workDir)).toEqual([]); + expect(argv).toContain('rel$(touch PWNED)ease'); + }); + + it('doubles dollar signs in job container env values', async () => { + const container = await createHelmContainer( + 'no-repo', + 'chart', + 'rel', + 'env-test', + '3.12.0', + [], + [], + ChartType.ORG_CHART, + 'svc$$name', + 'job$(X)' + ); + const env = Object.fromEntries(container.env.map((e: any) => [e.name, e.value])); + expect(simulateK8sExpand(env.LC_SERVICE_NAME)).toBe('svc$$name'); + expect(simulateK8sExpand(env.LC_JOB_NAME)).toBe('job$(X)'); + }); +}); + +describe('git clone scripts with hostile refs', () => { + const run = (script: string) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'clone-')); + const binDir = fs.mkdtempSync(path.join(os.tmpdir(), 'clone-bin-')); + const argvFile = path.join(binDir, 'argv'); + fs.writeFileSync( + path.join(binDir, 'git'), + `#!/bin/sh\nprintf '%s\\0' "$@" >> "${argvFile}"\nprintf '\\1' >> "${argvFile}"\n`, + { mode: 0o755 } + ); + try { + const result = spawnSync('/bin/sh', ['-c', `mkdir -p /tmp/_unused; ${simulateK8sExpand(script)}`], { + cwd: dir, + encoding: 'utf8', + env: { ...process.env, GIT_USERNAME: 'u', GIT_PASSWORD: 'p', PATH: `${binDir}:${process.env.PATH}` }, + }); + return { result, listing: fs.readdirSync(dir), argv: fs.readFileSync(argvFile, 'utf8') }; + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + fs.rmSync(binDir, { recursive: true, force: true }); + } + }; + + it.each(['feat/$(touch PWNED)', 'a;touch PWNED', 'a`touch PWNED`b', "it's$$x"])( + 'passes branch %s to git as a single literal argument', + (branch) => { + const script = createCloneScript('owner/repo', branch, 'abc$(touch PWNED)'); + const { result, listing, argv } = run(script.replace(' && cd /workspace', '')); + expect(result.status).toBe(0); + expect(listing).not.toContain('PWNED'); + expect(argv).toContain(`\0${branch}\0`); + } + ); + + it('escapes dollar signs in revision and target dir for the clone container', () => { + const container = createGitCloneContainer('owner/repo', 'rev$(touch PWNED)', 'u', 'p'); + expect(container.args[0]).toContain("'rev$$(touch PWNED)'"); + }); +}); + +describe('legacy --set value compatibility', () => { + // Shapes found in real configs: values sh used to rewrite inside double quotes. + const LEGACY_VALUES = [ + 'master.disableCommands[0]="FLUSHALL"', + 'auth.username=""', + 'secrets.env.WEB_SERVICE="svc.example.svc.cluster.local/pass-api"', + 'job.annotations.helm\\.sh/hook=post-install\\,post-upgrade', + 'ingress.extraAnnotations.external-dns\\.alpha\\.kubernetes\\.io/hostname=host.example.com', + 'plain.key=plain', + 'url.key=https://host.example.com/a/b?x=1', + ...serializeHelmEnvMap( + { + PLAIN: 'value', + SLASHED: 'a/b/c', + NUMBER: 3, + FLAG: true, + TOKEN: { valueFrom: { secretKeyRef: { name: 'app-secrets', key: 'TOKEN' } } }, + }, + 'deployment.env', + { quoteStringValues: true } + ), + ]; + + const oldShellArgv = (entry: string): string[] => { + const eq = entry.indexOf('='); + const key = entry.slice(0, eq); + const val = escapeHelmValue(entry.slice(eq + 1)); + const result = spawnSync('/bin/sh', ['-c', `printf '%s\\0' --set "${key}=${val}"`], { encoding: 'utf8' }); + return result.stdout.split('\0').slice(0, -1); + }; + + const newArgv = (entry: string): string[] => { + const script = generateHelmInstallScript( + 'no-repo', + 'chart', + 'rel', + 'env-test', + [entry], + [], + ChartType.ORG_CHART, + undefined, + 'oci://registry.example.com/chart' + ); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'helm-legacy-')); + const argvFile = path.join(dir, 'argv'); + fs.writeFileSync( + path.join(dir, 'helm'), + `#!/bin/sh\nfor a in "$@"; do printf '%s\\0' "$a" >> "${argvFile}"; done\n`, + { + mode: 0o755, + } + ); + try { + spawnSync('/bin/sh', ['-c', simulateK8sExpand(script)], { + cwd: dir, + encoding: 'utf8', + env: { ...process.env, PATH: `${dir}:${process.env.PATH}` }, + }); + const argv = fs.readFileSync(argvFile, 'utf8').split('\0').slice(0, -1); + const idx = argv.indexOf('--set'); + return argv.slice(idx, idx + 2); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }; + + it.each(LEGACY_VALUES)('passes %s to helm exactly as the old shell evaluation did', (entry) => { + expect(newArgv(entry)).toEqual(oldShellArgv(entry)); + }); + + it('strips the quotes the env serializer adds to secret names', () => { + expect(newArgv('deployment.env.TOKEN.valueFrom.secretKeyRef.name="app-secrets"')).toEqual([ + '--set', + 'deployment.env.TOKEN.valueFrom.secretKeyRef.name=app-secrets', + ]); + }); +}); diff --git a/src/server/lib/nativeHelm/__tests__/utils.test.ts b/src/server/lib/nativeHelm/__tests__/utils.test.ts index 4b6443c4..622306c1 100644 --- a/src/server/lib/nativeHelm/__tests__/utils.test.ts +++ b/src/server/lib/nativeHelm/__tests__/utils.test.ts @@ -114,8 +114,8 @@ describe('nativeHelm utils', () => { ); expect(command).toContain('helm template sample-release oci://registry.example.com/charts/sample'); - expect(command).toContain('--set "feature.enabled"'); - expect(command).toContain('--set "ingress.path=\\/api\\/v1\\,health"'); + expect(command).toContain("--set 'feature.enabled'"); + expect(command).toContain("--set 'ingress.path=\\/api\\/v1\\,health'"); expect(command).not.toContain('ignored-chart-name'); }); diff --git a/src/server/lib/nativeHelm/helm.ts b/src/server/lib/nativeHelm/helm.ts index 0c49723b..7f0c62be 100644 --- a/src/server/lib/nativeHelm/helm.ts +++ b/src/server/lib/nativeHelm/helm.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { escapeK8sDollar } from '../shellQuote'; import yaml from 'js-yaml'; import fs from 'fs'; import Deploy from 'server/models/Deploy'; @@ -128,8 +129,8 @@ export async function createHelmContainer( { name: 'HELM_CACHE_HOME', value: '/workspace/.helm/cache' }, { name: 'HELM_CONFIG_HOME', value: '/workspace/.helm/config' }, { name: 'HELM_EXPERIMENTAL_OCI', value: '1' }, - { name: 'LC_SERVICE_NAME', value: serviceName }, - { name: 'LC_JOB_NAME', value: jobName }, + { name: 'LC_SERVICE_NAME', value: escapeK8sDollar(serviceName) }, + { name: 'LC_JOB_NAME', value: escapeK8sDollar(jobName) }, ], command: ['/bin/sh', '-c'], args: [script], diff --git a/src/server/lib/nativeHelm/utils.ts b/src/server/lib/nativeHelm/utils.ts index 75c658a9..2aa64834 100644 --- a/src/server/lib/nativeHelm/utils.ts +++ b/src/server/lib/nativeHelm/utils.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { shellQuote, quoteIfNeeded, quoteForJobScript, escapeK8sDollar } from '../shellQuote'; import Deploy from 'server/models/Deploy'; import GlobalConfigService from 'server/services/globalConfig'; import { ChartType, REPO_MAPPINGS, STATIC_ENV_JOB_TTL_SECONDS, HELM_JOB_TIMEOUT_SECONDS } from './constants'; @@ -56,19 +57,15 @@ export interface HelmConfiguration { helmVersion: string; } -function shellEscape(value: string): string { - return `'${value.replace(/'/g, `'\\''`)}'`; -} - function buildPostRendererFlags(postRenderer?: NativeHelmPostRendererConfig): string { if (!postRenderer?.command || postRenderer.enabled === false) { return ''; } - let flags = ` --post-renderer ${shellEscape(postRenderer.command)}`; + let flags = ` --post-renderer ${shellQuote(postRenderer.command)}`; (postRenderer.args || []).forEach((arg) => { - flags += ` --post-renderer-args ${shellEscape(arg)}`; + flags += ` --post-renderer-args ${shellQuote(arg)}`; }); return flags; @@ -109,6 +106,28 @@ function mergeNativeHelmConfig( }; } +// Custom values used to be evaluated by sh inside double quotes, and configs and the env serializer rely +// on two of its effects: a value wrapped in quotes loses them (`k="x"`, `k=""`), and `\\`, `\"`, `\$` +// and `\`` lose their backslash. Reproduce just those, without any expansion. +function unescapeShellDoubleQuoted(value: string): string { + return value.replace(/\\([\\"$`])/g, '$1'); +} + +function toHelmSetValue(val: string): string { + if (val.length >= 2 && val.startsWith('"') && val.endsWith('"')) { + let unwrapped: string; + try { + const parsed = JSON.parse(val); + unwrapped = typeof parsed === 'string' ? parsed : unescapeShellDoubleQuoted(val.slice(1, -1)); + } catch { + unwrapped = unescapeShellDoubleQuoted(val.slice(1, -1)); + } + return unwrapped.replace(/(? -1) { const key = value.substring(0, equalIndex); const val = value.substring(equalIndex + 1); - const escapedVal = escapeHelmValue(val); - command += ` --set "${key}=${escapedVal}"`; + command += ` --set ${shellQuote(`${key}=${toHelmSetValue(val)}`)}`; } else { - command += ` --set "${value}"`; + command += ` --set ${shellQuote(value)}`; } }; @@ -176,15 +194,15 @@ export function constructHelmCommand( }); secretSetFiles.forEach((file) => { - command += ` --set-file "${file.helmKey}=${file.mountPath}"`; + command += ` --set-file ${shellQuote(`${file.helmKey}=${file.mountPath}`)}`; }); valuesFiles.forEach((file) => { if (chartType === ChartType.LOCAL) { const normalizedFile = file.startsWith('./') || file.startsWith('../') ? file : `./${file}`; - command += ` -f ${normalizedFile}`; + command += ` -f ${quoteIfNeeded(normalizedFile)}`; } else { - command += ` -f ${file}`; + command += ` -f ${quoteIfNeeded(file)}`; } }); const allArgs = [defaultArgs, args].filter(Boolean).join(' '); @@ -227,7 +245,12 @@ export function generateHelmInstallScript( secretSetFiles ); - let script = ['set -e', `echo "Starting helm deployment for ${releaseName}"`, ''].join('\n'); + const scriptHelmQuote = (value: string) => escapeK8sDollar(shellQuote(value)); + const scriptHelmCommand = escapeK8sDollar(helmCommand); + + let script = ['set -e', `printf '%s\\n' ${scriptHelmQuote(`Starting helm deployment for ${releaseName}`)}`, ''].join( + '\n' + ); if (repoName !== 'no-repo' && repoName.includes('/')) { script += `cd /workspace @@ -246,15 +269,15 @@ ls -la const [repoName] = chartPath.split('/'); const repoUrl = getRepoUrl(repoName); script += ` -echo "Adding helm repository ${repoName}: ${repoUrl}" -helm repo add ${repoName} ${repoUrl} +printf '%s\\n' ${scriptHelmQuote(`Adding helm repository ${repoName}: ${repoUrl}`)} +helm repo add ${quoteForJobScript(repoName)} ${quoteForJobScript(repoUrl)} helm repo update `; } else if (chartRepoUrl) { const repoAlias = getRepoAliasFromUrl(chartRepoUrl); script += ` -echo "Adding helm repository ${repoAlias}: ${chartRepoUrl}" -helm repo add ${repoAlias} ${chartRepoUrl} +printf '%s\\n' ${scriptHelmQuote(`Adding helm repository ${repoAlias}: ${chartRepoUrl}`)} +helm repo add ${quoteForJobScript(repoAlias)} ${quoteForJobScript(chartRepoUrl)} helm repo update `; } @@ -269,8 +292,8 @@ ${generateRegistryLoginScript(registryAuth)} } script += ` -echo "Executing: ${helmCommand}" -${helmCommand} +printf '%s\\n' ${scriptHelmQuote(`Executing: ${helmCommand}`)} +${scriptHelmCommand} echo "Helm deployment completed successfully" `; diff --git a/src/server/lib/shellQuote.ts b/src/server/lib/shellQuote.ts new file mode 100644 index 00000000..eb6864e4 --- /dev/null +++ b/src/server/lib/shellQuote.ts @@ -0,0 +1,35 @@ +/** + * Copyright 2025 GoodRx, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const SAFE_SHELL_WORD = /^[A-Za-z0-9_@%+=:,./~-]+$/; + +export function shellQuote(value: string): string { + return `'${value.replace(/'/g, `'\\''`)}'`; +} + +export function quoteIfNeeded(value: string): string { + return SAFE_SHELL_WORD.test(value) ? value : shellQuote(value); +} + +// Kubernetes expands $(VAR) and turns $$ into $ in container args and env values, so every +// user-controlled `$` must be doubled for the container to receive the original text. +export function escapeK8sDollar(value: string): string { + return value.replace(/\$/g, '$$$$'); +} + +export function quoteForJobScript(value: string): string { + return escapeK8sDollar(quoteIfNeeded(value)); +}