Skip to content

Commit d947265

Browse files
mini2smini2s
authored
fix(security): remediate command injection, path traversal, and SSRF vulnerabilities (#1353)
Command injection (CVSS 8.4-10.0): - src/utils/git.ts: autoCommit switches from exec (shell) to execFile (argv array), neutralizing injection via shell metacharacters in relPath - packages/evals/src/cli/processTask.ts: processTaskInContainer uses execa('docker', args) without shell:true; each -e K=V is its own argv - packages/evals/src/cli/runTaskInVscode.ts: pass jobToken via env object instead of interpolating into the shell command (mirrors runTaskInCli) - .roo/rules-issue-writer/1_workflow.xml + 3_best_practices.xml: use a quoted heredoc (--body-file - <<'ISSUE_EOF') so issue body is never shell-expanded; correct the unsafe 'robust quoting' guidance - apps/web-evals/src/lib/schemas.ts: add regex allowlist to jobToken SSRF (CVSS 6.9): - apps/web-roo-code/src/app/api/og/route.tsx: derive baseUrl from NEXT_PUBLIC_SITE_URL instead of the client-controlled Host header Path traversal (aligned to the approval model; outside-workspace access remains a user-controlled feature, so no hard workspace-boundary deny): - src/core/tools/FileOutline.ts: add askApproval before reading - src/core/tools/WriteToFileTool.ts: remove untracked pre-approval createDirectoriesForFile; defer to diffViewProvider which tracks/rollbacks - src/core/tools/GenerateImageTool.ts: defer input-image read to after approval; surface inputImageOutsideWorkspace in the approval prompt - src/core/checkpoints/index.ts: add isPathWithin boundary check to the non-interactive metadata helper (runs even on tool denial) - src/utils/pathUtils.ts: add isPathWithin(filePath, baseDir) checking against a given base dir to avoid the workspaceFolders false-positive regression in CLI / custom-working-dir modes Test: writeToFileTool.spec.ts updated to assert deferred dir creation.
1 parent 6734998 commit d947265

13 files changed

Lines changed: 254 additions & 118 deletions

File tree

‎.roo/rules-issue-writer/1_workflow.xml‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -338,15 +338,26 @@ Title: [ISSUE_TITLE]
338338
- Title: derive from Summary (≤ 80 chars, plain language)
339339
- Body: the finalized issue body
340340

341-
Execute:
341+
Execute (the quoted heredoc `<<'ISSUE_EOF'` passes the body verbatim
342+
with NO shell expansion, so quotes/`$`/backticks in the body cannot
343+
inject commands; the title is passed via gh's --title arg):
342344
<execute_command>
343-
<command>gh issue create --repo "[OWNER_REPO]" --title "[ISSUE_TITLE]" --body "$(printf '%s\n' "[ISSUE_BODY]")"</command>
345+
<command>gh issue create --repo "[OWNER_REPO]" --title "[ISSUE_TITLE]" --body-file - <<'ISSUE_EOF'
346+
[ISSUE_BODY]
347+
ISSUE_EOF</command>
344348
</execute_command>
345349

346350
- If "Submit now and assign to me":
347-
Execute (assignment at creation; falls back to edit if needed):
351+
Execute (assignment at creation; falls back to edit if needed). Same
352+
quoted-heredoc pattern is used in both create attempts:
348353
<execute_command>
349-
<command>ISSUE_URL=$(gh issue create --repo "[OWNER_REPO]" --title "[ISSUE_TITLE]" --body "$(printf '%s\n' "[ISSUE_BODY]")" --assignee "@me") || true; if [ -z "$ISSUE_URL" ]; then ISSUE_URL=$(gh issue create --repo "[OWNER_REPO]" --title "[ISSUE_TITLE]" --body "$(printf '%s\n' "[ISSUE_BODY]")"); gh issue edit "$ISSUE_URL" --add-assignee "@me"; fi; echo "$ISSUE_URL"</command>
354+
<command>ISSUE_URL=$(gh issue create --repo "[OWNER_REPO]" --title "[ISSUE_TITLE]" --body-file - --assignee "@me" <<'ISSUE_EOF'
355+
[ISSUE_BODY]
356+
ISSUE_EOF
357+
) || true; if [ -z "$ISSUE_URL" ]; then ISSUE_URL=$(gh issue create --repo "[OWNER_REPO]" --title "[ISSUE_TITLE]" --body-file - <<'ISSUE_EOF'
358+
[ISSUE_BODY]
359+
ISSUE_EOF
360+
); gh issue edit "$ISSUE_URL" --add-assignee "@me"; fi; echo "$ISSUE_URL"</command>
350361
</execute_command>
351362

352363
- Any other response:

‎.roo/rules-issue-writer/3_best_practices.xml‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,10 @@
5757
gh issue edit <issue-url-or-number> --add-assignee "@me".
5858
</assignment>
5959
<command_safety>
60-
Use --body with robust quoting (for example: --body "$(printf '%s\n' "[ISSUE_BODY]")") or a heredoc; do not create temporary files or reference file paths. Always include --repo "[OWNER_REPO]" and echo the resulting issue URL.
60+
Pass the issue body via a quoted heredoc with --body-file - (for example: `gh issue create ... --body-file - <<'ISSUE_EOF'` followed by the body and `ISSUE_EOF`). The quoted delimiter `<<'ISSUE_EOF'` disables ALL shell expansion, so quotes, `$`, backticks, or `;` inside the body cannot inject commands.
61+
NEVER embed [ISSUE_BODY] inside a double-quoted shell argument such as `--body "$(printf '%s\n' "[ISSUE_BODY]")"` — a `"` in the body closes the quote and allows command injection.
62+
For [ISSUE_TITLE], keep the derived title to plain alphanumerics, spaces, and common punctuation only; strip any `"`, `$`, backticks, or `;` before substitution, since the title is passed via gh's --title argument.
63+
Do not create temporary files or reference file paths. Always include --repo "[OWNER_REPO]" and echo the resulting issue URL.
6164
In execute_command calls, output only the command string; never include XML tags, CDATA markers, code fences, or backticks in the command payload.
6265
</command_safety>
6366
<error_handling>

‎apps/web-evals/src/lib/schemas.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,13 @@ export const createRunSchema = z
3535
concurrency: z.number().int().min(CONCURRENCY_MIN).max(CONCURRENCY_MAX),
3636
timeout: z.number().int().min(TIMEOUT_MIN).max(TIMEOUT_MAX),
3737
iterations: z.number().int().min(ITERATIONS_MIN).max(ITERATIONS_MAX),
38-
jobToken: z.string().optional(),
38+
// Restrict to a safe character set. jobToken is eventually interpolated
39+
// into shell/docker argv; rejecting shell metacharacters here is defense
40+
// in depth on top of the non-shell execa call in processTask.ts.
41+
jobToken: z
42+
.string()
43+
.regex(/^[A-Za-z0-9._-]+$/, "Roo Code Cloud Token contains invalid characters.")
44+
.optional(),
3945
executionMethod: executionMethodSchema,
4046
})
4147
.refine((data) => data.suite === "full" || (data.exercises || []).length > 0, {

‎apps/web-roo-code/src/app/api/og/route.tsx‎

Lines changed: 73 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,13 @@ export async function GET(request: NextRequest) {
4949
// Check if we should try to use the background image
5050
const useBackgroundImage = searchParams.get("bg") !== "false"
5151

52-
// Dynamically get the base URL from the current request
53-
// This ensures it works correctly in development, preview, and production environments
54-
const baseUrl = `${requestUrl.protocol}//${requestUrl.host}`
52+
// Use a trusted base URL from the environment rather than the client-
53+
// controlled Host header (requestUrl.host). The background image URL is
54+
// passed to Satori, which fetches it server-side via fetch(); deriving it
55+
// from the Host header enabled a blind SSRF (an attacker could point the
56+
// server at an internal host/IP). The OG image is a static asset served
57+
// from the site origin, so this base URL is fixed per deployment.
58+
const baseUrl = process.env.NEXT_PUBLIC_SITE_URL || "https://roocode.com"
5559
const variant = title.length % 2 === 0 ? "a" : "b"
5660
const backgroundUrl = `${baseUrl}/og/base_${variant}.png`
5761

@@ -67,88 +71,86 @@ export async function GET(request: NextRequest) {
6771
}
6872

6973
return new ImageResponse(
70-
(
71-
<div
72-
style={{
73-
width: "100%",
74-
height: "100%",
75-
display: "flex",
76-
position: "relative",
77-
// Use gradient background as default/fallback
78-
background: "linear-gradient(135deg, #1e3a5f 0%, #0f1922 50%, #1a2332 100%)",
79-
}}>
80-
{/* Optional Background Image - only render if explicitly requested */}
81-
{useBackgroundImage && (
82-
<div
74+
<div
75+
style={{
76+
width: "100%",
77+
height: "100%",
78+
display: "flex",
79+
position: "relative",
80+
// Use gradient background as default/fallback
81+
background: "linear-gradient(135deg, #1e3a5f 0%, #0f1922 50%, #1a2332 100%)",
82+
}}>
83+
{/* Optional Background Image - only render if explicitly requested */}
84+
{useBackgroundImage && (
85+
<div
86+
style={{
87+
position: "absolute",
88+
top: 0,
89+
left: 0,
90+
width: "100%",
91+
height: "100%",
92+
display: "flex",
93+
}}>
94+
{/* eslint-disable-next-line @next/next/no-img-element */}
95+
<img
96+
src={backgroundUrl}
97+
alt=""
98+
width={1200}
99+
height={630}
83100
style={{
84-
position: "absolute",
85-
top: 0,
86-
left: 0,
87101
width: "100%",
88102
height: "100%",
89-
display: "flex",
90-
}}>
91-
{/* eslint-disable-next-line @next/next/no-img-element */}
92-
<img
93-
src={backgroundUrl}
94-
alt=""
95-
width={1200}
96-
height={630}
97-
style={{
98-
width: "100%",
99-
height: "100%",
100-
objectFit: "cover",
101-
}}
102-
/>
103-
</div>
104-
)}
103+
objectFit: "cover",
104+
}}
105+
/>
106+
</div>
107+
)}
105108

106-
{/* Text Content */}
107-
<div
109+
{/* Text Content */}
110+
<div
111+
style={{
112+
position: "absolute",
113+
display: "flex",
114+
flexDirection: "column",
115+
justifyContent: "flex-end",
116+
top: "220px",
117+
left: "80px",
118+
right: "80px",
119+
bottom: "80px",
120+
}}>
121+
{/* Main Title */}
122+
<h1
108123
style={{
109-
position: "absolute",
110-
display: "flex",
111-
flexDirection: "column",
112-
justifyContent: "flex-end",
113-
top: "220px",
114-
left: "80px",
115-
right: "80px",
116-
bottom: "80px",
124+
fontSize: 70,
125+
fontWeight: 700,
126+
fontFamily: "Inter, Helvetica Neue, Helvetica, sans-serif",
127+
color: "white",
128+
lineHeight: 1.2,
129+
margin: 0,
130+
maxHeight: "2.4em",
131+
overflow: "hidden",
117132
}}>
118-
{/* Main Title */}
119-
<h1
133+
{title}
134+
</h1>
135+
136+
{/* Secondary Description */}
137+
{description && (
138+
<h2
120139
style={{
121140
fontSize: 70,
122-
fontWeight: 700,
123-
fontFamily: "Inter, Helvetica Neue, Helvetica, sans-serif",
124-
color: "white",
141+
fontWeight: 400,
142+
fontFamily: "Inter, Helvetica Neue, Helvetica, Arial, sans-serif",
143+
color: "rgba(255, 255, 255, 0.9)",
125144
lineHeight: 1.2,
126145
margin: 0,
127146
maxHeight: "2.4em",
128147
overflow: "hidden",
129148
}}>
130-
{title}
131-
</h1>
132-
133-
{/* Secondary Description */}
134-
{description && (
135-
<h2
136-
style={{
137-
fontSize: 70,
138-
fontWeight: 400,
139-
fontFamily: "Inter, Helvetica Neue, Helvetica, Arial, sans-serif",
140-
color: "rgba(255, 255, 255, 0.9)",
141-
lineHeight: 1.2,
142-
margin: 0,
143-
maxHeight: "2.4em",
144-
overflow: "hidden",
145-
}}>
146-
{description}
147-
</h2>
148-
)}
149-
</div>
149+
{description}
150+
</h2>
151+
)}
150152
</div>
151-
),
153+
</div>,
152154
{
153155
width: 1200,
154156
height: 630,

‎packages/evals/src/cli/processTask.ts‎

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -75,16 +75,25 @@ export const processTaskInContainer = async ({
7575
logger: Logger
7676
maxRetries?: number
7777
}) => {
78+
// Build docker arguments as separate argv elements. Each flag and each
79+
// "-e KEY=VALUE" pair is its own array entry, so user-controlled values
80+
// (jobToken, API keys) are never interpolated into a shell string. The
81+
// command below is passed to execa WITHOUT `shell: true`, which means it is
82+
// spawned directly (no /bin/sh -c) and shell metacharacters cannot inject.
7883
const baseArgs = [
7984
"--rm",
80-
"--network evals_default",
81-
"-v /var/run/docker.sock:/var/run/docker.sock",
82-
"-v /tmp/evals:/var/log/evals",
83-
"-e HOST_EXECUTION_METHOD=docker",
85+
"--network",
86+
"evals_default",
87+
"-v",
88+
"/var/run/docker.sock:/var/run/docker.sock",
89+
"-v",
90+
"/tmp/evals:/var/log/evals",
91+
"-e",
92+
"HOST_EXECUTION_METHOD=docker",
8493
]
8594

8695
if (jobToken) {
87-
baseArgs.push(`-e ROO_CODE_CLOUD_TOKEN=${jobToken}`)
96+
baseArgs.push("-e", `ROO_CODE_CLOUD_TOKEN=${jobToken}`)
8897
}
8998

9099
// Pass API keys to the container so the CLI can authenticate
@@ -99,7 +108,7 @@ export const processTaskInContainer = async ({
99108

100109
for (const envVar of apiKeyEnvVars) {
101110
if (process.env[envVar]) {
102-
baseArgs.push(`-e ${envVar}=${process.env[envVar]}`)
111+
baseArgs.push("-e", `${envVar}=${process.env[envVar]}`)
103112
}
104113
}
105114

@@ -108,7 +117,18 @@ export const processTaskInContainer = async ({
108117

109118
for (let attempt = 0; attempt <= maxRetries; attempt++) {
110119
const containerName = `evals-task-${taskId}.${attempt}`
111-
const args = [`--name ${containerName}`, `-e EVALS_ATTEMPT=${attempt}`, ...baseArgs]
120+
const args = [
121+
"run",
122+
"--name",
123+
containerName,
124+
"-e",
125+
`EVALS_ATTEMPT=${attempt}`,
126+
...baseArgs,
127+
"evals-runner",
128+
"sh",
129+
"-c",
130+
command,
131+
]
112132
const isRetry = attempt > 0
113133

114134
if (isRetry) {
@@ -121,7 +141,9 @@ export const processTaskInContainer = async ({
121141
`${isRetry ? "retrying" : "executing"} container command (attempt ${attempt + 1}/${maxRetries + 1})`,
122142
)
123143

124-
const subprocess = execa(`docker run ${args.join(" ")} evals-runner sh -c "${command}"`, { shell: true })
144+
// No `shell: true`: docker is spawned directly with an argv array, so
145+
// jobToken / API-key values cannot break out via shell metacharacters.
146+
const subprocess = execa("docker", args)
125147
// subprocess.stdout?.on("data", (data) => console.log(data.toString()))
126148
// subprocess.stderr?.on("data", (data) => console.error(data.toString()))
127149

‎packages/evals/src/cli/runTaskInVscode.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,20 +27,22 @@ export const runTaskInVscode = async ({ run, task, publish, logger, jobToken }:
2727
const prompt = fs.readFileSync(path.resolve(EVALS_REPO_PATH, `prompts/${language}.md`), "utf-8")
2828
const workspacePath = path.resolve(EVALS_REPO_PATH, language, exercise)
2929
const ipcSocketPath = path.resolve(os.tmpdir(), `evals-${run.id}-${task.id}.sock`)
30-
const env = { ROO_CODE_IPC_SOCKET_PATH: ipcSocketPath }
30+
// Pass jobToken through the process environment rather than baking it into
31+
// the shell command string. This mirrors the safe runTaskWithCli path and
32+
// prevents shell injection if jobToken ever contains shell metacharacters.
33+
const env: Record<string, string> = { ROO_CODE_IPC_SOCKET_PATH: ipcSocketPath }
34+
if (jobToken) {
35+
env.ROO_CODE_CLOUD_TOKEN = jobToken
36+
}
3137
const controller = new AbortController()
3238
const cancelSignal = controller.signal
3339
const containerized = isDockerContainer()
3440
const logDir = containerized ? `/var/log/evals/runs/${run.id}` : `/tmp/evals/runs/${run.id}`
3541

36-
let codeCommand = containerized
42+
const codeCommand = containerized
3743
? `xvfb-run --auto-servernum --server-num=1 code --wait --log trace --disable-workspace-trust --disable-gpu --disable-lcd-text --no-sandbox --user-data-dir /roo/.vscode --password-store="basic" -n ${workspacePath}`
3844
: `code --disable-workspace-trust -n ${workspacePath}`
3945

40-
if (jobToken) {
41-
codeCommand = `ROO_CODE_CLOUD_TOKEN=${jobToken} ${codeCommand}`
42-
}
43-
4446
logger.info(codeCommand)
4547

4648
// Sleep for a random amount of time between 5 and 10 seconds, unless we're

‎src/core/checkpoints/index.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { TelemetryService } from "@roo-code/telemetry"
77
import { Task } from "../task/Task"
88

99
import { getWorkspacePath } from "../../utils/path"
10+
import { isPathWithin } from "../../utils/pathUtils"
1011
import { checkGitInstalled } from "../../utils/git"
1112
import { t } from "../../i18n"
1213

@@ -31,6 +32,16 @@ async function updateCospecMetadataForCheckpoint(
3132
}
3233
const fileName = path.basename(editFilePath)
3334
const fileAbsPath = path.resolve(workspaceDir, editFilePath)
35+
// Guard against path traversal: isCoworkflowDocument can be bypassed with a
36+
// path like ".cospec/../../<target>/.cospec/requirements.md" (it only checks
37+
// that a .cospec segment exists somewhere). Verify the resolved path stays
38+
// within the checkpoint's workspace dir before reading/writing metadata.
39+
// This helper is a non-interactive metadata recorder (it also runs in the
40+
// tool-denial path), so outside-workspace targets are silently skipped —
41+
// outside-workspace files are not meant to get cospec metadata.
42+
if (!isPathWithin(fileAbsPath, workspaceDir)) {
43+
return
44+
}
3445
const cospecDir = path.dirname(fileAbsPath)
3546
const metadata = await CospecMetadataManager.getMetadataOrDefault(cospecDir)
3647
Object.assign(metadata, {

‎src/core/tools/FileOutline.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import * as path from "path"
77
import { Task } from "../task/Task"
88
import { getReadablePath } from "../../utils/path"
99
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
10+
import { formatResponse } from "../prompts/responses"
11+
import type { ClineSayTool } from "@roo-code/types"
1012
import type { ToolUse } from "../../shared/tools"
1113

1214
import { readFileSync } from "fs"
@@ -230,6 +232,24 @@ export class FileOutlineTool extends BaseTool<"file_outline"> {
230232

231233
const { parser, query } = parserData
232234

235+
// Request user approval before reading the file, consistent with
236+
// ReadFileTool/ListFilesTool. The isOutsideWorkspace flag is surfaced
237+
// to the approval UI so the existing auto-approval settings
238+
// (alwaysAllowReadOnly / alwaysAllowReadOnlyOutsideWorkspace) govern
239+
// access — outside-workspace reads remain a user-controlled feature.
240+
const sharedMessageProps: ClineSayTool = {
241+
tool: "readFile",
242+
path: getReadablePath(task.cwd, file_path),
243+
content: absolutePath,
244+
isOutsideWorkspace,
245+
}
246+
const didApprove = await askApproval("tool", JSON.stringify(sharedMessageProps satisfies ClineSayTool))
247+
248+
if (!didApprove) {
249+
pushToolResult(formatResponse.toolDenied())
250+
return
251+
}
252+
233253
// 读取文件内容
234254
const sourceCode = readFileSync(absolutePath, "utf-8")
235255

0 commit comments

Comments
 (0)