fix: safety guard incorrectly blocks commands with URLs by hkc5 · Pull Request #1254 · sipeed/picoclaw

hkc5 · 2026-03-08T23:02:16Z

Problem

The safety guard was incorrectly blocking commands containing URLs. For example, agent-browser open https://github.com would be blocked with the error:

Command blocked by safety guard (path outside working dir)

Root Cause

The absolutePathPattern regex matches absolute file paths in commands. When parsing a URL like https://github.com, the regex captures //github.com as a match (the path portion after https:). This was then treated as an absolute file path and blocked because it's outside the working directory.

Fix

Added a check to skip any path match that starts with //, as these are URL path components, not file system paths.

Changes

Modified guardCommand in pkg/tools/shell.go to skip URL path components
Added test case TestShellTool_URLsNotBlocked to verify the fix

Fixes #1203

The absolutePathPattern regex was matching URL path components like //github.com as file system paths, causing commands containing URLs to be incorrectly blocked by the workspace restriction safety guard. For example, 'agent-browser open https://github.com' would be blocked because //github.com was treated as an absolute file path outside the working directory. The fix adds a check to skip any path match that starts with '//', as these are URL path components, not file system paths. Fixes sipeed#1203

hkc5 · 2026-03-09T08:47:07Z

@imguoguo

nikolasdehor

Clean fix for a legitimate false positive in the safety guard. URLs containing // after the scheme (e.g., https://github.com) were being matched as absolute paths.

The fix is minimal and correct: skipping matches that start with // avoids treating URL path components as filesystem paths. This is safe because:

No legitimate absolute filesystem path starts with // in normal usage
UNC paths (Windows \\server\share) use backslashes, not forward slashes

The test coverage is good: 6 different URL schemes/commands are tested, covering https, http, ftp, and various tools (curl, wget, git clone, browser).

One minor note: // is technically a valid path on Unix (POSIX says it's implementation-defined and may refer to network paths on some systems like Cygwin). In practice, this edge case is negligible and not worth complicating the fix for.

LGTM.

hkc5 · 2026-03-10T21:35:07Z

Thanks for the review Nicolas, I guess we need another reviewer
tagging from the CONTRIBUTING.md
@yinwm @alexhoshina @lxowalle @Zhaoyikaiii

@alexhoshina

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 sipeed#1254

alexhoshina · 2026-03-11T16:32:02Z

make lint plz

…bypass Using strings.Index(cmd, raw) always returned the first occurrence of the matched substring, allowing a bypass where the same //path appeared both inside a URL and as a standalone shell path (e.g. echo https://etc/passwd && cat //etc/passwd would skip the second match). Switch to FindAllStringIndex so each match is evaluated at its actual position in the command string. Adds TestShellTool_URLBypassPrevented to cover the exploit scenario.

alexhoshina · 2026-03-12T15:21:32Z

+				for _, scheme := range webSchemes {
+					if strings.HasSuffix(before, scheme) {


nit: HTTPS:example.com will be blocked. This issue does not affect merging; it is merely a minor detail.

alexhoshina · 2026-03-12T15:23:22Z

+	commands := []string{
+		"agent-browser open https://github.com",
+		"curl https://api.example.com/data",
+		"wget http://example.com/file",
+		"browser open https://github.com/user/repo",
+		"fetch ftp://ftp.example.com/file.txt",
+		"git clone https://github.com/sipeed/picoclaw.git",
+	}


nit: Real network requests may cause the exec tool to time out or slow down the CI.

hkc5 · 2026-03-13T08:26:11Z

Thanks @alexhoshina for the thorough review! Happy to follow up with separate PRs for any remaining nits, would appreciate a merge when you get a chance!

Orgmar · 2026-03-13T10:16:23Z

@hkc5 Clean fix for the URL false positive in the safety guard. The regex matching //github.com from URLs as an absolute path is a subtle edge case. Simple check for the // prefix handles it well.

We have a PicoClaw Dev Group on Discord where contributors can connect and collaborate. If you're interested, send an email to support@sipeed.com with the subject [Join PicoClaw Dev Group] + your GitHub account and we'll send you the invite link!

@alexhoshina

* fix: safety guard incorrectly blocks commands with URLs The absolutePathPattern regex was matching URL path components like //github.com as file system paths, causing commands containing URLs to be incorrectly blocked by the workspace restriction safety guard. For example, 'agent-browser open https://github.com' would be blocked because //github.com was treated as an absolute file path outside the working directory. The fix adds a check to skip any path match that starts with '//', as these are URL path components, not file system paths. Fixes sipeed#1203 * 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 sipeed#1254 * style: fix gofumpt formatting * fix(safety-guard): use exact match position to prevent URL exemption bypass Using strings.Index(cmd, raw) always returned the first occurrence of the matched substring, allowing a bypass where the same //path appeared both inside a URL and as a standalone shell path (e.g. echo https://etc/passwd && cat //etc/passwd would skip the second match). Switch to FindAllStringIndex so each match is evaluated at its actual position in the command string. Adds TestShellTool_URLBypassPrevented to cover the exploit scenario.

@alexhoshina

* fix: safety guard incorrectly blocks commands with URLs The absolutePathPattern regex was matching URL path components like //github.com as file system paths, causing commands containing URLs to be incorrectly blocked by the workspace restriction safety guard. For example, 'agent-browser open https://github.com' would be blocked because //github.com was treated as an absolute file path outside the working directory. The fix adds a check to skip any path match that starts with '//', as these are URL path components, not file system paths. Fixes sipeed#1203 * 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 sipeed#1254 * style: fix gofumpt formatting * fix(safety-guard): use exact match position to prevent URL exemption bypass Using strings.Index(cmd, raw) always returned the first occurrence of the matched substring, allowing a bypass where the same //path appeared both inside a URL and as a standalone shell path (e.g. echo https://etc/passwd && cat //etc/passwd would skip the second match). Switch to FindAllStringIndex so each match is evaluated at its actual position in the command string. Adds TestShellTool_URLBypassPrevented to cover the exploit scenario.

@alexhoshina

* fix: safety guard incorrectly blocks commands with URLs The absolutePathPattern regex was matching URL path components like //github.com as file system paths, causing commands containing URLs to be incorrectly blocked by the workspace restriction safety guard. For example, 'agent-browser open https://github.com' would be blocked because //github.com was treated as an absolute file path outside the working directory. The fix adds a check to skip any path match that starts with '//', as these are URL path components, not file system paths. Fixes sipeed#1203 * 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 sipeed#1254 * style: fix gofumpt formatting * fix(safety-guard): use exact match position to prevent URL exemption bypass Using strings.Index(cmd, raw) always returned the first occurrence of the matched substring, allowing a bypass where the same //path appeared both inside a URL and as a standalone shell path (e.g. echo https://etc/passwd && cat //etc/passwd would skip the second match). Switch to FindAllStringIndex so each match is evaluated at its actual position in the command string. Adds TestShellTool_URLBypassPrevented to cover the exploit scenario.

@alexhoshina

* fix: safety guard incorrectly blocks commands with URLs The absolutePathPattern regex was matching URL path components like //github.com as file system paths, causing commands containing URLs to be incorrectly blocked by the workspace restriction safety guard. For example, 'agent-browser open https://github.com' would be blocked because //github.com was treated as an absolute file path outside the working directory. The fix adds a check to skip any path match that starts with '//', as these are URL path components, not file system paths. Fixes sipeed#1203 * 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 sipeed#1254 * style: fix gofumpt formatting * fix(safety-guard): use exact match position to prevent URL exemption bypass Using strings.Index(cmd, raw) always returned the first occurrence of the matched substring, allowing a bypass where the same //path appeared both inside a URL and as a standalone shell path (e.g. echo https://etc/passwd && cat //etc/passwd would skip the second match). Switch to FindAllStringIndex so each match is evaluated at its actual position in the command string. Adds TestShellTool_URLBypassPrevented to cover the exploit scenario.

@alexhoshina

* fix: safety guard incorrectly blocks commands with URLs The absolutePathPattern regex was matching URL path components like //github.com as file system paths, causing commands containing URLs to be incorrectly blocked by the workspace restriction safety guard. For example, 'agent-browser open https://github.com' would be blocked because //github.com was treated as an absolute file path outside the working directory. The fix adds a check to skip any path match that starts with '//', as these are URL path components, not file system paths. Fixes sipeed#1203 * 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 sipeed#1254 * style: fix gofumpt formatting * fix(safety-guard): use exact match position to prevent URL exemption bypass Using strings.Index(cmd, raw) always returned the first occurrence of the matched substring, allowing a bypass where the same //path appeared both inside a URL and as a standalone shell path (e.g. echo https://etc/passwd && cat //etc/passwd would skip the second match). Switch to FindAllStringIndex so each match is evaluated at its actual position in the command string. Adds TestShellTool_URLBypassPrevented to cover the exploit scenario.

sipeed-bot bot added type: bug Something isn't working domain: tool go Pull requests that update go code labels Mar 8, 2026

This was referenced Mar 9, 2026

🦞 OpenClaw 生态日报 2026-03-09 duanyytop/agents-radar#107

Open

🦞 Bản tin hàng ngày hệ sinh thái OpenClaw 2026-03-09 compasify/agents-radar#15

Open

🦞 OpenClaw 生态日报 2026-03-09 rollysys/agents-radar#58

Open

nikolasdehor approved these changes Mar 10, 2026

View reviewed changes

nikolasdehor mentioned this pull request Mar 10, 2026

fix(tools): safety guard URL path handling and security enhancements #1217

Closed

10 tasks

github-actions bot mentioned this pull request Mar 11, 2026

🦞 OpenClaw 生态日报 2026-03-11 DenisZheng/agents-radar#9

Open

alexhoshina reviewed Mar 11, 2026

View reviewed changes

Comment thread pkg/tools/shell.go Outdated

style: fix gofumpt formatting

21cd92e

alexhoshina requested changes Mar 12, 2026

View reviewed changes

Comment thread pkg/tools/shell.go Outdated

alexhoshina approved these changes Mar 12, 2026

View reviewed changes

alexhoshina merged commit 6b72326 into sipeed:main Mar 13, 2026
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: safety guard incorrectly blocks commands with URLs#1254

fix: safety guard incorrectly blocks commands with URLs#1254
alexhoshina merged 4 commits intosipeed:mainfrom
hkc5:fix-safety-guard-url-blocking

hkc5 commented Mar 8, 2026

Uh oh!

hkc5 commented Mar 9, 2026

Uh oh!

nikolasdehor left a comment

Uh oh!

hkc5 commented Mar 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

alexhoshina commented Mar 11, 2026

Uh oh!

Uh oh!

alexhoshina Mar 12, 2026

Uh oh!

alexhoshina Mar 12, 2026

Uh oh!

hkc5 commented Mar 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Orgmar commented Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		for _, scheme := range webSchemes {
		if strings.HasSuffix(before, scheme) {

Conversation

hkc5 commented Mar 8, 2026

Problem

Root Cause

Fix

Changes

Uh oh!

hkc5 commented Mar 9, 2026

Uh oh!

nikolasdehor left a comment

Choose a reason for hiding this comment

Uh oh!

hkc5 commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alexhoshina commented Mar 11, 2026

Uh oh!

Uh oh!

alexhoshina Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

alexhoshina Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

hkc5 commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Orgmar commented Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hkc5 commented Mar 10, 2026 •

edited

Loading

hkc5 commented Mar 13, 2026 •

edited

Loading