fix(native-build): quote helm deploy values and escape dollar signs for Kubernetes

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=<y>',
+  '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');
+  });
+});

src/server/lib/nativeBuild/__tests__/buildkit.test.ts

@@ -1414,11 +1414,32 @@ const HOSTILE_VALUES: Record<string, string> = {
   '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, string>): 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<string, string> => {
+    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)'");
   });
 });

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=<KEY>`. 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<string, BuildEngine> = {
 set -e
 
 # Detect registry type and perform appropriate login
-REGISTRY_DOMAIN=${shellQuote(ecrDomain)}
+REGISTRY_DOMAIN=${escapeK8sDollar(shellQuote(ecrDomain))}
 
 # AWS ECR Detection (format: <account-id>.dkr.ecr.<region>.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,
   };

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: [

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=");
     });
   });