Security: Potential Command Injection via Untrusted URL in Browser Open

[tomaioo]

Summary

Security: Potential Command Injection via Untrusted URL in Browser Open

Problem

Severity: Medium | File: internal/browser/open.go:L17

The OpenURL function in internal/browser/open.go passes a caller-supplied URL directly to exec.Command without sanitization. On Linux and FreeBSD, this URL is passed as an argument to xdg-open. If a malicious application or script can control this URL, it might be possible to inject arbitrary command-line arguments (argument injection) depending on how xdg-open parses them, potentially leading to command execution.

Solution

Ensure that the URL passed to OpenURL is strictly validated and prefixed with a safe scheme (like http:// or https://) before being passed to exec.Command. Reject URLs that start with a dash (-) to prevent argument injection.

Changes

• internal/browser/open.go (modified)

Summary by CodeRabbit

• Bug Fixes
  • Improved URL validation when opening links.
  • Blocked unsupported URL schemes and inputs that could be interpreted as command-line options.
  • Prevented the browser launcher from running when a URL fails validation.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fa93bfab-8e19-4c14-8c13-fba61f4c950d

📥 Commits

Reviewing files that changed from the base of the PR and between ce4a996 and c94156e.

📒 Files selected for processing (1)

• internal/browser/open.go

---

Walkthrough

OpenURL now validates URLs before launching a browser command. Validation rejects leading-dash inputs, malformed URLs, and schemes other than http or https; invalid inputs return errors without starting the launcher.

Changes

Browser URL Safety

Layer / File(s)	Summary
Validate URLs before browser launch internal/browser/open.go	validateURL rejects argument-injection prefixes, parse failures, and unsupported schemes. OpenURL returns validation errors before starting the platform-specific launcher.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the security fix for untrusted browser URLs and the command-injection risk addressed by the PR.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gnanam1990]

Verdict: Approve (with one minor request)

The change is safe, correctly scoped, and strictly tightens behavior over main — but the title oversells it: there is no command injection at this call site, and the one new security control ships with zero test coverage.

---

What this actually changes

internal/browser/open.go:34-46 adds validateURL, called from OpenURL at :18-20. It rejects a leading - and pins the scheme to http/https. That is the whole PR — git diff <merge-base>..HEAD --name-only returns exactly one path, internal/browser/open.go.

On the title: no command injection existed here. openCommand returns an argv pair (internal/browser/open.go:50-58) and it is passed to exec.Command(name, args...) (:21-22) — no shell is ever spawned, so URL content cannot become a command regardless of metacharacters. What you have added is a scheme allowlist, which is a legitimate belt-and-braces guard at the sink (open on macOS and rundll32 url.dll,FileProtocolHandler on Windows both dispatch to registered protocol handlers, so pinning to http/https genuinely narrows that). Worth retitling to something like "harden browser.OpenURL with an http/https scheme allowlist" so the security ledger stays accurate.

Done well

• The pre-existing argv-array design was already correct and you did not disturb it. The common failure mode on a ticket like this is to add quoting/escaping, which would have made things strictly worse. Resisting that is the right call.
• Returning an error rather than silently no-oping preserves the documented contract at :14-16 — callers can still fall back to printing the URL.
• Loopback OAuth callbacks (http://127.0.0.1:8080/callback?code=1) still pass, so none of the wizard flows regress.

---

[Minor] validateURL has no test coverage — deleting it keeps CI green

internal/browser/open_test.go is unmodified by this PR; its only test, TestOpenCommand at open_test.go:8, exercises openCommand and never reaches validateURL or OpenURL. validateURL appears in no _test.go file anywhere in the repo.

Concretely: replace the body of validateURL (internal/browser/open.go:34-46) with return nil — fully reverting the security fix — and go test ./internal/browser/ still reports ok. A future refactor that drops or relaxes the scheme allowlist ships silently.

For a change whose entire value is the guard, the guard should be the thing under test. Suggested fix — a table-driven TestValidateURL in internal/browser/open_test.go asserting on error presence (not message text, so it stays refactor-tolerant):

• accept: https://x.test, http://127.0.0.1:8080/callback?code=1, HTTPS://Example.COM
• reject: file:///etc/passwd, javascript:alert(1), -a/Applications/Calculator.app, smb://host/share, example.com

I verified all of the above behave as described by running the committed logic directly (scratch file, since removed).

---

Things that looked like bugs and were checked and cleared

• strings.HasPrefix(rawURL, "-") at :35 looks like dead code. It is redundant — url.Parse never yields scheme http/https for a dash-prefixed string, so the allowlist at :42 would catch those anyway (I brute-forced 65k dash-prefixed permutations; zero reach an http/https scheme). But it is executed and taken for that input class, not unreachable, and it is the load-bearing defense if the allowlist is ever relaxed. Keep it.
• validateURL accepts host-less URLs (https:///etc/passwd, http://), unlike browserOpenURLArg (internal/tools/local_browser.go:646-666) and oauth.ValidateEndpointURL (internal/oauth/flow.go:98-104), which both require parsed.Host != "". No reachable harm: argv passing means no injection, the scheme stays pinned so no handler can be steered, and every production caller of OpenURL (internal/tui/oauth_device.go:46; internal/tui/onboarding.go:539,816; internal/tui/provider_wizard.go:164,191,271,532; internal/tui/provider_wizard_discovery.go:206) passes URLs already gated by oauth.ValidateEndpointURL upstream (internal/oauth/manager.go:226,232,238, internal/mcp/oauth.go:149). Adding || parsed.Host == "" would be a harmless consistency nit, nothing more.

[Nit] Un-hardened sibling path — pre-existing, explicitly not this PR's fault

The one place in the repo where an untrusted URL genuinely does reach a shell is the Windows helper wrapper: PrefixArgs: []string{"/d", "/s", "/c", ...} in internal/localcontrol/browser.go:362, reachable from the model-controlled browser_open tool via internal/tools/local_browser.go. I confirmed this is 100% pre-existing — both files hash identically at the merge base and neither appears in this PR's diff. Not a change request; flagging only because it is where the "untrusted URL" threat in the title actually lives, and it may deserve its own issue.

---

Build / test

Against the true merge base ce4a996 (note: the stated base fbf8598 is a descendant of the branch point — git diff fbf8598..HEAD misleadingly shows 43 files / 2214 deletions that are main-only work; the real diff is one file):

• gofmt -l ./internal/browser/ — clean
• go build ./... — exit 0
• go vet ./internal/browser/... — clean
• go test ./internal/browser/... -count=1 — ok (0.148s); -race also ok (1.310s), no reports (relevant given the go func() { _ = cmd.Wait() }() at :28)

Consumer packages internal/tui and internal/tools show four failures — TestHandleAddDirCommand (internal/tui/add_dir_test.go:43), TestScopedToolsAllowReadOnlyRootsWithoutWrite (internal/tools/file_tools_test.go:495), TestRegistryAppliesSandboxBeforeToolExecution and TestRegistrySandboxGatesPathAliasKeys (internal/tools/registry_test.go:235,262). All four reproduce identically on the merge base with byte-equivalent assertion messages; they are the known sandbox/write-root environment failures and are not attributable to this PR, which touches no file in those packages.

Merge is kevin's call per the program gate.