fix(gateway,tools): restore broken auth/session APIs and web provider wiring#686
fix(gateway,tools): restore broken auth/session APIs and web provider wiring#686badMade wants to merge 4 commits into
Conversation
… wiring
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.
🔎 Lint report:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several changes, including a security check in the webhook platform to reject unresolved placeholder secrets, a new helper to retrieve the active terminal working directory, and the removal of the DuckDuckGo Search (ddgs) backend from the auto-detection candidates to prevent unintended downgrades. Feedback on these changes suggests casting source.guild_id to a string in gateway/run.py to prevent potential AttributeError crashes, and removing a redundant local os import in gateway/session_context.py.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
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``.
There was a problem hiding this comment.
Pull request overview
This PR addresses a cluster of gateway/tools regressions by restoring missing compatibility shims, fixing auth/session API breakage, and tightening provider/security wiring so existing integrations and tests work again.
Changes:
- Web backend selection: stop auto-detecting
ddgs, rename the availability probe to_ddgs_package_available, and keep a backward-compat alias. - Browser + session compatibility: apply the pre-navigation SSRF guard consistently for local navigation paths, and reintroduce
get_terminal_cwd()for callers that import it. - Webhook hardening + gateway auth fix: reject unresolved
${VAR}placeholder secrets during webhook signature validation, and prevent ateam_idNameErrorin gateway authorization checks.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/web_tools.py | Removes ddgs from auto-detect fallback; renames the ddgs import probe with a compat alias. |
| tools/browser_tool.py | Adjusts SSRF pre-navigation gating so local navigation doesn’t skip the private/internal URL guard. |
| gateway/session_context.py | Restores get_terminal_cwd(default="") to fix import compatibility for run_agent initialization paths. |
| gateway/run.py | Defines Slack team_id early in _is_user_authorized to prevent runtime NameError in existing branches. |
| gateway/platforms/webhook.py | Rejects unresolved ${ENV_VAR} placeholder secrets before HMAC validation to avoid accepting guessable literals. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) | ||
| ): |
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.
Auto-merge: checks failingThe following checks did not pass:
Please fix the failing checks before this PR can be merged. |
|
@copilot resolve the merge conflicts in this pull request |
…n-fWaQ3 # Conflicts: # gateway/platforms/webhook.py # gateway/run.py # gateway/session_context.py # tools/web_tools.py
Fixes the regression cluster from #682 — restores compatibility broken by recent refactors. Cuts gateway/tools test failures from 102 → 49 (53 tests fixed; remaining 49 are pre-existing, unrelated to this PR).
Summary
gateway/run.py: defineteam_idat the top of_is_user_authorizedfromsource.guild_id(Slack-scoped) so the existingif team_id:branches no longer crash withNameError. This was the root cause of ~40 gateway auth test failures (test_unauthorized_dm_behavior,test_discord_bot_auth_bypass,test_feishu_bot_auth_bypass,test_google_chat,test_signal,test_wecom_callback,test_pre_gateway_dispatch,test_internal_event_bypass_pairing).gateway/session_context.py: addget_terminal_cwd(default="").run_agent.pyimports it in 4 places (context-file discovery, checkpoint manager) and was raisingImportErroron agent init, breakingtests/cron/test_codex_execution_paths.py.gateway/platforms/webhook.py: reject unresolved${WEBHOOK_SECRET}-style env-template placeholders before HMAC validation. Without this, the literal placeholder string is used as the secret — anyone who guesses it can forge a signature. Fixestest_validate_placeholder_secret_rejects_literal_hmac.tools/web_tools.py: rename_ddgs_package_importable→_ddgs_package_available(with a backward-compat alias for older tests/callers), and drop ddgs from the auto-detect fallback. ddgs ships in dev extras, so its mere presence insys.moduleswas silently downgrading installs that never asked for DuckDuckGo. ddgs is still picked when explicitly configured viaweb.backend: ddgs. Fixes all 8test_web_providers_ddgs.pyfailures.tools/browser_tool.py: enforce the pre-navigation SSRF check for local backends too, matching the file's own comment ("Local browser backends still enforce this guard by default"). Without this, local mode skipped the pre-nav block and only caught a private redirect in the post-nav check, masking the original URL in the error. Fixes 2test_browser_ssrf_local.pyregressions.Test plan
tests/gateway/test_session_env.py— 12 passedtests/gateway/test_unauthorized_dm_behavior.py— 26 passed (was 22 failing)tests/gateway/test_webhook_adapter.py— 55 passed (was 1 failing)tests/tools/test_web_providers_ddgs.py— 19 passed (was 8 failing)tests/tools/test_web_providers_brave_free.py— 22 passed (compat alias keeps it green)tests/tools/test_browser_ssrf_local.py— 23 passed (was 2 failing)tests/cron/test_codex_execution_paths.py— 2 passed (was 2 failing)tests/cron/full sweep — 344 passedtests/gateway/test_slack.py,tests/gateway/test_slack_approval_buttons.py— 205 passed (no regressions to Slack workspace scoping)tests/gateway/ tests/tools/sweep — 10,210 passed, 49 failed (all pre-existing, verified against baseline)https://claude.ai/code/session_01BKdJmbJ3AxAjUWESqrWUsT
Generated by Claude Code