fix(shell): validate working_directory to prevent namespace escape

[chickenlj]

Summary

• ShellExecuteTool.execute() blindly prepended cd <working_directory> && to shell commands without any validation. An LLM-supplied absolute path, ~, or .. could escape the namespace-scoped workspace directory.
• Now rejects absolute paths, home-dir expansion (~), and parent-directory traversal (..).
• Shell-quotes the path to prevent injection via special characters.

Security: This is a namespace escape vulnerability — the LLM could execute commands outside the user's sandboxed workspace.

Fixes #1777

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...gentscope/harness/agent/tool/ShellExecuteTool.java	0.00%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[AgentScopeJavaBot]

🤖 AI Review

This PR adds input validation to ShellExecuteTool.execute() to prevent namespace escape via the LLM-supplied workingDirectory parameter. Previously, the working directory was concatenated into the shell command without any sanitization, allowing the LLM to use absolute paths, ~, or .. to execute commands outside the sandboxed workspace. The fix validates the path and applies proper POSIX single-quote shell escaping. The overall approach is sound and addresses the described vulnerability (fixes #1777). However, the contains("..") check is overly broad (false positives on legitimate directory names) and there are no unit tests covering this security-critical validation logic.

[AgentScopeJavaBot]

[recommended] contains("..") is overly broad — rejects legitimate directory names.

A path like src/file..bak or module..v2/src would be rejected even though .. is not used as a parent-directory traversal component. This is a false positive that could block legitimate LLM workflows.

Consider using Path.normalize() instead:

Path normalized = Path.of(wd).normalize();
if (normalized.startsWith("..")) {
    return "Error: ...";
}

Path.normalize() resolves all .. segments, so even creative traversals like subdir/../../etc are caught correctly.

[AgentScopeJavaBot]

[recommended] No unit tests for this security-critical validation logic.

This is the primary defense against namespace escape via the shell tool. There are currently zero tests for ShellExecuteTool. Security-critical input validation should have dedicated test coverage including:

• Rejected paths: /etc/passwd, ~/secrets, ../../etc, foo/../../etc
• Accepted paths: src/main, tests, sub/dir
• Edge cases: file..bak (should be accepted), paths with single quotes, paths with spaces, empty/blank strings, null
• Shell quoting correctness: verify the constructed command properly escapes adversarial directory names

[AgentScopeJavaBot]

🤖 AI Review

This PR adds input validation to ShellExecuteTool.execute() to prevent namespace escape via the LLM-supplied workingDirectory parameter. Previously, the working directory was concatenated into the shell command without any sanitization, allowing the LLM to use absolute paths, ~, or .. to execute commands outside the sandboxed workspace. The fix validates the path and applies proper POSIX single-quote shell escaping. The overall approach is sound and addresses the described vulnerability (fixes #1777). However, the contains("..") check is overly broad (false positives on legitimate directory names) and there are no unit tests covering this security-critical validation logic.

[AgentScopeJavaBot]

[recommended] contains("..") is overly broad — rejects legitimate directory names.

A path like src/file..bak or module..v2/src would be rejected even though .. is not used as a parent-directory traversal component. This is a false positive that could block legitimate LLM workflows.

Consider using Path.normalize() instead:

Path normalized = Path.of(wd).normalize();
if (normalized.startsWith("..")) {
    return "Error: ...";
}

Path.normalize() resolves all .. segments, so even creative traversals like subdir/../../etc are caught correctly.

[AgentScopeJavaBot]

[recommended] No unit tests for this security-critical validation logic.

This is the primary defense against namespace escape via the shell tool. There are currently zero tests for ShellExecuteTool. Security-critical input validation should have dedicated test coverage including:

• Rejected paths: /etc/passwd, ~/secrets, ../../etc, foo/../../etc
• Accepted paths: src/main, tests, sub/dir
• Edge cases: file..bak (should be accepted), paths with single quotes, paths with spaces, empty/blank strings, null
• Shell quoting correctness: verify the constructed command properly escapes adversarial directory names

[itxaiohanglover]

Good security fix — validating that working_directory is relative prevents path traversal attacks. The shell escaping (wd.replace("'", "'\\'''")) is the correct approach for single-quote escaping in POSIX shells. One question: should we also validate that the path doesn't contain null bytes? \x00 could cause unexpected behavior in some shell implementations.