diff --git a/application/agents/tool_executor.py b/application/agents/tool_executor.py index 66be5d1a..18472612 100644 --- a/application/agents/tool_executor.py +++ b/application/agents/tool_executor.py @@ -53,6 +53,47 @@ def truncate_tool_result(value: Any) -> Any: return f"{text[:PERSISTED_RESULT_MAX_LEN]}..." +# Control characters minus \t \n \r. NULs in particular are rejected by +# Postgres text/jsonb, so one binary-carrying tool result would otherwise +# kill every write lane it fans out to (conversation, activity log, +# tool_call_attempts) at once. +_CONTROL_CHARS = re.compile(r"[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]") + +# Ceiling on the persisted full copy of a tool result (``result_full`` in +# the conversation row and the ``tool_call_attempts`` result). Generous — +# the LLM copy is bounded separately at TOOL_RESULT_MAX_TOKENS — but a +# ceiling all the same: one uncapped result has reached 634k tokens. +RESULT_FULL_MAX_CHARS = 400_000 + + +def sanitize_tool_result(value: Any) -> Any: + """Recursively strip NUL/control characters from strings in ``value``. + + Applied once where a tool result enters the executor, so every + downstream consumer — the LLM copy, the conversation row, the + ``tool_call_attempts`` journal, the stream event — gets clean text. + """ + if isinstance(value, str): + return _CONTROL_CHARS.sub("", value) if _CONTROL_CHARS.search(value) else value + if isinstance(value, dict): + return {sanitize_tool_result(k): sanitize_tool_result(v) for k, v in value.items()} + if isinstance(value, list): + return [sanitize_tool_result(item) for item in value] + if isinstance(value, tuple): + return tuple(sanitize_tool_result(item) for item in value) + return value + + +def bound_result_full(text: str) -> str: + """Cap the persisted full-result copy at ``RESULT_FULL_MAX_CHARS``.""" + if len(text) <= RESULT_FULL_MAX_CHARS: + return text + return ( + text[:RESULT_FULL_MAX_CHARS] + + f"\n[... tool result truncated at persistence: {len(text)} chars ...]" + ) + + def result_status(result: Any) -> str: """Derive the persisted status from a tool's result payload. @@ -882,6 +923,11 @@ class ToolExecutor: ) raise + # Single fan-out point: from here ``result`` reaches the LLM copy, + # the conversation row, tool_call_attempts, and the stream event — + # sanitize once so every lane gets clean text. + result = sanitize_tool_result(result) + get_artifact_id = getattr(tool, "get_artifact_id", None) if tool_data["name"] != "api_tool" else None artifact_id = None @@ -898,7 +944,7 @@ class ToolExecutor: artifact_id = str(artifact_id).strip() if artifact_id is not None else "" if artifact_id: tool_call_data["artifact_id"] = artifact_id - result_full = str(result) + result_full = bound_result_full(str(result)) tool_call_data["resolved_arguments"] = resolved_arguments tool_call_data["result_full"] = result_full tool_call_data["result"] = truncate_tool_result(result_full) diff --git a/application/agents/tools/read_webpage.py b/application/agents/tools/read_webpage.py index ea71076f..c8b25227 100644 --- a/application/agents/tools/read_webpage.py +++ b/application/agents/tools/read_webpage.py @@ -1,6 +1,107 @@ +import codecs + from markdownify import markdownify + from application.agents.tools.base import Tool -from application.security.safe_url import UnsafeUserUrlError, pinned_request +from application.security.safe_url import ( + ResponseTooLargeError, + UnsafeUserUrlError, + pinned_fetch_bytes, +) + +# Byte ceiling for a fetched page. One uncapped response (a PDF handed to +# this tool on 07-17) produced a 634k-token "text" result that killed the +# conversation save and every other write lane downstream. +MAX_RESPONSE_BYTES = 10 * 1024 * 1024 + +# Non-``text/*`` content types markdownify can still meaningfully convert +# to text (any ``text/*`` is allowed by prefix). Anything else (PDFs, +# images, archives, ...) must be refused with a clear error so the model +# picks another tool instead of receiving binary-as-text. +_ALLOWED_CONTENT_TYPES = frozenset( + { + "application/xhtml+xml", + "application/xml", + "application/json", + "application/ld+json", + "application/rss+xml", + "application/atom+xml", + } +) + +# Magic prefixes of common binary formats, checked in case the server +# omits or lies about the Content-Type header. +_BINARY_MAGIC_PREFIXES = ( + b"%PDF-", # PDF + b"PK\x03\x04", # zip / docx / xlsx + b"\x89PNG", # PNG + b"GIF8", # GIF + b"\xff\xd8\xff", # JPEG + b"\x1f\x8b", # gzip + b"OggS", # ogg + b"\x00\x01\x00\x00", # ttf +) + +_UNSUPPORTED_CONTENT_HINT = ( + "read_webpage only handles HTML/text pages; use a tool suited to this " + "file type instead." +) + + +def _declared_charset(content_type: str) -> str | None: + """Extract the ``charset`` parameter from a Content-Type header value.""" + for part in content_type.split(";")[1:]: + key, _, value = part.strip().partition("=") + if key.strip().lower() == "charset" and value: + return value.strip("\"' ") + return None + + +def _usable_charset(content_type: str) -> str | None: + """Return the declared charset if Python can actually decode with it.""" + charset = _declared_charset(content_type) + if not charset: + return None + try: + codecs.lookup(charset) + except LookupError: + return None + return charset + + +def _is_allowed_media_type(media_type: str) -> bool: + return media_type.startswith("text/") or media_type in _ALLOWED_CONTENT_TYPES + + +def _looks_binary(content: bytes, *, trust_declared_charset: bool = False) -> bool: + """Sniff the first KB for binary magic numbers or NUL bytes. + + The magic-prefix check is unconditional — a lying charset header must + not sneak a PDF through. The NUL check only applies when no usable + charset was declared: legitimate UTF-16 text is NUL-dense, and a + declared charset means the decode path (which strips NULs) handles it. + """ + head = content[:1024] + if head.startswith(_BINARY_MAGIC_PREFIXES): + return True + if trust_declared_charset: + return False + return b"\x00" in head + + +def _decode_body(content: bytes, content_type: str) -> str: + """Decode ``content`` with the declared charset, else UTF-8. + + Never falls back to ``response.text``'s guesses: requests' RFC-2616 + ISO-8859-1 default (text/* without charset) maps every raw byte 1:1, + which is exactly how a PDF once round-tripped into NUL-laden "text". + NULs are stripped after decoding (NUL is valid UTF-8, and the sniff + only sees the first KB) so the tool never returns them regardless of + caller. + """ + charset = _usable_charset(content_type) or "utf-8" + return content.decode(charset, errors="replace").replace("\x00", "") + class ReadWebpageTool(Tool): """ @@ -31,21 +132,42 @@ class ReadWebpageTool(Tool): return "Error: URL parameter is missing." try: - response = pinned_request( - "GET", + content, response = pinned_fetch_bytes( url, + max_bytes=MAX_RESPONSE_BYTES, headers={'User-Agent': 'DocsGPT-Agent/1.0'}, timeout=10, ) response.raise_for_status() - html_content = response.text + content_type = response.headers.get("Content-Type", "") + media_type = content_type.split(";")[0].strip().lower() + if media_type and not _is_allowed_media_type(media_type): + return ( + f"Error: URL returned content type '{media_type}', which " + f"cannot be converted to text. {_UNSUPPORTED_CONTENT_HINT}" + ) + if _looks_binary( + content, + trust_declared_charset=_usable_charset(content_type) is not None, + ): + return ( + "Error: URL returned binary content, which cannot be " + f"converted to text. {_UNSUPPORTED_CONTENT_HINT}" + ) + + html_content = _decode_body(content, content_type) markdown_content = markdownify(html_content, heading_style="ATX", newline_style="BACKSLASH") return markdown_content except UnsafeUserUrlError as e: return f"Error: URL validation failed - {e}" + except ResponseTooLargeError: + return ( + f"Error: The page is too large to read (over " + f"{MAX_RESPONSE_BYTES // (1024 * 1024)} MB)." + ) except Exception as e: return f"Error fetching URL {url}: {e}" @@ -59,8 +181,9 @@ class ReadWebpageTool(Tool): "description": ( "Fetch a webpage and return its content as clean Markdown " "text. Use it whenever the user shares a URL or the answer " - "depends on a specific page. Input must be a fully " - "qualified URL." + "depends on a specific page. Only works for HTML/text " + "pages — not PDFs or other binary files. Input must be a " + "fully qualified URL." ), "parameters": { "type": "object", diff --git a/application/api/answer/services/conversation_service.py b/application/api/answer/services/conversation_service.py index 1ca64955..f116a274 100644 --- a/application/api/answer/services/conversation_service.py +++ b/application/api/answer/services/conversation_service.py @@ -21,6 +21,7 @@ from application.storage.db.repositories.conversations import ( MessageUpdateOutcome, ) from application.storage.db.session import db_readonly, db_session +from application.utils import strip_null_bytes logger = logging.getLogger(__name__) @@ -338,14 +339,18 @@ class ConversationService: "error", f"{type(error).__name__}: {str(error)}" ) - update_fields: Dict[str, Any] = { - "response": response, - "status": status, - "thought": thought, - "sources": sources, - "tool_calls": tool_calls or [], - "metadata": merged_metadata, - } + # Postgres rejects NUL in text and jsonb; a NUL-laden tool result + # reaching this transaction would lose the whole conversation save. + update_fields: Dict[str, Any] = strip_null_bytes( + { + "response": response, + "status": status, + "thought": thought, + "sources": sources, + "tool_calls": tool_calls or [], + "metadata": merged_metadata, + } + ) if model_id is not None: update_fields["model_id"] = model_id diff --git a/application/api/user/tasks.py b/application/api/user/tasks.py index 7b45c7cc..fd577e24 100644 --- a/application/api/user/tasks.py +++ b/application/api/user/tasks.py @@ -1,6 +1,8 @@ import logging from datetime import timedelta +from sqlalchemy.exc import DataError + from application.api.user.idempotency import with_idempotency from application.celery_init import celery from application.worker import ( @@ -233,7 +235,10 @@ def _emit_attachment_poison_event(task_name, bound): ) -@celery.task(**DURABLE_TASK) +# ``dont_autoretry_for``: a DataError is deterministic (poison payload, +# e.g. NUL bytes or an over-long value) — retrying re-fails identically +# and multiplies log noise, so it goes straight to the failure path. +@celery.task(**DURABLE_TASK, dont_autoretry_for=(DataError,)) @with_idempotency( task_name="store_attachment", on_poison=_emit_attachment_poison_event, ) diff --git a/application/security/safe_url.py b/application/security/safe_url.py index a4b9eed9..974f928f 100644 --- a/application/security/safe_url.py +++ b/application/security/safe_url.py @@ -79,6 +79,12 @@ class UnsafeUserUrlError(ValueError): """ +class ResponseTooLargeError(Exception): + """Raised by :func:`pinned_fetch_bytes` when a response body exceeds + the caller's byte ceiling (declared via ``Content-Length`` or found + during the bounded read).""" + + def _strip_ipv6_brackets(host: str) -> str: """Return ``host`` with surrounding ``[`` / ``]`` removed if present.""" @@ -348,6 +354,84 @@ def pinned_request( session.close() +def pinned_fetch_bytes( + url: str, + *, + max_bytes: int, + headers: dict[str, str] | None = None, + timeout: float = 10.0, + allow_redirects: bool = False, +) -> tuple[bytes, requests.Response]: + """GET ``url`` with SSRF pinning, streaming at most ``max_bytes`` of body. + + The size limit is enforced twice: a declared ``Content-Length`` above + the ceiling is rejected before any body is read, and the streamed + read itself stops the moment the ceiling is crossed — a server that + lies about (or omits) ``Content-Length`` cannot bypass the cap. + + Returns: + Tuple of the body bytes and the (already-closed) ``Response``, + kept for its status code and headers. + + Raises: + UnsafeUserUrlError: If the URL fails the SSRF guard. + ResponseTooLargeError: If the body exceeds ``max_bytes``. + requests.RequestException: For network-level failures. + """ + + host, ip, parts = _validate_and_pick_ip(url) + + netloc = f"{_url_userinfo_prefix(parts.netloc)}{_ip_to_url_host(ip)}" + if parts.port is not None: + netloc = f"{netloc}:{parts.port}" + pinned_url = urlunsplit( + (parts.scheme, netloc, parts.path, parts.query, parts.fragment) + ) + + request_headers = dict(headers or {}) + host_header = host if parts.port is None else f"{host}:{parts.port}" + request_headers["Host"] = host_header + + session = requests.Session() + if parts.scheme == "https": + session.mount("https://", _PinnedHostAdapter(host)) + try: + response = session.request( + method="GET", + url=pinned_url, + headers=request_headers, + timeout=timeout, + allow_redirects=allow_redirects, + stream=True, + ) + try: + # An error response's body is never used (callers + # raise_for_status) — skip the transfer and let the caller + # surface the more informative HTTP-status error. + if response.status_code >= 400: + return b"", response + declared = response.headers.get("Content-Length") + if declared and declared.isdigit() and int(declared) > max_bytes: + raise ResponseTooLargeError( + f"response declares {declared} bytes, over the " + f"{max_bytes}-byte limit" + ) + chunks: list[bytes] = [] + received = 0 + for chunk in response.iter_content(chunk_size=65536): + received += len(chunk) + if received > max_bytes: + raise ResponseTooLargeError( + f"response body exceeds the {max_bytes}-byte limit" + ) + chunks.append(chunk) + return b"".join(chunks), response + finally: + response.close() + finally: + session.close() + + def pinned_post( url: str, *, diff --git a/application/storage/db/repositories/attachments.py b/application/storage/db/repositories/attachments.py index 08662920..eeddd5e9 100644 --- a/application/storage/db/repositories/attachments.py +++ b/application/storage/db/repositories/attachments.py @@ -8,6 +8,7 @@ from typing import Any, Optional from sqlalchemy import Connection, text from application.storage.db.base_repository import looks_like_uuid, row_to_dict +from application.utils import strip_null_bytes _UPDATABLE_SCALARS = { @@ -69,15 +70,21 @@ class AttachmentsRepository: ), { "user_id": user_id, - "filename": filename, + "filename": strip_null_bytes(filename), "upload_path": upload_path, "mime_type": mime_type, "size": size, - "content": content, + # A parsed document with embedded NULs (mislabeled binary) + # would otherwise fail the INSERT — Postgres rejects \x00. + "content": strip_null_bytes(content), "token_count": token_count, "openai_file_id": openai_file_id, "google_file_uri": google_file_uri, - "metadata": json.dumps(metadata) if metadata is not None else None, + "metadata": ( + json.dumps(strip_null_bytes(metadata)) + if metadata is not None + else None + ), "legacy_mongo_id": legacy_mongo_id, }, ) @@ -193,6 +200,7 @@ class AttachmentsRepository: set_clauses: list[str] = [] params: dict = {"id": attachment_id, "user_id": user_id} for col, val in filtered.items(): + val = strip_null_bytes(val) if col in _UPDATABLE_JSONB: set_clauses.append(f"{col} = CAST(:{col} AS jsonb)") params[col] = json.dumps(val) if val is not None else None @@ -247,6 +255,7 @@ class AttachmentsRepository: set_clauses: list[str] = [] params: dict = {"legacy_id": legacy_mongo_id, "user_id": user_id} for col, val in filtered.items(): + val = strip_null_bytes(val) if col in _UPDATABLE_JSONB: set_clauses.append(f"{col} = CAST(:{col} AS jsonb)") params[col] = json.dumps(val) if val is not None else None diff --git a/application/storage/db/repositories/stack_logs.py b/application/storage/db/repositories/stack_logs.py index fc1a6387..a5ac17fc 100644 --- a/application/storage/db/repositories/stack_logs.py +++ b/application/storage/db/repositories/stack_logs.py @@ -15,9 +15,28 @@ from typing import Optional from application.storage.db.redaction import redact_secrets from application.storage.db.serialization import PGNativeJSONEncoder +from application.utils import strip_null_bytes from sqlalchemy import Connection, text +# Longest string kept inside ``stacks``. The scalar columns are truncated +# by the caller, but stacks used to go in whole — one uncapped tool +# result (634k tokens, 07-17) rode into the activity log through here. +_STACKS_STRING_MAX_LEN = 10000 + + +def _bound_strings(value): + """Recursively truncate strings in ``value`` to ``_STACKS_STRING_MAX_LEN``.""" + if isinstance(value, str): + if len(value) <= _STACKS_STRING_MAX_LEN: + return value + return value[:_STACKS_STRING_MAX_LEN] + "...[truncated]" + if isinstance(value, dict): + return {k: _bound_strings(v) for k, v in value.items()} + if isinstance(value, (list, tuple)): + return [_bound_strings(item) for item in value] + return value + class StackLogsRepository: """Postgres-backed replacement for Mongo ``stack_logs`` collection.""" @@ -50,13 +69,16 @@ class StackLogsRepository: ), { "activity_id": activity_id, - "endpoint": endpoint, - "level": level, - "user_id": user_id, - "api_key": api_key, - "query": query, + "endpoint": strip_null_bytes(endpoint), + "level": strip_null_bytes(level), + "user_id": strip_null_bytes(user_id), + "api_key": strip_null_bytes(api_key), + "query": strip_null_bytes(query), "stacks": json.dumps( - redact_secrets(stacks or []), cls=PGNativeJSONEncoder + redact_secrets( + _bound_strings(strip_null_bytes(stacks or [])) + ), + cls=PGNativeJSONEncoder, ), "timestamp": timestamp, }, diff --git a/application/storage/db/repositories/tool_call_attempts.py b/application/storage/db/repositories/tool_call_attempts.py index 0954d733..3ead533b 100644 --- a/application/storage/db/repositories/tool_call_attempts.py +++ b/application/storage/db/repositories/tool_call_attempts.py @@ -8,6 +8,12 @@ from typing import Any, Optional from sqlalchemy import Connection, text from application.storage.db.serialization import PGNativeJSONEncoder +from application.utils import strip_null_bytes + + +def _dump_jsonb(value: Any) -> str: + """Serialize for a jsonb CAST, stripping NULs Postgres would reject.""" + return json.dumps(strip_null_bytes(value), cls=PGNativeJSONEncoder) class ToolCallAttemptsRepository: @@ -56,7 +62,7 @@ class ToolCallAttemptsRepository: "tool_id": tool_id, "tool_name": tool_name, "action_name": action_name, - "arguments": json.dumps(arguments if arguments is not None else {}, cls=PGNativeJSONEncoder), + "arguments": _dump_jsonb(arguments if arguments is not None else {}), "message_id": message_id, "user_id": user_id, "agent_id": agent_id, @@ -120,8 +126,8 @@ class ToolCallAttemptsRepository: "tool_id": tool_id, "tool_name": tool_name, "action_name": action_name, - "arguments": json.dumps(arguments if arguments is not None else {}, cls=PGNativeJSONEncoder), - "result": json.dumps(result_payload, cls=PGNativeJSONEncoder), + "arguments": _dump_jsonb(arguments if arguments is not None else {}), + "result": _dump_jsonb(result_payload), "message_id": message_id, "user_id": user_id, "agent_id": agent_id, @@ -162,7 +168,7 @@ class ToolCallAttemptsRepository: params: dict[str, Any] = { "call_id": call_id, "status": status, - "result": json.dumps(result_payload, cls=PGNativeJSONEncoder), + "result": _dump_jsonb(result_payload), } if message_id is not None: sql += ", message_id = CAST(:message_id AS uuid)" @@ -191,7 +197,10 @@ class ToolCallAttemptsRepository: "UPDATE tool_call_attempts SET status = 'failed', error = :error " "WHERE call_id = :call_id AND status = 'proposed'" ) - params: dict[str, Any] = {"call_id": call_id, "error": error} + params: dict[str, Any] = { + "call_id": call_id, + "error": strip_null_bytes(error), + } if user_id is not None: sql += " AND user_id IS NOT DISTINCT FROM :user_id" params["user_id"] = user_id diff --git a/application/streaming/message_journal.py b/application/streaming/message_journal.py index 30503d03..ef0e94b4 100644 --- a/application/streaming/message_journal.py +++ b/application/streaming/message_journal.py @@ -21,6 +21,7 @@ from application.storage.db.session import db_readonly, db_session from application.streaming.broadcast_channel import Topic from application.streaming.event_replay import encode_pubsub_message from application.streaming.keys import message_topic_name +from application.utils import strip_null_bytes as _strip_null_bytes logger = logging.getLogger(__name__) @@ -34,29 +35,9 @@ DEFAULT_BATCH_SIZE = 16 DEFAULT_BATCH_INTERVAL_MS = 100 -def _strip_null_bytes(value: Any) -> Any: - """Recursively strip ``\\x00`` from string keys/values in ``value``. - - Postgres JSONB rejects the NUL escape; an LLM emitting a stray NUL - in a chunk would otherwise raise ``DataError`` at INSERT and the row - would be lost from the journal (live stream proceeds, reconnect - snapshot misses the chunk). Mirrors the strip already done in - ``parser/embedding_pipeline.py`` and - ``api/user/attachments/routes.py``. - """ - if isinstance(value, str): - return value.replace("\x00", "") if "\x00" in value else value - if isinstance(value, dict): - return { - (k.replace("\x00", "") if isinstance(k, str) and "\x00" in k else k): - _strip_null_bytes(v) - for k, v in value.items() - } - if isinstance(value, list): - return [_strip_null_bytes(item) for item in value] - if isinstance(value, tuple): - return tuple(_strip_null_bytes(item) for item in value) - return value +# NUL stripping lives in ``application.utils.strip_null_bytes`` (shared +# with the conversation-finalize, activity-log, tool_call_attempts, and +# attachments write lanes); imported above as ``_strip_null_bytes``. # Postgres SQLSTATE for a foreign-key violation. ``message_events`` has diff --git a/application/utils.py b/application/utils.py index 176f7720..eea3569c 100644 --- a/application/utils.py +++ b/application/utils.py @@ -55,6 +55,30 @@ def safe_filename(filename): return safe_name +def strip_null_bytes(value): + """Recursively strip ``\\x00`` from string keys/values in ``value``. + + Postgres rejects NUL in both text and jsonb; one NUL-laden payload + (e.g. a binary response mis-decoded to text) would otherwise raise + ``DataError`` and lose the whole row. Shared by the message journal, + conversation finalize, activity log, tool_call_attempts, and + attachments write lanes. + """ + if isinstance(value, str): + return value.replace("\x00", "") if "\x00" in value else value + if isinstance(value, dict): + return { + (k.replace("\x00", "") if isinstance(k, str) and "\x00" in k else k): + strip_null_bytes(v) + for k, v in value.items() + } + if isinstance(value, list): + return [strip_null_bytes(item) for item in value] + if isinstance(value, tuple): + return tuple(strip_null_bytes(item) for item in value) + return value + + def num_tokens_from_string(string: str) -> int: encoding = get_encoding() if isinstance(string, str): diff --git a/tests/agents/test_read_webpage_tool.py b/tests/agents/test_read_webpage_tool.py index ae6e7ab4..f0560f00 100644 --- a/tests/agents/test_read_webpage_tool.py +++ b/tests/agents/test_read_webpage_tool.py @@ -1,11 +1,29 @@ """Tests for application/agents/tools/read_webpage.py""" -from unittest.mock import MagicMock, patch +from unittest.mock import patch import pytest import requests from application.agents.tools.read_webpage import ReadWebpageTool +from application.security.safe_url import ResponseTooLargeError + + +class _FakeResponse: + def __init__(self, status_code: int = 200, headers=None): + self.status_code = status_code + self.headers = headers if headers is not None else {} + + def raise_for_status(self): + if self.status_code >= 400: + raise requests.exceptions.HTTPError(str(self.status_code)) + + +def _fetch_result(body: bytes, content_type=None, status_code: int = 200): + headers = {} + if content_type is not None: + headers["Content-Type"] = content_type + return body, _FakeResponse(status_code=status_code, headers=headers) @pytest.fixture @@ -25,48 +43,211 @@ class TestReadWebpageExecuteAction: assert "Error" in result assert "URL parameter is missing" in result - @patch("application.agents.tools.read_webpage.pinned_request") - def test_successful_fetch(self, mock_pinned_request, tool): - mock_resp = MagicMock() - mock_resp.status_code = 200 - mock_resp.text = "
Content
" - mock_pinned_request.return_value = mock_resp + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_successful_fetch(self, mock_fetch, tool): + mock_fetch.return_value = _fetch_result( + b"Content
", + content_type="text/html; charset=utf-8", + ) result = tool.execute_action("read_webpage", url="https://example.com") assert "Title" in result assert "Content" in result - @patch("application.agents.tools.read_webpage.pinned_request") - def test_request_error(self, mock_pinned_request, tool): - mock_pinned_request.side_effect = requests.exceptions.ConnectionError("refused") + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_request_error(self, mock_fetch, tool): + mock_fetch.side_effect = requests.exceptions.ConnectionError("refused") result = tool.execute_action("read_webpage", url="https://example.com") assert "Error fetching URL" in result - @patch("application.agents.tools.read_webpage.pinned_request") - def test_ssrf_blocked(self, mock_pinned_request, tool): + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_ssrf_blocked(self, mock_fetch, tool): from application.security.safe_url import UnsafeUserUrlError - mock_pinned_request.side_effect = UnsafeUserUrlError("blocked") + mock_fetch.side_effect = UnsafeUserUrlError("blocked") result = tool.execute_action("read_webpage", url="http://169.254.169.254/") assert "Error" in result assert "validation failed" in result - @patch("application.agents.tools.read_webpage.pinned_request") - def test_http_error(self, mock_pinned_request, tool): - mock_resp = MagicMock() - mock_resp.raise_for_status.side_effect = requests.exceptions.HTTPError("404") - mock_pinned_request.return_value = mock_resp + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_http_error(self, mock_fetch, tool): + mock_fetch.return_value = _fetch_result(b"", status_code=404) result = tool.execute_action("read_webpage", url="https://example.com/404") assert "Error fetching URL" in result +@pytest.mark.unit +class TestReadWebpageContentGuards: + """Regression tests for the PDF-as-text incident: a binary body must + never come back as NUL-laden 'markdown'.""" + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_rejects_pdf_content_type(self, mock_fetch, tool): + mock_fetch.return_value = _fetch_result( + b"%PDF-1.7 binary...", content_type="application/pdf", + ) + + result = tool.execute_action("read_webpage", url="https://example.com/doc.pdf") + + assert result.startswith("Error") + assert "application/pdf" in result + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_rejects_pdf_magic_without_content_type(self, mock_fetch, tool): + # No Content-Type header at all — the 07-17 incident shape. + mock_fetch.return_value = _fetch_result(b"%PDF-1.4\n%\xe2\xe3\xcf\xd3...") + + result = tool.execute_action("read_webpage", url="https://example.com/doc.pdf") + + assert result.startswith("Error") + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_rejects_nul_laden_body_despite_html_content_type(self, mock_fetch, tool): + # Mislabeled binary: content-type lies, the NUL sniff must catch it. + mock_fetch.return_value = _fetch_result( + b"\x00\x00\x00binary\x00garbage\x00\x00", + content_type="text/html", + ) + + result = tool.execute_action("read_webpage", url="https://example.com/fake") + + assert result.startswith("Error") + assert "\x00" not in result + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_rejects_octet_stream(self, mock_fetch, tool): + mock_fetch.return_value = _fetch_result( + b"anything", content_type="application/octet-stream", + ) + + result = tool.execute_action("read_webpage", url="https://example.com/blob") + + assert result.startswith("Error") + assert "application/octet-stream" in result + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_response_too_large(self, mock_fetch, tool): + mock_fetch.side_effect = ResponseTooLargeError("body exceeds 10485760 bytes") + + result = tool.execute_action("read_webpage", url="https://example.com/huge") + + assert result.startswith("Error") + assert "large" in result.lower() + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_utf8_decoded_without_declared_charset(self, mock_fetch, tool): + # text/html with no charset param: requests' RFC-2616 ISO-8859-1 + # fallback would mojibake this; we must default to UTF-8. + mock_fetch.return_value = _fetch_result( + "café über
".encode("utf-8"), + content_type="text/html", + ) + + result = tool.execute_action("read_webpage", url="https://example.com") + + assert "café über" in result + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_declared_charset_respected(self, mock_fetch, tool): + mock_fetch.return_value = _fetch_result( + "café
".encode("latin-1"), + content_type="text/html; charset=ISO-8859-1", + ) + + result = tool.execute_action("read_webpage", url="https://example.com") + + assert "café" in result + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_unknown_declared_charset_falls_back_to_utf8(self, mock_fetch, tool): + mock_fetch.return_value = _fetch_result( + "ok
".encode("utf-8"), + content_type="text/html; charset=not-a-real-charset", + ) + + result = tool.execute_action("read_webpage", url="https://example.com") + + assert "ok" in result + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_text_plain_allowed(self, mock_fetch, tool): + mock_fetch.return_value = _fetch_result( + b"plain text document", content_type="text/plain", + ) + + result = tool.execute_action("read_webpage", url="https://example.com/readme") + + assert "plain text document" in result + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_utf16_page_with_declared_charset_allowed(self, mock_fetch, tool): + # UTF-16 text is NUL-dense; a correct charset declaration must + # win over the NUL sniff (which is for undeclared/mislabeled bodies). + mock_fetch.return_value = _fetch_result( + "Hello world".encode("utf-16"), + content_type="text/html; charset=utf-16", + ) + + result = tool.execute_action("read_webpage", url="https://example.com") + + assert "Hello world" in result + assert "\x00" not in result + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_pdf_magic_beats_declared_charset(self, mock_fetch, tool): + # The magic-prefix check stays unconditional: a lying + # ``text/html; charset=utf-8`` header must not sneak a PDF through. + mock_fetch.return_value = _fetch_result( + b"%PDF-1.7 binary...", + content_type="text/html; charset=utf-8", + ) + + result = tool.execute_action("read_webpage", url="https://example.com/fake") + + assert result.startswith("Error") + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_nuls_past_sniff_window_are_stripped(self, mock_fetch, tool): + # The sniff only sees the first KB; NULs beyond it must still + # never leave the tool (self-contained, not reliant on the + # executor's sanitizer). + body = b"" + b"x" * 1100 + b"\x00\x00 tail
" + mock_fetch.return_value = _fetch_result(body, content_type="text/html") + + result = tool.execute_action("read_webpage", url="https://example.com") + + assert "\x00" not in result + assert "tail" in result + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_text_csv_allowed(self, mock_fetch, tool): + mock_fetch.return_value = _fetch_result( + b"name,qty\nwidget,2", content_type="text/csv", + ) + + result = tool.execute_action("read_webpage", url="https://example.com/data.csv") + + assert "widget" in result + + @patch("application.agents.tools.read_webpage.pinned_fetch_bytes") + def test_rss_feed_allowed(self, mock_fetch, tool): + mock_fetch.return_value = _fetch_result( + b"