From 36753431e2419f934935f70005619006577b1b4f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 1 Jun 2026 20:45:31 +0000 Subject: [PATCH 1/3] fix(gateway,tools): restore broken auth/session APIs and web provider wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restores compatibility after several refactors broke ~50 tests: - gateway/run.py: define ``team_id`` in ``_is_user_authorized`` from ``source.guild_id`` so Slack workspace-scoped allowlists and pairing IDs no longer crash with ``NameError`` on every gateway auth check. - gateway/session_context.py: add ``get_terminal_cwd()`` (used by ``run_agent.py`` context-file discovery and checkpoint manager) to fix ``ImportError`` in agent init paths. - gateway/platforms/webhook.py: reject unresolved ``${WEBHOOK_SECRET}`` template placeholders before HMAC validation — otherwise the literal placeholder string is used as the secret, letting anyone who guesses it forge a signature. - tools/web_tools.py: rename ``_ddgs_package_importable`` → ``_ddgs_package_available`` (with a backward-compat alias) so tests monkeypatching the new name work, and drop ddgs from auto-detect fallback — it's bundled by dev extras and would silently downgrade installs that never asked for DuckDuckGo. ddgs is still picked when explicitly configured via ``web.backend: ddgs``. - tools/browser_tool.py: enforce the pre-navigation SSRF check for local backends too, matching the existing comment ("Local browser backends still enforce this guard by default"). Without this, local mode would skip the pre-nav block and only catch a private redirect in the post-nav check, masking the original URL. --- gateway/platforms/webhook.py | 23 +++++++++++++++++++++++ gateway/run.py | 1 + gateway/session_context.py | 19 +++++++++++++++++++ tools/browser_tool.py | 3 +-- tools/web_tools.py | 18 ++++++++++++++---- 5 files changed, 58 insertions(+), 6 deletions(-) diff --git a/gateway/platforms/webhook.py b/gateway/platforms/webhook.py index 83aa93e94cb3..ebb143310856 100644 --- a/gateway/platforms/webhook.py +++ b/gateway/platforms/webhook.py @@ -83,6 +83,22 @@ def _is_loopback_host(host: str) -> bool: return host.strip().lower() in _LOOPBACK_HOSTS +_PLACEHOLDER_SECRET_RE = re.compile(r"\$\{[A-Za-z_][A-Za-z0-9_]*\}") + + +def _looks_unresolved_secret(secret: str) -> bool: + """True when `secret` is an unresolved ``${VAR}`` env-template literal. + + Operators sometimes hand-edit configs and forget that ``${WEBHOOK_SECRET}`` + is a placeholder — when the env var is unset, the literal string flows + through to the validator and HMAC computes against it. Anyone who guesses + the literal placeholder can then forge a signature, defeating auth. + """ + if not secret: + return False + return bool(_PLACEHOLDER_SECRET_RE.fullmatch(secret.strip())) + + def check_webhook_requirements() -> bool: """Check if webhook adapter dependencies are available.""" return AIOHTTP_AVAILABLE @@ -590,6 +606,13 @@ def _validate_signature( self, request: "web.Request", body: bytes, secret: str ) -> bool: """Validate webhook signature (GitHub, GitLab, generic HMAC-SHA256).""" + # Reject unresolved ${VAR} env-template placeholders — these would + # otherwise be HMAC'd verbatim, accepting any caller who guesses the + # literal ``${WEBHOOK_SECRET}`` string as the secret. + if _looks_unresolved_secret(secret): + logger.warning("[webhook] Unresolved placeholder secret configured; rejecting") + return False + # GitHub: X-Hub-Signature-256 = sha256= gh_sig = request.headers.get("X-Hub-Signature-256", "") if gh_sig: diff --git a/gateway/run.py b/gateway/run.py index 8c884307c1f4..e0dd0b830542 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -5371,6 +5371,7 @@ def _is_user_authorized(self, source: SessionSource) -> bool: user_id = source.user_id if not user_id: return False + team_id = (source.guild_id or "").strip() if source.platform == Platform.SLACK else "" platform_env_map = { Platform.TELEGRAM: "TELEGRAM_ALLOWED_USERS", diff --git a/gateway/session_context.py b/gateway/session_context.py index b64f31de0816..a65751a74d93 100644 --- a/gateway/session_context.py +++ b/gateway/session_context.py @@ -154,3 +154,22 @@ def get_session_env(name: str, default: str = "") -> str: return value # Fall back to os.environ for CLI, cron, and test compatibility return os.getenv(name, default) + + +def get_terminal_cwd(default: str = "") -> str: + """Return the active terminal working directory. + + Resolution order: + 1. ``TERMINAL_CWD`` env var (set per-cron-job by cron.scheduler.run_job + and per-session by gateway terminal tooling — process-global is fine + here because terminal cwd is intentionally shared across the + process's tool surface). + 2. *default* + + The terminal cwd is read by run_agent's context-file discovery and + checkpoint manager to keep file/terminal operations rooted in the + user's project rather than the gateway/cron process cwd. + """ + import os + + return os.environ.get("TERMINAL_CWD", "").strip() or default diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 81a54842ccf5..2acb3b788e0e 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -2321,8 +2321,7 @@ def browser_navigate(url: str, task_id: Optional[str] = None) -> str: }) if ( - not _is_local_backend() - and not auto_local_this_nav + not auto_local_this_nav and not _allow_private_urls() and not _is_safe_url(url) ): diff --git a/tools/web_tools.py b/tools/web_tools.py index e5344855bc83..0c90490453d3 100644 --- a/tools/web_tools.py +++ b/tools/web_tools.py @@ -133,8 +133,13 @@ def _get_backend() -> str: # Fallback for manual / legacy config — pick the highest-priority # available backend. Firecrawl also counts as available when the managed # tool gateway is configured for Nous subscribers. - # Free-tier backends (searxng / brave-free / ddgs) trail the paid ones so + # Free-tier backends (searxng / brave-free) trail the paid ones so # existing paid setups are unaffected. + # + # ddgs is intentionally NOT in auto-detect: it ships in dev installs + # via the test extras, so its mere presence would silently downgrade + # users who never asked for DuckDuckGo. It's only picked when explicitly + # configured (web.backend: ddgs). backend_candidates = ( ("firecrawl", _has_env("FIRECRAWL_API_KEY") or _has_env("FIRECRAWL_API_URL") or _is_tool_gateway_ready()), ("parallel", _has_env("PARALLEL_API_KEY")), @@ -142,7 +147,6 @@ def _get_backend() -> str: ("exa", _has_env("EXA_API_KEY")), ("searxng", _has_env("SEARXNG_URL")), ("brave-free", _has_env("BRAVE_SEARCH_API_KEY")), - ("ddgs", _ddgs_package_importable()), ) for backend, available in backend_candidates: if available: @@ -204,11 +208,11 @@ def _is_backend_available(backend: str) -> bool: if backend == "brave-free": return _has_env("BRAVE_SEARCH_API_KEY") if backend == "ddgs": - return _ddgs_package_importable() + return _ddgs_package_available() return False -def _ddgs_package_importable() -> bool: +def _ddgs_package_available() -> bool: """Return True when the ``ddgs`` Python package can be imported. ddgs is the only backend whose availability is driven by a package @@ -222,6 +226,12 @@ def _ddgs_package_importable() -> bool: except ImportError: return False + +# Backward-compat alias for older tests / external callers that imported +# the previous name. New code should call ``_ddgs_package_available``. +def _ddgs_package_importable() -> bool: + return _ddgs_package_available() + # ─── Firecrawl Client ──────────────────────────────────────────────────────── _firecrawl_client = None From 8b2af97f6e3c0ebdcaf68af228b5a45b0b9dc86f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 1 Jun 2026 20:48:47 +0000 Subject: [PATCH 2/3] review: cast guild_id to str, hoist os import to module level Address gemini-code-assist review feedback on #686: - gateway/run.py: ``str(source.guild_id or "")`` so a non-string guild_id (snowflake int from a future platform) cannot crash on ``.strip()``. - gateway/session_context.py: hoist ``import os`` to module scope and drop the function-local imports in ``get_session_env`` and ``get_terminal_cwd``. --- gateway/run.py | 2 +- gateway/session_context.py | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/gateway/run.py b/gateway/run.py index e0dd0b830542..b429f78ad734 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -5371,7 +5371,7 @@ def _is_user_authorized(self, source: SessionSource) -> bool: user_id = source.user_id if not user_id: return False - team_id = (source.guild_id or "").strip() if source.platform == Platform.SLACK else "" + team_id = str(source.guild_id or "").strip() if source.platform == Platform.SLACK else "" platform_env_map = { Platform.TELEGRAM: "TELEGRAM_ALLOWED_USERS", diff --git a/gateway/session_context.py b/gateway/session_context.py index a65751a74d93..86d8787e38fd 100644 --- a/gateway/session_context.py +++ b/gateway/session_context.py @@ -36,6 +36,7 @@ platform = get_session_env("HERMES_SESSION_PLATFORM", "") """ +import os from contextvars import ContextVar from typing import Any @@ -145,8 +146,6 @@ def get_session_env(name: str, default: str = "") -> str: don't use ``set_session_vars`` at all). 3. *default* """ - import os - var = _VAR_MAP.get(name) if var is not None: value = var.get() @@ -170,6 +169,4 @@ def get_terminal_cwd(default: str = "") -> str: checkpoint manager to keep file/terminal operations rooted in the user's project rather than the gateway/cron process cwd. """ - import os - return os.environ.get("TERMINAL_CWD", "").strip() or default From 4198eb4c5664edf30e710bad2cc20730284bede8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 1 Jun 2026 20:53:41 +0000 Subject: [PATCH 3/3] review: enforce always-blocked floor for local browser backends Address Copilot review on #686. The cloud-metadata / IMDS floor (``is_always_blocked_url``) was being skipped for local backends via ``not _is_local_backend()`` at three sites in ``tools/browser_tool.py``. This contradicts the helper's contract ("blocked regardless of backend, routing, or allow_private_urls") and is a real credential-exfil risk: an agent on EC2/GCP/Azure with a local Chromium and ``browser.allow_private_urls: true`` could navigate to 169.254.169.254 and read IAM credentials via browser_snapshot. Drop the local-backend exception in: - ``_browser_url_security_block`` pre-nav guard - ``browser_navigate`` pre-nav guard - ``browser_navigate`` post-redirect guard The redundant duplicate floor check inside the camofox branch is folded back into the unified pre-nav floor. --- tools/browser_tool.py | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 2acb3b788e0e..4a959eba443e 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -126,12 +126,10 @@ def _browser_url_security_block(url: str, *, auto_local_this_nav: bool = False) ), } - if not _is_local_backend() and _is_always_blocked_url(url): + if _is_always_blocked_url(url): return {"success": False, "error": "Blocked: URL targets a cloud metadata endpoint"} if _is_camofox_mode(): - if _is_always_blocked_url(url): - return {"success": False, "error": "Blocked: URL targets a cloud metadata endpoint"} if not _is_safe_url(url): return {"success": False, "error": "Blocked: URL targets a private or internal address"} @@ -2302,23 +2300,19 @@ def browser_navigate(url: str, task_id: Optional[str] = None) -> str: # 169.254.169.254 / metadata.google.internal / ECS task metadata # via a browser, and routing those to a local Chromium sidecar # on an EC2/GCP/Azure host exfiltrates IAM credentials (#16234). - if not _is_local_backend() and _is_always_blocked_url(url): + # Local backends are NOT exempt — an agent on EC2 with a local + # Chromium and allow_private_urls=true could otherwise hit IMDS. + if _is_always_blocked_url(url): return json.dumps({ "success": False, "error": "Blocked: URL targets a cloud metadata endpoint", }) - if _is_camofox_mode(): - if _is_always_blocked_url(url): - return json.dumps({ - "success": False, - "error": "Blocked: URL targets a cloud metadata endpoint", - }) - if not _is_safe_url(url): - return json.dumps({ - "success": False, - "error": "Blocked: URL targets a private or internal address", - }) + if _is_camofox_mode() and not _is_safe_url(url): + return json.dumps({ + "success": False, + "error": "Blocked: URL targets a private or internal address", + }) if ( not auto_local_this_nav @@ -2382,11 +2376,10 @@ def browser_navigate(url: str, task_id: Optional[str] = None) -> str: # and for the hybrid local sidecar (we're already on a local browser # hitting a private URL by design). # Always-blocked floor (cloud metadata / IMDS) is enforced even - # when auto_local_this_nav is true — see pre-nav check for - # rationale (#16234). + # when auto_local_this_nav is true and for local backends — see + # pre-nav check for rationale (#16234). if ( - not _is_local_backend() - and final_url + final_url and final_url != url and _is_always_blocked_url(final_url) ):