fix(security): require an OAuth state on the callback (GHSA-75j5-qp3x-mx37)

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).

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',

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]:

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):
             </html>
             """
             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 = """
+            <html>
+            <head><title>Authorization Rejected</title></head>
+            <body>
+                <h1>Authorization Rejected</h1>
+                <p>This response does not match the sign-in this application started.</p>
+                <p>Close this window and start the login again from your application.</p>
+            </body>
+            </html>
+            """
         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,14 +272,15 @@ 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
         else:
             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()

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"

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",

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"

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 = "<script>alert(document.domain)</script>"
@@ -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")