Fix gateway URL resolution #443
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughRemoved ServerRegistry usage from client proxy and Temporal system activities, updated proxy signatures to accept gateway_url/gateway_token, enhanced server-side gateway URL/token extraction from headers/context/env, and added tests validating memo propagation and updated proxy calls. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Sys as SystemActivities
participant Proxy as client_proxy._resolve_gateway_url / wrappers
participant GW as MCP Gateway
note right of Proxy: Resolve gateway_url/token\n(gateway_url arg > headers/context > env)
Sys->>Proxy: call log_via_proxy/ask/notify/request(..., gateway_url?, gateway_token?)
Proxy->>GW: HTTP POST to /internal/... with headers (Authorization: Bearer? / X-MCP-Gateway-Token)
alt success
GW-->>Proxy: 200 JSON
Proxy-->>Sys: return result (bool/dict)
else failure
GW-->>Proxy: error/timeout
Proxy-->>Sys: bool False or error-dict
end
sequenceDiagram
autonumber
participant Client as HTTP Client
participant Server as app_server._workflow_run
participant WF as Workflow (FakeWorkflow/TestWorkflow)
note over Server: Build memo from kwargs → headers → base_url → env
Client->>Server: POST /start-workflow (with X-Forwarded-* / auth headers)
Server->>Server: derive gateway_url & gateway_token
Server->>WF: WF.create(memo=__mcp_agent_workflow_memo)
WF-->>Server: {workflow_id, run_id}
Server-->>Client: {workflow_id, run_id}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp_agent/server/app_server.py (1)
1374-1426: Block SSRF and token exfiltration from header-derived gateway URLsCurrent logic trusts X-MCP-Gateway-URL/X-Forwarded-Url and reconstructs from X-Forwarded-* without origin validation, then may export MCP_GATEWAY_TOKEN into the memo. A malicious client could set these headers to an attacker-controlled host, causing workers to POST internal payloads and the secret token to that host. Tighten trust and avoid exporting secrets for untrusted origins.
Key changes:
- Track the source of gateway_url (header/forwarded/base_url/env).
- Never export env token when the URL came from headers/forwarded unless host is explicitly allowlisted or same-origin.
- Optional: gate trusting of header-derived URLs behind MCP_TRUST_GATEWAY_HEADERS and allowlist via MCP_GATEWAY_ALLOWED_HOSTS.
Apply minimal hardening (preserves current behavior for your tests) by suppressing env token on header/forwarded sources unless allowed:
@@ - try: - # Prefer explicit kwargs, else infer from request context/headers - gateway_url = kwargs.get("gateway_url") - gateway_token = kwargs.get("gateway_token") + try: + # Prefer explicit kwargs, else infer from request context/headers + gateway_url = kwargs.get("gateway_url") + gateway_token = kwargs.get("gateway_token") + _gateway_url_source = None # 'header' | 'forwarded' | 'base_url' | 'env' @@ - if req is not None: + if req is not None: h = req.headers # Highest precedence: caller-provided full base URL header_url = h.get("X-MCP-Gateway-URL") or h.get("X-Forwarded-Url") - if gateway_url is None and header_url: - gateway_url = header_url + if gateway_url is None and header_url: + gateway_url = header_url + _gateway_url_source = "header" @@ - if gateway_token is None: - gateway_token = h.get("X-MCP-Gateway-Token") + if gateway_token is None: + gateway_token = h.get("X-MCP-Gateway-Token") @@ - if gateway_url is None and (h.get("X-Forwarded-Host") or h.get("Host")): + if gateway_url is None and (h.get("X-Forwarded-Host") or h.get("Host")): proto = h.get("X-Forwarded-Proto") or "http" host = h.get("X-Forwarded-Host") or h.get("Host") prefix = h.get("X-Forwarded-Prefix") or "" if prefix and not prefix.startswith("/"): prefix = "/" + prefix if host: - gateway_url = f"{proto}://{host}{prefix}" + gateway_url = f"{proto}://{host}{prefix}" + _gateway_url_source = "forwarded" @@ - if gateway_url is None: + if gateway_url is None: try: if getattr(req, "base_url", None): base_url = str(req.base_url).rstrip("/") if base_url and base_url.lower() != "none": - gateway_url = base_url + gateway_url = base_url + _gateway_url_source = "base_url" except Exception: gateway_url = None @@ - try: - import os as _os + try: + import os as _os @@ - if gateway_url is None: - env_url = _os.environ.get("MCP_GATEWAY_URL") - if env_url: - gateway_url = env_url - if gateway_token is None: - env_tok = _os.environ.get("MCP_GATEWAY_TOKEN") - if env_tok: - gateway_token = env_tok + if gateway_url is None: + env_url = _os.environ.get("MCP_GATEWAY_URL") + if env_url: + gateway_url = env_url + _gateway_url_source = "env" + # Only use env token by default; we'll suppress it for untrusted header-derived URLs below. + env_tok = None + if gateway_token is None: + env_tok = _os.environ.get("MCP_GATEWAY_TOKEN") + if env_tok: + gateway_token = env_tok except Exception: pass + + # If URL was derived from request headers, avoid leaking env token to untrusted origins. + # Allowlist via MCP_GATEWAY_ALLOWED_HOSTS (comma-separated) or same-origin host match. + try: + if _gateway_url_source in ("header", "forwarded") and gateway_url and gateway_token: + from urllib.parse import urlparse + gw_host = urlparse(str(gateway_url)).hostname or "" + req_host = "" + try: + req_host = (req.headers.get("X-Forwarded-Host") or req.headers.get("Host") or "").split(",")[0].strip() + except Exception: + req_host = "" + allowed_hosts = { + h.strip().lower() + for h in (_os.environ.get("MCP_GATEWAY_ALLOWED_HOSTS") or "").split(",") + if h.strip() + } + same_origin = gw_host.lower() == req_host.lower() if gw_host and req_host else False + allowed = gw_host.lower() in allowed_hosts if gw_host and allowed_hosts else False + # If token came from env and origin is neither allowlisted nor same-origin, drop it. + if env_tok and not (same_origin or allowed): + gateway_token = None + except Exception: + # Best-effort hardening; never fail the request on sanitize + pass @@ if gateway_url or gateway_token: workflow_memo = { "gateway_url": gateway_url, "gateway_token": gateway_token, }Optionally, for stricter deployments, also gate the use of header-derived URLs entirely:
- Only honor header_url/forwarded when MCP_TRUST_GATEWAY_HEADERS=true.
- Validate scheme and disallow credentials in URLs.
I can provide a follow-up patch if you want this stronger posture.
🧹 Nitpick comments (10)
src/mcp_agent/server/app_server.py (1)
1407-1419: Avoid re-importing os and clean up URL normalizationMinor nits:
- Re-importing os as _os inside the try adds noise; use the module imported at the top.
- Normalize gateway_url with rstrip("/") when sourced from env/headers to avoid double slashes later.
Example minimal tweak:
- try: - import os as _os + try: @@ - if gateway_url is None: - env_url = _os.environ.get("MCP_GATEWAY_URL") + if gateway_url is None: + env_url = os.environ.get("MCP_GATEWAY_URL") if env_url: - gateway_url = env_url + gateway_url = env_url.rstrip("/") if gateway_token is None: - env_tok = _os.environ.get("MCP_GATEWAY_TOKEN") + env_tok = os.environ.get("MCP_GATEWAY_TOKEN")tests/server/test_app_server_memo.py (2)
20-75: Great targeted coverage for forwarded headers; consider adding header URL and base_url casesThis test validates X-Forwarded-* reconstruction well. To fully cover the new precedence chain, please add:
- X-MCP-Gateway-URL path (should win over forwarded).
- X-Forwarded-Url path.
- base_url fallback when no forwarded/headers present.
- Prefix normalization when X-Forwarded-Prefix lacks leading slash.
I can draft these tests if helpful.
58-67: Simplify monkeypatching of create()You can avoid the lambda wrapper and keep the signature intact for readability:
- monkeypatch.setattr( - FakeWorkflow, - "create", - classmethod(lambda cls, name, context: create_and_stash(name, context)), - ) + monkeypatch.setattr(FakeWorkflow, "create", classmethod(create_and_stash))src/mcp_agent/mcp/client_proxy.py (3)
58-59: Harden timeout parsingEnv parsing can raise ValueError; fall back safely.
- timeout = float(os.environ.get("MCP_GATEWAY_TIMEOUT", "10")) + try: + timeout = float(os.environ.get("MCP_GATEWAY_TIMEOUT", "10")) + except ValueError: + timeout = 10.0
133-134: Harden timeout parsing (notify)- timeout = float(os.environ.get("MCP_GATEWAY_TIMEOUT", "10")) + try: + timeout = float(os.environ.get("MCP_GATEWAY_TIMEOUT", "10")) + except ValueError: + timeout = 10.0
165-166: Harden timeout parsing (request)- timeout = float(os.environ.get("MCP_GATEWAY_TIMEOUT", "20")) + try: + timeout = float(os.environ.get("MCP_GATEWAY_TIMEOUT", "20")) + except ValueError: + timeout = 20.0tests/executor/temporal/test_execution_id_and_interceptor.py (4)
97-100: API usage updated correctly; consider adding coverage for 'data' and token header.Calls now match the wrapper signature
(execution_id, level, namespace, message, ...). Add a test that:
- passes a non-empty
datapayload tolog_via_proxyand asserts it’s forwarded, and- sets
MCP_GATEWAY_TOKEN(or passesgateway_token=) and assertsX-MCP-Gateway-Tokenis sent.
103-106: Add network-exception path for notify.You cover HTTP >=400; also add a case where
httpx.AsyncClient.postraiseshttpx.RequestErrorsonotify_via_proxyreturnsFalse. This guards regressions in timeout/connection failure handling.
109-112: Add success-without-body path for request.Per implementation, a 200 with empty body yields
{"error": "invalid_response"}. Add a case to assert that behavior, and another to verify URL-quoting with special chars inexecution_idfor bothrequest_via_proxyandnotify_via_proxy.
5-8: Nit: remove asyncio marker from non-async tests.
@pytest.mark.asyncioon synchronous tests is unnecessary and can confuse tooling. Safe to drop on Lines 5 and 15.Also applies to: 15-18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/mcp_agent/executor/temporal/system_activities.py(0 hunks)src/mcp_agent/mcp/client_proxy.py(4 hunks)src/mcp_agent/server/app_server.py(1 hunks)tests/executor/temporal/test_execution_id_and_interceptor.py(1 hunks)tests/server/test_app_server_memo.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/mcp_agent/executor/temporal/system_activities.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/server/test_app_server_memo.py (1)
src/mcp_agent/server/app_server.py (1)
_workflow_run(1318-1459)
tests/executor/temporal/test_execution_id_and_interceptor.py (1)
src/mcp_agent/mcp/client_proxy.py (3)
log_via_proxy(39-80)notify_via_proxy(119-148)request_via_proxy(151-178)
🔇 Additional comments (1)
tests/server/test_app_server_memo.py (1)
77-115: Env fallback test looks solidCovers both URL and token propagation from env; pairs well with the forwarded-header case.
src/mcp_agent/mcp/client_proxy.py
Outdated
Comment on lines
39
to
53
| async def log_via_proxy(*args, **kwargs) -> bool: | ||
| """Backward-compatible wrapper. | ||
| Accepts either: | ||
| - legacy: (server_registry, execution_id, level, namespace, message, data=None, *, gateway_url=None, gateway_token=None) | ||
| - new: (execution_id, level, namespace, message, data=None, *, gateway_url=None, gateway_token=None) | ||
| """ | ||
| if args and (args[0] is None or not isinstance(args[0], str)): | ||
| args = args[1:] | ||
| execution_id, level, namespace, message, *rest = args | ||
| data = rest[0] if rest else None | ||
| gateway_url = kwargs.get("gateway_url") | ||
| gateway_token = kwargs.get("gateway_token") | ||
| base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=None) | ||
| url = f"{base}/internal/workflows/log" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Honor context_gateway_url in wrappers
Wrappers ignore a passed context-provided URL (e.g., from workflow memo). Plumb it through to match your stated precedence.
- gateway_url = kwargs.get("gateway_url")
- gateway_token = kwargs.get("gateway_token")
- base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=None)
+ gateway_url = kwargs.get("gateway_url")
+ gateway_token = kwargs.get("gateway_token")
+ context_gateway_url = kwargs.get("context_gateway_url")
+ base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=context_gateway_url)🤖 Prompt for AI Agents
In src/mcp_agent/mcp/client_proxy.py around lines 39 to 53, the wrapper ignores
a context-provided gateway URL; read context_gateway_url from kwargs (e.g.,
kwargs.get("context_gateway_url")) and pass it into _resolve_gateway_url instead
of always passing None so the resolver can apply the intended precedence between
explicit gateway_url and context-provided URL; ensure the wrapper does not drop
the context value when shifting legacy args and that any callers passing
context_gateway_url via kwargs are honored.
src/mcp_agent/mcp/client_proxy.py
Outdated
Comment on lines
83
to
92
| async def ask_via_proxy(*args, **kwargs) -> Dict[str, Any]: | ||
| # legacy: (server_registry, execution_id, prompt, metadata=None, ...) | ||
| if args and (args[0] is None or not isinstance(args[0], str)): | ||
| args = args[1:] | ||
| execution_id, prompt, *rest = args | ||
| metadata = rest[0] if rest else None | ||
| gateway_url = kwargs.get("gateway_url") | ||
| gateway_token = kwargs.get("gateway_token") | ||
| base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=None) | ||
| url = f"{base}/internal/human/prompts" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the same context URL and timeout fixes here
Mirror the changes from log_via_proxy: pass context_gateway_url and harden timeout parsing.
- gateway_url = kwargs.get("gateway_url")
- gateway_token = kwargs.get("gateway_token")
- base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=None)
+ gateway_url = kwargs.get("gateway_url")
+ gateway_token = kwargs.get("gateway_token")
+ context_gateway_url = kwargs.get("context_gateway_url")
+ base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=context_gateway_url)
@@
- timeout = float(os.environ.get("MCP_GATEWAY_TIMEOUT", "10"))
+ try:
+ timeout = float(os.environ.get("MCP_GATEWAY_TIMEOUT", "10"))
+ except ValueError:
+ timeout = 10.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def ask_via_proxy(*args, **kwargs) -> Dict[str, Any]: | |
| # legacy: (server_registry, execution_id, prompt, metadata=None, ...) | |
| if args and (args[0] is None or not isinstance(args[0], str)): | |
| args = args[1:] | |
| execution_id, prompt, *rest = args | |
| metadata = rest[0] if rest else None | |
| gateway_url = kwargs.get("gateway_url") | |
| gateway_token = kwargs.get("gateway_token") | |
| base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=None) | |
| url = f"{base}/internal/human/prompts" | |
| async def ask_via_proxy(*args, **kwargs) -> Dict[str, Any]: | |
| # legacy: (server_registry, execution_id, prompt, metadata=None, ...) | |
| if args and (args[0] is None or not isinstance(args[0], str)): | |
| args = args[1:] | |
| execution_id, prompt, *rest = args | |
| metadata = rest[0] if rest else None | |
| gateway_url = kwargs.get("gateway_url") | |
| gateway_token = kwargs.get("gateway_token") | |
| context_gateway_url = kwargs.get("context_gateway_url") | |
| base = _resolve_gateway_url( | |
| gateway_url=gateway_url, | |
| context_gateway_url=context_gateway_url | |
| ) | |
| url = f"{base}/internal/human/prompts" | |
| try: | |
| timeout = float(os.environ.get("MCP_GATEWAY_TIMEOUT", "10")) | |
| except ValueError: | |
| timeout = 10.0 | |
| # …rest of implementation… |
src/mcp_agent/mcp/client_proxy.py
Outdated
Comment on lines
119
to
127
| async def notify_via_proxy(*args, **kwargs) -> bool: | ||
| # legacy: (server_registry, execution_id, method, params=None, ...) | ||
| if args and (args[0] is None or not isinstance(args[0], str)): | ||
| args = args[1:] | ||
| execution_id, method, *rest = args | ||
| params = rest[0] if rest else None | ||
| gateway_url = kwargs.get("gateway_url") | ||
| gateway_token = kwargs.get("gateway_token") | ||
| base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=None) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply context URL in notify wrapper
- gateway_url = kwargs.get("gateway_url")
- gateway_token = kwargs.get("gateway_token")
- base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=None)
+ gateway_url = kwargs.get("gateway_url")
+ gateway_token = kwargs.get("gateway_token")
+ context_gateway_url = kwargs.get("context_gateway_url")
+ base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=context_gateway_url)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def notify_via_proxy(*args, **kwargs) -> bool: | |
| # legacy: (server_registry, execution_id, method, params=None, ...) | |
| if args and (args[0] is None or not isinstance(args[0], str)): | |
| args = args[1:] | |
| execution_id, method, *rest = args | |
| params = rest[0] if rest else None | |
| gateway_url = kwargs.get("gateway_url") | |
| gateway_token = kwargs.get("gateway_token") | |
| base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=None) | |
| async def notify_via_proxy(*args, **kwargs) -> bool: | |
| # legacy: (server_registry, execution_id, method, params=None, ...) | |
| if args and (args[0] is None or not isinstance(args[0], str)): | |
| args = args[1:] | |
| execution_id, method, *rest = args | |
| params = rest[0] if rest else None | |
| gateway_url = kwargs.get("gateway_url") | |
| gateway_token = kwargs.get("gateway_token") | |
| context_gateway_url = kwargs.get("context_gateway_url") | |
| base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=context_gateway_url) |
🤖 Prompt for AI Agents
In src/mcp_agent/mcp/client_proxy.py around lines 119 to 127, the
notify_via_proxy wrapper always passes context_gateway_url=None into
_resolve_gateway_url, so any provided context URL in kwargs is ignored; update
the wrapper to read context_gateway_url = kwargs.get("context_gateway_url") (or
similarly named key used by callers) and pass that value into
_resolve_gateway_url instead of None, preserving existing
gateway_url/gateway_token handling so resolution uses the provided context when
present.
src/mcp_agent/mcp/client_proxy.py
Outdated
Comment on lines
151
to
159
| async def request_via_proxy(*args, **kwargs) -> Dict[str, Any]: | ||
| # legacy: (server_registry, execution_id, method, params=None, ...) | ||
| if args and (args[0] is None or not isinstance(args[0], str)): | ||
| args = args[1:] | ||
| execution_id, method, *rest = args | ||
| params = rest[0] if rest else None | ||
| gateway_url = kwargs.get("gateway_url") | ||
| gateway_token = kwargs.get("gateway_token") | ||
| base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=None) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply context URL in request wrapper
- gateway_url = kwargs.get("gateway_url")
- gateway_token = kwargs.get("gateway_token")
- base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=None)
+ gateway_url = kwargs.get("gateway_url")
+ gateway_token = kwargs.get("gateway_token")
+ context_gateway_url = kwargs.get("context_gateway_url")
+ base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=context_gateway_url)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def request_via_proxy(*args, **kwargs) -> Dict[str, Any]: | |
| # legacy: (server_registry, execution_id, method, params=None, ...) | |
| if args and (args[0] is None or not isinstance(args[0], str)): | |
| args = args[1:] | |
| execution_id, method, *rest = args | |
| params = rest[0] if rest else None | |
| gateway_url = kwargs.get("gateway_url") | |
| gateway_token = kwargs.get("gateway_token") | |
| base = _resolve_gateway_url(gateway_url=gateway_url, context_gateway_url=None) | |
| async def request_via_proxy(*args, **kwargs) -> Dict[str, Any]: | |
| # legacy: (server_registry, execution_id, method, params=None, ...) | |
| if args and (args[0] is None or not isinstance(args[0], str)): | |
| args = args[1:] | |
| execution_id, method, *rest = args | |
| params = rest[0] if rest else None | |
| gateway_url = kwargs.get("gateway_url") | |
| gateway_token = kwargs.get("gateway_token") | |
| context_gateway_url = kwargs.get("context_gateway_url") | |
| base = _resolve_gateway_url( | |
| gateway_url=gateway_url, | |
| context_gateway_url=context_gateway_url | |
| ) |
🤖 Prompt for AI Agents
In src/mcp_agent/mcp/client_proxy.py around lines 151 to 159, the request
wrapper currently calls _resolve_gateway_url with context_gateway_url=None so
any supplied context URL is ignored; update the call to pass the context URL
from the wrapper (e.g., read kwargs.get("context_gateway_url") or the
appropriate context variable) into context_gateway_url so the resolved base
reflects the provided context; ensure you fall back to None only if no context
is supplied.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/mcp_agent/server/app_server.py (4)
338-350: Consolidate and harden internal-route auth (remove duplication, standardize checks).Auth logic is duplicated across three routes. Centralize it and standardize the response. Also trim bearer token whitespace.
Apply within this block:
- if gw_token: - bearer = request.headers.get("Authorization", "") - bearer_token = ( - bearer.split(" ", 1)[1] if bearer.lower().startswith("bearer ") else "" - ) - header_tok = request.headers.get("X-MCP-Gateway-Token", "") - if not ( - secrets.compare_digest(header_tok, gw_token) - or secrets.compare_digest(bearer_token, gw_token) - ): - return JSONResponse( - {"ok": False, "error": "unauthorized"}, status_code=401 - ) + if not _authorize_gateway_request(request): + return JSONResponse({"error": "unauthorized"}, status_code=401)Add this helper once (outside this range):
def _authorize_gateway_request(request: Request) -> bool: gw_token = os.environ.get("MCP_GATEWAY_TOKEN") if not gw_token: return True # no auth required bearer = request.headers.get("Authorization", "") header_tok = request.headers.get("X-MCP-Gateway-Token", "") or "" bearer_tok = bearer.split(" ", 1)[1].strip() if bearer.lower().startswith("bearer ") else "" tok = header_tok or bearer_tok try: return bool(tok) and secrets.compare_digest(tok, gw_token) except Exception: return False
405-408: Log dropped notifications for traceability.When best-effort notifications are dropped, log at debug for observability.
- if isinstance(method, str) and method.startswith("notifications/"): - return JSONResponse({"ok": True, "dropped": True}) + if isinstance(method, str) and method.startswith("notifications/"): + logger.debug( + "Dropped notification for execution_id=%s method=%s (upstream session closed)", + execution_id, method + ) + return JSONResponse({"ok": True, "dropped": True})
512-525: Reuse auth helper and align unauthorized payload.Use the shared helper for consistency; return a consistent body shape.
- if gw_token: - bearer = request.headers.get("Authorization", "") - bearer_token = ( - bearer.split(" ", 1)[1] if bearer.lower().startswith("bearer ") else "" - ) - header_tok = request.headers.get("X-MCP-Gateway-Token", "") - if not ( - secrets.compare_digest(header_tok, gw_token) - or secrets.compare_digest(bearer_token, gw_token) - ): - return JSONResponse( - {"ok": False, "error": "unauthorized"}, status_code=401 - ) + if not _authorize_gateway_request(request): + return JSONResponse({"error": "unauthorized"}, status_code=401)
558-569: Apply the same auth helper here for parity.Keeps behavior uniform across internal routes.
- if gw_token: - bearer = request.headers.get("Authorization", "") - bearer_token = ( - bearer.split(" ", 1)[1] if bearer.lower().startswith("bearer ") else "" - ) - header_tok = request.headers.get("X-MCP-Gateway-Token", "") - if not ( - secrets.compare_digest(header_tok, gw_token) - or secrets.compare_digest(bearer_token, gw_token) - ): - return JSONResponse({"error": "unauthorized"}, status_code=401) + if not _authorize_gateway_request(request): + return JSONResponse({"error": "unauthorized"}, status_code=401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mcp_agent/mcp/client_proxy.py(4 hunks)src/mcp_agent/server/app_server.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mcp_agent/mcp/client_proxy.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/mcp_agent/server/app_server.py (1)
src/mcp_agent/executor/temporal/session_proxy.py (2)
request(98-113)request(306-309)
Comment on lines
+1394
to
+1448
| # Prefer explicit kwargs, else infer from request context/headers | ||
| gateway_url = kwargs.get("gateway_url") | ||
| gateway_token = kwargs.get("gateway_token") | ||
|
|
||
| if gateway_url is None: | ||
| try: | ||
| req = getattr(ctx.request_context, "request", None) | ||
| if req is not None: | ||
| # Custom header if present | ||
| h = req.headers | ||
| gateway_url = ( | ||
| h.get("X-MCP-Gateway-URL") | ||
| or h.get("X-Forwarded-Url") | ||
| or h.get("X-Forwarded-Proto") | ||
| ) | ||
| # Best-effort reconstruction if only proto/host provided | ||
| if gateway_url is None: | ||
| proto = h.get("X-Forwarded-Proto") or "http" | ||
| host = h.get("X-Forwarded-Host") or h.get("Host") | ||
| if host: | ||
| gateway_url = f"{proto}://{host}" | ||
| except Exception: | ||
| pass | ||
| req = getattr(ctx.request_context, "request", None) | ||
| if req is not None: | ||
| h = req.headers | ||
| # Highest precedence: caller-provided full base URL | ||
| header_url = h.get("X-MCP-Gateway-URL") or h.get("X-Forwarded-Url") | ||
| if gateway_url is None and header_url: | ||
| gateway_url = header_url | ||
|
|
||
| # Token may be provided by the gateway/proxy | ||
| if gateway_token is None: | ||
| gateway_token = h.get("X-MCP-Gateway-Token") | ||
| if gateway_token is None: | ||
| # Support Authorization: Bearer <token> | ||
| auth = h.get("Authorization") | ||
| if auth and auth.lower().startswith("bearer "): | ||
| gateway_token = auth.split(" ", 1)[1] | ||
|
|
||
| # Prefer explicit reconstruction from X-Forwarded-* if present | ||
| if gateway_url is None and (h.get("X-Forwarded-Host") or h.get("Host")): | ||
| proto = h.get("X-Forwarded-Proto") or "http" | ||
| host = h.get("X-Forwarded-Host") or h.get("Host") | ||
| prefix = h.get("X-Forwarded-Prefix") or "" | ||
| if prefix and not prefix.startswith("/"): | ||
| prefix = "/" + prefix | ||
| if host: | ||
| gateway_url = f"{proto}://{host}{prefix}" | ||
|
|
||
| # Fallback to request's base_url which already includes scheme/host and any mount prefix | ||
| if gateway_url is None: | ||
| try: | ||
| if getattr(req, "base_url", None): | ||
| base_url = str(req.base_url).rstrip("/") | ||
| if base_url and base_url.lower() != "none": | ||
| gateway_url = base_url | ||
| except Exception: | ||
| gateway_url = None | ||
|
|
||
| if gateway_token is None: | ||
| try: | ||
| req = getattr(ctx.request_context, "request", None) | ||
| if req is not None: | ||
| gateway_token = req.headers.get("X-MCP-Gateway-Token") | ||
| except Exception: | ||
| pass | ||
| # Final fallback: environment variables (useful if proxies don't set headers) | ||
| try: | ||
| import os as _os | ||
|
|
||
| if gateway_url is None: | ||
| env_url = _os.environ.get("MCP_GATEWAY_URL") | ||
| if env_url: | ||
| gateway_url = env_url | ||
| if gateway_token is None: | ||
| env_tok = _os.environ.get("MCP_GATEWAY_TOKEN") | ||
| if env_tok: | ||
| gateway_token = env_tok | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Do not trust client-supplied gateway_url; gate on valid token and validate URL (SSRF/data exfil risk).
A malicious client can set X-MCP-Gateway-URL/X-Forwarded-* and steer worker callbacks (with potential sensitive logs/prompts) to an attacker-controlled URL. Only accept header-derived URLs when a valid gateway token accompanies the request; otherwise derive from request.base_url. Also normalize/validate scheme/host.
# Prefer explicit kwargs, else infer from request context/headers
gateway_url = kwargs.get("gateway_url")
gateway_token = kwargs.get("gateway_token")
req = getattr(ctx.request_context, "request", None)
if req is not None:
h = req.headers
# Highest precedence: caller-provided full base URL
header_url = h.get("X-MCP-Gateway-URL") or h.get("X-Forwarded-Url")
- if gateway_url is None and header_url:
- gateway_url = header_url
+ # Trust header URL only if caller presents a valid gateway token
+ env_tok = os.environ.get("MCP_GATEWAY_TOKEN")
+ trusted_headers = False
+ if gateway_token:
+ try:
+ trusted_headers = bool(env_tok and secrets.compare_digest(gateway_token, env_tok))
+ except Exception:
+ trusted_headers = False
+ if gateway_url is None and header_url and trusted_headers:
+ gateway_url = header_url
# Token may be provided by the gateway/proxy
if gateway_token is None:
gateway_token = h.get("X-MCP-Gateway-Token")
if gateway_token is None:
# Support Authorization: Bearer <token>
auth = h.get("Authorization")
if auth and auth.lower().startswith("bearer "):
- gateway_token = auth.split(" ", 1)[1]
+ gateway_token = auth.split(" ", 1)[1].strip()
# Prefer explicit reconstruction from X-Forwarded-* if present
- if gateway_url is None and (h.get("X-Forwarded-Host") or h.get("Host")):
+ if gateway_url is None and trusted_headers and (h.get("X-Forwarded-Host") or h.get("Host")):
proto = h.get("X-Forwarded-Proto") or "http"
host = h.get("X-Forwarded-Host") or h.get("Host")
prefix = h.get("X-Forwarded-Prefix") or ""
if prefix and not prefix.startswith("/"):
prefix = "/" + prefix
if host:
gateway_url = f"{proto}://{host}{prefix}"
# Fallback to request's base_url which already includes scheme/host and any mount prefix
if gateway_url is None:
try:
if getattr(req, "base_url", None):
base_url = str(req.base_url).rstrip("/")
if base_url and base_url.lower() != "none":
gateway_url = base_url
except Exception:
gateway_url = None
+
+ # Normalize and validate the URL (only http/https with netloc)
+ if isinstance(gateway_url, str):
+ u = gateway_url.strip().rstrip("/")
+ try:
+ from urllib.parse import urlsplit, urlunsplit
+ parts = urlsplit(u)
+ if parts.scheme not in ("http", "https") or not parts.netloc:
+ gateway_url = None
+ else:
+ gateway_url = urlunsplit((parts.scheme, parts.netloc, parts.path or "", "", ""))
+ except Exception:
+ gateway_url = None
# Final fallback: environment variables (useful if proxies don't set headers)
try:
- import os as _os
-
if gateway_url is None:
- env_url = _os.environ.get("MCP_GATEWAY_URL")
+ env_url = os.environ.get("MCP_GATEWAY_URL")
if env_url:
gateway_url = env_url
if gateway_token is None:
- env_tok = _os.environ.get("MCP_GATEWAY_TOKEN")
- if env_tok:
- gateway_token = env_tok
+ if env_tok:
+ gateway_token = env_tok
except Exception:
passCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/mcp_agent/server/app_server.py around lines 1394 to 1448, header-derived
gateway_url handling allows untrusted client-supplied URLs; restrict acceptance
to only when a valid gateway_token is present and validate/normalize the URL to
mitigate SSRF/data-exfil risks. Change logic so header_url
(X-MCP-Gateway-URL/X-Forwarded-Host/etc.) is used only if gateway_token is
non-empty and has been validated/verified for this request; otherwise ignore
header_url and fall back to req.base_url or environment. When accepting a
header-derived URL, parse and normalize it (use urllib.parse) and enforce:
scheme is http or https, netloc present, no embedded credentials, path
normalized (leading slash), and reject IP-literal hosts or resolve the hostname
and verify it is not a private/loopback address (use ipaddress and socket to
check); if any check fails, treat as invalid and fall back to req.base_url or
return an error. Ensure final gateway_url is rstrip("/") normalized before use.
andrew-lastmile
added a commit
that referenced
this pull request
Sep 11, 2025
* Temporarily exclude CLI from test coverage (#429) ### TL;DR Exclude CLI code from test coverage metrics for now. Will add tests when we're done sprinting 10000 mph  <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Adjusted test coverage collection to exclude non-critical CLI components, resulting in more accurate coverage metrics for core functionality. * **Chores** * Updated coverage reporting configuration to align with the new exclusion rules, ensuring consistent results across local and CI runs. <!-- end of auto-generated comment: release notes by coderabbit.ai --> * Add workflow commands to CLI (#424) ### TL;DR Added workflow management commands to the MCP Agent CLI, including describe, suspend, resume, and cancel operations. ### What changed? - Added four new workflow management commands: - `describe_workflow`: Shows detailed information about a workflow execution - `suspend_workflow`: Pauses a running workflow execution - `resume_workflow`: Resumes a previously suspended workflow - `cancel_workflow`: Permanently stops a workflow execution - Implemented corresponding API client methods in `WorkflowAPIClient`: - `suspend_workflow` - `resume_workflow` - `cancel_workflow` - Updated the CLI structure to expose these commands under `mcp-agent cloud workflows` - Added an alias for `describe_workflow` as `status` for backward compatibility ### How to test? Test the new workflow commands with a running workflow: ``` # Get workflow details mcp-agent cloud workflows describe run_abc123 mcp-agent cloud workflows status run_abc123 # alias # Suspend a workflow mcp-agent cloud workflows suspend run_abc123 # Resume a workflow (with optional payload) mcp-agent cloud workflows resume run_abc123 mcp-agent cloud workflows resume run_abc123 --payload '{"data": "value"}' # Cancel a workflow (with optional reason) mcp-agent cloud workflows cancel run_abc123 mcp-agent cloud workflows cancel run_abc123 --reason "User requested cancellation" ``` ### Why make this change? These commands provide essential workflow lifecycle management capabilities to users, allowing them to monitor and control workflow executions through the CLI. The ability to suspend, resume, and cancel workflows gives users more control over long-running operations and helps manage resources more efficiently. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Introduced “workflows” CLI group with commands: describe (alias: status), resume, suspend, and cancel. - Describe supports text, JSON, and YAML output; all commands work with server ID or URL and include improved error messages. - Refactor - Renamed CLI group from “workflow” to “workflows” and reorganized command registrations. - Consolidated internal utility imports (no behavior change). - Chores - Updated module descriptions. - Removed legacy workflow status package/exports in favor of the new workflows commands. <!-- end of auto-generated comment: release notes by coderabbit.ai --> * add servers workflow subcommand (#428) # Add servers workflows subcommand This PR adds a new `workflows` subcommand to the `mcp-agent cloud servers` command that allows users to list workflows associated with a specific server. The command supports: - Filtering by workflow status - Limiting the number of results - Multiple output formats (text, JSON, YAML) - Accepting server IDs, app config IDs, or server URLs as input Examples: ``` mcp-agent cloud servers workflows app_abc123 mcp-agent cloud servers workflows https://server.example.com --status running mcp-agent cloud servers workflows apcnf_xyz789 --limit 10 --format json ``` The PR also cleans up the examples in the existing workflow commands and adds the necessary API client support for listing workflows. * add workflow list and runs (#430) ### TL;DR Reorganized workflow commands `mcp-agent cloud workflows runs` `mcp-agent cloud workflows list` `mcp-agent cloud server workflows` (alias of workflows list) ### What changed? - Moved `list_workflows_for_server` from the servers module to the workflows module as `list_workflow_runs` - Added new workflow commands: `list_workflows` and `list_workflow_runs` - Updated CLI command structure to make workflows commands more intuitive - Applied consistent code formatting with black across all server and workflow related files ### How to test? Test the new and reorganized workflow commands: ```bash # List available workflow definitions mcp-agent cloud workflows list app_abc123 # List workflow runs (previously under servers workflows) mcp-agent cloud workflows runs app_abc123 # Test with different output formats mcp-agent cloud workflows list app_abc123 --format json mcp-agent cloud workflows runs app_abc123 --format yaml # Verify existing commands still work mcp-agent cloud servers list mcp-agent cloud workflows describe app_abc123 run_xyz789 ``` * [ez] Move deploy command to cloud namespace (#431) ### TL;DR Added `cloud deploy` command as an alias for the existing `deploy` command. * First pass at implementing the mcp-agent CLI (#409) * Initial scaffolding * initial CLI * checkpoint * checkpoint 2 * various updates to cli * fix lint and format * fix: should load secrets.yaml template instead when running init cli command * fix: prevent None values in either mcp-agent secrets and config yaml files from overwriting one another when merging both * fix: when running config check, use get_settings() instead of Settings() to ensure settings are loaded. * fix: handle None values for servers in MCPSettings so it defaults to empty dict and update secrets.yaml template so it does not overwrite mcp servers in config * Inform users to save and close editor to continue when running config edit command * fix: Update openai, anthropic and azure regex for keys cli command * Sort model list by provider and model name * Add filtering support for models list cli command * disable untested commands * lint, format, gen_schema * get rid of accidental otlp exporter changes from another branch * get rid of accidental commit from other branch --------- Co-authored-by: StreetLamb <[email protected]> * Docs MVP (#436) * Initial scaffolding * initial CLI * checkpoint * checkpoint 2 * various updates to cli * fix lint and format * fix: should load secrets.yaml template instead when running init cli command * fix: prevent None values in either mcp-agent secrets and config yaml files from overwriting one another when merging both * fix: when running config check, use get_settings() instead of Settings() to ensure settings are loaded. * fix: handle None values for servers in MCPSettings so it defaults to empty dict and update secrets.yaml template so it does not overwrite mcp servers in config * Inform users to save and close editor to continue when running config edit command * fix: Update openai, anthropic and azure regex for keys cli command * Sort model list by provider and model name * Add filtering support for models list cli command * disable untested commands * Fixes to docs * Updating the main.py and !developer_secrets for secrets * updating python entry files to main.py * Fix tracer.py --------- Co-authored-by: StreetLamb <[email protected]> Co-authored-by: Andrew Hoh <[email protected]> * fix: max complete token for openai gen structured (#438) * Fix regression in CLI ("cloud cloud") * docs fixes * Fix top-level cli cloud commands (deploy, login, etc) * Add eager tool validation to ensure json serializability of input params/result types * More docs updates * Refactor workflow runs list to use MCP tool calls (#439) ### TL;DR Refactored the workflow runs listing command to use MCP tool calls instead of direct API client calls. ### What changed? - Replaced the direct API client approach with MCP tool calls to retrieve workflow runs - Added a new `_list_workflow_runs_async` function that uses the MCP App and gen_client to communicate with the server - Improved status filtering and display logic to work with both object and dictionary response formats - Enhanced error handling and formatting of workflow run information - Updated the workflow data processing to handle different response formats more robustly ### How to test? ```bash # List workflow runs from a server mcp-agent cloud workflows runs <server_id_or_url> # Filter by status mcp-agent cloud workflows runs <server_id_or_url> --status running # Limit results mcp-agent cloud workflows runs <server_id_or_url> --limit 10 # Change output format mcp-agent cloud workflows runs <server_id_or_url> --format json ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Add status filtering for workflow runs, with common aliases (e.g., timeout → timed_out). - Add an optional limit to constrain the number of results. - Allow server selection via direct URL or config-based server ID. - Refactor - Update text output: columns now show Workflow ID, Name, Status, Run ID, Created; Principal removed. - Improve date formatting and consistent JSON/YAML/Text rendering. - Bug Fixes - Clearer error messages and safer handling when server info is missing or no data is returned. <!-- end of auto-generated comment: release notes by coderabbit.ai --> * Update workflows commands UI to be more consistant with the rest of the CLI (#432) ### TL;DR Improved CLI workflow command output formatting with better visual indicators and consistent styling. ### How to test? ``` mcp-agent cloud workflows cancel <run-id> mcp-agent cloud workflows describe <run-id> mcp-agent cloud workflows resume <run-id> ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Style** * Cancel workflow: added a blank line before the status and changed the success icon to 🚫 (yellow). * Describe workflow: replaced panel UI with a clean, header-based text layout (“🔍 Workflow Details”), showing name with colorized status and fields for Workflow ID, Run ID, and Created. Updated status indicators with emojis and colors; timestamp is now plain text on its own line. * Resume workflow: success message now applies consistent coloring to the entire line for improved readability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> * Feature: Update Workflow Tool Calls to Work with workflow_id (#435) * Support for workflow_id and run_id * Update temporal workflow registry * tests * Update LLMS.txt * Fix config * Return bool for cancel result * Validate ids provided * Fix cancel workflow id * Fix workflows-resume response * Add workflow-name specific resume and cancel tools * Fix return type * Fix examples * Remove redundant workflows-{name}-tool tool calls * Add _workflow_status back * Use registry helper * Changes from review * Add back evaluator_optimizer enum fix * Fix a hang that can happen at shutdown (#440) * Fix a shutdown hang * Fix tests * fix taskgroup closed in a different context than when it was started in error * some PR feedback fixes * PR feedback * Fix random failures of server aggregator not found for agent in temporal (#441) * Fix a shutdown hang * Fix tests * fix taskgroup closed in a different context than when it was started in error * some PR feedback fixes * Fix random failures of server aggregator not found for agent in temporal environment * Bump pyproject version * Fix gateway URL resolution (#443) * Fix gateway URL resolution Removed incorrect dependence on ServerRegistry for gateway URLs; the gateway is not an MCP server. App server (src/mcp_agent/server/app_server.py) builds workflow memo with: - gateway_url precedence: X-MCP-Gateway-URL or X-Forwarded-Url → reconstruct X-Forwarded-Proto/Host/Prefix → request.base_url → MCP_GATEWAY_URL env. - gateway_token precedence: X-MCP-Gateway-Token → MCP_GATEWAY_TOKEN env. Worker-side (SystemActivities/SessionProxy) uses memo.gateway_url and gateway_token; falls back to worker env. Client proxy helpers (src/mcp_agent/mcp/client_proxy.py): - _resolve_gateway_url: explicit param → context → env → local default. - Updated public signatures to drop server_registry parameter. * Cloud/deployable temporal example (#395) * Move workflows to workflows.py file * Fix router example * Add remaining dependencies * Update orchestrator to @app.async_tool example * Changes from review * Fix interactive_workflow to be runnable via tool * Fix resume tool params * Fix: Use helpful typer and invoke for root cli commands (#444) * Use helpful typer and invoke for root cli commands * Fix lint * Fix enum check (#445) * Fix/swap relative mcp agent dependency on deploy (#446) * Update wrangler wrapper to handle requirements.txt processing * Fix backup handling * pass api key to workflow (#447) * pass api key to workflow * guard against settings not existing --------- Co-authored-by: John Corbett <[email protected]> Co-authored-by: Sarmad Qadri <[email protected]> Co-authored-by: StreetLamb <[email protected]> Co-authored-by: Yi <[email protected]> Co-authored-by: Ryan Holinshead <[email protected]> Co-authored-by: roman-van-der-krogt <[email protected]>
This was referenced Sep 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- gateway_url precedence: X-MCP-Gateway-URL or X-Forwarded-Url → reconstruct X-Forwarded-Proto/Host/Prefix → request.base_url → MCP_GATEWAY_URL env.
- gateway_token precedence: X-MCP-Gateway-Token → MCP_GATEWAY_TOKEN env.
- _resolve_gateway_url: explicit param → context → env → local default.
- Updated public signatures to drop server_registry parameter.
Here's the overall flow we expect to happen:
Summary by CodeRabbit
New Features
Bug Fixes
Tests