Skip to content

Commit 792775f

Browse files
cchinchilla-devboyangsvl
authored andcommitted
feat: add request timeout to load_web_page
Merge #4887 ## Link to Issue or Description of Change Closes #4886 ## Update — 2026-04-30 After merging the latest `upstream/main`, the SSRF rewrite already covers URL-scheme validation and routes every fetch failure through a unified `Failed to fetch url` message. The unique contribution of this PR is now the request **timeout**; the body and tests below have been updated to reflect the post-merge scope. No code added by this PR duplicates what upstream already provides. ## Problem `load_web_page()` calls `requests.get()` without a `timeout`. If the target server is unresponsive, the agent hangs indefinitely. ## Solution Add `timeout=_DEFAULT_TIMEOUT_SECONDS` (30 seconds) to both HTTP entry points in the module: - `requests.get` in `_fetch_response` (proxy path). - `session.get` in `_fetch_direct_response` (pinned-IP path). Extend the `except` in `load_web_page` to also catch `requests.RequestException`, so timeout and connection errors return the standard `Failed to fetch url: {url}` message instead of propagating. **Design note:** the timeout is a module-level constant rather than a function parameter to keep it out of the LLM function-calling schema. It can be overridden via `load_web_page._DEFAULT_TIMEOUT_SECONDS = 30` if needed. ## Testing Plan ### Unit Tests - [x] Added/updated unit tests. - [x] All unit tests pass locally (`pytest tests/unittests/tools/test_load_web_page.py` → 10 passed). New/updated tests in `tests/unittests/tools/test_load_web_page.py`: - `test_load_web_page_uses_proxy_for_unresolved_public_hostnames` — updated to verify `timeout=10` is forwarded on the proxy path. - `test_load_web_page_passes_timeout_to_pinned_session` — verifies the timeout reaches the pinned-IP session. - `test_load_web_page_passes_timeout_to_proxied_get` — verifies the timeout is forwarded when a proxy is configured. - `test_load_web_page_returns_failure_on_timeout` — verifies `requests.exceptions.Timeout` is converted into `Failed to fetch url`. ### Manual E2E N/A — internal hardening; function signature unchanged. ## Checklist - [x] I have read the CONTRIBUTING.md document. - [x] I have performed a self-review of my own code. - [x] I have added tests that prove my fix is effective. - [x] New and existing unit tests pass locally with my changes. ## Additional Context This complements the existing SSRF protection (`allow_redirects=False`, hostname/IP validation, pinned-IP adapter) already present in the module after upstream/main was merged. Co-authored-by: Bo Yang <ybo@google.com> COPYBARA_INTEGRATE_REVIEW=#4887 from cchinchilla-dev:feat/load-web-page-timeout-and-url-validation 4bd4799 PiperOrigin-RevId: 930335977
1 parent 3e9f3da commit 792775f

2 files changed

Lines changed: 131 additions & 3 deletions

File tree

src/google/adk/tools/load_web_page.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@
3030

3131
_ALLOWED_URL_SCHEMES = frozenset({'http', 'https'})
3232
_DEFAULT_PORT_BY_SCHEME = {'http': 80, 'https': 443}
33+
# Default timeout in seconds for HTTP requests.
34+
_DEFAULT_TIMEOUT_SECONDS = 30
3335
_ResolvedAddress = ipaddress.IPv4Address | ipaddress.IPv6Address
3436

3537

@@ -230,6 +232,7 @@ def _fetch_direct_response(
230232
url,
231233
allow_redirects=False,
232234
proxies={'http': None, 'https': None},
235+
timeout=_DEFAULT_TIMEOUT_SECONDS,
233236
)
234237
except requests.RequestException as exc:
235238
last_error = exc
@@ -253,7 +256,9 @@ def _fetch_response(url: str) -> requests.Response:
253256
# localhost-style names can be rejected locally without breaking proxy use.
254257
if parsed_ip_literal is not None and _is_blocked_address(parsed_ip_literal):
255258
raise ValueError(f'Blocked host: {target.hostname}')
256-
return requests.get(url, allow_redirects=False)
259+
return requests.get(
260+
url, allow_redirects=False, timeout=_DEFAULT_TIMEOUT_SECONDS
261+
)
257262

258263
if parsed_ip_literal is not None:
259264
if _is_blocked_address(parsed_ip_literal):
@@ -285,7 +290,7 @@ def load_web_page(url: str) -> str:
285290

286291
try:
287292
response = _fetch_response(url)
288-
except ValueError:
293+
except (ValueError, requests.RequestException):
289294
return _failed_to_fetch_message(url)
290295

291296
# Set allow_redirects=False to prevent SSRF attacks via redirection.

tests/unittests/tools/test_load_web_page.py

Lines changed: 124 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,9 @@ def test_load_web_page_uses_proxy_for_unresolved_public_hostnames(monkeypatch):
147147

148148
assert result == 'This page has enough words to keep.'
149149
mock_get.assert_called_once_with(
150-
'https://does-not-resolve.invalid', allow_redirects=False
150+
'https://does-not-resolve.invalid',
151+
allow_redirects=False,
152+
timeout=load_web_page_module._DEFAULT_TIMEOUT_SECONDS,
151153
)
152154
mock_send.assert_not_called()
153155

@@ -272,3 +274,124 @@ def _send(
272274
'https://93.184.216.35',
273275
]
274276
mock_get.assert_not_called()
277+
278+
279+
def test_load_web_page_passes_timeout_to_pinned_session(monkeypatch):
280+
"""Verify that the default timeout is passed to the pinned IP session."""
281+
_clear_proxy_env(monkeypatch)
282+
monkeypatch.setattr(
283+
load_web_page_module.socket,
284+
'getaddrinfo',
285+
mock.Mock(
286+
return_value=[(
287+
socket.AF_INET,
288+
socket.SOCK_STREAM,
289+
socket.IPPROTO_TCP,
290+
'',
291+
('93.184.216.34', 0),
292+
)]
293+
),
294+
)
295+
monkeypatch.setattr(
296+
'bs4.BeautifulSoup',
297+
mock.Mock(
298+
return_value=mock.Mock(
299+
get_text=mock.Mock(
300+
return_value='This page has enough words to keep.'
301+
)
302+
)
303+
),
304+
)
305+
captured_timeouts: list[object] = []
306+
307+
def _send(
308+
self,
309+
request,
310+
stream=False,
311+
timeout=None,
312+
verify=True,
313+
cert=None,
314+
proxies=None,
315+
):
316+
del self, request, stream, verify, cert, proxies
317+
captured_timeouts.append(timeout)
318+
return _create_response(
319+
'<html><body><p>This page has enough words to keep.</p></body></html>'
320+
)
321+
322+
monkeypatch.setattr(load_web_page_module.HTTPAdapter, 'send', _send)
323+
324+
load_web_page('https://example.com')
325+
326+
assert captured_timeouts == [load_web_page_module._DEFAULT_TIMEOUT_SECONDS]
327+
328+
329+
def test_load_web_page_passes_timeout_to_proxied_get(monkeypatch):
330+
"""Verify that the default timeout is passed to requests.get when proxy is used."""
331+
monkeypatch.setenv('HTTPS_PROXY', 'http://proxy.example.test:8080')
332+
monkeypatch.setenv('NO_PROXY', '')
333+
monkeypatch.setattr(
334+
load_web_page_module.socket,
335+
'getaddrinfo',
336+
mock.Mock(side_effect=AssertionError('unexpected local DNS lookup')),
337+
)
338+
monkeypatch.setattr(
339+
'bs4.BeautifulSoup',
340+
mock.Mock(
341+
return_value=mock.Mock(
342+
get_text=mock.Mock(
343+
return_value='This page has enough words to keep.'
344+
)
345+
)
346+
),
347+
)
348+
mock_get = mock.Mock(
349+
return_value=_create_response(
350+
'<html><body><p>This page has enough words to keep.</p></body></html>'
351+
)
352+
)
353+
monkeypatch.setattr(load_web_page_module.requests, 'get', mock_get)
354+
355+
load_web_page('https://does-not-resolve.invalid')
356+
357+
mock_get.assert_called_once_with(
358+
'https://does-not-resolve.invalid',
359+
allow_redirects=False,
360+
timeout=load_web_page_module._DEFAULT_TIMEOUT_SECONDS,
361+
)
362+
363+
364+
def test_load_web_page_returns_failure_on_timeout(monkeypatch):
365+
"""Verify that a timeout exception is converted to a failed to fetch message."""
366+
_clear_proxy_env(monkeypatch)
367+
monkeypatch.setattr(
368+
load_web_page_module.socket,
369+
'getaddrinfo',
370+
mock.Mock(
371+
return_value=[(
372+
socket.AF_INET,
373+
socket.SOCK_STREAM,
374+
socket.IPPROTO_TCP,
375+
'',
376+
('93.184.216.34', 0),
377+
)]
378+
),
379+
)
380+
381+
def _send(
382+
self,
383+
request,
384+
stream=False,
385+
timeout=None,
386+
verify=True,
387+
cert=None,
388+
proxies=None,
389+
):
390+
del self, request, stream, timeout, verify, cert, proxies
391+
raise requests.exceptions.Timeout('boom')
392+
393+
monkeypatch.setattr(load_web_page_module.HTTPAdapter, 'send', _send)
394+
395+
result = load_web_page('https://example.com')
396+
397+
assert result == 'Failed to fetch url: https://example.com'

0 commit comments

Comments
 (0)