Skip to content

Comments

fix: prevent command injection in example URL opening#2082

Merged
maxisbey merged 1 commit intomainfrom
fix/example-command-injection
Feb 18, 2026
Merged

fix: prevent command injection in example URL opening#2082
maxisbey merged 1 commit intomainfrom
fix/example-command-injection

Conversation

@maxisbey
Copy link
Contributor

@maxisbey maxisbey commented Feb 18, 2026

Summary

Fix a command injection issue in the URL elicitation example client (examples/snippets/clients/url_elicitation_client.py).

Problem

The open_browser() function used subprocess.run(["start", url], shell=True) on Windows, which allows shell metacharacter injection via crafted URLs. A malicious MCP server could send a URL like https://example.com/?state=abc&calc during URL elicitation, and cmd.exe would interpret & as a command separator, executing calc (or any arbitrary command).

Fix

  • Replace all platform-specific subprocess calls with webbrowser.open(), which handles cross-platform browser opening safely without shell invocation
  • Add URL scheme validation (allowlist: http, https) before prompting the user, returning ElicitResult(action="decline") for disallowed schemes
  • Simplify open_browser() to a pure browser-opening helper
  • Remove unused subprocess and sys imports

This aligns with the safe pattern already used in examples/clients/simple-auth-client/mcp_simple_auth_client/main.py.

AI Disclaimer

@maxisbey maxisbey marked this pull request as ready for review February 18, 2026 14:55
Comment on lines 128 to 129
if parsed.scheme.lower() not in ALLOWED_SCHEMES:
print(f"Refusing to open URL with unsupported scheme '{parsed.scheme}': {url}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this throw e.g. a ValueError instead of just printing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea moved out of here into the handle elicitation function

@maxisbey maxisbey force-pushed the fix/example-command-injection branch from 90b1aa0 to c516b8b Compare February 18, 2026 15:05
@@ -124,12 +132,7 @@ def extract_domain(url: str) -> str:
def open_browser(url: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this even useful now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea not really, will remove.

Replace platform-specific subprocess calls with webbrowser.open() and
add URL scheme validation to the elicitation example client.

The previous Windows code path used shell=True with subprocess, which
allowed command injection via crafted URLs containing shell
metacharacters (e.g., & as a command separator in cmd.exe).

Changes:
- Remove subprocess/sys imports, use webbrowser.open() for all platforms
- Add URL scheme allowlist (http/https only) in handle_url_elicitation,
  validated before prompting the user for consent
- Return ElicitResult(action='decline') for disallowed schemes instead
  of silently continuing with action='accept'
- Simplify open_browser() to a pure browser-opening helper
- Align with the safe pattern already used in the OAuth example client
@maxisbey maxisbey force-pushed the fix/example-command-injection branch from c516b8b to 591d4d9 Compare February 18, 2026 15:09
@maxisbey maxisbey merged commit b9431d4 into main Feb 18, 2026
30 checks passed
@maxisbey maxisbey deleted the fix/example-command-injection branch February 18, 2026 15:16
maxisbey added a commit that referenced this pull request Feb 18, 2026
Backport of #2082 to v1.x.

Replace platform-specific subprocess calls with webbrowser.open() and add
URL scheme validation (http/https allowlist) to block dangerous protocol
handlers in the URL elicitation example client.
haraom

This comment was marked as spam.

haraom

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants