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

get_auth_url emitted no state and the callback validated none (RFC 6749 10.12), so any page that could reach the local callback server could hand it an authorization code of the attacker's choosing.

A random state is now minted per authorization request, carried in the auth URL and required on the callback: missing, mismatched or replayed state means the code is rejected and never stored. The server also binds 127.0.0.1 explicitly and shuts down once it has handled a valid callback rather than idling out the 180s window.

The report's fourth item (random high port) is not actionable: Meta validates redirect URIs against the list registered on the app, so the flow only works on pre-registered ports.

Bumps to 1.0.124 and documents the advisory in SECURITY.md.

Author: nictuku

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