diff --git a/SECURITY.md b/SECURITY.md index 7144521f..37c68ae6 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -258,3 +258,46 @@ an empty list) should use `get_ad_image` / `get_ad_creatives` instead. - Credited to [@Gal3m](https://github.com/Gal3m) and [@mohammad228](https://github.com/mohammad228). + +### GHSA-75j5-qp3x-mx37 — OAuth callback server: reflected XSS, `/token` disclosure, and missing CSRF `state` + +- **Severity:** Medium +- **Affected versions:** `<= 1.0.120` for the XSS and `/token` disclosure; + `<= 1.0.123` for the missing `state` check. +- **Fixed in:** `1.0.121` (XSS, `/token`) and `1.0.124` (`state`, bind address, + server lifetime). +- **Affected configurations:** Installs that run the local OAuth login flow + (`--login`, the `get_login_link` tool, or `authenticate()`), which listens on + `127.0.0.1:8080-8089` during the authorization window. The hosted MCP at + `*.mcp.pipeboard.co` does not run the callback server. + +**What went wrong.** Three weaknesses in `callback_server.py` were reported +together as a credential-theft chain: + +1. The `error` query parameter was reflected into the HTML error page without + escaping — fixed in `1.0.121`, tracked as GHSA-6v2r-2m4r-768m. +2. `GET /token` returned the stored OAuth authorization artifact to any + same-origin page — the endpoint was removed in `1.0.121`. +3. `get_auth_url` emitted no `state` parameter and the callback validated none + (RFC 6749 §10.12), so a page that could reach the callback server could hand + it an authorization code of the attacker's choosing. + +**Fix for item 3 (`1.0.124`).** A cryptographically random `state` is minted per +authorization request, carried in the authorization URL, and required on the +callback: a code whose `state` is missing, mismatched, or replayed is rejected +and never stored. The state is single-use. The server now binds `127.0.0.1` +explicitly rather than whatever `localhost` resolves to, and shuts down as soon +as it has handled a valid callback instead of idling out the 180-second window. + +**On the report's fourth suggestion (random high port).** The callback port +cannot be randomized: Meta validates the redirect URI against the list +registered on the app, so the flow only works on pre-registered ports. The +server stays on `8080-8089` by necessity; the `state` check, not port secrecy, +is what makes an unsolicited callback useless. + +**Action for operators.** +- Upgrade to `1.0.124` or later. +- If you completed an interactive login on a machine where an untrusted page may + have been open, reconnect the Meta account so a fresh token is issued. +- Credited to [@Gal3m](https://github.com/Gal3m) and + [@mohammad228](https://github.com/mohammad228). diff --git a/meta_ads_mcp/__init__.py b/meta_ads_mcp/__init__.py index eecfbe9b..d014a8ed 100644 --- a/meta_ads_mcp/__init__.py +++ b/meta_ads_mcp/__init__.py @@ -6,7 +6,7 @@ from meta_ads_mcp.core.server import main -__version__ = "1.0.123" +__version__ = "1.0.124" __all__ = [ 'get_ad_accounts', diff --git a/meta_ads_mcp/core/auth.py b/meta_ads_mcp/core/auth.py index 75bca3b6..31ea2bc7 100644 --- a/meta_ads_mcp/core/auth.py +++ b/meta_ads_mcp/core/auth.py @@ -16,7 +16,9 @@ start_callback_server, shutdown_callback_server, token_container, - callback_server_port + callback_server_port, + get_oauth_state, + new_oauth_state ) # Auth constants @@ -219,12 +221,18 @@ def _save_token_to_cache(self) -> None: def get_auth_url(self) -> str: """Generate the Facebook OAuth URL for desktop app flow""" + # The callback only accepts a redirect carrying this state back, so the + # URL must not be built without one. start_callback_server() mints a + # fresh state per flow; mint here too for callers that print the URL + # without starting the server. + state = get_oauth_state() or new_oauth_state() return ( f"https://www.facebook.com/v24.0/dialog/oauth?" f"client_id={self.app_id}&" f"redirect_uri={self.redirect_uri}&" f"scope={AUTH_SCOPE}&" - f"response_type={AUTH_RESPONSE_TYPE}" + f"response_type={AUTH_RESPONSE_TYPE}&" + f"state={state}" ) def authenticate(self, force_refresh: bool = False) -> Optional[str]: diff --git a/meta_ads_mcp/core/callback_server.py b/meta_ads_mcp/core/callback_server.py index 5784557b..676f369e 100644 --- a/meta_ads_mcp/core/callback_server.py +++ b/meta_ads_mcp/core/callback_server.py @@ -28,10 +28,52 @@ # Timeout in seconds before shutting down the callback server CALLBACK_SERVER_TIMEOUT = 180 # 3 minutes timeout +# CSRF state for the authorization request currently in flight (RFC 6749 §10.12). +# The callback only accepts a redirect that echoes back the state we minted for +# the flow we started, so a page that navigates the browser to /callback with an +# attacker-supplied code cannot bind the attacker's Meta account to this install. +_oauth_state = None +_oauth_state_lock = threading.Lock() + +# The redirect URI names localhost, so bind the loopback interface explicitly +# rather than whatever "localhost" resolves to on this host. +CALLBACK_SERVER_HOST = "127.0.0.1" + # Maximum number of characters of an OAuth error echoed back to the browser MAX_ERROR_DISPLAY_LENGTH = 200 +def new_oauth_state() -> str: + """Mint and store the state parameter for a new authorization request.""" + global _oauth_state + with _oauth_state_lock: + _oauth_state = secrets.token_urlsafe(32) + return _oauth_state + + +def get_oauth_state() -> Optional[str]: + """Return the state of the authorization request in flight, if any.""" + with _oauth_state_lock: + return _oauth_state + + +def consume_oauth_state(received: Optional[str]) -> bool: + """Check the state echoed by a callback and burn it, so it is single-use. + + Returns False when no flow is in flight, when the callback carries no state, + or when the values differ. + """ + global _oauth_state + with _oauth_state_lock: + expected = _oauth_state + if not expected or not received: + return False + if not secrets.compare_digest(expected, received): + return False + _oauth_state = None + return True + + class CallbackHandler(BaseHTTPRequestHandler): def do_GET(self): try: @@ -92,6 +134,23 @@ def _handle_oauth_callback(self): """ logger.error(f"OAuth authorization failed: {error}") + elif code and not consume_oauth_state(state): + # The redirect does not belong to an authorization request this + # process started, so the code it carries is not ours to store. + logger.warning( + "OAuth callback rejected: state parameter is missing or does not " + "match the authorization request in flight" + ) + html = """ + + Authorization Rejected + +

Authorization Rejected

+

This response does not match the sign-in this application started.

+

Close this window and start the login again from your application.

+ + + """ elif code: # Success case - we have the authorization code logger.info(f"Received authorization code: {code[:10]}...") @@ -103,6 +162,11 @@ def _handle_oauth_callback(self): "state": state, "timestamp": time.monotonic() }) + + # The code is in hand, so stop listening. Deferred to a timer thread + # because shutdown() from the serving thread would deadlock, and so + # this response still reaches the browser. + threading.Timer(1.0, shutdown_callback_server).start() # The success page needs its own inline script, so allow just that # one script via a per-response nonce rather than loosening the CSP. @@ -208,7 +272,7 @@ def start_callback_server() -> int: try: # Test if port is available with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - s.bind(('localhost', port)) + s.bind((CALLBACK_SERVER_HOST, port)) break except OSError: port += 1 @@ -216,6 +280,7 @@ def start_callback_server() -> int: raise Exception(f"Could not find an available port after {max_attempts} attempts") callback_server_port = port + new_oauth_state() # Start the server in a separate thread callback_server_thread = threading.Thread(target=server_thread, daemon=True) @@ -244,7 +309,7 @@ def server_thread(): global callback_server_running, callback_server_instance try: - callback_server_instance = HTTPServer(('localhost', callback_server_port), CallbackHandler) + callback_server_instance = HTTPServer((CALLBACK_SERVER_HOST, callback_server_port), CallbackHandler) callback_server_running = True print(f"Callback server thread started on port {callback_server_port}") callback_server_instance.serve_forever() diff --git a/pyproject.toml b/pyproject.toml index 95bf8c45..1f2d1af8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "meta-ads-mcp" -version = "1.0.123" +version = "1.0.124" description = "Model Context Protocol (MCP) server for Meta Ads - Use Remote MCP at pipeboard.co for easiest setup" readme = "README.md" requires-python = ">=3.10" diff --git a/server.json b/server.json index 300d4b10..a7ad00ce 100644 --- a/server.json +++ b/server.json @@ -2,7 +2,7 @@ "$schema": "https://static.modelcontextprotocol.io/schemas/2025-12-11/server.schema.json", "name": "co.pipeboard/meta-ads-mcp", "description": "Facebook / Meta Ads automation with AI: analyze performance, test creatives, optimize spend.", - "version": "1.0.123", + "version": "1.0.124", "remotes": [ { "type": "streamable-http", diff --git a/tests/test_callback_server_state.py b/tests/test_callback_server_state.py new file mode 100644 index 00000000..fd26c72a --- /dev/null +++ b/tests/test_callback_server_state.py @@ -0,0 +1,131 @@ +"""Regression tests for the OAuth `state` check (GHSA-75j5-qp3x-mx37, item 3). + +`get_auth_url` emitted no `state` and the callback validated none, so any page +that could reach the callback server could hand it an authorization code of the +attacker's choosing (login CSRF, RFC 6749 §10.12). The callback now only stores +a code whose `state` matches the one minted for the flow this process started, +and the state is single-use. +""" + +import threading +from http.server import HTTPServer +from urllib.parse import parse_qs, urlparse + +import httpx +import pytest + +from meta_ads_mcp.core import callback_server +from meta_ads_mcp.core.callback_server import ( + CALLBACK_SERVER_HOST, + CallbackHandler, + consume_oauth_state, + get_oauth_state, + new_oauth_state, + token_container, +) + + +@pytest.fixture +def callback_url(): + """Run a real CallbackHandler on an ephemeral loopback port.""" + server = HTTPServer((CALLBACK_SERVER_HOST, 0), CallbackHandler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + yield f"http://{CALLBACK_SERVER_HOST}:{server.server_port}" + finally: + server.shutdown() + server.server_close() + thread.join(timeout=5) + + +@pytest.fixture(autouse=True) +def clean_auth_state(): + """Keep the module-level flow state from leaking between tests.""" + callback_server._oauth_state = None + token_container.pop("auth_code", None) + token_container.pop("state", None) + yield + callback_server._oauth_state = None + token_container.pop("auth_code", None) + token_container.pop("state", None) + + +def test_code_without_state_is_rejected(callback_url): + new_oauth_state() + + response = httpx.get(f"{callback_url}/callback?code=attacker-code") + + assert response.status_code == 200 + assert "Authorization Rejected" in response.text + assert "auth_code" not in token_container + + +def test_code_with_wrong_state_is_rejected(callback_url): + new_oauth_state() + + response = httpx.get(f"{callback_url}/callback?code=attacker-code&state=guessed") + + assert "Authorization Rejected" in response.text + assert "auth_code" not in token_container + + +def test_code_with_matching_state_is_accepted(callback_url): + state = new_oauth_state() + + response = httpx.get(f"{callback_url}/callback?code=real-code&state={state}") + + assert "Authorization Successful" in response.text + assert token_container["auth_code"] == "real-code" + + +def test_state_is_single_use(callback_url): + state = new_oauth_state() + httpx.get(f"{callback_url}/callback?code=real-code&state={state}") + token_container.pop("auth_code", None) + + replay = httpx.get(f"{callback_url}/callback?code=replayed-code&state={state}") + + assert "Authorization Rejected" in replay.text + assert "auth_code" not in token_container + + +def test_code_is_rejected_when_no_flow_is_in_flight(callback_url): + """Nothing minted a state, so nothing may be stored.""" + response = httpx.get(f"{callback_url}/callback?code=attacker-code&state=anything") + + assert "Authorization Rejected" in response.text + assert "auth_code" not in token_container + + +def test_consume_rejects_empty_and_mismatched_values(): + state = new_oauth_state() + + assert consume_oauth_state(None) is False + assert consume_oauth_state("") is False + assert consume_oauth_state(state + "x") is False + assert get_oauth_state() == state, "a failed check must not burn the state" + assert consume_oauth_state(state) is True + assert get_oauth_state() is None + + +def test_auth_url_carries_the_state_the_callback_expects(): + from meta_ads_mcp.core.auth import auth_manager + + state = new_oauth_state() + query = parse_qs(urlparse(auth_manager.get_auth_url()).query) + + assert query["state"] == [state] + + +def test_auth_url_mints_a_state_when_no_flow_is_in_flight(): + from meta_ads_mcp.core.auth import auth_manager + + query = parse_qs(urlparse(auth_manager.get_auth_url()).query) + + assert query["state"][0] + assert query["state"][0] == get_oauth_state() + + +def test_callback_server_binds_loopback_only(): + assert CALLBACK_SERVER_HOST == "127.0.0.1" diff --git a/tests/test_callback_server_xss.py b/tests/test_callback_server_xss.py index b4c5848d..36130df3 100644 --- a/tests/test_callback_server_xss.py +++ b/tests/test_callback_server_xss.py @@ -16,6 +16,7 @@ from meta_ads_mcp.core.callback_server import ( CallbackHandler, MAX_ERROR_DISPLAY_LENGTH, + new_oauth_state, ) XSS_PAYLOAD = "" @@ -71,7 +72,8 @@ def test_error_page_sets_locked_down_security_headers(callback_url): def test_success_page_allows_only_its_own_nonced_script(callback_url): - response = httpx.get(f"{callback_url}/callback?code=test-auth-code&state=xyz") + state = new_oauth_state() + response = httpx.get(f"{callback_url}/callback?code=test-auth-code&state={state}") csp = response.headers["content-security-policy"] assert csp.startswith("default-src 'none'; script-src 'nonce-") @@ -84,7 +86,8 @@ def test_success_page_allows_only_its_own_nonced_script(callback_url): def test_token_endpoint_no_longer_exposes_the_auth_code(callback_url): """/token was unreferenced and handed the stored auth code to any same-origin page reachable via the XSS above.""" - httpx.get(f"{callback_url}/callback?code=test-auth-code&state=xyz") + state = new_oauth_state() + httpx.get(f"{callback_url}/callback?code=test-auth-code&state={state}") response = httpx.get(f"{callback_url}/token")