Skip to content

fix: don't treat str.strip as sanitization in mcp-unsanitized-return - #4130

Open
kankan223 wants to merge 1 commit into
semgrep:developfrom
kankan223:fix/mcp-unsanitized-return-strip
Open

kankan223 wants to merge 1 commit into
semgrep:developfrom
kankan223:fix/mcp-unsanitized-return-strip

Conversation

@kankan223

Copy link
Copy Markdown

Link to an issue, if relevant

Fixes the false negative reported for mcp-unsanitized-return-python: str.strip(...)
was listed as a sanitizer, so untrusted HTTP response data passed through it and was
returned from an MCP tool with no finding.

What changed

ai/ai-best-practices/mcp-unsanitized-return/mcp-unsanitized-return.yaml

  • Removed - pattern: str.strip(...) from pattern-sanitizers. Stripping whitespace
    does not neutralize prompt injection: "Ignore previous instructions ..." survives
    str.strip() unchanged. The remaining sanitizers (sanitize(...), bleach.clean(...),
    html.escape(...)) are real output neutralizers and are unchanged.

ai/ai-best-practices/mcp-unsanitized-return/mcp-unsanitized-return.py

  • Added true positives for the reported bypass and its method-call form:
    • return str.strip(response.text)
    • return response.text.strip()
  • Existing true negative (html.escape(response.text)) and non-tainted return are unchanged.

Why it's a bug

The rule's message warns that untrusted response data "may contain prompt injection
payloads or malicious content that could manipulate the LLM", but whitespace stripping
leaves that content intact, so the taint flow was suppressed and no finding was reported.

Minimal reproduction (zero findings before this change, one finding after):

import requests
from mcp.server.fastmcp import FastMCP
 
mcp = FastMCP("test-server")
 
@mcp.tool()
def fetch_page(url: str) -> str:
    response = requests.get(url)
    return str.strip(response.text)  # mcp-unsanitized-return-python

Whitespace stripping does not neutralize prompt-injection content, so
str.strip(...) let untrusted HTTP response data reach MCP tool returns
unflagged. Keep only real sanitizers.
Copilot AI balanced review requested due to automatic review settings November 2, 2026 11:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.


@mcp.tool()
def strip_method(url: str) -> str:
response = requests.get(url)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟠 High severity and reachable blocking issue identified in your code:
Line 34 has a potentially vulnerable usage of urllib3, possibly introducing a high severity vulnerability.

ℹ️ Why this is reachable

A reachable issue is a possible security risk because your project actually executes the vulnerable code. This issue is reachable because your code calls vulnerable code in this version of urllib3.
Affected versions of urllib3 are vulnerable to Allocation of Resources Without Limits or Throttling. urllib3's streaming chunked-transfer-encoding read path (HTTPResponse.read_chunked(), reached via HTTPResponse.stream() and response iteration) reads each chunk-size line without a length limit. A malicious server can send 'Transfer-Encoding: chunked' followed by a very long chunk-size line with no newline, forcing the client to buffer unbounded amounts of memory and causing a denial of service.

References: https://euvd.enisa.europa.eu/vulnerability/EUVD-2026-89017, GHSA, CVE

To resolve this comment:
Upgrade this dependency to at least version 2.8.0 at Pipfile.lock.

💬 Ignore this finding

To ignore this, reply with:

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

You can view more details on this finding in the Semgrep AppSec Platform here.

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.

2 participants