fix(tools): stop workspace guard misreading scheme-less URLs#2965
fix(tools): stop workspace guard misreading scheme-less URLs#2965maxmilian wants to merge 2 commits into
Conversation
…1042) When restrict_to_workspace is enabled, guardCommand extracted any "/..."-led run from the command and treated it as an absolute path. A scheme-less URL such as `curl -s "wttr.in/Beijing?T"` (curl accepts URLs without an explicit http:// prefix) had its "/Beijing?T" component matched, resolved to an absolute path, and blocked as a workspace escape -- making most curl-based skills unusable. Skip a "/path" match only when the run of characters immediately before the slash parses as a hostname (host-legal bytes [A-Za-z0-9.-], not starting with "-", containing a "."). Because a skipped match is glued to its host with no separator, the shell treats it as a single URL/relative token, never a standalone absolute path. Flag-glued paths ("tar -xC/etc/cron.d"), variable-prefixed paths ("cat $x/etc/passwd") and colon path-lists ("java -cp app.jar:/etc/passwd", "scp host:/etc/passwd") are not host-like and stay blocked; ".." traversal is caught earlier. Add TestShellTool_SchemelessURLGuard covering both directions, including the flag/var/colon escape regressions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hi @maxmilian , thanks for the PR,
In practice, in a workspace that contains a versioned symlink like We need to either properly identify a true URL token-instead of just matching any prefix with a dot-or re-run the validation on the full token before triggering the |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Repro:
On Could we tighten the heuristic and add a regression test for a dotted symlink/path case? Right now this looks like a security regression, not just an edge case. |
Description
When
restrict_to_workspaceis enabled, theexectool'sguardCommandextracted every
/...-led run from the command and treated each as an absolutepath. A scheme-less URL — which
curlaccepts without an explicithttp://prefix, e.g.
curl -s "wttr.in/Beijing?T"— had its/Beijing?Tcomponentmatched, resolved with
filepath.Abs, andfilepath.Rel(cwd, …)turned it into../../Beijing?T, tripping the.."path outside working dir" check. Reportersnote this makes most curl-based skills unusable.
Fix
Skip a
/pathmatch only when the run of characters immediately before theslash parses as a hostname: host-legal bytes
[A-Za-z0-9.-], not starting with-, containing a., not starting/ending with..Because a skipped match is glued to its host with no separator, the shell
treats the whole word as a single URL/relative token — never a standalone
absolute path. The skip therefore cannot whitelist a real escape:
curl wttr.in/Beijingwttr.incurl 192.168.1.1/status192.168.1.1tar -xC/etc/cron.d …-xC(starts-)cat $x/etc/passwdx(no dot)java -cp app.jar:/etc/passwd:)scp host:/etc/passwd:)cat /etc/passwd..traversal is still caught by the existing pre-loop check; the existingweb-scheme (
//hostafterhttp:/https:/…) skip is unchanged.Type of Change
AI Code Generation
The fix approach was human-selected and the security boundary was hardened
across three independent adversarial review passes — two of which found and
closed sandbox-escape classes (flag-glued
-Cpaths, and:path-lists) beforethis PR was opened.
Related Issue
Fixes #1042
Technical Context
pkg/tools/shell.goabsolutePathPattern(/[^\s"']+) matches aURL's host-relative path component; the prior exemption only covered
//aftera web scheme, not scheme-less hosts.
hostLikeBeforeSlashgates the skip on a hostname check.:is deliberately excluded from the host-character walk so classpath /remote-spec path-lists (
java -cp a.jar:/abs,scp host:/abs) are not mistakenfor
host:portand stay blocked.Test Environment
go test ./pkg/tools/: newTestShellTool_SchemelessURLGuardpasses (5scheme-less URLs allowed + 5 escape regressions blocked); existing
TestShellTool_URLsNotBlocked/URLBypassPrevented/PathTraversalVariants/
RestrictToWorkspace/SafePathsall pass.TestShellTool_DevNullAllowedandTestShellTool_FileURISandboxingfaillocally only due to the macOS
/tmp→/private/tmpsymlink (pre-existing onmain, unrelated to this change); they pass on Linux CI.Checklist
go vetpasses