Skip to content

Sanitize untrusted output in fastmcp list and fastmcp call#3409

Open
jlowin wants to merge 1 commit intomainfrom
codex/fix-terminal-escape-injection-vulnerability-in-cli
Open

Sanitize untrusted output in fastmcp list and fastmcp call#3409
jlowin wants to merge 1 commit intomainfrom
codex/fix-terminal-escape-injection-vulnerability-in-cli

Conversation

@jlowin
Copy link
Member

@jlowin jlowin commented Mar 6, 2026

Motivation

  • The CLI was printing server-controlled strings (tool names/descriptions, prompt/resource text, and tool output) directly through Rich markup, allowing terminal/Rich markup and ANSI/OSC injection from malicious MCP servers.
  • The intent is to prevent terminal spoofing or control-sequence abuse while preserving the human-readable CLI output.

Description

  • Add a helper _sanitize_untrusted_text that uses rich.markup.escape and encodes non-printable/control characters as visible \xNN sequences while preserving newlines/tabs and printable UTF-8 content.
  • Apply the sanitizer to all CLI text output paths that render server-controlled content in text mode: tool call results and errors, resource text, prompt roles/messages, and fastmcp list output for tools, resources, and prompts.
  • Add unit tests verifying that Rich markup and control characters are escaped and that the sanitizer returns expected literal strings; include regression coverage in tests/cli/test_client_commands.py.

Testing

  • Ran uv sync successfully to prepare the environment.
  • Ran the full test suite with uv run pytest -n auto, which reported unrelated failures/timeouts outside of the CLI change (13 failed, 1 error) in this environment and are not caused by this patch.
  • Ran focused CLI tests uv run pytest tests/cli/test_client_commands.py, which passed (78 passed).
  • Ran static checks: uv run ruff check passed for the modified files, and uv run prek run --all-files failed to initialize hooks due to an external network checkout error (bootstrapping a third-party hook), unrelated to the code changes.

Codex Task

🤖 Generated with GPT-5.2-Codex
@marvin-context-protocol marvin-context-protocol bot added bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. cli Related to FastMCP CLI commands (run, dev, install) or CLI functionality. and removed aardvark labels Mar 6, 2026
@marvin-context-protocol
Copy link
Contributor

Test Failure Analysis

Summary: The static analysis (prek run --all-files) failed because ruff-format reformatted src/fastmcp/cli/client.py — the file was committed without running the formatter first.

Root Cause: Two console.print(...) calls in src/fastmcp/cli/client.py have lines exceeding the allowed line length. ruff-format auto-reformats them to multi-line style, but those changes weren't committed.

Suggested Solution: Run ruff format src/fastmcp/cli/client.py (or prek run --all-files) locally and commit the result. The diff is minimal — just two console.print(...) calls that need to be split across multiple lines:

# Before (too long):
console.print(f"    {_sanitize_untrusted_text(tool.description)}")
console.print(f"  [cyan]{_sanitize_untrusted_text(str(r.uri))}[/cyan]")

# After (ruff-formatted):
console.print(
    f"    {_sanitize_untrusted_text(tool.description)}"
)
console.print(
    f"  [cyan]{_sanitize_untrusted_text(str(r.uri))}[/cyan]"
)
Detailed Analysis

Failing check: ruff-format — 1 file reformatted

Log excerpt:

ruff format..............................................................Failed
- hook id: ruff-format
- files were modified by this hook

  1 file reformatted, 666 files left unchanged

Full diff applied by ruff-format:

--- a/src/fastmcp/cli/client.py
+++ b/src/fastmcp/cli/client.py
@@ -745,7 +745,9 @@
                     sig = format_tool_signature(tool)
                     console.print(f"  [cyan]{_sanitize_untrusted_text(sig)}[/cyan]")
                     if tool.description:
-                        console.print(f"    {_sanitize_untrusted_text(tool.description)}")
+                        console.print(
+                            f"    {_sanitize_untrusted_text(tool.description)}"
+                        )
                     if input_schema:
@@ -759,7 +761,9 @@
                 if not res:
                     console.print("  [dim]No resources found.[/dim]")
                 for r in res:
-                    console.print(f"  [cyan]{_sanitize_untrusted_text(str(r.uri))}[/cyan]")
+                    console.print(
+                        f"  [cyan]{_sanitize_untrusted_text(str(r.uri))}[/cyan]"
+                    )

Note: The loq (file size limits) check reported 7 violations but explicitly stated loq violations not enforced... yet! — those are warnings and did not cause the failure.

Related Files
  • src/fastmcp/cli/client.py — the file that needs formatting; contains the two console.print(...) calls that exceed line length

@jlowin jlowin added the DON'T MERGE PR is not ready for merging. Used by authors to prevent premature merging. label Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. cli Related to FastMCP CLI commands (run, dev, install) or CLI functionality. codex DON'T MERGE PR is not ready for merging. Used by authors to prevent premature merging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant