Skip to content

Commit 16b6862

Browse files
mharnettclaude
andauthored
security: port the 3 reachable upstream fixes, not the other 48 commits (#5)
The weekly upstream-diff sweep reported this fork 51 commits behind pipeboard-co/meta-ads-mcp. The divergence is deliberate (v1.2.0 excised pipeboard.co cloud-auth, v1.1.0 made writes opt-in, plus the OAuth authorization-code migration, the resilience layer and the test rebuild), so merging upstream means reconciling against code we removed on purpose. Most of the 51 are Meta-API tool-shape work and readme churn. Reachability was checked against how we actually launch: run-mcp.sh -> `python -m meta_ads_mcp`, stdio only, never --transport streamable-http. Three of the 51 are reachable; the rest are not. 1. 555ae48 (pipeboard-co#145) — token leak. make_api_request took params by reference and then set request_params["access_token"], so the caller's own dict came back carrying the live Meta token. core/ads.py echoes that same dict back to the user in two places: "attempted_updates" on Meta error subcode 1815573 and "update_data_sent" from the exception handler below it. Hand-ported as a one-line shallow copy (upstream's patch no longer applies — our api.py has diverged around it). Wire format unchanged. 2. 7d99263 (pipeboard-co#138, GHSA-45gf-fjxp-cjpq) — SSRF. download_image and try_multiple_download_methods fetched a caller-supplied URL server-side with redirects and no host/IP validation, so the server could reach loopback, private-network and cloud-metadata addresses. Upstream's core/utils.py hunk applied cleanly; its tests/test_ssrf_url_validation.py came with it. Skipped its version-bump files. 3. 93083c6 (pipeboard-co#136, CVE-2026-48710) — starlette floor raised to >=1.0.1 in pyproject.toml and requirements.txt; venv resolved 1.0.0 -> 1.6.0. mcp stays pinned at 1.12.2 (upstream's commit also bumps mcp to 1.23.0; not taken). Hygiene only — starlette is exercised only by the HTTP transport. Every image-fetch codepath is covered, verified by reading call sites rather than trusting the new tests: ads.py:792, :848, :1082 (download_image) and :1265 (try_multiple_download_methods) are the only outbound fetches, and all three download_image callers already return a clean error on falsy bytes, so a blocked URL surfaces as an error instead of a crash. Severity qualifier: update_ad_creative and upload_ad_image are both in write_gate.WRITE_TOOLS and run-mcp.sh does not set META_ADS_MCP_WRITE, so both are hidden from ListTools and refused at call time in most contexts. They are live in exactly one scope — ~/.claude.json sets META_ADS_MCP_WRITE=true for the clients/neon-one project. Knowingly NOT ported, so the gap is documented rather than implied: ef2592d (GHSA-8353-5qhw-8hfw), 14d7371 (GHSA-9gw6-46qc-99vr) and 95e8527 all harden the HTTP auth middleware. Unreachable while we run stdio only, but --transport streamable-http still exists at core/server.py:213, so that is a config fact, not a code guarantee. cbe4c63 (drop PIPEBOARD_API_TOKEN auth) we already did independently in v1.2.0. Tests: RED tests/test_api_token_leak.py::TestCallerParamsNotMutated 3 failed, 5 passed GREEN 8 passed RED tests/test_ssrf_url_validation.py ImportError (no validate_public_url) GREEN 33 passed Full suite: 532 passed, 2 failed — both pre-existing at origin/main (verified in a detached worktree at 660ff33): test_logging_conversion's server-init logging test, and test_write_gate's drift alarm, which reports compute_image_crops / get_ad_video / get_creative_details registered without a WRITE_TOOLS or READ_TOOLS classification. All three are reads, so the gate is not leaking writes. Untouched here — separate issue. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent c50021f commit 16b6862

6 files changed

Lines changed: 432 additions & 15 deletions

File tree

‎meta_ads_mcp/core/api.py‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,13 @@ async def make_api_request(
9292
"User-Agent": USER_AGENT,
9393
}
9494

95-
request_params = params or {}
95+
# Shallow-copy the caller's params: this function injects the access_token
96+
# below. Mutating the caller's dict leaked the live token into any tool
97+
# response that echoes its params back after the call — update_ad_creative's
98+
# "attempted_updates" (Meta error subcode 1815573) and "update_data_sent"
99+
# both do. Wire format is unchanged; the token is still sent.
100+
# Ported from upstream 555ae48 (pipeboard-co/meta-ads-mcp#145).
101+
request_params = dict(params) if params else {}
96102
request_params["access_token"] = access_token
97103

98104
# Logging the request (masking token for security)

‎meta_ads_mcp/core/utils.py‎

Lines changed: 148 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@
1212
import logging
1313
import pathlib
1414
import platform
15+
import ipaddress
16+
import socket
17+
from urllib.parse import urlparse
1518

1619
# Check for Meta app credentials in environment
1720
META_APP_ID = os.environ.get("META_APP_ID", "")
@@ -138,37 +141,153 @@ def extract_creative_image_urls(creative: Dict[str, Any]) -> List[str]:
138141
return unique_urls
139142

140143

144+
# --- Server-side request forgery (SSRF) guard for outbound image fetches ---
145+
#
146+
# upload_ad_image and the image-viewing tools fetch a caller-supplied URL
147+
# server-side. Without validation an attacker could point the URL at internal
148+
# services (http://127.0.0.1/...), private networks (10.x/192.168.x/172.16.x),
149+
# or the cloud metadata endpoint (http://169.254.169.254/) and use the server
150+
# as a proxy. See GHSA-45gf-fjxp-cjpq.
151+
#
152+
# Known residual: a hostname that resolves to a public IP at validation time
153+
# but to a private IP at connection time (DNS rebinding) is not fully closed,
154+
# since httpx resolves independently when it connects. The practical vectors
155+
# (a directly-internal URL, and a public URL that redirects inward) are blocked.
156+
157+
class BlockedURLError(Exception):
158+
"""Raised when a URL targets a disallowed (non-public) address."""
159+
160+
161+
_ALLOWED_URL_SCHEMES = ("http", "https")
162+
163+
164+
def _ip_is_disallowed(ip) -> bool:
165+
"""Return True if `ip` is not a public, routable address.
166+
167+
Blocks private, loopback, link-local (incl. 169.254.169.254 cloud
168+
metadata), reserved, multicast, and unspecified addresses. IPv4-mapped
169+
IPv6 addresses (e.g. ::ffff:127.0.0.1) are unwrapped first so they can't
170+
be used to smuggle a private IPv4 target past the check.
171+
"""
172+
mapped = getattr(ip, "ipv4_mapped", None)
173+
if mapped is not None:
174+
ip = mapped
175+
return (
176+
ip.is_private
177+
or ip.is_loopback
178+
or ip.is_link_local
179+
or ip.is_reserved
180+
or ip.is_multicast
181+
or ip.is_unspecified
182+
)
183+
184+
185+
def validate_public_url(url: str) -> None:
186+
"""Validate that `url` is safe to fetch from the server (SSRF guard).
187+
188+
Raises BlockedURLError if the URL is not http(s), has no host, or resolves
189+
to any non-public address. A literal-IP host is checked directly; a
190+
hostname is resolved and every returned address must be public.
191+
"""
192+
if not url or not isinstance(url, str):
193+
raise BlockedURLError("No URL provided")
194+
195+
parsed = urlparse(url.strip())
196+
scheme = (parsed.scheme or "").lower()
197+
if scheme not in _ALLOWED_URL_SCHEMES:
198+
raise BlockedURLError(
199+
f"URL scheme '{parsed.scheme}' is not allowed; "
200+
"only http and https URLs can be fetched"
201+
)
202+
203+
host = parsed.hostname
204+
if not host:
205+
raise BlockedURLError("URL has no host")
206+
207+
try:
208+
candidate_ips = [ipaddress.ip_address(host)]
209+
except ValueError:
210+
# Not a literal IP — resolve the hostname and check every address.
211+
try:
212+
infos = socket.getaddrinfo(host, None)
213+
except socket.gaierror as e:
214+
raise BlockedURLError(f"Could not resolve host '{host}': {e}")
215+
candidate_ips = []
216+
for info in infos:
217+
ip_text = info[4][0].split("%")[0] # strip any IPv6 scope id
218+
try:
219+
candidate_ips.append(ipaddress.ip_address(ip_text))
220+
except ValueError:
221+
continue
222+
if not candidate_ips:
223+
raise BlockedURLError(f"Could not resolve host '{host}' to any IP address")
224+
225+
for ip in candidate_ips:
226+
if _ip_is_disallowed(ip):
227+
raise BlockedURLError(
228+
f"Refusing to fetch '{host}': it resolves to a non-public address "
229+
f"({ip}). Private, loopback, link-local, and cloud-metadata "
230+
"addresses are blocked to prevent server-side request forgery."
231+
)
232+
233+
234+
async def _ssrf_guard_request_hook(request: "httpx.Request") -> None:
235+
"""httpx request event hook that re-validates every outbound request.
236+
237+
Fires for the initial request and for each redirect hop, so a public URL
238+
cannot redirect into a private/internal address.
239+
"""
240+
validate_public_url(str(request.url))
241+
242+
141243
async def download_image(url: str) -> Optional[bytes]:
142244
"""
143245
Download an image from a URL.
144-
246+
145247
Args:
146248
url: Image URL
147-
249+
148250
Returns:
149251
Image data as bytes if successful, None otherwise
150252
"""
253+
# SSRF guard: refuse non-public targets before opening any connection.
254+
try:
255+
validate_public_url(url)
256+
except BlockedURLError as e:
257+
logger.warning("Refusing to download image from disallowed URL: %s", e)
258+
print(f"Refusing to download image from disallowed URL: {e}")
259+
return None
260+
151261
try:
152262
print(f"Attempting to download image from URL: {url}")
153-
263+
154264
# Use minimal headers like curl does
155265
headers = {
156266
"User-Agent": "curl/8.4.0",
157267
"Accept": "*/*"
158268
}
159-
160-
async with httpx.AsyncClient(follow_redirects=True, timeout=30.0) as client:
269+
270+
async with httpx.AsyncClient(
271+
follow_redirects=True,
272+
timeout=30.0,
273+
event_hooks={"request": [_ssrf_guard_request_hook]},
274+
) as client:
161275
# Simple GET request just like curl
162276
response = await client.get(url, headers=headers)
163-
277+
164278
# Check response
165279
if response.status_code == 200:
166280
print(f"Successfully downloaded image: {len(response.content)} bytes")
167281
return response.content
168282
else:
169283
print(f"Failed to download image: HTTP {response.status_code}")
170284
return None
171-
285+
286+
except BlockedURLError as e:
287+
# A redirect pointed at a disallowed (non-public) address.
288+
logger.warning("Blocked SSRF redirect during image download: %s", e)
289+
print(f"Blocked image download (redirect to disallowed address): {e}")
290+
return None
172291
except httpx.HTTPStatusError as e:
173292
print(f"HTTP Error when downloading image: {e}")
174293
return None
@@ -189,33 +308,48 @@ async def try_multiple_download_methods(url: str) -> Optional[bytes]:
189308
190309
Returns:
191310
Image data as bytes if successful, None otherwise
311+
312+
Raises:
313+
BlockedURLError: if `url` targets a non-public address (SSRF guard),
314+
raised up-front so callers can surface a clear rejection message.
192315
"""
316+
# SSRF guard: validate once up-front and propagate a clear error. Each
317+
# client below also re-validates every request (including redirect hops)
318+
# via _ssrf_guard_request_hook, so a public URL cannot redirect inward.
319+
validate_public_url(url)
320+
193321
# Method 1: Direct download with custom headers
194322
image_data = await download_image(url)
195323
if image_data:
196324
return image_data
197-
325+
198326
print("Direct download failed, trying alternative methods...")
199-
327+
200328
# Method 2: Try adding Facebook cookie simulation
201329
try:
202330
headers = {
203331
"User-Agent": "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.124 Safari/537.36",
204332
"Accept": "image/webp,image/apng,image/*,*/*;q=0.8",
205333
"Cookie": "presence=EDvF3EtimeF1697900316EuserFA21B00112233445566AA0EstateFDutF0CEchF_7bCC" # Fake cookie
206334
}
207-
208-
async with httpx.AsyncClient(follow_redirects=True) as client:
335+
336+
async with httpx.AsyncClient(
337+
follow_redirects=True,
338+
event_hooks={"request": [_ssrf_guard_request_hook]},
339+
) as client:
209340
response = await client.get(url, headers=headers, timeout=30.0)
210341
response.raise_for_status()
211342
print(f"Method 2 succeeded with cookie simulation: {len(response.content)} bytes")
212343
return response.content
213344
except Exception as e:
214345
print(f"Method 2 failed: {str(e)}")
215-
346+
216347
# Method 3: Try with session that keeps redirects and cookies
217348
try:
218-
async with httpx.AsyncClient(follow_redirects=True) as client:
349+
async with httpx.AsyncClient(
350+
follow_redirects=True,
351+
event_hooks={"request": [_ssrf_guard_request_hook]},
352+
) as client:
219353
# First visit Facebook to get cookies
220354
await client.get("https://www.facebook.com/", timeout=30.0)
221355
# Then try the image URL
@@ -225,7 +359,7 @@ async def try_multiple_download_methods(url: str) -> Optional[bytes]:
225359
return response.content
226360
except Exception as e:
227361
print(f"Method 3 failed: {str(e)}")
228-
362+
229363
return None
230364

231365

‎pyproject.toml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,11 @@ classifiers = [
2121
dependencies = [
2222
"httpx>=0.26.0",
2323
"mcp[cli]==1.12.2",
24+
# Transitive via mcp (which floors it at >=0.27, so this only raises it).
25+
# Patched floor for CVE-2026-48710 (BadHost), vulnerable in starlette <= 1.0.0.
26+
# Only reachable via the streamable-http transport, which run-mcp.sh does not
27+
# use — hygiene, not a live exposure. Ported from upstream 93083c6 (#136).
28+
"starlette>=1.0.1",
2429
"python-dotenv>=1.1.0",
2530
"requests>=2.32.3",
2631
"Pillow>=10.0.0",

‎requirements.txt‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
httpx>=0.26.0
22
mcp[cli]==1.12.2
3+
# Transitive via mcp; patched floor for CVE-2026-48710 (BadHost), vulnerable in
4+
# starlette <= 1.0.0. Only reachable via the streamable-http transport.
5+
starlette>=1.0.1
36
python-dotenv>=1.1.0
47
requests>=2.32.3
58
Pillow>=10.0.0

‎tests/test_api_token_leak.py‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,3 +40,106 @@ def test_redact_preserves_other_params(self):
4040
assert "account_id=123" in redacted
4141
assert "limit=10" in redacted
4242
assert "access_token=" not in redacted
43+
44+
45+
class TestCallerParamsNotMutated:
46+
"""make_api_request must not inject credentials into the caller's dict.
47+
48+
Ported from upstream pipeboard-co/meta-ads-mcp 555ae48 (PR #145). The
49+
function took params by reference and then set request_params["access_token"],
50+
so the caller's own dict came back carrying the live Meta token. Any tool
51+
that echoes its params to the user *after* the call then leaked it — in this
52+
fork that is update_ad_creative, which returns "attempted_updates" on Meta
53+
error subcode 1815573 and "update_data_sent" from its exception handler
54+
(core/ads.py), both of which echo the same dict handed to make_api_request.
55+
"""
56+
57+
TOKEN = "LEAKED_IF_MUTATED_0123456789"
58+
59+
@staticmethod
60+
def _mock_client(captured):
61+
"""AsyncClient stand-in that records the params it was handed."""
62+
from unittest.mock import AsyncMock, MagicMock
63+
64+
response = MagicMock()
65+
response.status_code = 200
66+
response.json.return_value = {"data": []}
67+
response.raise_for_status.return_value = None
68+
69+
client = MagicMock()
70+
for verb in ("get", "delete"):
71+
setattr(client, verb, AsyncMock(
72+
side_effect=lambda *a, **kw: captured.update(kw.get("params") or {}) or response
73+
))
74+
client.post = AsyncMock(
75+
side_effect=lambda *a, **kw: captured.update(kw.get("data") or {}) or response
76+
)
77+
78+
ctx = MagicMock()
79+
ctx.__aenter__ = AsyncMock(return_value=client)
80+
ctx.__aexit__ = AsyncMock(return_value=False)
81+
return MagicMock(return_value=ctx)
82+
83+
async def test_get_does_not_inject_token_into_callers_params(self):
84+
from unittest.mock import patch
85+
from meta_ads_mcp.core.api import make_api_request
86+
87+
caller_params = {"fields": "id,name"}
88+
captured = {}
89+
with patch("meta_ads_mcp.core.api.httpx.AsyncClient", self._mock_client(captured)):
90+
await make_api_request("act_123/ads", self.TOKEN, caller_params)
91+
92+
assert caller_params == {"fields": "id,name"}
93+
assert "access_token" not in caller_params
94+
assert self.TOKEN not in str(caller_params)
95+
96+
async def test_post_does_not_inject_token_into_callers_params(self):
97+
from unittest.mock import patch
98+
from meta_ads_mcp.core.api import make_api_request
99+
100+
# Mirrors update_ad_creative's shape: the dict it later echoes back as
101+
# "attempted_updates" / "update_data_sent".
102+
update_data = {"name": "New creative name"}
103+
captured = {}
104+
with patch("meta_ads_mcp.core.api.httpx.AsyncClient", self._mock_client(captured)):
105+
await make_api_request("123456/", self.TOKEN, update_data, method="POST")
106+
107+
assert update_data == {"name": "New creative name"}
108+
assert "access_token" not in update_data
109+
110+
async def test_token_still_reaches_the_wire(self):
111+
"""Anchor: the fix must not stop sending the token, only stop mutating."""
112+
from unittest.mock import patch
113+
from meta_ads_mcp.core.api import make_api_request
114+
115+
caller_params = {"fields": "id,name"}
116+
captured = {}
117+
with patch("meta_ads_mcp.core.api.httpx.AsyncClient", self._mock_client(captured)):
118+
await make_api_request("act_123/ads", self.TOKEN, caller_params)
119+
120+
assert captured.get("access_token") == self.TOKEN
121+
assert captured.get("fields") == "id,name"
122+
123+
async def test_none_params_is_still_accepted(self):
124+
from unittest.mock import patch
125+
from meta_ads_mcp.core.api import make_api_request
126+
127+
captured = {}
128+
with patch("meta_ads_mcp.core.api.httpx.AsyncClient", self._mock_client(captured)):
129+
result = await make_api_request("act_123/ads", self.TOKEN, None)
130+
131+
assert result == {"data": []}
132+
assert captured.get("access_token") == self.TOKEN
133+
134+
async def test_same_dict_reused_across_two_calls_stays_clean(self):
135+
"""A caller that reuses one params dict must not accumulate credentials."""
136+
from unittest.mock import patch
137+
from meta_ads_mcp.core.api import make_api_request
138+
139+
caller_params = {"fields": "id"}
140+
captured = {}
141+
with patch("meta_ads_mcp.core.api.httpx.AsyncClient", self._mock_client(captured)):
142+
await make_api_request("act_123/ads", self.TOKEN, caller_params)
143+
await make_api_request("act_123/adsets", "SECOND_TOKEN_VALUE", caller_params)
144+
145+
assert caller_params == {"fields": "id"}

0 commit comments

Comments
 (0)