diff --git a/docs/en/configuration/config-files.md b/docs/en/configuration/config-files.md index f2e9cbde..b6621036 100644 --- a/docs/en/configuration/config-files.md +++ b/docs/en/configuration/config-files.md @@ -85,6 +85,9 @@ api_key = "sk-xxx" base_url = "https://api.pythinker.com/coding/v1/fetch" api_key = "sk-xxx" +[web] +allowed_domains = ["example.com", "docs.python.org"] + [mcp.client] tool_call_timeout_ms = 60000 ``` @@ -199,6 +202,16 @@ Configures web fetch service. When enabled, the `FetchURL` tool prioritizes usin When configuring the Pythinker platform using the `/login` command, search and fetch services are automatically configured. ::: +### `web` + +`web` configures policy shared by the `FetchURL` and `SearchWeb` tools. + +| Field | Type | Default | Description | +| --- | --- | --- | --- | +| `allowed_domains` | `array` | _unset_ | When set, web fetch and search may only touch these domains and their subdomains. `FetchURL` rejects URLs on other hosts before making any request — including redirect targets, which are re-validated on every hop — and `SearchWeb` drops results from other domains. Unset or empty means unrestricted. Entries must be bare hostnames (e.g. `example.com`), not URLs, paths, or `host:port`. | + +This is a coarse governance control layered on top of the existing SSRF protections (which always block private, loopback, link-local, multicast, and reserved addresses); it does not replace them. Matching is label-aware: `example.com` matches `example.com` and `docs.example.com`, but not `notexample.com`. + ### `mcp` `mcp` configures MCP client behavior. diff --git a/src/pythinker_code/agents/default/system.md b/src/pythinker_code/agents/default/system.md index 212e0fd3..e69583a9 100644 --- a/src/pythinker_code/agents/default/system.md +++ b/src/pythinker_code/agents/default/system.md @@ -188,7 +188,7 @@ The operating environment is not in a sandbox. Any actions you do will immediate ## Date and Time -The current date and time in ISO format is `${PYTHINKER_NOW}`. This is only a reference for you when searching the web, or checking file modification time, etc. If you need the exact time, use Shell tool with proper command. +The current date and time in ISO format is `${PYTHINKER_NOW}`. Treat this as the authoritative present — it reflects the real "now", which is later than your training data suggests. Anchor all reasoning about the current date, the year, recency, and what counts as the "latest" version or release to `${PYTHINKER_NOW}`; do not fall back on an earlier year you might assume from training. Use it as your reference when searching the web or checking file modification times. If you need the exact time, use the Shell tool with a proper command. ## Working Directory @@ -261,6 +261,15 @@ Identify the skills that are likely to be useful for the tasks you are currently Only read skill details when needed to conserve the context window. +# Output Formatting + +Your responses are rendered as Markdown in a terminal. Emit well-formed Markdown so it renders cleanly: + +- **Tables:** put the header row on its own line, the `|---|---|` delimiter row on the immediately following line (no blank line between them), and one row per line. Never glue a table onto adjacent prose (e.g. `Findings| Col |`) and never cram multiple rows onto one line. Leave a blank line before and after the table. +- Prefer a short bullet list over a table when there are only a few items or any cell is long; reserve tables for genuinely tabular data with short cells. +- **Code fences are for code only.** Use triple-backtick blocks tagged with a language (for example, `python` or `toml`) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly. +- **Status icons sparingly.** A check/cross/dot can mark a single headline result, but do not prefix every line with one. Use plain words for severity and outcomes (e.g. `High`, `PASS`, `0 findings`). The terminal renders icons as calm monochrome glyphs only outside code fences — another reason not to box reports. + # Ultimate Reminders At any time, you should be HELPFUL, CONCISE, and ACCURATE. Be thorough in your actions — test what you build, verify what you change — not in your explanations. diff --git a/src/pythinker_code/config.py b/src/pythinker_code/config.py index 1432853a..b90c754e 100644 --- a/src/pythinker_code/config.py +++ b/src/pythinker_code/config.py @@ -14,6 +14,7 @@ SecretStr, ValidationError, field_serializer, + field_validator, model_validator, ) from tomlkit.exceptions import TOMLKitError @@ -217,6 +218,41 @@ class Services(BaseModel): """Pythinker AI Fetch configuration.""" +class WebConfig(BaseModel): + """Web fetch/search policy.""" + + allowed_domains: list[str] | None = Field( + default=None, + description=( + "If set, web fetch and search may only touch these domains and their " + "subdomains. None or empty means unrestricted (default)." + ), + ) + + @field_validator("allowed_domains") + @classmethod + def _validate_allowed_domains(cls, value: list[str] | None) -> list[str] | None: + for entry in value or []: + cleaned = entry.strip() + if not cleaned: + raise ValueError( + "Invalid allowed_domains entry: empty or whitespace-only hostname. " + "Remove it, or omit allowed_domains entirely to leave web access " + "unrestricted." + ) + if cleaned.strip(".") == "": + raise ValueError( + f"Invalid allowed_domains entry {entry!r}: hostname must contain " + "domain labels, not only dots." + ) + if any(char.isspace() for char in cleaned) or any(char in cleaned for char in "/:"): + raise ValueError( + f"Invalid allowed_domains entry {entry!r}: use a bare hostname " + "like 'example.com', not a URL, path, or host:port." + ) + return value + + class FeedbackConfig(BaseModel): """User-submitted feedback endpoint configuration.""" @@ -364,6 +400,7 @@ class Config(BaseModel): ) services: Services = Field(default_factory=Services, description="Services configuration") memory: MemoryConfig = Field(default_factory=MemoryConfig, description="Memory configuration") + web: WebConfig = Field(default_factory=WebConfig, description="Web fetch/search policy") feedback: FeedbackConfig = Field( default_factory=FeedbackConfig, description="User-submitted feedback endpoint configuration", diff --git a/src/pythinker_code/tools/web/_allowlist.py b/src/pythinker_code/tools/web/_allowlist.py new file mode 100644 index 00000000..6d91ebdb --- /dev/null +++ b/src/pythinker_code/tools/web/_allowlist.py @@ -0,0 +1,27 @@ +"""Domain allowlist matching shared by the web fetch and search tools.""" + +from __future__ import annotations + + +def _normalize(entry: str) -> str: + return entry.strip().strip(".").lower() + + +def host_in_allowlist(host: str | None, allowed: list[str] | None) -> bool: + """Return whether *host* is permitted by the *allowed* domain list. + + A ``None`` or empty allowlist imposes no restriction (returns ``True``), + preserving unconfigured behavior. Otherwise a host matches when it equals an + allowlist entry or is a subdomain of one. Matching is label-aware and + case-insensitive: ``example.com`` matches ``example.com`` and + ``docs.example.com`` but not ``notexample.com``. + """ + entries = [normalized for entry in (allowed or []) if (normalized := _normalize(entry))] + if not entries: + return True + + host = (host or "").strip().rstrip(".").lower() + if not host: + return False + + return any(host == entry or host.endswith(f".{entry}") for entry in entries) diff --git a/src/pythinker_code/tools/web/fetch.md b/src/pythinker_code/tools/web/fetch.md index 73ebcc80..e7b63dfd 100644 --- a/src/pythinker_code/tools/web/fetch.md +++ b/src/pythinker_code/tools/web/fetch.md @@ -1 +1 @@ -Fetch a web page from a URL and extract main text content from it. +Fetch a web page from a URL and extract main text content from it. Requests may be restricted to a configured set of allowed domains; fetching a disallowed host (including via a redirect) returns an error. diff --git a/src/pythinker_code/tools/web/fetch.py b/src/pythinker_code/tools/web/fetch.py index 202491f9..f8ea539c 100644 --- a/src/pythinker_code/tools/web/fetch.py +++ b/src/pythinker_code/tools/web/fetch.py @@ -15,6 +15,7 @@ from pythinker_code.soul.agent import Runtime from pythinker_code.soul.toolset import get_current_tool_call_or_none from pythinker_code.tools.utils import ToolResultBuilder, load_desc +from pythinker_code.tools.web._allowlist import host_in_allowlist from pythinker_code.utils.aiohttp import new_client_session from pythinker_code.utils.logging import logger @@ -23,13 +24,16 @@ _REDIRECT_STATUSES = frozenset({301, 302, 303, 307, 308}) -def _validate_fetch_url(url: str) -> str | None: +def _validate_fetch_url(url: str, allowed_domains: list[str] | None = None) -> str | None: parsed = urlparse(url) if parsed.scheme not in {"http", "https"}: return "Only http and https URLs are supported." if not parsed.hostname: return "URL must include a host." + if not host_in_allowlist(parsed.hostname, allowed_domains): + return "URL host is not in the configured web allowlist." + try: infos = socket.getaddrinfo(parsed.hostname, parsed.port, type=socket.SOCK_STREAM) except socket.gaierror: @@ -76,7 +80,10 @@ def __init__(self, reason: str) -> None: async def _get_revalidating_redirects( - session: aiohttp.ClientSession, url: str, headers: dict[str, str] + session: aiohttp.ClientSession, + url: str, + headers: dict[str, str], + allowed_domains: list[str] | None = None, ) -> aiohttp.ClientResponse: """GET ``url``, following redirects manually and re-validating every hop. @@ -84,15 +91,18 @@ async def _get_revalidating_redirects( a public URL that 30x-redirects to a private/link-local address (e.g. a cloud metadata endpoint at 169.254.169.254) would otherwise sail past ``_validate_fetch_url``. We disable automatic redirects and validate each - ``Location`` before following it. + ``Location`` against both the SSRF guard and the configured domain allowlist + before following it. Returns the final, open, non-redirect response (the caller owns closing it). Raises ``_FetchBlocked`` if any hop is blocked or the redirect limit is hit. """ current = url - for _ in range(MAX_FETCH_REDIRECTS + 1): - if reason := _validate_fetch_url(current): - raise _FetchBlocked(reason) + for hop in range(MAX_FETCH_REDIRECTS + 1): + if reason := _validate_fetch_url(current, allowed_domains): + if hop == 0: + raise _FetchBlocked(reason) + raise _FetchBlocked(f"redirect to a disallowed location: {reason}") response = await session.get(current, headers=headers, allow_redirects=False) location = response.headers.get(aiohttp.hdrs.LOCATION) if response.status in _REDIRECT_STATUSES and location: @@ -116,6 +126,7 @@ def __init__(self, config: Config, runtime: Runtime): super().__init__() self._runtime = runtime self._service_config = config.services.pythinker_ai_fetch + self._allowed_domains = config.web.allowed_domains @override async def __call__(self, params: Params) -> ToolReturnValue: @@ -134,11 +145,18 @@ async def __call__(self, params: Params) -> ToolReturnValue: return ret logger.warning("Failed to fetch URL via service: {error}", error=ret.message) # fallback to local fetch if service fetch fails - return await self.fetch_with_http_get(params) + return await self.fetch_with_http_get(params, self._allowed_domains) @staticmethod - async def fetch_with_http_get(params: Params) -> ToolReturnValue: + async def fetch_with_http_get( + params: Params, allowed_domains: list[str] | None = None + ) -> ToolReturnValue: builder = ToolResultBuilder(max_line_length=None) + # Validate the initial URL up front so a disallowed host is rejected + # before any network session is opened; the redirect helper below + # re-validates every subsequent hop. + if reason := _validate_fetch_url(params.url, allowed_domains): + return builder.error(f"Failed to fetch URL: {reason}", brief="URL blocked") headers = { "User-Agent": ( "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 " @@ -150,7 +168,9 @@ async def fetch_with_http_get(params: Params) -> ToolReturnValue: fetch_timeout = aiohttp.ClientTimeout(total=180, sock_read=60, sock_connect=15) async with new_client_session(timeout=fetch_timeout) as session: try: - response = await _get_revalidating_redirects(session, params.url, headers) + response = await _get_revalidating_redirects( + session, params.url, headers, allowed_domains + ) except _FetchBlocked as blocked: return builder.error( f"Failed to fetch URL: {blocked.reason}", brief="URL blocked" @@ -245,7 +265,7 @@ async def _fetch_with_service(self, params: Params) -> ToolReturnValue: "Fetch service is not configured. You may want to try other methods to fetch.", brief="Fetch service not configured", ) - if reason := _validate_fetch_url(params.url): + if reason := _validate_fetch_url(params.url, self._allowed_domains): return builder.error(f"Failed to fetch URL: {reason}", brief="URL blocked") headers = { diff --git a/src/pythinker_code/tools/web/search.md b/src/pythinker_code/tools/web/search.md index 19e4cec7..1d6ea79b 100644 --- a/src/pythinker_code/tools/web/search.md +++ b/src/pythinker_code/tools/web/search.md @@ -1 +1 @@ -WebSearch tool allows you to search on the internet to get latest information, including news, documents, release notes, blog posts, papers, etc. +WebSearch tool allows you to search on the internet to get latest information, including news, documents, release notes, blog posts, papers, etc. Results may be limited to a configured set of allowed domains. diff --git a/src/pythinker_code/tools/web/search.py b/src/pythinker_code/tools/web/search.py index c7bb8a26..ee80b8b7 100644 --- a/src/pythinker_code/tools/web/search.py +++ b/src/pythinker_code/tools/web/search.py @@ -1,5 +1,6 @@ from pathlib import Path from typing import override +from urllib.parse import urlparse import aiohttp from pydantic import BaseModel, Field, ValidationError @@ -12,6 +13,7 @@ from pythinker_code.soul.toolset import get_current_tool_call_or_none from pythinker_code.tools import SkipThisTool from pythinker_code.tools.utils import ToolResultBuilder, load_desc +from pythinker_code.tools.web._allowlist import host_in_allowlist from pythinker_code.utils.aiohttp import new_client_session from pythinker_code.utils.logging import logger @@ -53,6 +55,7 @@ def __init__(self, config: Config, runtime: Runtime): self._api_key = config.services.pythinker_ai_search.api_key self._oauth_ref = config.services.pythinker_ai_search.oauth self._custom_headers = config.services.pythinker_ai_search.custom_headers or {} + self._allowed_domains = config.web.allowed_domains @override async def __call__(self, params: Params) -> ToolReturnValue: @@ -145,6 +148,26 @@ async def __call__(self, params: Params) -> ToolReturnValue: brief="Search request failed", ) + if self._allowed_domains: + kept = [ + result + for result in results + if host_in_allowlist(urlparse(result.url).hostname, self._allowed_domains) + ] + dropped = len(results) - len(kept) + results = kept + if dropped: + builder.extras(allowlist_filtered=dropped) + if not results: + # Structured zero-result signal so the renderer reports "0 + # results" instead of misreading the prose below as one result. + builder.extras(returned_results=0) + return builder.ok( + f"All {dropped} search result(s) were outside the configured " + "web allowlist and have been omitted.", + brief="Filtered by allowlist", + ) + for i, result in enumerate(results): if i > 0: builder.write("---\n\n") diff --git a/src/pythinker_code/ui/shell/components/markdown.py b/src/pythinker_code/ui/shell/components/markdown.py index a92964b9..804cdd58 100644 --- a/src/pythinker_code/ui/shell/components/markdown.py +++ b/src/pythinker_code/ui/shell/components/markdown.py @@ -76,6 +76,11 @@ ) _PRIORITY_MATRIX_SEVERITIES = ("CRITICAL", "HIGH", "MEDIUM", "LOW", "INFO") _TABLE_SEPARATOR_RE = re.compile(r"^\s*\|?\s*:?-{3,}:?\s*(?:\|\s*:?-{3,}:?\s*)+\|?\s*$") +# A GFM table delimiter run, e.g. ``|---|:--:|---|``. Two or more dashes per +# cell keeps stray inline ``|-|`` out of the match. +_DELIM_RUN_RE = re.compile(r"\|(?:\s*:?-{2,}:?\s*\|)+") +# A header line: optional prose prefix, then a trailing run of pipe cells. +_HEADER_RE = re.compile(r"^(?P.*?)(?P(?:\|[^\n|]*)+\|)\s*$") __all__ = [ @@ -344,6 +349,168 @@ def _simplify_markdown_report_icons(markup: str) -> str: return "".join(lines) +def _split_pipe_cells(segment: str) -> list[str]: + """Split a ``| a | b |`` run into stripped inner cells (drops the frame).""" + parts = re.split(r"(? bool: + stripped = line.strip() + return stripped.startswith("|") and stripped.count("|") >= 2 + + +def _delimiter_markers(run: str) -> list[str]: + """Return per-column alignment markers (``---``, ``:---``, ``---:``, ``:---:``).""" + markers: list[str] = [] + for cell in _split_pipe_cells(run): + left = cell.startswith(":") + right = cell.endswith(":") + if left and right: + markers.append(":---:") + elif right: + markers.append("---:") + elif left: + markers.append(":---") + else: + markers.append("---") + return markers + + +def _normalize_table_block(text: str) -> str: + """Repair malformed GFM tables in a fence-free block of markdown. + + Models occasionally glue a table header onto preceding prose, drop the + newline between the header and the ``|---|`` delimiter, or cram data rows + onto the delimiter line — markdown-it then renders the whole thing as raw + text. Anchored on the delimiter run, this rebuilds each region it is + *confident* is a table (delimiter at line start, header and data cell counts + both equal to the delimiter's column count) and passes everything else + through untouched. Well-formed tables are rebuilt to identical-rendering + markdown, so the pass is safe to apply unconditionally. + """ + out = "" + while True: + match = _DELIM_RUN_RE.search(text) + if match is None: + return out + text + markers = _delimiter_markers(match.group(0)) + n_cols = len(markers) + head = text[: match.start()] + tail = text[match.end() :] + + # The delimiter must start its own line — guards against inline ``|-|``. + # Any leading whitespace is the table's indentation (e.g. nested under a + # list item); preserve it when re-emitting so we never promote an + # indented table to top level. + indent = head[head.rfind("\n") + 1 :] + if n_cols < 2 or indent.strip() != "": + out += text[: match.end()] + text = tail + continue + + head_lines = head.split("\n") + while head_lines and head_lines[-1] == "": + head_lines.pop() + header_match = _HEADER_RE.match(head_lines[-1]) if head_lines else None + header_cells = _split_pipe_cells(header_match.group("cells")) if header_match else [] + if header_match is None or len(header_cells) != n_cols: + out += text[: match.end()] + text = tail + continue + + # Data rows: the same-line remainder after the delimiter plus any + # following pipe rows, re-chunked into rows of ``n_cols`` cells. + tail_lines = tail.split("\n") + data_segments = [tail_lines[0]] if tail_lines[0].strip() else [] + consumed = 1 + for line in tail_lines[1:]: + if _is_pipe_row(line): + data_segments.append(line) + consumed += 1 + else: + break + data_rows: list[list[str]] = [] + bail = False + for segment in data_segments: + cells = _split_pipe_cells(segment) + if not cells: + continue + if len(cells) % n_cols != 0: + bail = True # ambiguous (e.g. glued rows with empty cells) — leave as-is + break + for i in range(0, len(cells), n_cols): + data_rows.append(cells[i : i + n_cols]) + if bail: + out += text[: match.end()] + text = tail + continue + + preamble = head_lines[:-1] + prose = header_match.group("prefix").rstrip() + if preamble: + out += "\n".join(preamble) + "\n" + if prose: + out += prose + "\n" + # A GFM table must be preceded by a blank line (it cannot interrupt a + # paragraph), so ensure one before emitting the header. + if out and not out.endswith("\n\n"): + out += "\n" if out.endswith("\n") else "\n\n" + out += f"{indent}| " + " | ".join(header_cells) + " |\n" + out += f"{indent}| " + " | ".join(markers) + " |\n" + for row in data_rows: + out += f"{indent}| " + " | ".join(row) + " |\n" + + remainder = "\n".join(tail_lines[consumed:]) + if not remainder.strip(): + return out + text = remainder if remainder.startswith("\n") else "\n" + remainder + + +def _normalize_markdown_tables(markup: str) -> str: + """Apply :func:`_normalize_table_block` to every fence-free span of markup.""" + if "|" not in markup or "-" not in markup: + return markup + + out: list[str] = [] + buffer: list[str] = [] + in_fence = False + fence_char = "" + fence_len = 0 + + def flush() -> None: + if buffer: + out.append(_normalize_table_block("\n".join(buffer))) + buffer.clear() + + for line in markup.splitlines(): + match = _FENCE_RE.match(line) + if in_fence: + fence = match.group("fence") if match else "" + if fence and fence[0] == fence_char and len(fence) >= fence_len: + in_fence = False + out.append(line) + continue + if match: + flush() + in_fence = True + fence_char = match.group("fence")[0] + fence_len = len(match.group("fence")) + out.append(line) + continue + buffer.append(line) + flush() + + result = "\n".join(out) + if markup.endswith("\n") and not result.endswith("\n"): + result += "\n" + return result + + class PythinkerMarkdown(Markdown): """Drop-in replacement for ``rich.markdown.Markdown`` with the Pythinker palette. @@ -358,7 +525,8 @@ class PythinkerMarkdown(Markdown): def __init__(self, markup: str, *args: Any, **kwargs: Any) -> None: safe_markup = sanitize_ansi(markup) repaired_markup = _repair_crammed_markdown_tables(safe_markup) - super().__init__(_simplify_markdown_report_icons(repaired_markup), *args, **kwargs) + normalized_markup = _normalize_markdown_tables(repaired_markup) + super().__init__(_simplify_markdown_report_icons(normalized_markup), *args, **kwargs) def __rich_console__(self, console: Console, options: ConsoleOptions) -> RenderResult: overrides = _markdown_style_overrides() diff --git a/src/pythinker_code/ui/shell/tool_renderers/web.py b/src/pythinker_code/ui/shell/tool_renderers/web.py index 1ccb7540..f0f0e906 100644 --- a/src/pythinker_code/ui/shell/tool_renderers/web.py +++ b/src/pythinker_code/ui/shell/tool_renderers/web.py @@ -2,6 +2,8 @@ from __future__ import annotations +from typing import cast + from rich.console import Group, RenderableType from rich.text import Text @@ -173,6 +175,24 @@ def _search_result_count(text: str) -> int: return len([line for line in text.splitlines() if line.strip()]) +def _allowlist_filtered_count(result: ToolResultPayload) -> int: + """Number of search results dropped by the domain allowlist, if any.""" + extras_raw = result.details.get("extras") + extras = cast("dict[str, object]", extras_raw) if isinstance(extras_raw, dict) else {} + value = extras.get("allowlist_filtered") + if isinstance(value, int) and value > 0: + return value + return 0 + + +def _explicit_result_count(result: ToolResultPayload) -> int | None: + """The tool's own result count, if it emitted one (preferred over text).""" + extras_raw = result.details.get("extras") + extras = cast("dict[str, object]", extras_raw) if isinstance(extras_raw, dict) else {} + value = extras.get("returned_results") + return value if isinstance(value, int) and value >= 0 else None + + def _render_search_result( ctx: ToolRenderContext, result: ToolResultPayload ) -> RenderableType | None: @@ -191,11 +211,14 @@ def _render_search_result( return Group(body, fg("muted", f"... ({remaining} more lines, ctrl+o to expand)")) return body - count = _search_result_count(result.text) + explicit = _explicit_result_count(result) + count = explicit if explicit is not None else _search_result_count(result.text) summary = Text() summary.append("Found ", style=tui_rich_style("tool_output")) summary.append(str(count), style=tui_rich_style("tool_title")) summary.append(f" {_plural(count, 'result')}", style=tui_rich_style("tool_output")) + if filtered := _allowlist_filtered_count(result): + summary.append(f" · {filtered} filtered to allowlist", style=tui_rich_style("muted")) ctx.state["__suppress_generic_expand_hint__"] = True if count and not ctx.expanded: summary.append(" ") diff --git a/tasks/todo.md b/tasks/todo.md index 7ec8ae30..e9ff38a6 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -467,3 +467,43 @@ PyPI 1.0.0 of pythinker-cli stays published forever. Anyone who installed it bef 4. **Migration text in README**: Do you want a "migrating from pythinker-cli" callout in 1.1.0's README, or just silently switch? 5. **CHANGELOG framing**: Is this a breaking change that warrants 2.0.0, or a layout change that's fine at 1.1.0? PyPI users perspective: install command changed, that's user-visible breakage. Could argue 2.0.0. + +--- + +# Web fetch/search domain allowlist (2026-05-27) + +Port of the one genuinely portable concept from pythinker-x's web search +(`allowed_domains`) onto our self-hosted FetchURL/SearchWeb tools. Design spec: +`docs/superpowers/specs/2026-05-27-web-allowed-domains-design.md`. + +- [x] `WebConfig.allowed_domains` config (+ field validator rejecting URLs/paths/host:port) +- [x] `host_in_allowlist` helper (label-aware subdomain match, unrestricted when empty) +- [x] FetchURL: reject out-of-allowlist hosts in `_validate_fetch_url` (no request made) +- [x] SearchWeb: post-filter results, surface dropped count via `extras` +- [x] TUI: muted "· N filtered to allowlist" indicator on the search result header +- [x] Tests: helper, config validation, fetch rejection, search filter, renderer indicator +- [x] Docs: `docs/en/configuration/config-files.md` `web` section + example + +## Review +- All affected suites green (tools/core/ui = 2517 passed earlier; affected subset 100 passed). +- ruff + ruff format clean; pyright clean on all changed files. The 8 pre-existing + pyright errors live in `cli/mcp.py` and `soul/toolset.py` (untouched, baseline). +- Dropped from scope (cosmetic/redundant in our architecture): action taxonomy relabel, + disabled/cached/live mode gating. See design doc "Out of scope". + +## Out of scope (observed, not changed) +- Pre-existing pyright errors in `cli/mcp.py`, `soul/toolset.py`. + +## Review follow-up (2026-05-27) — context7-validated hardening +Reviewed the allowlist against context7 (aiohttp v3.13.2, pydantic v2) + 2026 agent-tool practice. +- [x] HIGH: redirect bypass — `fetch_with_http_get` now sets `allow_redirects=False` and follows + redirects manually (max 5), re-validating each hop via `_validate_fetch_url` (allowlist + + SSRF). Closes a pre-existing SSRF gap the allowlist had inherited. Tests: follows validated + redirect, blocks redirect to disallowed host (never contacted), rejects redirect loop. +- [x] LOW: `fetch.md` / `search.md` now state the allowlist constraint to the model. +- [x] LOW: `WebConfig` validator now rejects empty/whitespace-only entries (was silently unrestricted). +- Confirmed-good (context7): pydantic validator matches docs exactly; `extras` TUI channel; + fail-closed on unparsable hosts; allowlist-before-DNS ordering. +- Known/accepted limitation (pre-existing, not addressed): DNS-rebinding TOCTOU — `_validate_fetch_url` + resolves+checks IPs but aiohttp re-resolves at connect time. Out of scope; would need a pinning connector. +- Snapshots updated: `test_default_config_dump`, `test_fetch_url_description`, `test_search_web_description`. diff --git a/tests/core/test_config.py b/tests/core/test_config.py index d8badffa..1aeb7c84 100644 --- a/tests/core/test_config.py +++ b/tests/core/test_config.py @@ -68,6 +68,7 @@ def test_default_config_dump(): "journal_recaps": False, "consolidation": False, }, + "web": {"allowed_domains": None}, "feedback": { "endpoint_url": "", "api_key": None, diff --git a/tests/core/test_default_agent.py b/tests/core/test_default_agent.py index 72f02e5d..472ec6f3 100644 --- a/tests/core/test_default_agent.py +++ b/tests/core/test_default_agent.py @@ -204,7 +204,7 @@ async def test_default_agent(runtime: Runtime): ## Date and Time -The current date and time in ISO format is `1970-01-01T00:00:00+00:00`. This is only a reference for you when searching the web, or checking file modification time, etc. If you need the exact time, use Shell tool with proper command. +The current date and time in ISO format is `1970-01-01T00:00:00+00:00`. Treat this as the authoritative present — it reflects the real "now", which is later than your training data suggests. Anchor all reasoning about the current date, the year, recency, and what counts as the "latest" version or release to `1970-01-01T00:00:00+00:00`; do not fall back on an earlier year you might assume from training. Use it as your reference when searching the web or checking file modification times. If you need the exact time, use the Shell tool with a proper command. ## Working Directory @@ -269,6 +269,15 @@ async def test_default_agent(runtime: Runtime): Only read skill details when needed to conserve the context window. +# Output Formatting + +Your responses are rendered as Markdown in a terminal. Emit well-formed Markdown so it renders cleanly: + +- **Tables:** put the header row on its own line, the `|---|---|` delimiter row on the immediately following line (no blank line between them), and one row per line. Never glue a table onto adjacent prose (e.g. `Findings| Col |`) and never cram multiple rows onto one line. Leave a blank line before and after the table. +- Prefer a short bullet list over a table when there are only a few items or any cell is long; reserve tables for genuinely tabular data with short cells. +- **Code fences are for code only.** Use triple-backtick blocks tagged with a language (for example, `python` or `toml`) solely for source, config, or commands — one snippet per block. Never wrap a prose report, finding list, checklist, or ASCII box in a fence to align or frame it; write it as normal Markdown (headings, bullets, tables) so it renders cleanly. +- **Status icons sparingly.** A check/cross/dot can mark a single headline result, but do not prefix every line with one. Use plain words for severity and outcomes (e.g. `High`, `PASS`, `0 findings`). The terminal renders icons as calm monochrome glyphs only outside code fences — another reason not to box reports. + # Ultimate Reminders At any time, you should be HELPFUL, CONCISE, and ACCURATE. Be thorough in your actions — test what you build, verify what you change — not in your explanations. diff --git a/tests/core/test_load_agent.py b/tests/core/test_load_agent.py index ccbf62bf..7e9ecd00 100644 --- a/tests/core/test_load_agent.py +++ b/tests/core/test_load_agent.py @@ -55,6 +55,24 @@ def test_system_prompt_contains_platform_info(builtin_args: BuiltinSystemPromptA assert builtin_args.PYTHINKER_SHELL in prompt +def test_system_prompt_treats_injected_date_as_authoritative( + builtin_args: BuiltinSystemPromptArgs, +): + """The injected date must be framed as authoritative so the model anchors + its sense of 'now' to it instead of a training-era year.""" + from pythinker_code.agentspec import DEFAULT_AGENT_FILE + + prompt = _load_system_prompt( + DEFAULT_AGENT_FILE.parent / "system.md", + {"ROLE_ADDITIONAL": ""}, + builtin_args, + ) + + assert builtin_args.PYTHINKER_NOW in prompt + assert "authoritative present" in prompt + assert "do not fall back on an earlier year" in prompt + + def test_system_prompt_enforces_context_first_orchestration( builtin_args: BuiltinSystemPromptArgs, ): @@ -74,6 +92,27 @@ def test_system_prompt_enforces_context_first_orchestration( assert "Treat subagent claims as leads, not proof" in prompt +def test_system_prompt_includes_markdown_table_formatting_guidance( + builtin_args: BuiltinSystemPromptArgs, +): + """Default prompt must reach the model with table-formatting rules so it + stops emitting headers glued to prose (which render as raw text).""" + from pythinker_code.agentspec import DEFAULT_AGENT_FILE + + prompt = _load_system_prompt( + DEFAULT_AGENT_FILE.parent / "system.md", + {"ROLE_ADDITIONAL": ""}, + builtin_args, + ) + + assert "# Output Formatting" in prompt + assert "glue a table onto adjacent prose" in prompt + # Reports must not be wrapped in code fences (that is what preserves raw + # emoji and breaks column alignment), and status icons should be sparing. + assert "Code fences are for code only" in prompt + assert "Status icons sparingly" in prompt + + def test_default_subagent_prompts_keep_robust_contracts(): """Specialist subagents should retain evidence, planning, and verification gates.""" from pythinker_code.agentspec import DEFAULT_AGENT_FILE, load_agent_spec diff --git a/tests/tools/test_fetch_url.py b/tests/tools/test_fetch_url.py index 33b4f2a4..7aae3986 100644 --- a/tests/tools/test_fetch_url.py +++ b/tests/tools/test_fetch_url.py @@ -24,7 +24,7 @@ def _bypass_ssrf_validation(monkeypatch: pytest.MonkeyPatch) -> None: exercises malformed-URL handling that pre-dates the validator. Disable it for these unit tests; production callers still get the protection. """ - monkeypatch.setattr(fetch_module, "_validate_fetch_url", lambda _url: None) + monkeypatch.setattr(fetch_module, "_validate_fetch_url", lambda _url, _allowed=None: None) class MockServerFactory(Protocol): @@ -241,6 +241,89 @@ async def mocked_fetch(resp: str, *, content_type: str = "text/html") -> ToolRet assert result.message == "The returned content is the full content of the page." +async def _start_redirect_server(routes) -> tuple[str, web.AppRunner]: + """Start a server with the given (path, handler) routes; return base URL + runner.""" + app = web.Application() + for path, handler in routes: + app.router.add_get(path, handler) + runner = web.AppRunner(app) + await runner.setup() + site = web.TCPSite(runner, host="127.0.0.1", port=0) + await site.start() + port = site._server.sockets[0].getsockname()[1] # type: ignore[attr-defined] + return f"http://127.0.0.1:{port}", runner + + +async def test_fetch_url_follows_validated_redirect(fetch_url_tool: FetchURL) -> None: + """A redirect to an allowed location is followed (validation is bypassed here).""" + + async def start(request: web.Request) -> web.Response: # noqa: ARG001 + raise web.HTTPFound("/end") + + async def end(request: web.Request) -> web.Response: # noqa: ARG001 + return web.Response(text="redirected body content", content_type="text/plain") + + base, runner = await _start_redirect_server([("/start", start), ("/end", end)]) + try: + result = await fetch_url_tool(Params(url=f"{base}/start")) + finally: + await runner.cleanup() + + assert not result.is_error + assert "redirected body content" in result.output + + +async def test_fetch_url_blocks_redirect_to_disallowed_host( + fetch_url_tool: FetchURL, monkeypatch: pytest.MonkeyPatch +) -> None: + """A redirect whose target fails validation is blocked *before* being fetched.""" + + # Override the module-wide bypass: allow the initial URL, block the redirect + # target (any URL whose path is /blocked). + monkeypatch.setattr( + fetch_module, + "_validate_fetch_url", + lambda url, _allowed=None: ("host blocked" if "/blocked" in url else None), + ) + + blocked_hit = False + + async def start(request: web.Request) -> web.Response: # noqa: ARG001 + raise web.HTTPFound("/blocked") + + async def blocked(request: web.Request) -> web.Response: # noqa: ARG001 + nonlocal blocked_hit + blocked_hit = True + return web.Response(text="secret", content_type="text/plain") + + base, runner = await _start_redirect_server([("/start", start), ("/blocked", blocked)]) + try: + result = await fetch_url_tool(Params(url=f"{base}/start")) + finally: + await runner.cleanup() + + assert result.is_error + assert "redirect" in result.message.lower() + # The security guarantee: the disallowed location was never contacted. + assert blocked_hit is False + + +async def test_fetch_url_rejects_redirect_loop(fetch_url_tool: FetchURL) -> None: + """A redirect loop terminates with a 'too many redirects' error.""" + + async def loop(request: web.Request) -> web.Response: # noqa: ARG001 + raise web.HTTPFound("/loop") + + base, runner = await _start_redirect_server([("/loop", loop)]) + try: + result = await fetch_url_tool(Params(url=f"{base}/loop")) + finally: + await runner.cleanup() + + assert result.is_error + assert "too many redirects" in result.message.lower() + + async def test_fetch_url_with_service(runtime) -> None: """Test fetching using the pythinker_ai_fetch service.""" from pythinker_code.config import Config, PythinkerAIFetchConfig, Services @@ -339,7 +422,7 @@ async def test_fetch_url_redirect_to_blocked_target_is_rejected( """A 30x redirect whose target fails SSRF validation must be rejected, not silently followed (aiohttp would otherwise follow it without re-checking).""" - def _validator(url: str) -> str | None: + def _validator(url: str, _allowed: object = None) -> str | None: return "internal address blocked" if "blocked.invalid" in url else None monkeypatch.setattr(fetch_module, "_validate_fetch_url", _validator) diff --git a/tests/tools/test_tool_descriptions.py b/tests/tools/test_tool_descriptions.py index 74439b17..fedd900c 100644 --- a/tests/tools/test_tool_descriptions.py +++ b/tests/tools/test_tool_descriptions.py @@ -375,12 +375,12 @@ def test_str_replace_file_description(str_replace_file_tool: StrReplaceFile): def test_search_web_description(search_web_tool: SearchWeb): """Test the description of PythinkerAISearch tool.""" assert search_web_tool.base.description == snapshot( - "WebSearch tool allows you to search on the internet to get latest information, including news, documents, release notes, blog posts, papers, etc.\n" + "WebSearch tool allows you to search on the internet to get latest information, including news, documents, release notes, blog posts, papers, etc. Results may be limited to a configured set of allowed domains.\n" ) def test_fetch_url_description(fetch_url_tool: FetchURL): """Test the description of FetchURL tool.""" assert fetch_url_tool.base.description == snapshot( - "Fetch a web page from a URL and extract main text content from it.\n" + "Fetch a web page from a URL and extract main text content from it. Requests may be restricted to a configured set of allowed domains; fetching a disallowed host (including via a redirect) returns an error.\n" ) diff --git a/tests/tools/test_web_allowlist.py b/tests/tools/test_web_allowlist.py new file mode 100644 index 00000000..c61e51bf --- /dev/null +++ b/tests/tools/test_web_allowlist.py @@ -0,0 +1,66 @@ +"""Tests for the web domain allowlist helper.""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from pythinker_code.config import WebConfig +from pythinker_code.tools.web._allowlist import host_in_allowlist + + +@pytest.mark.parametrize( + ("host", "allowed", "expected"), + [ + # None / empty allowlist is unrestricted. + ("anything.com", None, True), + ("anything.com", [], True), + ("anything.com", [" ", "."], True), # entries normalize to empty + # Exact match. + ("example.com", ["example.com"], True), + # Subdomain match. + ("docs.example.com", ["example.com"], True), + ("a.b.example.com", ["example.com"], True), + # Lookalike must NOT match. + ("notexample.com", ["example.com"], False), + ("example.com.evil.com", ["example.com"], False), + # Different domain. + ("other.org", ["example.com"], False), + # Case-insensitivity and normalization (leading dot, whitespace). + ("DOCS.Example.COM", [" .Example.com "], True), + # Trailing-dot FQDN host. + ("docs.example.com.", ["example.com"], True), + # Trailing-dot allowlist *entry* matches a plain host. + ("docs.example.com", ["example.com."], True), + ("example.com", ["example.com."], True), + # Multiple entries: match any. + ("foo.org", ["example.com", "foo.org"], True), + # Empty / None host with a non-empty allowlist is rejected. + ("", ["example.com"], False), + (None, ["example.com"], False), + ], +) +def test_host_in_allowlist(host: str | None, allowed: list[str] | None, expected: bool) -> None: + assert host_in_allowlist(host, allowed) is expected + + +def test_web_config_accepts_bare_hostnames() -> None: + cfg = WebConfig(allowed_domains=["example.com", "docs.python.org"]) + assert cfg.allowed_domains == ["example.com", "docs.python.org"] + + +@pytest.mark.parametrize( + "bad_entry", + [ + "https://example.com", # scheme + "example.com/path", # path + "example.com:8080", # port + "two words.com", # whitespace + ".", # dots-only would normalize to empty (unrestricted) — reject + "..", # dots-only + "ex\nample.com", # newline whitespace must be rejected too + ], +) +def test_web_config_rejects_malformed_entries(bad_entry: str) -> None: + with pytest.raises(ValidationError): + WebConfig(allowed_domains=[bad_entry]) diff --git a/tests/tools/test_web_allowlist_tools.py b/tests/tools/test_web_allowlist_tools.py new file mode 100644 index 00000000..d69a5270 --- /dev/null +++ b/tests/tools/test_web_allowlist_tools.py @@ -0,0 +1,99 @@ +# ruff: noqa + +"""Tool-level tests for the web domain allowlist on FetchURL and SearchWeb.""" + +from __future__ import annotations + +from aiohttp import web +from pydantic import SecretStr + +from pythinker_code.config import Config, PythinkerAISearchConfig +from pythinker_code.soul.toolset import current_tool_call +from pythinker_code.tools.web import fetch as fetch_module +from pythinker_code.tools.web.fetch import FetchURL, Params, _validate_fetch_url +from pythinker_code.tools.web.search import Params as SearchParams +from pythinker_code.tools.web.search import SearchWeb +from pythinker_code.wire.types import ToolCall + + +def test_validate_fetch_url_allows_in_allowlist_host() -> None: + # Allowed host (subdomain) passes validation; SSRF check is unrelated here. + assert _validate_fetch_url("https://docs.example.com/x", ["example.com"]) is None + + +def test_validate_fetch_url_rejects_out_of_allowlist_host() -> None: + reason = _validate_fetch_url("https://evil.org/x", ["example.com"]) + assert reason == "URL host is not in the configured web allowlist." + + +async def test_fetch_url_rejected_by_allowlist_makes_no_request( + config: Config, runtime, monkeypatch +) -> None: + config.web.allowed_domains = ["example.com"] + + def _no_network(*_args, **_kwargs): + raise AssertionError("network must not be opened for a disallowed host") + + monkeypatch.setattr(fetch_module, "new_client_session", _no_network) + + tool = FetchURL(config=config, runtime=runtime) + result = await tool(Params(url="https://evil.org/page")) + + assert result.is_error + assert "allowlist" in result.message.lower() + + +async def test_search_web_filters_results_by_allowlist(config: Config, runtime) -> None: + payload = { + "search_results": [ + { + "site_name": "Example", + "title": "Allowed", + "url": "https://docs.example.com/a", + "snippet": "kept", + }, + { + "site_name": "Evil", + "title": "Blocked", + "url": "https://evil.org/b", + "snippet": "dropped", + }, + ] + } + + async def handler(request: web.Request) -> web.Response: # noqa: ARG001 + return web.json_response(payload) + + app = web.Application() + app.router.add_post("/search", handler) + runner = web.AppRunner(app) + await runner.setup() + site = web.TCPSite(runner, host="127.0.0.1", port=0) + await site.start() + port = site._server.sockets[0].getsockname()[1] # type: ignore[index] + + try: + config.services.pythinker_ai_search = PythinkerAISearchConfig( + base_url=f"http://127.0.0.1:{port}/search", + api_key=SecretStr("test-key"), + ) + config.web.allowed_domains = ["example.com"] + tool = SearchWeb(config, runtime) + + token = current_tool_call.set( + ToolCall( + id="test-call-id", + function=ToolCall.FunctionBody(name="SearchWeb", arguments=None), + ) + ) + try: + result = await tool(SearchParams(query="x")) + finally: + current_tool_call.reset(token) + finally: + await runner.cleanup() + + assert not result.is_error + assert "docs.example.com" in result.output + assert "evil.org" not in result.output + assert (result.extras or {}).get("allowlist_filtered") == 1 diff --git a/tests/ui/test_shell_markdown.py b/tests/ui/test_shell_markdown.py index 0316a8e4..027dd451 100644 --- a/tests/ui/test_shell_markdown.py +++ b/tests/ui/test_shell_markdown.py @@ -63,6 +63,37 @@ def test_shell_markdown_renders_priority_matrix_as_grouped_rows() -> None: assert "────────────────" not in output +def test_shell_markdown_repairs_glued_table_header() -> None: + # The model sometimes glues the header onto preceding prose and drops the + # newline before the |---| delimiter; markdown-it then renders it as raw + # text. The normalizer should rebuild a real table. + output = _render_text( + PythinkerMarkdown( + "● LOW — Various| Category | Issue | Locations |\n\n" + "|----------|-------|-----------| | Error handling | Bare except | 12 files |\n" + ) + ) + # Header text on its own line, no raw delimiter pipes left in the output. + assert "Category" in output and "Locations" in output + assert "Error handling" in output and "12 files" in output + assert "---" not in output + assert "|----------|" not in output + + +def test_shell_markdown_leaves_inline_pipes_alone() -> None: + # A stray inline |-| in prose must not be mistaken for a table delimiter. + text = "Use the `a | b` operator. See |--| inline here." + output = _render_text(PythinkerMarkdown(text)) + assert "operator" in output and "inline here" in output + + +def test_shell_markdown_keeps_table_like_pipes_in_code_fence() -> None: + output = _render_text(PythinkerMarkdown("```\n| not | a | table |\n|-----|---|-------|\n```\n")) + # Inside a fence the pipes and delimiter survive verbatim. + assert "| not | a | table |" in output + assert "|-----|---|-------|" in output + + def test_shell_markdown_pads_code_block_with_blank_rows() -> None: # The code block should read as a distinct section, with a blank row framing # the panel above and below so it never crowds the surrounding prose. diff --git a/tests/ui_and_conv/test_tui_card_tool_renderers.py b/tests/ui_and_conv/test_tui_card_tool_renderers.py index ae0aea30..7d098077 100644 --- a/tests/ui_and_conv/test_tui_card_tool_renderers.py +++ b/tests/ui_and_conv/test_tui_card_tool_renderers.py @@ -46,6 +46,7 @@ def _render( is_error: bool = False, expanded: bool = False, width: int = 100, + details: dict | None = None, ) -> str: defn = get_tool_renderer(tool) assert defn is not None, f"renderer not registered for {tool!r}" @@ -53,7 +54,7 @@ def _render( comp.update_args(args) comp.set_args_complete() comp.mark_execution_started() - comp.set_result(ToolResultPayload(text=output, is_error=is_error)) + comp.set_result(ToolResultPayload(text=output, is_error=is_error, details=details or {})) comp.set_expanded(expanded) return render_plain(comp.render(), width=width) @@ -907,6 +908,43 @@ def test_search_counts_structured_result_blocks(): assert "Found 2 results" in rendered +def test_search_shows_allowlist_filtered_indicator(): + rendered = _render( + "SearchWeb", + {"query": "python"}, + output="Title: One\nDate: \nURL: https://example.com/1\nSummary: A\n\n", + details={"extras": {"allowlist_filtered": 2}}, + ) + assert "Found 1 result" in rendered + assert "2 filtered to allowlist" in rendered + + +def test_search_no_allowlist_indicator_when_not_filtered(): + rendered = _render( + "SearchWeb", + {"query": "python"}, + output="Title: One\nDate: \nURL: https://example.com/1\nSummary: A\n\n", + ) + assert "filtered to allowlist" not in rendered + + +def test_search_all_results_filtered_reports_zero(): + # When every result is dropped by the allowlist, SearchWeb emits prose plus a + # structured returned_results=0 signal; the renderer must prefer that count + # instead of misreading the one-line prose as a single result. + rendered = _render( + "SearchWeb", + {"query": "python"}, + output=( + "All 2 search result(s) were outside the configured web allowlist " + "and have been omitted." + ), + details={"extras": {"allowlist_filtered": 2, "returned_results": 0}}, + ) + assert "Found 0 results" in rendered + assert "2 filtered to allowlist" in rendered + + # --------------------------------------------------------------------------- # Background tasks # --------------------------------------------------------------------------- diff --git a/tests/utils/test_pyinstaller_utils.py b/tests/utils/test_pyinstaller_utils.py index 35aea41f..e0f867be 100644 --- a/tests/utils/test_pyinstaller_utils.py +++ b/tests/utils/test_pyinstaller_utils.py @@ -284,6 +284,7 @@ def test_pyinstaller_hiddenimports(): "pythinker_code.tools.todo", "pythinker_code.tools.utils", "pythinker_code.tools.web", + "pythinker_code.tools.web._allowlist", "pythinker_code.tools.web.fetch", "pythinker_code.tools.web.search", "setproctitle",