Skip to content

Commit 460ccb6

Browse files
fix(security): stop tool output leaking the Meta access token; remove broken capi diagnostics tool
Graph paged responses carry paging.next/previous URLs with the live access_token in the query string; only error payloads were scrubbed, so successful responses (e.g. get_pixel_events) echoed the system-user token to callers. - make_api_request strips paging.next/previous at any depth (cursors kept) - meta_api_tool redacts any access_token=/appsecret_proof= value in every string result as a last line of defence - get_login_link no longer shows a 10-char prefix of the cached token - remove get_capi_diagnostics: Meta no longer serves /{pixel_id}/server_events_diagnostic (Graph v25: 400 code 2500) - insights help text points at paging.cursors.after - README tool count 84 -> 83, CLAUDE.md notes both changes - tests/test_response_token_scrub.py (13 tests); suite 557 passed Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtL9JLvfuUcv4v3Y7YrNcM
1 parent bedb666 commit 460ccb6

7 files changed

Lines changed: 258 additions & 32 deletions

File tree

‎CLAUDE.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,19 @@ Adding a new file to `meta_ads_mcp/core/` with `@mcp_server.tool()`-decorated fu
1919

2020
A module can be present in the repo, pass CI, and ship in every built Docker image while being 100% invisible as a callable tool — because CI/tests only import the module directly, they don't exercise the `server.py` HTTP-transport startup path the same way the deployed server does. **Always verify via a live `tools/list` call after registering a new module, not just a passing test suite.**
2121

22+
## Tool output must never carry the access token
23+
24+
Graph list responses include `paging.next` / `paging.previous` URLs with `access_token=<live system-user token>` in the query string. Before 2026-10-02 only *error* payloads were scrubbed, so any paged success response (real case: `get_pixel_events`) echoed the never-expiring token into the caller's transcript. Two layers now prevent it, both in `meta_ads_mcp/core/api.py`:
25+
26+
1. `make_api_request` runs `_strip_paging_links` on every JSON response — drops `paging.next`/`paging.previous` at any depth, keeps `paging.cursors` (paginate with `after`, never by following a URL).
27+
2. `meta_api_tool` runs `_redact_secret_params` over every string result — masks any `access_token=` / `appsecret_proof=` value, whatever field it sits in.
28+
29+
A new tool that bypasses `meta_api_tool` or builds its own HTTP call loses layer 2 — keep using the decorator. Regression tests: `tests/test_response_token_scrub.py`.
30+
31+
## Removed upstream tools
32+
33+
`get_capi_diagnostics` was removed 2026-10-02: Meta no longer serves `/{pixel_id}/server_events_diagnostic` (Graph v25 returns 400 code 2500 "Unknown path components"). If upstream's sync reintroduces it, drop it again — it can only error.
34+
2235
## GHCR permissions can silently break on a GitHub org transfer
2336

2437
See `pmin-brain/context/tool-gotchas.md` § GitHub for the full incident writeup. Short version: this repo (and others) transferred from the `PM-Labs` org to `pmlabs-org`. The git remote still resolves (`git push` to the old `PM-Labs/...` URL succeeds via GitHub's redirect), but the GHCR container package's own "Manage Actions access" grant did not carry over — CI's `docker push` step 403'd on every run for about a week (2026-08-05 to 2026-08-11) despite correct `permissions: packages: write` in the workflow YAML. A separate `Test and Build` workflow (tests only, no push) kept reporting green the whole time, so the failure went unnoticed until someone checked the `deploy` job's logs specifically.

‎README.md‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ For advanced users who need to self-host, the package can be installed from sour
201201

202202
### Available MCP Tools
203203

204-
**84 tools** across 30 categories. Quick reference:
204+
**83 tools** across 30 categories. Quick reference:
205205

206206
| Category | Tools |
207207
|---|---|
@@ -220,7 +220,7 @@ For advanced users who need to self-host, the package can be installed from sour
220220
| [Other](#other) | `fetch` (record cache lookup, see [Search](#search)) |
221221
| [Custom Audiences](#custom-audiences) | `create_custom_audience`, `get_custom_audiences`, `update_custom_audience`, `delete_custom_audience`, `add_users_to_custom_audience` |
222222
| [Lookalike Audiences](#lookalike-audiences) | `create_lookalike_audience`, `get_lookalike_audience_status` |
223-
| [Conversion API (CAPI)](#conversion-api-capi) | `send_conversion_events`, `get_capi_diagnostics` |
223+
| [Conversion API (CAPI)](#conversion-api-capi) | `send_conversion_events` |
224224
| [Custom Conversions](#custom-conversions) | `create_custom_conversion`, `get_custom_conversions` |
225225
| [Advanced Insights](#advanced-insights) | `get_insights_with_breakdowns`, `create_async_insights_report`, `get_async_insights_report` |
226226
| [Automated Rules](#automated-rules) | `create_automated_rule`, `get_automated_rules`, `get_rule_execution_history` |
@@ -333,7 +333,6 @@ All write tools (`create_*`, `update_*`) default new objects to `status: PAUSED`
333333
#### Conversion API (CAPI)
334334

335335
- **`send_conversion_events`** — Send server-side conversion events via Meta's Conversions API. Inputs: `pixel_id`, `events` (list — each with `event_name`, `event_time`, `user_data`, optional `custom_data`/`event_source_url`/`action_source`), `test_event_code`. PII fields in `user_data` (`em`, `ph`, `fn`, `ln`, `ct`, `st`, `zp`, `country`, `ge`, `db`, `external_id`) are SHA-256 hashed automatically if not already hashed.
336-
- **`get_capi_diagnostics`** — Event quality/deduplication diagnostics for a pixel's server-side events. Input: `pixel_id`.
337336

338337
#### Custom Conversions
339338

‎meta_ads_mcp/core/api.py‎

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import asyncio
99
import functools
1010
import os
11+
import re
1112
from urllib.parse import urlsplit, urlunsplit, parse_qsl, urlencode
1213
from . import auth
1314
from .auth import needs_authentication, auth_manager, start_callback_server, shutdown_callback_server
@@ -42,6 +43,42 @@ def _redact_url(url: str) -> str:
4243
# Be conservative: if parsing fails, drop the query string entirely.
4344
return url.split("?", 1)[0]
4445

46+
47+
# Graph list responses carry `paging.next` / `paging.previous` URLs with
48+
# `access_token=<live token>` in the query string. _redact_url only covers
49+
# error payloads, so successful paged responses leaked the operator token
50+
# into the caller's transcript. Two layers close that: _strip_paging_links
51+
# (at the source, in make_api_request) and _redact_secret_params (a final
52+
# safety net over every tool's output, in meta_api_tool).
53+
_PAGING_LINK_KEYS = ("next", "previous")
54+
_SECRET_PARAM_RE = re.compile(
55+
r"(" + "|".join(re.escape(p) for p in sorted(_SENSITIVE_QUERY_PARAMS)) + r")=[^&\s\"'\\]+"
56+
)
57+
58+
59+
def _strip_paging_links(obj: Any) -> Any:
60+
"""Drop `paging.next` / `paging.previous` at any depth.
61+
62+
Only keys inside a `paging` object are removed, so a data field that is
63+
merely named `next` is untouched. `paging.cursors` stays, so callers can
64+
still paginate by passing the `after` cursor.
65+
"""
66+
if isinstance(obj, dict):
67+
cleaned = {k: _strip_paging_links(v) for k, v in obj.items()}
68+
paging = cleaned.get("paging")
69+
if isinstance(paging, dict):
70+
cleaned["paging"] = {k: v for k, v in paging.items() if k not in _PAGING_LINK_KEYS}
71+
return cleaned
72+
if isinstance(obj, list):
73+
return [_strip_paging_links(v) for v in obj]
74+
return obj
75+
76+
77+
def _redact_secret_params(text: str) -> str:
78+
"""Mask `access_token=` / `appsecret_proof=` query values anywhere in text."""
79+
return _SECRET_PARAM_RE.sub(r"\1=REDACTED", text)
80+
81+
4582
class McpToolError(Exception):
4683
"""Base class for MCP tool errors that must set isError: true.
4784
@@ -281,7 +318,7 @@ async def make_api_request(
281318

282319
# Ensure the response is JSON and return it as a dictionary
283320
try:
284-
return response.json()
321+
return _strip_paging_links(response.json())
285322
except json.JSONDecodeError:
286323
# If not JSON, return text content in a structured format
287324
return {
@@ -411,8 +448,7 @@ async def make_api_request(
411448
# Generic wrapper for all Meta API tools
412449
def meta_api_tool(func):
413450
"""Decorator for Meta API tools that handles authentication and error handling."""
414-
@functools.wraps(func)
415-
async def wrapper(*args, **kwargs):
451+
async def _guarded(*args, **kwargs):
416452
try:
417453
# Log function call
418454
logger.debug(f"Function call: {func.__name__}")
@@ -542,4 +578,13 @@ async def wrapper(*args, **kwargs):
542578
logger.error(f"Error in {func.__name__}: {str(e)}")
543579
return json.dumps({"error": str(e)}, indent=2)
544580

581+
@functools.wraps(func)
582+
async def wrapper(*args, **kwargs):
583+
result = await _guarded(*args, **kwargs)
584+
# Last line of defence: no tool output may carry the access token or
585+
# appsecret_proof, whichever field a Graph URL ends up in.
586+
if isinstance(result, str):
587+
return _redact_secret_params(result)
588+
return result
589+
545590
return wrapper

‎meta_ads_mcp/core/authentication.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ async def get_login_link(access_token: Optional[str] = None) -> str:
9292
return json.dumps({
9393
"message": "✅ Already Authenticated",
9494
"status": "You're successfully authenticated with Meta Ads!",
95-
"token_info": f"Token preview: {cached_token[:10]}...",
95+
"token_info": "A token is cached (value hidden).",
9696
"created_at": auth_manager.token_info.created_at if hasattr(auth_manager, "token_info") else None,
9797
"expires_in": auth_manager.token_info.expires_in if hasattr(auth_manager, "token_info") else None,
9898
"authentication_method": "meta_oauth",

‎meta_ads_mcp/core/conversions.py‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -84,30 +84,6 @@ async def send_conversion_events(
8484
return json.dumps({"error": "Failed to send conversion events", "details": str(e)}, indent=2)
8585

8686

87-
@mcp_server.tool()
88-
@meta_api_tool
89-
async def get_capi_diagnostics(
90-
pixel_id: str,
91-
access_token: Optional[str] = None,
92-
) -> str:
93-
"""
94-
Get Conversions API diagnostics for a pixel to check event quality and deduplication.
95-
96-
Args:
97-
pixel_id: Meta Pixel / Dataset ID
98-
access_token: Meta API access token (optional - will use cached token if not provided)
99-
"""
100-
if not pixel_id:
101-
return json.dumps({"error": "No pixel ID provided"}, indent=2)
102-
103-
endpoint = f"{pixel_id}/server_events_diagnostic"
104-
params = {
105-
"fields": "diagnostics",
106-
}
107-
data = await make_api_request(endpoint, access_token, params)
108-
return json.dumps(data, indent=2)
109-
110-
11187
@mcp_server.tool()
11288
@meta_api_tool
11389
async def create_custom_conversion(

‎meta_ads_mcp/core/insights.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ async def get_insights(object_id: str = "", access_token: Optional[str] = None,
117117
comscore_market_code
118118
level: Level of aggregation (ad, adset, campaign, account)
119119
limit: Maximum number of results to return per page (default: 25, Meta API allows much higher values)
120-
after: Pagination cursor to get the next set of results. Use the 'after' cursor from previous response's paging.next field.
120+
after: Pagination cursor to get the next set of results. Use the 'after' cursor from previous response's paging.cursors.after field.
121121
action_attribution_windows: Optional list of attribution windows (e.g., ["1d_click", "7d_click", "1d_view"]).
122122
When specified, actions include additional fields for each window. The 'value' field always shows 7d_click.
123123
action_breakdowns: Optional list of action_breakdowns to apply to action-typed metrics. Pass [] to disable

‎tests/test_response_token_scrub.py‎

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
#!/usr/bin/env python3
2+
"""
3+
Regression tests: successful tool responses must never carry the access token.
4+
5+
Graph API list responses include `paging.next` / `paging.previous` URLs with
6+
`access_token=<live token>` (and `appsecret_proof`) embedded in the query
7+
string. Error responses were already scrubbed via _redact_url; successful
8+
responses were passed straight through, so any tool that returned a paged
9+
Graph response (real case: get_pixel_events) echoed the operator's
10+
never-expiring system-user token into the caller's transcript.
11+
12+
Two layers are covered:
13+
1. make_api_request drops paging.next / paging.previous at any depth
14+
(cursors stay, so callers can still paginate with `after`).
15+
2. meta_api_tool redacts any leftover access_token= / appsecret_proof=
16+
query values in the final output, whatever field they sit in.
17+
"""
18+
19+
import json
20+
21+
import pytest
22+
from unittest.mock import AsyncMock, MagicMock, patch
23+
24+
from meta_ads_mcp.core.api import (
25+
_redact_secret_params,
26+
_strip_paging_links,
27+
make_api_request,
28+
meta_api_tool,
29+
)
30+
31+
TOKEN = "EAAFAKETOKEN0123456789abcdefXYZ"
32+
PROOF = "deadbeefcafe0123456789"
33+
LEAKY_NEXT = f"https://graph.facebook.com/v25.0/123/stats?access_token={TOKEN}&limit=25&after=abc"
34+
LEAKY_PREV = f"https://graph.facebook.com/v25.0/123/stats?access_token={TOKEN}&appsecret_proof={PROOF}&before=xyz"
35+
36+
37+
def _paged_payload():
38+
return {
39+
"data": [{"value": "PageView", "count": 14}],
40+
"paging": {
41+
"cursors": {"before": "B", "after": "A"},
42+
"next": LEAKY_NEXT,
43+
"previous": LEAKY_PREV,
44+
},
45+
}
46+
47+
48+
# --- Layer 1: _strip_paging_links ------------------------------------------
49+
50+
def test_strip_removes_next_and_previous_keeps_cursors():
51+
cleaned = _strip_paging_links(_paged_payload())
52+
assert cleaned["paging"] == {"cursors": {"before": "B", "after": "A"}}
53+
assert cleaned["data"] == [{"value": "PageView", "count": 14}]
54+
55+
56+
def test_strip_works_at_any_depth():
57+
"""Field-expansion responses nest paging inside objects and lists."""
58+
payload = {
59+
"id": "act_1",
60+
"adspixels": _paged_payload(),
61+
"list": [{"inner": _paged_payload()}],
62+
}
63+
text = json.dumps(_strip_paging_links(payload))
64+
assert TOKEN not in text
65+
assert PROOF not in text
66+
assert text.count('"after": "A"') == 2 # cursors survive in both nested spots
67+
68+
69+
def test_strip_leaves_non_paging_next_keys_alone():
70+
"""Only `next`/`previous` inside a `paging` object are links; a field that
71+
merely happens to be called `next` elsewhere is real data."""
72+
payload = {"next": "keep-me", "data": [{"previous": "keep-me-too"}]}
73+
assert _strip_paging_links(payload) == payload
74+
75+
76+
def test_strip_handles_scalars_and_empty_paging():
77+
assert _strip_paging_links("text") == "text"
78+
assert _strip_paging_links(None) is None
79+
assert _strip_paging_links({"paging": {}}) == {"paging": {}}
80+
assert _strip_paging_links({"paging": "not-a-dict"}) == {"paging": "not-a-dict"}
81+
82+
83+
# --- Layer 1 end-to-end: make_api_request ----------------------------------
84+
85+
@pytest.fixture
86+
def mock_httpx_client():
87+
with patch("meta_ads_mcp.core.api.httpx.AsyncClient") as mock_client_cls:
88+
client = MagicMock()
89+
response = MagicMock()
90+
response.status_code = 200
91+
response.headers = {}
92+
response.json.return_value = _paged_payload()
93+
response.raise_for_status.return_value = None
94+
client.get = AsyncMock(return_value=response)
95+
mock_client_cls.return_value.__aenter__ = AsyncMock(return_value=client)
96+
mock_client_cls.return_value.__aexit__ = AsyncMock(return_value=False)
97+
yield client
98+
99+
100+
@pytest.mark.asyncio
101+
async def test_make_api_request_strips_paging_links(mock_httpx_client):
102+
result = await make_api_request("123/stats", "SECRET_TOKEN", {})
103+
assert "next" not in result["paging"]
104+
assert "previous" not in result["paging"]
105+
assert result["paging"]["cursors"]["after"] == "A"
106+
assert TOKEN not in json.dumps(result)
107+
108+
109+
# --- Layer 2: _redact_secret_params ----------------------------------------
110+
111+
def test_redact_masks_token_and_proof_values():
112+
text = f'{{"u": "https://x/y?access_token={TOKEN}&limit=5&appsecret_proof={PROOF}"}}'
113+
out = _redact_secret_params(text)
114+
assert TOKEN not in out and PROOF not in out
115+
assert "access_token=REDACTED" in out
116+
assert "appsecret_proof=REDACTED" in out
117+
assert "limit=5" in out # unrelated params untouched
118+
119+
120+
def test_redact_handles_value_at_end_and_escaped_json():
121+
assert _redact_secret_params(f"?access_token={TOKEN}") == "?access_token=REDACTED"
122+
escaped = json.dumps(f"https://x/y?a=1&access_token={TOKEN}")
123+
assert TOKEN not in _redact_secret_params(escaped)
124+
125+
126+
def test_redact_leaves_clean_text_unchanged():
127+
clean = '{"data": [{"name": "access_token_rotation_policy"}], "limit": 25}'
128+
assert _redact_secret_params(clean) == clean
129+
130+
131+
# --- Layer 2 end-to-end: meta_api_tool wrapper -----------------------------
132+
133+
@pytest.mark.asyncio
134+
async def test_wrapper_redacts_token_in_any_string_field():
135+
@meta_api_tool
136+
async def fake_tool(access_token=None):
137+
# A leak in a field the paging-strip layer knows nothing about.
138+
return json.dumps({"preview_url": f"https://x/y?access_token={TOKEN}"})
139+
140+
out = await fake_tool(access_token="SECRET_TOKEN")
141+
assert TOKEN not in out
142+
assert "access_token=REDACTED" in out
143+
144+
145+
@pytest.mark.asyncio
146+
async def test_wrapper_redacts_token_in_dict_results():
147+
@meta_api_tool
148+
async def fake_tool(access_token=None):
149+
return {"link": f"https://x/y?access_token={TOKEN}"}
150+
151+
out = await fake_tool(access_token="SECRET_TOKEN")
152+
assert TOKEN not in out
153+
154+
155+
@pytest.mark.asyncio
156+
async def test_wrapper_passes_clean_output_through_unchanged():
157+
payload = json.dumps({"data": [1, 2, 3]}, indent=2)
158+
159+
@meta_api_tool
160+
async def fake_tool(access_token=None):
161+
return payload
162+
163+
assert await fake_tool(access_token="SECRET_TOKEN") == payload
164+
165+
166+
# --- get_login_link must not show any part of the cached token --------------
167+
168+
@pytest.mark.asyncio
169+
async def test_login_link_never_shows_token_prefix(monkeypatch):
170+
"""It used to return `Token preview: <first 10 chars>...` — a partial
171+
disclosure of the live token. Now it only says that a token is cached."""
172+
from meta_ads_mcp.core import authentication
173+
174+
monkeypatch.delenv("META_ADS_DISABLE_CALLBACK_SERVER", raising=False)
175+
token_info = MagicMock(created_at=1, expires_in=2)
176+
with patch.object(authentication.auth_manager, "get_access_token", return_value=TOKEN), \
177+
patch.object(authentication.auth_manager, "token_info", token_info, create=True):
178+
out = await authentication.get_login_link()
179+
180+
assert "Already Authenticated" in out
181+
assert TOKEN[:10] not in out
182+
assert "Token preview" not in out
183+
184+
185+
# --- Removed tool -----------------------------------------------------------
186+
187+
def test_get_capi_diagnostics_is_gone():
188+
"""Meta no longer serves /server_events_diagnostic (Graph v25 returns
189+
400 code 2500 'Unknown path components'), so the tool was removed."""
190+
from meta_ads_mcp.core import conversions
191+
192+
assert not hasattr(conversions, "get_capi_diagnostics")
193+
assert hasattr(conversions, "send_conversion_events") # sibling untouched

0 commit comments

Comments
 (0)