[security] fix(bot): guard web_fetch remote targets#2051
Open
Hinotoi-agent wants to merge 1 commit into
Open
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This PR hardens the bot
web_fetchtool against private-network SSRF by applying OpenViking's shared network target guard to the server-side fetch path.web_fetchalready checked basic URL shape and scheme, but it then handed the URL tohttpx.AsyncClient(follow_redirects=True)without enforcing the same public-network boundary used by other remote-fetch surfaces. This meant a bot-triggered fetch could target loopback, private, link-local, or other non-public addresses, and redirect hops also needed to be validated before the outbound request was sent.This change:
web_fetchURLs before constructing the HTTP clientSecurity issues covered
web_fetchSSRFbot/vikingbot/agent/tools/web.pyBefore this PR
web_fetchacceptedhttp://andhttps://URLs after basic URL parsing.httpx.AsyncClient(follow_redirects=True)to fetch content server-side.After this PR
ensure_public_remote_target()before any HTTP client is created.build_httpx_request_validation_hooks(ensure_public_remote_target)so each outbound request, including redirects, is validated before it is sent.Why this matters
Bot fetch tools run on the server side. If a user-controlled or model-influenced URL can reach loopback, RFC1918/private, link-local, or reserved ranges, the tool can become an SSRF primitive against services that are reachable from the bot runtime but not from the original user.
That can expose internal admin panels, metadata services, local development services, service credentials, or other network-adjacent resources depending on deployment topology. Redirect validation is also important because an apparently public URL can redirect to a private address after the initial validation step.
How this differs from related issue/PR
This is related to the same trust-boundary family as the earlier resource-ingestion SSRF hardening in #1133, but it covers a separate fetch surface.
web_fetchtool inbot/vikingbot/agent/tools/web.py.openviking.utils.network_guardhelpers instead of adding a second network-policy implementation.#1596 also touched
bot/vikingbot/agent/tools/web.py, but that change focused on HTML parsing/runtime cleanup and did not add network target validation forweb_fetch.Attack flow
web_fetchtool.Affected code
bot/vikingbot/agent/tools/web.pyWebFetchTool.execute()httpx.AsyncClient(..., follow_redirects=True, ...)fetch pathRoot cause
CVSS assessment
web_fetchSSRF to non-public network targetsCVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:H/I:N/A:Nweb_fetch. Successful exploitation can expose confidential data reachable from the server-side runtime. Integrity and availability impact are not claimed by this PR.Safe reproduction steps
On vulnerable code, call the bot
web_fetchtool with a loopback URL such as:A safe local harness can replace the HTTP client with a failing stub and verify whether the tool attempts to construct a client for the blocked destination.
For redirect coverage, use a public-looking URL that redirects to a private target and verify that the request hook validates the redirect destination before the redirected request is sent.
Expected vulnerable behavior
Before this patch:
After this patch:
Changes in this PR
ensure_public_remote_target()andbuild_httpx_request_validation_hooks()fromopenviking.utils.network_guard.web_fetchURL before constructinghttpx.AsyncClient.Files changed
bot/vikingbot/agent/tools/web.pyweb_fetchrequests.bot/tests/test_web_fetch_network_guard.pyMaintainer impact
http://andhttps://fetches continue to work when they resolve to allowed public network targets.Fix rationale
The correct boundary is the outbound network target, not only URL syntax. Applying the shared guard at both layers avoids drift:
openviking.utils.network_guardkeeps the bot tool aligned with existing OpenViking SSRF policy instead of creating a duplicate implementationType of change
Test plan
Run locally:
PYTHONPATH=bot:. python3 -m pytest -q -o addopts='' bot/tests/test_web_fetch_network_guard.py tests/misc/test_network_guard.py python3 -m ruff format --check bot/vikingbot/agent/tools/web.py bot/tests/test_web_fetch_network_guard.py python3 -m ruff check bot/vikingbot/agent/tools/web.py bot/tests/test_web_fetch_network_guard.py git diff --checkResult:
The targeted pytest command uses
-o addopts=''to avoid unrelated local coverage-plugin/addopts issues while exercising the new bot regression tests and the shared network-guard test suite.Disclosure notes
This PR is intentionally bounded to the bot
web_fetchserver-side fetch path. It does not claim to audit every remote-fetch surface in the repository. It reuses the existing shared network guard and covers the separate bot tool surface that was not addressed by the earlier resource-ingestion SSRF hardening.