fix: handle file:// URIs correctly in safety guard
The previous fix skipped all paths starting with '//', which incorrectly also skipped file:// URIs that could escape the workspace sandbox. Changes: - Only skip '//' paths when preceded by web URL schemes (http:, https:, ftp:, etc.) - file:// URIs are now properly checked against workspace boundaries - Added TestShellTool_FileURISandboxing to verify the fix Fixes security issue raised by @alexhoshina in PR #1254
This commit is contained in:
parent
50896f203d
commit
e05013f70c
2 changed files with 66 additions and 2 deletions
|
|
@ -339,12 +339,33 @@ func (t *ExecTool) guardCommand(command, cwd string) string {
|
||||||
matches := absolutePathPattern.FindAllString(cmd, -1)
|
matches := absolutePathPattern.FindAllString(cmd, -1)
|
||||||
|
|
||||||
for _, raw := range matches {
|
for _, raw := range matches {
|
||||||
// Skip URL path components that look like they're from URLs.
|
// Skip URL path components that look like they're from web URLs.
|
||||||
// When a URL like "https://github.com" is parsed, the regex captures
|
// When a URL like "https://github.com" is parsed, the regex captures
|
||||||
// "//github.com" as a match (the path portion after "https:").
|
// "//github.com" as a match (the path portion after "https:").
|
||||||
// These double-slash prefixes indicate URL paths, not file system paths.
|
// These double-slash prefixes indicate URL paths, not file system paths.
|
||||||
|
// However, we must NOT skip file:// URIs as they could escape the sandbox.
|
||||||
|
// Only skip if preceded by a web URL scheme (http:, https:, ftp:, etc.).
|
||||||
if strings.HasPrefix(raw, "//") {
|
if strings.HasPrefix(raw, "//") {
|
||||||
continue
|
// Check if this // path is preceded by a web URL scheme
|
||||||
|
// by looking for patterns like "http://", "https://", "ftp://" before the match
|
||||||
|
idx := strings.Index(cmd, raw)
|
||||||
|
if idx > 0 {
|
||||||
|
// Look for the scheme prefix (e.g., "https:") before the //
|
||||||
|
before := cmd[:idx]
|
||||||
|
// Check if it ends with a web URL scheme followed by colon
|
||||||
|
// Web schemes: http, https, ftp, ftps, sftp, ssh, git
|
||||||
|
webSchemes := []string{"http:", "https:", "ftp:", "ftps:", "sftp:", "ssh:", "git:"}
|
||||||
|
isWebURL := false
|
||||||
|
for _, scheme := range webSchemes {
|
||||||
|
if strings.HasSuffix(before, scheme) {
|
||||||
|
isWebURL = true
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if isWebURL {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
p, err := filepath.Abs(raw)
|
p, err := filepath.Abs(raw)
|
||||||
|
|
|
||||||
|
|
@ -472,3 +472,46 @@ func TestShellTool_URLsNotBlocked(t *testing.T) {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestShellTool_FileURISandboxing verifies that file:// URIs that escape the
|
||||||
|
// workspace are still blocked, even though other URLs are allowed (issue #1254).
|
||||||
|
func TestShellTool_FileURISandboxing(t *testing.T) {
|
||||||
|
tmpDir := t.TempDir()
|
||||||
|
tool, err := NewExecTool(tmpDir, true)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unable to configure exec tool: %s", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// These file:// URIs should be blocked if they reference paths outside the workspace.
|
||||||
|
// Unlike web URLs (http://, https://, ftp://), file:// URIs can be used to escape the sandbox.
|
||||||
|
blockedCommands := []string{
|
||||||
|
"cat file:///etc/passwd",
|
||||||
|
"cat file:///etc/hosts",
|
||||||
|
"cat file:///root/.ssh/id_rsa",
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, cmd := range blockedCommands {
|
||||||
|
result := tool.Execute(context.Background(), map[string]any{"command": cmd})
|
||||||
|
if !result.IsError || !strings.Contains(result.ForLLM, "path outside working dir") {
|
||||||
|
t.Errorf("file:// URI outside workspace should be blocked: %s", cmd)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// These file:// URIs should be allowed if they reference paths inside the workspace.
|
||||||
|
// Create a test file inside the temp directory
|
||||||
|
testFile := filepath.Join(tmpDir, "test.txt")
|
||||||
|
if err := os.WriteFile(testFile, []byte("test content"), 0644); err != nil {
|
||||||
|
t.Fatalf("failed to create test file: %s", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
allowedCommands := []string{
|
||||||
|
"cat file://" + testFile,
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, cmd := range allowedCommands {
|
||||||
|
result := tool.Execute(context.Background(), map[string]any{"command": cmd})
|
||||||
|
if result.IsError && strings.Contains(result.ForLLM, "path outside working dir") {
|
||||||
|
t.Errorf("file:// URI inside workspace should be allowed: %s\n error: %s", cmd, result.ForLLM)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue