From 324a00846d72b0e17ac6b7c4b5858bd9084fe2a4 Mon Sep 17 00:00:00 2001 From: A <6723574+louisgv@users.noreply.github.com> Date: Fri, 13 Feb 2026 20:02:10 +0000 Subject: [PATCH] fix: harden upload_file path validation with strict allowlist regex across 10 clouds Replace fragile blocklist validation and printf '%q' escaping in upload_file() with strict allowlist regex [a-zA-Z0-9/_.~-]+ across all non-SSH cloud providers. For codesandbox, additionally migrate from shell command interpolation to SDK filesystem API via environment variables, eliminating the injection surface entirely. Affected clouds: codesandbox, daytona, e2b, fly, koyeb, modal, northflank, railway, render, sprite Fixes #989 Agent: security-auditor Co-Authored-By: Claude Opus 4.6 (1M context) --- .../codesandbox-provider-patterns.test.ts | 36 ++++------ .../new-cloud-provider-patterns.test.ts | 12 ++-- .../__tests__/upload-file-security.test.ts | 72 +++++++++---------- codesandbox/lib/common.sh | 28 +++++--- daytona/lib/common.sh | 6 +- e2b/lib/common.sh | 6 +- fly/lib/common.sh | 6 +- koyeb/lib/common.sh | 6 +- modal/lib/common.sh | 11 ++- northflank/lib/common.sh | 6 +- railway/lib/common.sh | 12 ++-- render/lib/common.sh | 8 +-- sprite/lib/common.sh | 18 ++--- 13 files changed, 116 insertions(+), 111 deletions(-) diff --git a/cli/src/__tests__/codesandbox-provider-patterns.test.ts b/cli/src/__tests__/codesandbox-provider-patterns.test.ts index c8512edad..3087e3bc4 100644 --- a/cli/src/__tests__/codesandbox-provider-patterns.test.ts +++ b/cli/src/__tests__/codesandbox-provider-patterns.test.ts @@ -343,18 +343,9 @@ describe("codesandbox upload_file() security", () => { expect(body).not.toBeNull(); }); - it("should validate remote_path against injection characters", () => { - // Must reject single quotes, dollar signs, backticks, or newlines - expect(body!).toContain(`"'"`); - expect(body!).toMatch(/['"]\$['"]/); - }); - - it("should reject backtick characters in remote_path", () => { - expect(body!).toContain("`"); - }); - - it("should reject newlines in remote_path", () => { - expect(body!).toMatch(/\\n/); + it("should use strict allowlist regex for remote_path validation", () => { + // Must use allowlist regex instead of blocklist + expect(body!).toMatch(/\[a-zA-Z0-9/); }); it("should base64-encode file content", () => { @@ -368,21 +359,20 @@ describe("codesandbox upload_file() security", () => { expect(body!).toContain("|| base64"); }); - it("should decode base64 on the remote side", () => { - expect(body!).toContain("base64 -d"); - }); - - it("should use printf '%s' for safe content output", () => { - expect(body!).toContain("printf '%s'"); + it("should use SDK filesystem API via env vars", () => { + // Must pass path and content via environment variables, not shell interpolation + expect(body!).toContain("_CSB_REMOTE_PATH"); + expect(body!).toContain("_CSB_CONTENT"); + expect(body!).toContain("process.env._CSB_REMOTE_PATH"); + expect(body!).toContain("process.env._CSB_CONTENT"); }); - it("should use escaped_path for safe path embedding", () => { - expect(body!).toContain("escaped_path"); - expect(body!).toContain("printf '%q'"); + it("should use _csb_sdk_eval for remote execution", () => { + expect(body!).toContain("_csb_sdk_eval"); }); - it("should use run_server for remote execution", () => { - expect(body!).toContain("run_server"); + it("should validate sandbox ID", () => { + expect(body!).toContain("validate_sandbox_id"); }); it("should return error on invalid remote path", () => { diff --git a/cli/src/__tests__/new-cloud-provider-patterns.test.ts b/cli/src/__tests__/new-cloud-provider-patterns.test.ts index eafaba2c9..570d81bc2 100644 --- a/cli/src/__tests__/new-cloud-provider-patterns.test.ts +++ b/cli/src/__tests__/new-cloud-provider-patterns.test.ts @@ -518,8 +518,7 @@ describe("CodeSandbox does NOT use SSH patterns", () => { }); describe("CodeSandbox upload_file security", () => { - it("should validate remote path for injection characters", () => { - // upload_file should check for single quote, $, `, newline in remote_path + it("should validate remote path with strict allowlist regex", () => { expect(codesandboxLib).toContain("remote_path"); // The validation line checks for unsafe characters const uploadLines = codesandboxLib.split("\n").filter((l) => @@ -527,13 +526,18 @@ describe("CodeSandbox upload_file security", () => { ); // There should be at least one validation check that rejects unsafe chars expect(uploadLines.length).toBeGreaterThan(0); - // The validation checks for dollar sign and backtick characters - expect(codesandboxLib).toContain("*'$'*"); + // Uses strict allowlist regex instead of blocklist + expect(codesandboxLib).toMatch(/\[a-zA-Z0-9/); }); it("should use base64 for safe file content transfer", () => { expect(codesandboxLib).toContain("base64"); }); + + it("should use SDK filesystem API via env vars", () => { + expect(codesandboxLib).toContain("_CSB_REMOTE_PATH"); + expect(codesandboxLib).toContain("process.env._CSB_REMOTE_PATH"); + }); }); describe("CodeSandbox agent script patterns", () => { diff --git a/cli/src/__tests__/upload-file-security.test.ts b/cli/src/__tests__/upload-file-security.test.ts index a52ad3ed1..54b8e9502 100644 --- a/cli/src/__tests__/upload-file-security.test.ts +++ b/cli/src/__tests__/upload-file-security.test.ts @@ -6,13 +6,12 @@ import type { Manifest } from "../manifest"; /** * Security regression tests for upload_file() functions across all cloud libs. * - * PR #453 fixed command injection vulnerabilities in upload_file() for 5 clouds - * (fly, northflank, daytona, e2b, koyeb) by replacing unsafe printf '%q' - * patterns with validated single-quoted embedding. + * PR #453 fixed command injection vulnerabilities in upload_file() for 5 clouds. + * PR #989 hardened all exec-based upload_file() to use strict allowlist regex + * validation ([a-zA-Z0-9/_.~-]+) instead of fragile blocklist/printf '%q' patterns. * * These tests ensure: - * 1. Non-SSH upload_file functions validate remote_path for injection chars - * OR use printf '%q' escaping for safe embedding + * 1. Non-SSH upload_file functions validate remote_path with strict allowlist regex * 2. Content is base64-encoded (shell-safe) before embedding in commands * 3. No unquoted variable expansion in command strings * 4. No use of raw content embedding without base64 @@ -147,19 +146,21 @@ describe("upload_file() Security Patterns", () => { // ── Remote path injection protection ─────────────────────────────── // Exec-based clouds must protect against remote_path injection via EITHER: - // (a) Character validation: reject ', $, `, newlines in remote_path - // (b) printf '%q' escaping: shell-escape the path before embedding + // (a) Strict allowlist regex: only allow safe path characters [a-zA-Z0-9/_.~-] + // (b) Character validation: reject ', $, `, newlines in remote_path + // (c) printf '%q' escaping: shell-escape the path before embedding describe("exec-based upload_file: remote path injection protection", () => { for (const { cloud, body } of execBasedClouds) { it(`${cloud} should protect remote_path against injection`, () => { + const hasAllowlistRegex = /\[a-zA-Z0-9/.test(body); const hasCharValidation = (body.includes(`"'"`) || body.includes("'\\''")) && (body.includes("'$'") || body.includes("\\$")); const hasPrintfEscape = body.includes("printf '%q'"); // Must use at least one protection method expect( - hasCharValidation || hasPrintfEscape + hasAllowlistRegex || hasCharValidation || hasPrintfEscape ).toBe(true); }); } @@ -184,16 +185,20 @@ describe("upload_file() Security Patterns", () => { describe("exec-based upload_file: safe command construction", () => { for (const { cloud, body } of execBasedClouds) { - it(`${cloud} should use base64 decode in the remote command`, () => { + it(`${cloud} should use base64 decode or SDK filesystem API`, () => { const hasSafeDecode = - body.includes("base64 -d") || body.includes("base64 --decode"); + body.includes("base64 -d") || body.includes("base64 --decode") || + body.includes("Buffer.from") || body.includes("writeFile"); expect(hasSafeDecode).toBe(true); }); - it(`${cloud} should use printf '%s' for safe content output`, () => { - // Safe: printf '%s' '${content}' | base64 -d + it(`${cloud} should use safe content delivery (printf or env var)`, () => { + // Safe: printf '%s' '${content}' | base64 -d (shell-based) + // Safe: process.env._CSB_CONTENT (env var-based, SDK) // Unsafe: echo ${content} | base64 -d - expect(body).toContain("printf '%s'"); + const hasSafeDelivery = + body.includes("printf '%s'") || body.includes("process.env."); + expect(hasSafeDelivery).toBe(true); }); } }); @@ -242,9 +247,9 @@ describe("upload_file() Security Patterns", () => { } }); - // ── Regression: specific clouds fixed in PR #453 ─────────────────── + // ── Regression: specific clouds fixed in PR #453 & #989 ───────────── - describe("PR #453 regression: fixed clouds have path validation", () => { + describe("PR #453/#989 regression: fixed clouds have strict path validation", () => { const fixedClouds = ["fly", "northflank", "daytona", "e2b", "koyeb"]; for (const cloud of fixedClouds) { @@ -255,9 +260,8 @@ describe("upload_file() Security Patterns", () => { expect(info.body).toContain("SECURITY"); }); - it(`${cloud} should validate against single-quote breakout`, () => { - // These specific clouds use the char-validation pattern, not printf '%q' - expect(info.body).toContain(`"'"`); + it(`${cloud} should use strict allowlist regex for path validation`, () => { + expect(info.body).toMatch(/\[a-zA-Z0-9/); }); it(`${cloud} should use safe content embedding`, () => { @@ -268,31 +272,27 @@ describe("upload_file() Security Patterns", () => { it(`${cloud} should use safe path embedding`, () => { const hasSafePathEmbed = - info.body.includes("'${remote_path}'") || info.body.includes("'$remote_path'") || - info.body.includes("escaped_path"); + info.body.includes("'${remote_path}'") || info.body.includes("'$remote_path'"); expect(hasSafePathEmbed).toBe(true); }); } }); - // ── Printf '%q' pattern clouds: railway, modal ───────────────────── + // ── Additional exec-based clouds with strict validation ────────────── - describe("printf '%q' pattern clouds have proper escaping", () => { - // These clouds use printf '%q' to escape paths (not char-by-char validation) - const printfClouds = execBasedClouds.filter(({ body }) => - body.includes("printf '%q'") && !body.includes(`"'"`) - ); + describe("additional exec-based clouds have strict path validation", () => { + // These clouds (railway, modal, render, codesandbox) also use strict allowlist + const additionalClouds = ["railway", "modal", "render", "codesandbox"]; + for (const cloud of additionalClouds) { + const info = cloudUploadTypes.get(cloud); + if (!info || info.type !== "exec-based") continue; - for (const { cloud, body } of printfClouds) { - it(`${cloud} should use printf '%q' to escape remote_path`, () => { - expect(body).toContain("printf '%q'"); - // Escaped variable can be named escaped_path, escaped_remote, etc. - expect(body).toMatch(/escaped_\w+/); + it(`${cloud} should use strict allowlist regex for path validation`, () => { + expect(info.body).toMatch(/\[a-zA-Z0-9/); }); - it(`${cloud} should base64-encode content before embedding`, () => { - expect(body).toContain("base64"); - expect(body).toContain("'${content}'"); + it(`${cloud} should base64-encode content`, () => { + expect(info.body).toContain("base64"); }); } }); @@ -332,8 +332,8 @@ describe("upload_file() Security Patterns", () => { expect(info.body).toContain("sprite exec"); }); - it(`${cloud} should escape paths with printf '%q'`, () => { - expect(info.body).toContain("printf '%q'"); + it(`${cloud} should use strict allowlist regex for path validation`, () => { + expect(info.body).toMatch(/\[a-zA-Z0-9/); }); } }); diff --git a/codesandbox/lib/common.sh b/codesandbox/lib/common.sh index 615cbf047..b16c10d5b 100644 --- a/codesandbox/lib/common.sh +++ b/codesandbox/lib/common.sh @@ -195,20 +195,32 @@ upload_file() { local local_path="${1}" local remote_path="${2}" - # Validate remote_path to prevent command injection - if [[ "$remote_path" == *"'"* || "$remote_path" == *'$'* || "$remote_path" == *'`'* || "$remote_path" == *$'\n'* ]]; then - log_error "Invalid remote path (contains unsafe characters): $remote_path" + # SECURITY: Strict allowlist validation — only safe path characters + if [[ ! "${remote_path}" =~ ^[a-zA-Z0-9/_.~-]+$ ]]; then + log_error "Invalid remote path (must contain only alphanumeric, /, _, ., ~, -): ${remote_path}" return 1 fi local content content=$(base64 -w0 "${local_path}" 2>/dev/null || base64 "${local_path}") - # SECURITY: Properly escape remote_path to prevent injection - local escaped_path - escaped_path=$(printf '%q' "${remote_path}") - # base64 output is safe (alphanumeric + /+=) so no injection risk - run_server "printf '%s' '${content}' | base64 -d > ${escaped_path}" + # SECURITY: Use SDK filesystem API via env vars — no shell interpolation + validate_sandbox_id "${CODESANDBOX_SANDBOX_ID}" || return 1 + CSB_API_KEY="${CSB_API_KEY}" _CSB_SB_ID="${CODESANDBOX_SANDBOX_ID}" \ + _CSB_REMOTE_PATH="${remote_path}" _CSB_CONTENT="${content}" \ + _csb_sdk_eval " + const fs = require('fs'); + const path = require('path'); + const sb = await sdk.sandboxes.get(process.env._CSB_SB_ID); + const c = await sb.connect(); + const remotePath = process.env._CSB_REMOTE_PATH; + const dir = path.dirname(remotePath); + if (dir !== '.' && dir !== '/') { + await c.commands.run('mkdir -p ' + dir); + } + const content = Buffer.from(process.env._CSB_CONTENT, 'base64'); + await c.fs.writeFile(remotePath, content); + " } interactive_session() { diff --git a/daytona/lib/common.sh b/daytona/lib/common.sh index 16a0b342d..466b62bbb 100644 --- a/daytona/lib/common.sh +++ b/daytona/lib/common.sh @@ -217,9 +217,9 @@ upload_file() { local local_path="${1}" local remote_path="${2}" - # SECURITY: Validate remote_path to prevent command injection via single-quote breakout - if [[ "$remote_path" == *"'"* || "$remote_path" == *'$'* || "$remote_path" == *'`'* || "$remote_path" == *$'\n'* ]]; then - log_error "Invalid remote path (contains unsafe characters): $remote_path" + # SECURITY: Strict allowlist validation — only safe path characters + if [[ ! "${remote_path}" =~ ^[a-zA-Z0-9/_.~-]+$ ]]; then + log_error "Invalid remote path (must contain only alphanumeric, /, _, ., ~, -): ${remote_path}" return 1 fi diff --git a/e2b/lib/common.sh b/e2b/lib/common.sh index fd616c40c..62edf1a58 100644 --- a/e2b/lib/common.sh +++ b/e2b/lib/common.sh @@ -125,9 +125,9 @@ upload_file() { local local_path="${1}" local remote_path="${2}" - # SECURITY: Validate remote_path to prevent command injection via single-quote breakout - if [[ "$remote_path" == *"'"* || "$remote_path" == *'$'* || "$remote_path" == *'`'* || "$remote_path" == *$'\n'* ]]; then - log_error "Invalid remote path (contains unsafe characters): $remote_path" + # SECURITY: Strict allowlist validation — only safe path characters + if [[ ! "${remote_path}" =~ ^[a-zA-Z0-9/_.~-]+$ ]]; then + log_error "Invalid remote path (must contain only alphanumeric, /, _, ., ~, -): ${remote_path}" return 1 fi diff --git a/fly/lib/common.sh b/fly/lib/common.sh index 6722a0a8e..e35f59532 100644 --- a/fly/lib/common.sh +++ b/fly/lib/common.sh @@ -330,9 +330,9 @@ upload_file() { local local_path="$1" local remote_path="$2" - # SECURITY: Validate remote_path to prevent command injection via single-quote breakout - if [[ "$remote_path" == *"'"* || "$remote_path" == *'$'* || "$remote_path" == *'`'* || "$remote_path" == *$'\n'* ]]; then - log_error "Invalid remote path (contains unsafe characters): $remote_path" + # SECURITY: Strict allowlist validation — only safe path characters + if [[ ! "${remote_path}" =~ ^[a-zA-Z0-9/_.~-]+$ ]]; then + log_error "Invalid remote path (must contain only alphanumeric, /, _, ., ~, -): ${remote_path}" return 1 fi diff --git a/koyeb/lib/common.sh b/koyeb/lib/common.sh index 9ecd00a7c..9089eb1af 100644 --- a/koyeb/lib/common.sh +++ b/koyeb/lib/common.sh @@ -255,9 +255,9 @@ upload_file() { return 1 fi - # SECURITY: Validate remote_path to prevent command injection via single-quote breakout - if [[ "$remote_path" == *"'"* || "$remote_path" == *'$'* || "$remote_path" == *'`'* || "$remote_path" == *$'\n'* ]]; then - log_error "Invalid remote path (contains unsafe characters): $remote_path" + # SECURITY: Strict allowlist validation — only safe path characters + if [[ ! "${remote_path}" =~ ^[a-zA-Z0-9/_.~-]+$ ]]; then + log_error "Invalid remote path (must contain only alphanumeric, /, _, ., ~, -): ${remote_path}" return 1 fi diff --git a/modal/lib/common.sh b/modal/lib/common.sh index 662a7b9e3..a7eb41f35 100644 --- a/modal/lib/common.sh +++ b/modal/lib/common.sh @@ -182,19 +182,16 @@ upload_file() { local local_path="${1}" local remote_path="${2}" - # Validate remote_path to prevent command injection - if [[ "$remote_path" == *"'"* || "$remote_path" == *'$'* || "$remote_path" == *'`'* || "$remote_path" == *$'\n'* ]]; then - log_error "Invalid remote path (contains unsafe characters): $remote_path" + # SECURITY: Strict allowlist validation — only safe path characters + if [[ ! "${remote_path}" =~ ^[a-zA-Z0-9/_.~-]+$ ]]; then + log_error "Invalid remote path (must contain only alphanumeric, /, _, ., ~, -): ${remote_path}" return 1 fi local content content=$(base64 -w0 "${local_path}" 2>/dev/null || base64 "${local_path}") - # SECURITY: Properly escape remote_path to prevent single-quote breakout injection - local escaped_path - escaped_path=$(printf '%q' "${remote_path}") # base64 output is safe (alphanumeric + /+=) so no injection risk - run_server "printf '%s' '${content}' | base64 -d > ${escaped_path}" + run_server "printf '%s' '${content}' | base64 -d > '${remote_path}'" } interactive_session() { diff --git a/northflank/lib/common.sh b/northflank/lib/common.sh index 02bdfef6b..1385450c2 100644 --- a/northflank/lib/common.sh +++ b/northflank/lib/common.sh @@ -182,9 +182,9 @@ upload_file() { local local_path="${1}" local remote_path="${2}" - # SECURITY: Validate remote_path to prevent command injection via single-quote breakout - if [[ "$remote_path" == *"'"* || "$remote_path" == *'$'* || "$remote_path" == *'`'* || "$remote_path" == *$'\n'* ]]; then - log_error "Invalid remote path (contains unsafe characters): $remote_path" + # SECURITY: Strict allowlist validation — only safe path characters + if [[ ! "${remote_path}" =~ ^[a-zA-Z0-9/_.~-]+$ ]]; then + log_error "Invalid remote path (must contain only alphanumeric, /, _, ., ~, -): ${remote_path}" return 1 fi diff --git a/railway/lib/common.sh b/railway/lib/common.sh index 2b5025ec9..97de6e8dd 100644 --- a/railway/lib/common.sh +++ b/railway/lib/common.sh @@ -184,17 +184,19 @@ upload_file() { return 1 fi + # SECURITY: Strict allowlist validation — only safe path characters + if [[ ! "${remote_path}" =~ ^[a-zA-Z0-9/_.~-]+$ ]]; then + log_error "Invalid remote path (must contain only alphanumeric, /, _, ., ~, -): ${remote_path}" + return 1 + fi + # SECURITY: base64 -w0 produces single-line output (no newline injection) # macOS base64 doesn't support -w0 but produces single-line by default local content content=$(base64 -w0 "$local_path" 2>/dev/null || base64 "$local_path") - # SECURITY: Properly escape remote_path to prevent injection via single-quote breakout - local escaped_path - escaped_path=$(printf '%q' "$remote_path") - # base64 output is alphanumeric+/+= so safe without escaping - run_server "printf '%s' '${content}' | base64 -d > ${escaped_path}" + run_server "printf '%s' '${content}' | base64 -d > '${remote_path}'" } # Wait for system readiness (Railway containers start with Ubuntu base) diff --git a/render/lib/common.sh b/render/lib/common.sh index e653c7e1c..1ff8ebd40 100644 --- a/render/lib/common.sh +++ b/render/lib/common.sh @@ -217,9 +217,9 @@ upload_file() { return 1 fi - # Validate remote_path to prevent command injection - if [[ "$remote_path" == *"'"* || "$remote_path" == *'$'* || "$remote_path" == *'`'* || "$remote_path" == *$'\n'* ]]; then - log_error "Invalid remote path (contains unsafe characters): $remote_path" + # SECURITY: Strict allowlist validation — only safe path characters + if [[ ! "${remote_path}" =~ ^[a-zA-Z0-9/_.~-]+$ ]]; then + log_error "Invalid remote path (must contain only alphanumeric, /, _, ., ~, -): ${remote_path}" return 1 fi @@ -228,7 +228,7 @@ upload_file() { content=$(base64 < "$local_path") # Write file on remote service - run_server "printf '%s' '$content' | base64 -d > '$remote_path'" + run_server "printf '%s' '${content}' | base64 -d > '${remote_path}'" } # Wait for basic system readiness (Render services are pre-configured) diff --git a/sprite/lib/common.sh b/sprite/lib/common.sh index dc25d9067..155c99f92 100644 --- a/sprite/lib/common.sh +++ b/sprite/lib/common.sh @@ -217,23 +217,23 @@ inject_env_vars_sprite() { # Upload file to sprite (for use with setup_claude_code_config callback) # Usage: upload_file_sprite SPRITE_NAME LOCAL_PATH REMOTE_PATH # Example: upload_file_sprite "$SPRITE_NAME" "/tmp/settings.json" "/root/.claude/settings.json" -# SECURITY: Uses proper quoting to prevent path injection +# SECURITY: Strict path validation + proper quoting to prevent injection upload_file_sprite() { local sprite_name="${1}" local local_path="${2}" local remote_path="${3}" - # Generate a unique temp path to avoid collisions + # SECURITY: Strict allowlist validation — only safe path characters + if [[ ! "${remote_path}" =~ ^[a-zA-Z0-9/_.~-]+$ ]]; then + log_error "Invalid remote path (must contain only alphanumeric, /, _, ., ~, -): ${remote_path}" + return 1 + fi + + # Generate a unique temp path to avoid collisions (safe: only uses basename + PID) local temp_remote temp_remote="/tmp/sprite_upload_$(basename "${remote_path}")_$$" - # Use printf %q for proper shell escaping of paths to prevent injection - local escaped_remote - escaped_remote=$(printf '%q' "${remote_path}") - local escaped_temp - escaped_temp=$(printf '%q' "${temp_remote}") - - sprite exec -s "${sprite_name}" -file "${local_path}:${temp_remote}" -- bash -c "mkdir -p \$(dirname ${escaped_remote}) && mv ${escaped_temp} ${escaped_remote}" + sprite exec -s "${sprite_name}" -file "${local_path}:${temp_remote}" -- bash -c "mkdir -p \$(dirname '${remote_path}') && mv '${temp_remote}' '${remote_path}'" } # Note: Provider-agnostic functions (nc_listen, open_browser, OAuth helpers, validate_model_id) are now in shared/common.sh