From 10287711bf632782c110504f2dba22920a17ab41 Mon Sep 17 00:00:00 2001 From: Vigneshraj Sekar Babu Date: Mon, 26 Oct 2026 11:43:52 -0700 Subject: [PATCH 1/2] fix(native-build): quote helm deploy values and escape dollar signs for Kubernetes The native Helm deploy script built its helm command by string concatenation inside double quotes, so env values with shell metacharacters were evaluated by sh. Git clone scripts interpolated branch and revision the same way. Single-quote every helm --set/--set-file value and the other user-derived tokens, log commands with printf instead of echo, and quote clone refs. Kubernetes also expands $(VAR) and collapses $$ in container args and env values, so double every user-controlled $ in build, deploy and clone job manifests. Move shell quoting helpers into a shared module. --- src/server/lib/__tests__/shellQuote.test.ts | 72 +++++++ .../nativeBuild/__tests__/buildkit.test.ts | 40 +++- src/server/lib/nativeBuild/engines.ts | 16 +- src/server/lib/nativeBuild/utils.ts | 17 +- .../lib/nativeHelm/__tests__/helm.test.ts | 24 +-- .../__tests__/shellInjection.test.ts | 199 ++++++++++++++++++ .../lib/nativeHelm/__tests__/utils.test.ts | 4 +- src/server/lib/nativeHelm/helm.ts | 5 +- src/server/lib/nativeHelm/utils.ts | 58 ++--- src/server/lib/shellQuote.ts | 35 +++ 10 files changed, 410 insertions(+), 60 deletions(-) create mode 100644 src/server/lib/__tests__/shellQuote.test.ts create mode 100644 src/server/lib/nativeHelm/__tests__/shellInjection.test.ts create mode 100644 src/server/lib/shellQuote.ts 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..2f7e54a5 --- /dev/null +++ b/src/server/lib/nativeHelm/__tests__/shellInjection.test.ts @@ -0,0 +1,199 @@ +/** + * 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 { 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)}`); + 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)'"); + }); +}); 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..1b4846aa 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; @@ -124,37 +121,37 @@ export function constructHelmCommand( postRenderer?: NativeHelmPostRendererConfig, secretSetFiles: HelmSecretSetFile[] = [] ): string { - let command = `helm ${action} ${releaseName}`; + let command = `helm ${action} ${quoteIfNeeded(releaseName)}`; if (chartType === ChartType.LOCAL) { const normalizedPath = chartPath.startsWith('./') || chartPath.startsWith('../') ? chartPath : `./${chartPath}`; - command += ` ${normalizedPath}`; + command += ` ${quoteIfNeeded(normalizedPath)}`; } else if (chartType === ChartType.PUBLIC) { const isOciChart = chartRepoUrl?.startsWith('oci://'); if (isOciChart) { - command += ` ${chartRepoUrl}`; + command += ` ${quoteIfNeeded(chartRepoUrl as string)}`; } else if (chartPath.includes('/')) { - command += ` ${chartPath}`; + command += ` ${quoteIfNeeded(chartPath)}`; } else if (chartRepoUrl) { const repoAlias = getRepoAliasFromUrl(chartRepoUrl); - command += ` ${repoAlias}/${chartPath}`; + command += ` ${quoteIfNeeded(`${repoAlias}/${chartPath}`)}`; } else { - command += ` ${chartPath}`; + command += ` ${quoteIfNeeded(chartPath)}`; } } else { const isOciChart = chartRepoUrl?.startsWith('oci://'); if (isOciChart) { - command += ` ${chartRepoUrl}`; + command += ` ${quoteIfNeeded(chartRepoUrl as string)}`; } else { - command += ` ${chartPath}`; + command += ` ${quoteIfNeeded(chartPath)}`; } } - command += ` --namespace ${namespace}`; + command += ` --namespace ${quoteIfNeeded(namespace)}`; if (chartVersion && (chartType === ChartType.PUBLIC || chartType === ChartType.ORG_CHART)) { - command += ` --version ${chartVersion}`; + command += ` --version ${quoteIfNeeded(chartVersion)}`; } command += buildPostRendererFlags(postRenderer); @@ -165,9 +162,9 @@ export function constructHelmCommand( const key = value.substring(0, equalIndex); const val = value.substring(equalIndex + 1); const escapedVal = escapeHelmValue(val); - command += ` --set "${key}=${escapedVal}"`; + command += ` --set ${shellQuote(`${key}=${escapedVal}`)}`; } else { - command += ` --set "${value}"`; + command += ` --set ${shellQuote(value)}`; } }; @@ -176,15 +173,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 +224,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 +248,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 +271,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)); +} From 049a70ea7e2cfca722114ea7acc2671ca65c2b96 Mon Sep 17 00:00:00 2001 From: Vigneshraj Sekar Babu Date: Mon, 26 Oct 2026 12:44:34 -0700 Subject: [PATCH 2/2] fix(native-helm): keep legacy quote and backslash handling for --set values Config values and the env serializer relied on sh stripping wrapping double quotes and the backslash in \\, \" , \$ and \` inside double quotes. Reproduce those effects without any expansion, escape bare commas in wrapped values for Helm, and add a differential test against the old shell evaluation. --- .../__tests__/shellInjection.test.ts | 81 ++++++++++++++++++- src/server/lib/nativeHelm/utils.ts | 25 +++++- 2 files changed, 103 insertions(+), 3 deletions(-) diff --git a/src/server/lib/nativeHelm/__tests__/shellInjection.test.ts b/src/server/lib/nativeHelm/__tests__/shellInjection.test.ts index 2f7e54a5..5500c78b 100644 --- a/src/server/lib/nativeHelm/__tests__/shellInjection.test.ts +++ b/src/server/lib/nativeHelm/__tests__/shellInjection.test.ts @@ -30,6 +30,7 @@ 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 => { @@ -116,7 +117,7 @@ describe('native helm deploy script with shell metacharacters', () => { const { result, argv } = runScript(script, { NODE_ENV: 'production' }); expect(result.status).toBe(0); - expect(argv).toContain(`deployment.env.KEY=${escapeHelmValue(value)}`); + expect(argv).toContain(`deployment.env.KEY=${escapeHelmValue(value).replace(/\\([\\"$`])/g, '$1')}`); expect(argv).toContain('plain.key=plain'); expect(fs.readdirSync(workDir)).toEqual([]); }); @@ -197,3 +198,81 @@ describe('git clone scripts with hostile refs', () => { 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/utils.ts b/src/server/lib/nativeHelm/utils.ts index 1b4846aa..2aa64834 100644 --- a/src/server/lib/nativeHelm/utils.ts +++ b/src/server/lib/nativeHelm/utils.ts @@ -106,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 ${shellQuote(`${key}=${escapedVal}`)}`; + command += ` --set ${shellQuote(`${key}=${toHelmSetValue(val)}`)}`; } else { command += ` --set ${shellQuote(value)}`; }