From 65336e9deb1f8394ca15fed893e0021af5d2a693 Mon Sep 17 00:00:00 2001 From: QuanCheng <915158214@qq.com> Date: Sat, 25 Jul 2026 08:36:35 +0000 Subject: [PATCH 01/10] add workspace image search and a server-side URL fetch tool Improves the workspace's shared search and content-fetching so agents can actually read pages and provide images, plus the security that ships with them. Content fetching: - workspace_fetch_url / POST /v1/fetch: read any URL as text without holding a shared browser tab. Static HTTP first; JS-heavy pages (Notion, SPAs) are rendered in an ephemeral browser session and closed within the request. - Per-workspace browser tab quota (DB-backed) + an idle-tab reaper, so an agent no longer sees '3/3 full' while its own tab list is empty, and abandoned tabs stop holding slots. - claude adapter hard-bans the native WebFetch (can't render JS) in favor of workspace_fetch_url; WebSearch stays allowed. Images: - workspace_image_search / POST /v1/search/images: Brave image search (BRAVE_SEARCH_API_KEY env). Returns image URLs agents embed as markdown, or persist with workspace_image_save. - POST /v1/files/from_url downloads a result into workspace storage and, with post_to_channel, posts it into the chat as an inline attachment. Security that comes with the feature: - SSRF-safe outbound fetch (app/net_security.py): scheme allowlist, reject URL credentials, resolve + block private/loopback/link-local/metadata IPs (incl. IPv4-mapped IPv6), re-validate each redirect hop, trust_env=False, and a streamed size cap. Applied to /v1/fetch and /v1/files/from_url. - Downloads only render a raster-image allowlist inline; SVG/HTML are served as attachments with X-Content-Type-Options: nosniff (stored-XSS guard). - Brave key is read from env only, never workspace settings, so it can't leak through workspace API responses. --- .../agent-connector/src/adapters/claude.js | 14 +- .../src/adapters/workspace-prompt.js | 67 +++- packages/agent-connector/src/mcp-server.js | 112 +++++++ .../agent-connector/src/workspace-client.js | 44 ++- .../openagents/adapters/workspace_prompt.py | 93 +++++- workspace/backend/app/browser.py | 81 +++++ workspace/backend/app/net_security.py | 167 ++++++++++ workspace/backend/app/routers/fetch.py | 311 ++++++++++++++++++ workspace/backend/app/routers/files.py | 214 +++++++++++- workspace/backend/app/routers/search.py | 131 ++++++++ workspace/backend/tests/test_browser_quota.py | 110 +++++++ workspace/backend/tests/test_fetch.py | 227 +++++++++++++ workspace/backend/tests/test_net_security.py | 109 ++++++ workspace/backend/tests/test_search_images.py | 272 +++++++++++++++ 14 files changed, 1930 insertions(+), 22 deletions(-) create mode 100644 workspace/backend/app/net_security.py create mode 100644 workspace/backend/app/routers/fetch.py create mode 100644 workspace/backend/app/routers/search.py create mode 100644 workspace/backend/tests/test_browser_quota.py create mode 100644 workspace/backend/tests/test_fetch.py create mode 100644 workspace/backend/tests/test_net_security.py create mode 100644 workspace/backend/tests/test_search_images.py diff --git a/packages/agent-connector/src/adapters/claude.js b/packages/agent-connector/src/adapters/claude.js index 9eec83dcc..95d108a93 100644 --- a/packages/agent-connector/src/adapters/claude.js +++ b/packages/agent-connector/src/adapters/claude.js @@ -419,7 +419,14 @@ class ClaudeAdapter extends BaseAdapter { const cmd = [claudeBin, '-p', prompt, '--output-format', 'stream-json', '--verbose']; cmd.push('--append-system-prompt', systemPrompt); - cmd.push('--disallowedTools', 'AskUserQuestion', 'CronCreate', 'CronDelete', 'CronList', 'ScheduleWakeup'); + const disallowedTools = ['AskUserQuestion', 'CronCreate', 'CronDelete', 'CronList', 'ScheduleWakeup']; + if (browserEnabled) { + // Hard-ban the native WebFetch: it can't render JS and bypasses the + // workspace fetch chain. Prompt-level bans proved insufficient. + // WebSearch stays allowed (pure search, no page fetching). + disallowedTools.push('WebFetch'); + } + cmd.push('--disallowedTools', ...disallowedTools); // Resume existing conversation (skipped on retry after stale session) const sessionId = this._channelSessions[channelName]; @@ -493,8 +500,13 @@ class ClaudeAdapter extends BaseAdapter { mcpTools.push(`${pfx}workspace_list_files`, `${pfx}workspace_read_file`); mcpWriteTools.push(`${pfx}workspace_write_file`, `${pfx}workspace_delete_file`); } + if (!this.disabledModules.has('search')) { + mcpTools.push(`${pfx}workspace_image_search`); + mcpWriteTools.push(`${pfx}workspace_image_save`); + } if (!this.disabledModules.has('browser')) { mcpTools.push( + `${pfx}workspace_fetch_url`, `${pfx}workspace_browser_list_tabs`, `${pfx}workspace_browser_snapshot`, `${pfx}workspace_browser_screenshot` diff --git a/packages/agent-connector/src/adapters/workspace-prompt.js b/packages/agent-connector/src/adapters/workspace-prompt.js index 90385e7a8..a816a548a 100644 --- a/packages/agent-connector/src/adapters/workspace-prompt.js +++ b/packages/agent-connector/src/adapters/workspace-prompt.js @@ -30,10 +30,17 @@ function buildBrowserDirective(browserEnabled) { return ( '\n## Browser Use (MANDATORY)\n' + 'This workspace has the **shared Browser Fabric session** enabled. ' + - 'All web browsing MUST go through it so the user can watch the ' + - 'session live in their right-side panel and so cookies / state ' + - 'persist across agents.\n\n' + - '**Use ONLY these tools for any web browsing:**\n' + + 'All web browsing MUST go through the workspace tools so the user can ' + + 'watch the session live in their right-side panel and so cookies / ' + + 'state persist across agents.\n\n' + + '**To READ a web page, ALWAYS use `mcp__openagents-workspace__workspace_fetch_url` first.** ' + + 'It handles JavaScript-heavy pages (Notion, SPAs) automatically and does ' + + 'not consume a shared browser tab. Only open a shared browser tab when ' + + 'you need to interact with the page (click, type, log in) or when ' + + 'workspace_fetch_url reports AUTH_REQUIRED / BOT_CHALLENGE — in that ' + + 'case open the URL in a tab and ask a human to complete the login in ' + + 'the live view.\n\n' + + '**Tools for interactive browsing:**\n' + '- `mcp__openagents-workspace__workspace_browser_open`\n' + '- `mcp__openagents-workspace__workspace_browser_navigate`\n' + '- `mcp__openagents-workspace__workspace_browser_click`\n' + @@ -43,13 +50,19 @@ function buildBrowserDirective(browserEnabled) { '- `mcp__openagents-workspace__workspace_browser_list_tabs`\n' + '- `mcp__openagents-workspace__workspace_browser_close`\n' + '\n' + + 'Shared browser tabs are a limited per-workspace resource: close your ' + + 'tab (`workspace_browser_close`) as soon as you are done with it. Idle ' + + 'tabs are auto-closed after a few minutes.\n\n' + 'If you don\'t have these MCP tools, use `Bash` + `curl` against ' + - '`/v1/browser/tabs` (documented below in Shared Browser).\n\n' + + '`/v1/fetch` and `/v1/browser/tabs` (documented below in Shared Browser).\n\n' + '**FORBIDDEN — do NOT call any of these:**\n' + '- `mcp__browsermcp__*` (any local Browser MCP extension tool)\n' + '- `mcp__playwright__*`, `mcp__puppeteer__*`, `mcp__chrome-devtools__*`, or any other local-browser MCP\n' + - '- `WebFetch`, `WebSearch`, `web_fetch`, `web_search`, or any built-in network/browser tool\n' + + '- `WebFetch` / `web_fetch` — it cannot render JavaScript and fails on ' + + 'many pages; use `workspace_fetch_url` instead\n' + '\n' + + '`WebSearch` / `web_search` (pure search, no page fetching) IS allowed — ' + + 'but read the result URLs with `workspace_fetch_url`, not WebFetch.\n\n' + 'If a local browser tool errors with "extension isn\'t connected" or ' + '"connect your browser", do NOT ask the user to connect anything — ' + 'the local extension is irrelevant here. Immediately switch to the ' + @@ -207,6 +220,7 @@ function buildApiSkillsPrompt({ endpoint, workspaceId, token, agentName, channel // Capabilities preamble const caps = []; if (!disabled.has('files')) caps.push('share and read files with other agents and users'); + if (!disabled.has('search')) caps.push('search the web for images and post them into the chat'); if (!disabled.has('browser')) caps.push('browse websites in a shared browser'); if (!disabled.has('knowledge')) caps.push('create and access a shared knowledge base'); caps.push('discover other agents in the workspace'); @@ -287,6 +301,34 @@ function buildApiSkillsPrompt({ endpoint, workspaceId, token, agentName, channel sections.push(s); } + // Image search + if (!disabled.has('search')) { + let s = '\n### Image Search\n\n'; + s += ( + 'You CAN find images on the web and show them in the chat.\n\n' + + '**Search images:**\n' + + `${curl} -s -X POST ${baseUrl}/v1/search/images ` + + `-H "${h}" -H "Content-Type: application/json" ` + + `-d '{"query":"golden gate bridge","network":"${workspaceId}","count":10}'\n\n` + + '**To show an image in chat**, embed the result\'s `image_url` in your reply ' + + 'as markdown: `![title](image_url)` — it renders inline.\n\n' + ); + if (!isPlan) { + s += ( + '**To keep a copy in the workspace AND post it as an attachment** ' + + '(survives external links going dead):\n' + + `${curl} -s -X POST ${baseUrl}/v1/files/from_url ` + + `-H "${h}" -H "Content-Type: application/json" ` + + `-d '{"url":"IMAGE_URL","network":"${workspaceId}",` + + `"channel_name":"${channelName}","source":"openagents:${agentName}",` + + `"post_to_channel":true,"caption":"optional message text"}'\n\n` + + 'Mention the source page when you share images, and never present a ' + + 'search result as license-free.\n' + ); + } + sections.push(s); + } + // Browser if (!disabled.has('browser')) { let s = '\n### Shared Browser\n\n'; @@ -304,7 +346,14 @@ function buildApiSkillsPrompt({ endpoint, workspaceId, token, agentName, channel if (!isPlan) { s += ( - '**To browse a website**, exec these steps (use exec for each):\n' + + '**To just READ a page (preferred — no tab needed, handles JS pages):**\n' + + `${curl} -s -X POST ${baseUrl}/v1/fetch ` + + `-H "${h}" -H "Content-Type: application/json" ` + + `-d '{"url":"https://example.com","network":"${workspaceId}",` + + `"source":"openagents:${agentName}"}'\n` + + 'If it returns error_code AUTH_REQUIRED or BOT_CHALLENGE, open the URL ' + + 'in a shared tab (below) and share its `live_url` so a human can log in.\n\n' + + '**To browse interactively** (click/type/login), exec these steps (use exec for each):\n' + `Step 1 — open tab: ` + `${curl} -s -X POST ${baseUrl}/v1/browser/tabs ` + `-H "${h}" -H "Content-Type: application/json" ` + @@ -314,7 +363,9 @@ function buildApiSkillsPrompt({ endpoint, workspaceId, token, agentName, channel `${curl} -s -H "${h}" ${baseUrl}/v1/browser/tabs/TAB_ID/snapshot\n` + `Step 3 — close tab: ` + `${curl} -s -X DELETE -H "${h}" ${baseUrl}/v1/browser/tabs/TAB_ID\n` + - '(Replace TAB_ID with the `id` from the step 1 response)\n\n' + '(Replace TAB_ID with the `id` from the step 1 response)\n' + + 'Tabs are a limited per-workspace resource — always close yours when done; ' + + 'idle tabs are auto-closed after a few minutes.\n\n' ); } diff --git a/packages/agent-connector/src/mcp-server.js b/packages/agent-connector/src/mcp-server.js index 77c8a1f9a..d50b44e19 100644 --- a/packages/agent-connector/src/mcp-server.js +++ b/packages/agent-connector/src/mcp-server.js @@ -100,9 +100,66 @@ function buildToolDefs(disabledModules) { ); } + // -- Search module -- + if (!disabledModules.has('search')) { + tools.push( + { + name: 'workspace_image_search', + description: + 'Search the web for images. Returns image URLs you can show in chat directly by embedding ' + + 'markdown — ![title](image_url) — in your reply, or persist with workspace_image_save.', + inputSchema: { + type: 'object', + properties: { + query: { type: 'string', description: 'Image search query' }, + count: { type: 'number', description: 'Number of results (default 10, max 20)' }, + }, + required: ['query'], + }, + }, + { + name: 'workspace_image_save', + description: + 'Download an image (or any file) URL into workspace storage. By default it is also posted ' + + 'into the current channel as an inline image attachment (set post_to_channel=false to only save).', + inputSchema: { + type: 'object', + properties: { + url: { type: 'string', description: 'Direct image/file URL (e.g. image_url from workspace_image_search)' }, + filename: { type: 'string', description: 'Filename to store as (derived from URL if omitted)' }, + caption: { type: 'string', description: 'Message text to accompany the posted image' }, + post_to_channel: { type: 'boolean', description: 'Post into the chat as an attachment (default true)' }, + }, + required: ['url'], + }, + }, + ); + } + // -- Browser module -- if (!disabledModules.has('browser')) { tools.push( + { + name: 'workspace_fetch_url', + description: + 'Read a web page as text WITHOUT opening a shared browser tab. Preferred way to read any URL: ' + + 'tries a fast static fetch first, and automatically renders JavaScript-heavy pages (Notion, SPAs) ' + + 'in a temporary browser session. Only open a shared browser tab (workspace_browser_open) when you ' + + 'need to interact with the page (click/type/login) or the result says AUTH_REQUIRED.', + inputSchema: { + type: 'object', + properties: { + url: { type: 'string', description: 'URL to fetch (http/https)' }, + mode: { + type: 'string', + enum: ['auto', 'static', 'render'], + description: 'auto (default): static first, browser render if needed. static: never use a browser. render: force browser rendering.', + }, + max_chars: { type: 'number', description: 'Max characters of content to return (default 20000)' }, + }, + required: ['url'], + }, + }, { name: 'workspace_browser_open', description: @@ -629,6 +686,61 @@ class McpServer { // ── Browser ── + case 'workspace_fetch_url': { + try { + const result = await this.ws.fetchUrl(this.workspaceId, this.token, args.url, { + mode: args.mode || 'auto', + maxChars: args.max_chars, + source: `openagents:${this.agentName}`, + }); + const header = [ + `URL: ${result.url}`, + result.title ? `Title: ${result.title}` : null, + `Source: ${result.content_source}${result.truncated ? ' (truncated)' : ''}`, + ].filter(Boolean).join('\n'); + return text(`${header}\n\n${result.content || '(empty page)'}`); + } catch (e) { + const detail = e.data || {}; + const parts = [`Fetch failed: ${e.message}`]; + if (detail.error_code) parts.push(`Code: ${detail.error_code}`); + if (detail.hint) parts.push(`Hint: ${detail.hint}`); + return text(parts.join('\n')); + } + } + + case 'workspace_image_search': { + const data = await this.ws.searchImages(this.workspaceId, this.token, args.query, { + count: args.count || 10, + }); + const results = (data && data.results) || []; + if (!results.length) return text(`No image results for "${args.query}".`); + const lines = results.map((r, i) => { + const dims = r.width && r.height ? ` (${r.width}x${r.height})` : ''; + return `${i + 1}. ${r.title || 'untitled'}${dims}\n` + + ` image_url: ${r.image_url}\n` + + ` source: ${r.page_url || r.source || 'unknown'}`; + }); + return text( + `Found ${results.length} images for "${args.query}":\n\n${lines.join('\n')}\n\n` + + 'To show one in chat, embed it in your reply as markdown: ![title](image_url). ' + + 'To keep a copy in the workspace (and post it as an attachment), call workspace_image_save with the image_url.' + ); + } + + case 'workspace_image_save': { + const result = await this.ws.uploadFileFromUrl(this.workspaceId, this.token, args.url, { + filename: args.filename, + channelName: this.channelName, + source: `openagents:${this.agentName}`, + postToChannel: args.post_to_channel !== false, + caption: args.caption, + }); + const posted = result.posted_to_channel + ? ' and posted to the channel' + : ''; + return text(`Saved ${result.filename} (${result.content_type}, ${result.size} bytes, id: ${result.id})${posted}.`); + } + case 'workspace_browser_open': { const opts = { url: args.url || 'about:blank', diff --git a/packages/agent-connector/src/workspace-client.js b/packages/agent-connector/src/workspace-client.js index 81dd1edb6..7df4109af 100644 --- a/packages/agent-connector/src/workspace-client.js +++ b/packages/agent-connector/src/workspace-client.js @@ -471,6 +471,31 @@ class WorkspaceClient { return data.data || data; } + /** + * Search the web for images via POST /v1/search/images. + */ + async searchImages(workspaceId, token, query, { count = 10 } = {}) { + const body = { query, network: workspaceId, count }; + const data = await this._post('/v1/search/images', body, this._wsHeaders(token), 30000); + return data.data || data; + } + + /** + * Download a URL into workspace storage via POST /v1/files/from_url. + * With postToChannel, the file is also posted into the chat as an + * inline attachment. + */ + async uploadFileFromUrl(workspaceId, token, url, { + filename, channelName, source = 'human:user', postToChannel = false, caption, + } = {}) { + const body = { url, network: workspaceId, source, post_to_channel: postToChannel }; + if (filename) body.filename = filename; + if (channelName) body.channel_name = channelName; + if (caption) body.caption = caption; + const data = await this._post('/v1/files/from_url', body, this._wsHeaders(token), 90000); + return data.data || data; + } + /** * List files via GET /v1/files. */ @@ -583,6 +608,19 @@ class WorkspaceClient { return data.data || data; } + /** + * Server-side fetch chain via POST /v1/fetch — reads a page WITHOUT + * holding a shared browser tab. Static HTTP first; JS-heavy pages are + * rendered in an ephemeral browser session that is closed immediately. + */ + async fetchUrl(workspaceId, token, url, { mode = 'auto', maxChars, source } = {}) { + const body = { url, network: workspaceId, mode }; + if (maxChars) body.max_chars = maxChars; + if (source) body.source = source; + const data = await this._post('/v1/fetch', body, this._wsHeaders(token), 90000); + return data.data || data; + } + /** * List persistent browser contexts via GET /v1/browser/contexts. */ @@ -878,7 +916,11 @@ class WorkspaceClient { if (typeof msg === 'string' && msg.toLowerCase().includes('session_revoked')) { reject(new SessionRevokedError(msg)); } else { - reject(new Error(msg)); + const err = new Error(msg); + // Preserve structured error details (error_code, hint, + // quota occupancy, ...) so tool handlers can surface them. + if (parsed.data && typeof parsed.data === 'object') err.data = parsed.data; + reject(err); } } else { resolve(parsed); diff --git a/sdk/src/openagents/adapters/workspace_prompt.py b/sdk/src/openagents/adapters/workspace_prompt.py index eee392141..55def7562 100644 --- a/sdk/src/openagents/adapters/workspace_prompt.py +++ b/sdk/src/openagents/adapters/workspace_prompt.py @@ -30,10 +30,17 @@ def build_browser_directive(browser_enabled: bool) -> str: return ( "\n## Browser Use (MANDATORY)\n" "This workspace has the **shared Browser Fabric session** enabled. " - "All web browsing MUST go through it so the user can watch the " - "session live in their right-side panel and so cookies / state " - "persist across agents.\n\n" - "**Use ONLY these tools for any web browsing:**\n" + "All web browsing MUST go through the workspace tools so the user can " + "watch the session live in their right-side panel and so cookies / " + "state persist across agents.\n\n" + "**To READ a web page, ALWAYS use `mcp__openagents-workspace__workspace_fetch_url` first.** " + "It handles JavaScript-heavy pages (Notion, SPAs) automatically and does " + "not consume a shared browser tab. Only open a shared browser tab when " + "you need to interact with the page (click, type, log in) or when " + "workspace_fetch_url reports AUTH_REQUIRED / BOT_CHALLENGE — in that " + "case open the URL in a tab and ask a human to complete the login in " + "the live view.\n\n" + "**Tools for interactive browsing:**\n" "- `mcp__openagents-workspace__workspace_browser_open`\n" "- `mcp__openagents-workspace__workspace_browser_navigate`\n" "- `mcp__openagents-workspace__workspace_browser_click`\n" @@ -43,13 +50,19 @@ def build_browser_directive(browser_enabled: bool) -> str: "- `mcp__openagents-workspace__workspace_browser_list_tabs`\n" "- `mcp__openagents-workspace__workspace_browser_close`\n" "\n" + "Shared browser tabs are a limited per-workspace resource: close your " + "tab (`workspace_browser_close`) as soon as you are done with it. Idle " + "tabs are auto-closed after a few minutes.\n\n" "If you don't have these MCP tools, use `Bash` + `curl` against " - "`/v1/browser/tabs` (documented below in Shared Browser).\n\n" + "`/v1/fetch` and `/v1/browser/tabs` (documented below in Shared Browser).\n\n" "**FORBIDDEN — do NOT call any of these:**\n" "- `mcp__browsermcp__*` (any local Browser MCP extension tool)\n" "- `mcp__playwright__*`, `mcp__puppeteer__*`, `mcp__chrome-devtools__*`, or any other local-browser MCP\n" - "- `WebFetch`, `WebSearch`, `web_fetch`, `web_search`, or any built-in network/browser tool\n" + "- `WebFetch` / `web_fetch` — it cannot render JavaScript and fails on " + "many pages; use `workspace_fetch_url` instead\n" "\n" + "`WebSearch` / `web_search` (pure search, no page fetching) IS allowed — " + "but read the result URLs with `workspace_fetch_url`, not WebFetch.\n\n" "If a local browser tool errors with \"extension isn't connected\" or " "\"connect your browser\", do NOT ask the user to connect anything — " "the local extension is irrelevant here. Immediately switch to the " @@ -138,6 +151,8 @@ def build_api_skills_prompt( caps = [] if "files" not in _disabled: caps.append("share and read files with other agents and users") + if "search" not in _disabled: + caps.append("search the web for images and post them into the chat") if "browser" not in _disabled: caps.append("browse websites in a shared browser") caps.append("discover other agents in the workspace") @@ -196,6 +211,32 @@ def build_api_skills_prompt( sections.append(s) + # ── Image search ── + if "search" not in _disabled: + s = "\n### Image Search\n\n" + s += ( + "You CAN find images on the web and show them in the chat.\n\n" + "**Search images:**\n" + f"curl -s -X POST {base_url}/v1/search/images " + f'-H "{h}" -H "Content-Type: application/json" ' + f'-d \'{{"query":"golden gate bridge","network":"{workspace_id}","count":10}}\'\n\n' + "**To show an image in chat**, embed the result's `image_url` in your reply " + "as markdown: `![title](image_url)` — it renders inline.\n\n" + ) + if not is_plan: + s += ( + "**To keep a copy in the workspace AND post it as an attachment** " + "(survives external links going dead):\n" + f"curl -s -X POST {base_url}/v1/files/from_url " + f'-H "{h}" -H "Content-Type: application/json" ' + f'-d \'{{"url":"IMAGE_URL","network":"{workspace_id}",' + f'"channel_name":"{channel_name}","source":"openagents:{agent_name}",' + f'"post_to_channel":true,"caption":"optional message text"}}\'\n\n' + "Mention the source page when you share images, and never present a " + "search result as license-free.\n" + ) + sections.append(s) + # ── Browser ── if "browser" not in _disabled: s = "\n### Shared Browser\n\n" @@ -402,6 +443,8 @@ def _build_opencode_api_skills_prompt( caps = [] if "files" not in _disabled: caps.append("share and read files with other agents and users") + if "search" not in _disabled: + caps.append("search the web for images and post them into the chat") if "browser" not in _disabled: caps.append("browse websites in a shared browser") caps.append("discover other agents in the workspace") @@ -458,13 +501,45 @@ def _build_opencode_api_skills_prompt( sections.append(s) + # ── Image search ── + if "search" not in _disabled: + s = "\n### Image Search\n\n" + s += ( + "You CAN find images on the web and show them in the chat.\n\n" + "**Search images:**\n" + f"curl -s -X POST {base_url}/v1/search/images " + f'-H "{h}" -H "Content-Type: application/json" ' + f'-d \'{{"query":"golden gate bridge","network":"{workspace_id}","count":10}}\'\n\n' + "**To show an image in chat**, embed the result's `image_url` in your reply " + "as markdown: `![title](image_url)` — it renders inline.\n\n" + ) + if not is_plan: + s += ( + "**To keep a copy in the workspace AND post it as an attachment:**\n" + f"curl -s -X POST {base_url}/v1/files/from_url " + f'-H "{h}" -H "Content-Type: application/json" ' + f'-d \'{{"url":"IMAGE_URL","network":"{workspace_id}",' + f'"channel_name":"{channel_name}","source":"openagents:{agent_name}",' + f'"post_to_channel":true,"caption":"optional message text"}}\'\n\n' + "Mention the source page when you share images, and never present a " + "search result as license-free.\n" + ) + sections.append(s) + # ── Browser ── if "browser" not in _disabled: s = "\n### Shared Browser\n\n" if not is_plan: s += ( - "**To browse a website**, run these steps in bash:\n" + "**To just READ a page (preferred — no tab needed, handles JS pages):**\n" + f"curl -s -X POST {base_url}/v1/fetch " + f'-H "{h}" -H "Content-Type: application/json" ' + f'-d \'{{"url":"https://example.com","network":"{workspace_id}",' + f'"source":"openagents:{agent_name}"}}\'\n' + "If it returns error_code AUTH_REQUIRED or BOT_CHALLENGE, open the URL " + "in a shared tab (below) and share its `live_url` so a human can log in.\n\n" + "**To browse interactively** (click/type/login), run these steps in bash:\n" f"Step 1 — open tab: " f"curl -s -X POST {base_url}/v1/browser/tabs " f'-H "{h}" -H "Content-Type: application/json" ' @@ -474,7 +549,9 @@ def _build_opencode_api_skills_prompt( f'curl -s -H "{h}" {base_url}/v1/browser/tabs/TAB_ID/snapshot\n' f"Step 3 — close tab: " f'curl -s -X DELETE -H "{h}" {base_url}/v1/browser/tabs/TAB_ID\n' - f"(Replace TAB_ID with the id from step 1 response)\n\n" + f"(Replace TAB_ID with the id from step 1 response)\n" + "Tabs are a limited per-workspace resource — always close yours when done; " + "idle tabs are auto-closed after a few minutes.\n\n" ) s += ( diff --git a/workspace/backend/app/browser.py b/workspace/backend/app/browser.py index ae4ff2c60..67df8b857 100644 --- a/workspace/backend/app/browser.py +++ b/workspace/backend/app/browser.py @@ -25,6 +25,32 @@ BROWSERFABRIC_URL = os.environ.get("BROWSERFABRIC_URL", "https://api.browserfabric.com") BROWSERFABRIC_PROVISION_SECRET = os.environ.get("BROWSERFABRIC_PROVISION_SECRET", "") +# render_page_text: let a JS page settle before snapshotting, and retry while it +# still looks like an empty shell. +RENDER_SETTLE_SECONDS = float(os.environ.get("RENDER_SETTLE_SECONDS", "1.5")) +RENDER_SETTLE_ATTEMPTS = int(os.environ.get("RENDER_SETTLE_ATTEMPTS", "3")) +RENDER_MIN_TEXT_CHARS = int(os.environ.get("RENDER_MIN_TEXT_CHARS", "50")) + + +class BrowserNavigationError(RuntimeError): + """Navigation failure with a machine-readable code the agent can act on.""" + + def __init__(self, code: str, message: str): + self.code = code + super().__init__(message) + + +def classify_navigation_error(exc: Exception) -> str: + """Map a raw navigation exception to a stable error code.""" + low = str(exc).lower() + if "err_name_not_resolved" in low or "err_cert" in low or "ssl" in low or "tls" in low or "dns" in low: + return "DNS_OR_TLS_ERROR" + if "timeout" in low or "timed out" in low: + return "NAV_TIMEOUT" + if "err_blocked" in low or "err_connection_refused" in low or "403" in low: + return "CONTENT_BLOCKED" + return "NAVIGATION_FAILED" + class BrowserManager: """Singleton managing shared browser tabs via Browser Fabric or local Playwright.""" @@ -396,6 +422,61 @@ async def close_tab(self, tab_id: str, session_id_hint: str = None, api_key: str pass return True, None + async def render_page_text(self, url: str, api_key: str = None) -> dict: + """One-shot render for the fetch chain: create a session/page, navigate, + snapshot, close. Never registers a tab, so it doesn't consume the + workspace tab quota. Returns {url, title, text}. + + Raises BrowserNavigationError on navigation failure. The caller + (/v1/fetch) has already validated the entry URL against private hosts. + """ + if self.is_cloud_for(api_key): + key = api_key or BROWSERFABRIC_API_KEY + result = await self._bf_call("create_session", {"headless": True}, api_key=key) + session_id = result["result"]["session_id"] + try: + try: + await self._bf_call( + "navigate", {"url": url, "wait_until": "domcontentloaded"}, session_id, api_key=key + ) + except Exception as e: + raise BrowserNavigationError(classify_navigation_error(e), str(e)[:500]) from e + # domcontentloaded fires before client-side frameworks (Notion, + # SPAs) paint. Snapshot after a short settle and retry while the + # page is still an empty shell. + text = "" + for _ in range(RENDER_SETTLE_ATTEMPTS): + await asyncio.sleep(RENDER_SETTLE_SECONDS) + snap = await self._bf_call("snapshot", {}, session_id, api_key=key) + text = snap.get("result", {}).get("snapshot", "") or "" + if len(text.strip()) >= RENDER_MIN_TEXT_CHARS: + break + info = await self._bf_call("get_page_info", {}, session_id, api_key=key) + page_info = info.get("result", {}) + return {"url": page_info.get("url", url), "title": page_info.get("title", ""), "text": text} + finally: + try: + await self._bf_call("close_session", {}, session_id, api_key=key) + except Exception as e: + logger.warning("Failed to close ephemeral BF session %s: %s", session_id, e) + else: + async with self._global_lock: + await self._ensure_local_browser() + page = await self._browser.new_page() + try: + try: + await page.goto(url, wait_until="domcontentloaded", timeout=30000) + except Exception as e: + raise BrowserNavigationError(classify_navigation_error(e), str(e)[:500]) from e + await page.wait_for_timeout(1500) # let client-side rendering settle + text = await page.inner_text("body") + return {"url": page.url, "title": await page.title(), "text": text} + finally: + try: + await page.close() + except Exception: + pass + async def shutdown(self) -> None: """Close all tabs and the browser.""" for tab_id in list(self._sessions.keys()) + list(self._pages.keys()): diff --git a/workspace/backend/app/net_security.py b/workspace/backend/app/net_security.py new file mode 100644 index 000000000..5b7694c92 --- /dev/null +++ b/workspace/backend/app/net_security.py @@ -0,0 +1,167 @@ +# -*- coding: utf-8 -*- +""" +SSRF-safe outbound HTTP for agent-triggered fetches. + +Anything that fetches a URL chosen by a workspace agent (POST /v1/fetch, +POST /v1/files/from_url) MUST go through here. A raw httpx.get would let an +agent reach the backend's own network: cloud metadata (169.254.169.254), +localhost, and private/internal services. + +Protections: + - scheme restricted to http/https + - URLs with embedded credentials rejected + - every hostname is resolved and ALL resolved IPs must be public + (private / loopback / link-local / reserved / multicast are blocked, + including IPv4-mapped IPv6) + - redirects are followed manually and re-validated at every hop + - trust_env=False so backend proxy env vars can't redirect the request + - streamed with an enforced byte cap so a huge body can't OOM the worker + +Known residual: a DNS-rebinding window exists between validation and the +socket connect. The high-severity vectors (direct internal URLs, metadata +IPs, redirect-to-internal) are all closed. +""" + +import asyncio +import ipaddress +import logging +from typing import Optional +from urllib.parse import urljoin, urlparse + +import httpx + +logger = logging.getLogger(__name__) + +ALLOWED_SCHEMES = {"http", "https"} +DEFAULT_MAX_REDIRECTS = 4 + + +class UnsafeURLError(Exception): + """A URL was rejected before or during fetching. Carries a stable code.""" + + def __init__(self, code: str, message: str): + self.code = code + super().__init__(message) + + +def _ip_is_public(ip: ipaddress._BaseAddress) -> bool: + if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped is not None: + ip = ip.ipv4_mapped + return not ( + ip.is_private + or ip.is_loopback + or ip.is_link_local + or ip.is_reserved + or ip.is_multicast + or ip.is_unspecified + ) + + +async def _resolve_ips(host: str, port: int) -> set: + loop = asyncio.get_event_loop() + try: + infos = await loop.getaddrinfo(host, port, proto=0, type=0) + except OSError as e: + raise UnsafeURLError("DNS_RESOLUTION_FAILED", f"Could not resolve host '{host}': {e}") from e + return {info[4][0] for info in infos} + + +async def validate_public_url(url: str) -> None: + """Raise UnsafeURLError unless `url` is an http(s) URL that resolves only + to public IP addresses and carries no embedded credentials.""" + parsed = urlparse(url) + if parsed.scheme not in ALLOWED_SCHEMES: + raise UnsafeURLError("UNSUPPORTED_SCHEME", "Only http(s) URLs are supported") + if parsed.username or parsed.password: + raise UnsafeURLError("URL_CREDENTIALS_NOT_ALLOWED", "URLs with embedded credentials are not allowed") + host = parsed.hostname + if not host: + raise UnsafeURLError("INVALID_URL", "URL has no host") + try: + port = parsed.port or (443 if parsed.scheme == "https" else 80) + except ValueError: + raise UnsafeURLError("INVALID_URL", "URL has an invalid port") + + try: + literal = ipaddress.ip_address(host) + ips = {str(literal)} + except ValueError: + ips = await _resolve_ips(host, port) + + if not ips: + raise UnsafeURLError("DNS_RESOLUTION_FAILED", f"Host '{host}' did not resolve") + for ip_str in ips: + if not _ip_is_public(ipaddress.ip_address(ip_str)): + raise UnsafeURLError( + "BLOCKED_PRIVATE_ADDRESS", + f"Host '{host}' resolves to a non-public address ({ip_str})", + ) + + +class SafeFetchResult: + __slots__ = ("content", "status_code", "headers", "final_url", "truncated") + + def __init__(self, content, status_code, headers, final_url, truncated): + self.content = content + self.status_code = status_code + self.headers = headers + self.final_url = final_url + self.truncated = truncated + + @property + def content_type(self) -> str: + return (self.headers.get("content-type", "") or "").split(";")[0].strip().lower() + + +async def safe_fetch( + url: str, + *, + max_bytes: int, + timeout: float, + headers: Optional[dict] = None, + truncate: bool = False, + max_redirects: int = DEFAULT_MAX_REDIRECTS, +) -> SafeFetchResult: + """SSRF-safe GET with manual, re-validated redirects and a streamed byte cap. + + truncate=True -> stop reading at max_bytes and flag `truncated` (text reads). + truncate=False -> raise UnsafeURLError('RESPONSE_TOO_LARGE') past max_bytes. + """ + request_headers = dict(headers or {}) + async with httpx.AsyncClient(follow_redirects=False, trust_env=False, timeout=timeout) as client: + current = url + for _ in range(max_redirects + 1): + await validate_public_url(current) + async with client.stream("GET", current, headers=request_headers) as resp: + if resp.is_redirect: + location = resp.headers.get("location") + if not location: + raise UnsafeURLError("NAVIGATION_FAILED", "Redirect without a Location header") + current = urljoin(current, location) + continue + + declared = resp.headers.get("content-length") + if declared and declared.isdigit() and int(declared) > max_bytes and not truncate: + raise UnsafeURLError("RESPONSE_TOO_LARGE", f"Response is {int(declared)} bytes; limit is {max_bytes}") + + chunks = [] + total = 0 + truncated = False + async for chunk in resp.aiter_bytes(): + total += len(chunk) + if total > max_bytes: + if truncate: + chunks.append(chunk[: len(chunk) - (total - max_bytes)]) + truncated = True + break + raise UnsafeURLError("RESPONSE_TOO_LARGE", f"Response exceeded {max_bytes} bytes") + chunks.append(chunk) + + return SafeFetchResult( + content=b"".join(chunks), + status_code=resp.status_code, + headers=resp.headers, + final_url=current, + truncated=truncated, + ) + raise UnsafeURLError("TOO_MANY_REDIRECTS", f"Exceeded {max_redirects} redirects") diff --git a/workspace/backend/app/routers/fetch.py b/workspace/backend/app/routers/fetch.py new file mode 100644 index 000000000..6ad5fcc60 --- /dev/null +++ b/workspace/backend/app/routers/fetch.py @@ -0,0 +1,311 @@ +# -*- coding: utf-8 -*- +""" +Server-side URL fetch chain for agents. + +POST /v1/fetch — read a web page without holding a shared-browser tab: + + 1. Static HTTP GET + text extraction (fast, no browser). + 2. If the page is a JS shell (Notion, Next.js apps, ...), render it in an + ephemeral browser session (created and closed within this request — + never counts against the workspace tab quota). + 3. If the rendered page is a login wall / bot challenge, return + AUTH_REQUIRED so the agent can open a shared browser tab and let a + human take over. + +Errors carry a stable error_code: + JS_RENDER_TIMEOUT | AUTH_REQUIRED | BOT_CHALLENGE | DNS_OR_TLS_ERROR | + CONTENT_BLOCKED | NAVIGATION_FAILED | UNSUPPORTED_CONTENT +""" + +import logging +import re +from html.parser import HTMLParser +from typing import Optional + +import httpx +from fastapi import APIRouter, Depends, Header +from pydantic import BaseModel +from sqlalchemy.orm import Session + +from app.browser import BrowserManager, BrowserNavigationError, classify_navigation_error +from app.database import get_db +from app.net_security import UnsafeURLError, safe_fetch, validate_public_url +from app.response import ResponseCode, json_response, success_response +from app.routers.browser import _resolve_bf_key +from app.routers.network import _resolve_workspace, _verify_workspace_access + +logger = logging.getLogger(__name__) + +router = APIRouter(prefix="/v1", tags=["Fetch"]) + +STATIC_TIMEOUT_SECONDS = 15.0 +MAX_RESPONSE_BYTES = 2 * 1024 * 1024 +DEFAULT_MAX_CHARS = 20000 +USER_AGENT = ( + "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) " + "Chrome/126.0.0.0 Safari/537.36 OpenAgentsFetch/1.0" +) + +# A static fetch that yields less text than this (from a large HTML payload) +# is treated as a JS shell and escalated to browser rendering. +JS_SHELL_MIN_TEXT_CHARS = 200 + +_JS_SHELL_MARKERS = ( + "enable javascript", + "requires javascript", + "javascript is disabled", + "javascript to run this app", + "please turn on javascript", +) + +_AUTH_MARKERS = ( + "sign in to continue", + "log in to continue", + "please sign in", + "please log in", + "authentication required", + "登录后查看", + "请先登录", +) + +_BOT_MARKERS = ( + "verify you are human", + "checking your browser", + "cloudflare", + "are you a robot", + "unusual traffic", + "captcha", + "验证码", +) + + +class _TextExtractor(HTMLParser): + """Minimal main-text extraction: strips tags, drops script/style/noscript + (noscript content is kept separately for JS-shell detection).""" + + _SKIP = {"script", "style", "svg", "template"} + + def __init__(self): + super().__init__(convert_charrefs=True) + self.chunks: list[str] = [] + self.noscript_chunks: list[str] = [] + self.title = "" + self._skip_depth = 0 + self._in_noscript = False + self._in_title = False + + def handle_starttag(self, tag, attrs): + if tag in self._SKIP: + self._skip_depth += 1 + elif tag == "noscript": + self._in_noscript = True + elif tag == "title": + self._in_title = True + + def handle_endtag(self, tag): + if tag in self._SKIP and self._skip_depth > 0: + self._skip_depth -= 1 + elif tag == "noscript": + self._in_noscript = False + elif tag == "title": + self._in_title = False + + def handle_data(self, data): + if self._skip_depth: + return + if self._in_title: + self.title += data + return + target = self.noscript_chunks if self._in_noscript else self.chunks + stripped = data.strip() + if stripped: + target.append(stripped) + + +def _extract_text(html: str) -> dict: + parser = _TextExtractor() + try: + parser.feed(html) + parser.close() + except Exception: + pass + text = re.sub(r"\n{3,}", "\n\n", "\n".join(parser.chunks)) + return { + "text": text, + "noscript": " ".join(parser.noscript_chunks), + "title": parser.title.strip(), + } + + +def _looks_like_js_shell(html: str, extracted: dict) -> bool: + combined = (extracted["noscript"] + " " + extracted["text"][:2000]).lower() + if any(marker in combined for marker in _JS_SHELL_MARKERS): + return True + return len(html) > 10000 and len(extracted["text"]) < JS_SHELL_MIN_TEXT_CHARS + + +def _detect_wall(text: str, title: str) -> Optional[str]: + """Return AUTH_REQUIRED / BOT_CHALLENGE if the page is a wall, else None. + + Markers alone are too false-positive-prone (any page with a login link + mentions "sign in"), so only classify short pages as walls. + """ + if len(text) > 1500: + return None + sample = (title + " " + text[:1500]).lower() + if any(marker in sample for marker in _BOT_MARKERS): + return "BOT_CHALLENGE" + if any(marker in sample for marker in _AUTH_MARKERS): + return "AUTH_REQUIRED" + return None + + +class FetchRequest(BaseModel): + url: str + network: str + source: Optional[str] = "human:user" + mode: str = "auto" # auto | static | render + max_chars: int = DEFAULT_MAX_CHARS + + +def _error(code: ResponseCode, message: str, error_code: str, **extra) -> object: + return json_response(code, message, data={"error_code": error_code, **extra}) + + +def _success(content: str, source: str, url: str, title: str, max_chars: int) -> object: + truncated = len(content) > max_chars + return success_response({ + "url": url, + "title": title, + "content": content[:max_chars], + "truncated": truncated, + "content_source": source, + }) + + +async def _static_fetch(url: str) -> dict: + """Tier 1: SSRF-safe streamed HTTP GET. Returns {html, final_url, + status_code} or raises UnsafeURLError / BrowserNavigationError.""" + result = await safe_fetch( + url, + max_bytes=MAX_RESPONSE_BYTES, + timeout=STATIC_TIMEOUT_SECONDS, + headers={"User-Agent": USER_AGENT, "Accept-Language": "en,zh;q=0.8"}, + truncate=True, # a partial page is fine for a text read + ) + content_type = result.content_type + if content_type and not any( + t in content_type + for t in ("text/html", "text/plain", "application/xhtml", "application/xml", "application/json") + ): + raise BrowserNavigationError( + "UNSUPPORTED_CONTENT", + f"Content-type '{content_type}' is not text; download it via the files API instead", + ) + return { + "html": result.content.decode("utf-8", errors="replace"), + "final_url": result.final_url, + "status_code": result.status_code, + } + + +@router.post("/fetch") +async def fetch_url( + body: FetchRequest, + x_workspace_token: Optional[str] = Header(None), + authorization: Optional[str] = Header(None), + db: Session = Depends(get_db), +): + workspace = _resolve_workspace(db, body.network) + if not workspace: + return json_response(ResponseCode.NOT_FOUND, "Network not found") + if not _verify_workspace_access(workspace, x_workspace_token, authorization): + return json_response(ResponseCode.UNAUTHORIZED, "Invalid workspace credentials") + + # SSRF guard — reject internal/metadata targets before any outbound call + # (covers both the static tier and the browser-render tier below). + try: + await validate_public_url(body.url) + except UnsafeURLError as e: + return _error(ResponseCode.BAD_REQUEST, str(e), e.code) + + max_chars = max(1000, min(body.max_chars, 100000)) + manager = BrowserManager.get() + + # ---- Tier 1: static HTTP ---- + static_result = None + static_error = None + if body.mode in ("auto", "static"): + try: + static_result = await _static_fetch(body.url) + except UnsafeURLError as e: + # A redirect hop pointed at an internal address — never fall back. + return _error(ResponseCode.BAD_REQUEST, str(e), e.code) + except BrowserNavigationError as e: + return _error(ResponseCode.BAD_REQUEST, str(e), e.code) + except httpx.TimeoutException as e: + static_error = ("NAV_TIMEOUT", f"Static fetch timed out after {STATIC_TIMEOUT_SECONDS}s: {e}") + except httpx.HTTPError as e: + static_error = (classify_navigation_error(e), f"Static fetch failed: {e}") + + if static_result is not None: + extracted = _extract_text(static_result["html"]) + wall = _detect_wall(extracted["text"], extracted["title"]) + upstream_error = static_result["status_code"] >= 400 + needs_render = ( + upstream_error + or _looks_like_js_shell(static_result["html"], extracted) + or wall is not None + ) + if body.mode == "static" or not needs_render: + if wall: + return _error( + ResponseCode.BAD_REQUEST, + "The page is behind a login wall or bot challenge", + wall, + hint="Open the URL in the shared browser (workspace_browser_open) and ask a human to complete the login there.", + ) + # In static mode we don't escalate to a browser, so an upstream + # 4xx/5xx must be reported as an error, not returned as success. + if body.mode == "static" and upstream_error: + return _error( + ResponseCode.BAD_REQUEST, + f"Upstream returned HTTP {static_result['status_code']}", + "UPSTREAM_HTTP_ERROR", + status=static_result["status_code"], + ) + return _success(extracted["text"], "static", static_result["final_url"], extracted["title"], max_chars) + elif body.mode == "static": + code, message = static_error + return _error(ResponseCode.BAD_REQUEST, message, code) + + # ---- Tier 2: ephemeral browser render ---- + bf_key = await _resolve_bf_key(workspace, db) + try: + rendered = await manager.render_page_text(body.url, api_key=bf_key) + except BrowserNavigationError as e: + code = "JS_RENDER_TIMEOUT" if e.code == "NAV_TIMEOUT" else e.code + return _error(ResponseCode.BAD_REQUEST, f"Browser render failed: {e}", code) + except Exception as e: + logger.error("Ephemeral render failed for %s: %s", body.url, e) + # If the static tier had usable content, degrade gracefully to it + if static_result is not None: + extracted = _extract_text(static_result["html"]) + if extracted["text"]: + return _success(extracted["text"], "static", static_result["final_url"], extracted["title"], max_chars) + if static_error is not None: + code, message = static_error + return _error(ResponseCode.BAD_REQUEST, message, code) + return _error(ResponseCode.INTERNAL_ERROR, "Browser render failed", "NAVIGATION_FAILED") + + # ---- Tier 3: wall detection on the rendered page ---- + wall = _detect_wall(rendered["text"], rendered["title"]) + if wall: + return _error( + ResponseCode.BAD_REQUEST, + "The page is behind a login wall or bot challenge", + wall, + hint="Open the URL in the shared browser (workspace_browser_open) and ask a human to complete the login there.", + ) + + return _success(rendered["text"], "browser", rendered["url"], rendered["title"], max_chars) diff --git a/workspace/backend/app/routers/files.py b/workspace/backend/app/routers/files.py index a13d26e16..cf6a696bd 100644 --- a/workspace/backend/app/routers/files.py +++ b/workspace/backend/app/routers/files.py @@ -21,7 +21,9 @@ import uuid from datetime import datetime, timezone from typing import Optional -from urllib.parse import quote +from urllib.parse import quote, urlparse + +import httpx from fastapi import APIRouter, Depends, File, Form, Header, Query, UploadFile from fastapi.responses import Response @@ -32,6 +34,7 @@ from app.config import config from app.database import get_db from app.file_types import FILTER_GROUPS, KIND_GROUPS, kind_for +from app.net_security import UnsafeURLError, safe_fetch from app.models import FileRecord, Workspace from app.response import ResponseCode, json_response, success_response from app.routers.network import ( @@ -46,6 +49,17 @@ router = APIRouter(prefix="/v1", tags=["Files"]) +# Content types safe to render inline in the workspace origin. Raster images +# only — NOT image/svg+xml (scriptable) and NOT text/html. +INLINE_SAFE_CONTENT_TYPES = { + "image/png", + "image/jpeg", + "image/gif", + "image/webp", + "image/bmp", + "image/x-icon", +} + def _organize_filename(filename: str, content_type: str) -> str: """Put uploaded files into uploaded_files/ with a timestamped name.""" @@ -83,6 +97,24 @@ class Base64UploadRequest(BaseModel): channel_name: Optional[str] = None network: str source: Optional[str] = "human:user" + post_to_channel: bool = False # also post a chat message with the file attached + caption: Optional[str] = None # message text when post_to_channel is set + + +class FromUrlUploadRequest(BaseModel): + """Download a file from a URL into workspace storage (for agents). + + Lets agents persist an image found via /v1/search/images (or any direct + file URL) and, with post_to_channel, share it in the chat as an inline + attachment in one call. + """ + url: str + network: str + filename: Optional[str] = None + channel_name: Optional[str] = None + source: Optional[str] = "human:user" + post_to_channel: bool = False + caption: Optional[str] = None class FolderCreateRequest(BaseModel): @@ -405,6 +437,40 @@ async def upload_file( }) +async def _post_attachment_message( + db: Session, + workspace: Workspace, + record: FileRecord, + caption: Optional[str], + token: Optional[str], +) -> bool: + """Post a chat message carrying the file as an inline attachment. + + Same payload shape as the cloud image agents (services/cloud_agent.py), + which the frontend already renders inline. Returns False when the file + has no channel to post into. + """ + if not record.channel_name: + return False + event = Event( + type="workspace.message.posted", + source=record.uploaded_by, + target=f"channel/{record.channel_name}", + payload={ + "content": caption or record.filename.rsplit("/", 1)[-1], + "message_type": "chat", + "attachments": [{ + "file_id": record.id, + "filename": record.filename, + "content_type": record.content_type, + "size": record.size, + }], + }, + ) + await _emit_event(event, workspace, db, token=token or workspace.password_hash) + return True + + # --------------------------------------------------------------------------- # POST /v1/files/base64 — JSON base64 upload (for agents) # --------------------------------------------------------------------------- @@ -473,6 +539,10 @@ async def upload_file_base64( ) await _emit_event(event, workspace, db, token=x_workspace_token or workspace.password_hash) + posted = False + if body.post_to_channel: + posted = await _post_attachment_message(db, workspace, record, body.caption, x_workspace_token) + return success_response({ "id": file_id, "filename": organized_filename, @@ -480,6 +550,135 @@ async def upload_file_base64( "size": len(data), "uploaded_by": body.source or "human:user", "created_at": record.created_at.isoformat() if record.created_at else None, + "posted_to_channel": posted, + }) + + +# --------------------------------------------------------------------------- +# POST /v1/files/from_url — download a URL into workspace storage (for agents) +# --------------------------------------------------------------------------- + +DOWNLOAD_TIMEOUT_SECONDS = 30.0 +_DOWNLOAD_UA = ( + "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) " + "Chrome/126.0.0.0 Safari/537.36 OpenAgentsFetch/1.0" +) + + +@router.post("/files/from_url") +async def upload_file_from_url( + body: FromUrlUploadRequest, + x_workspace_token: Optional[str] = Header(None), + authorization: Optional[str] = Header(None), + db: Session = Depends(get_db), +): + """Download a file (typically an image search result) into workspace + storage; with post_to_channel it is also shared in the chat as an + inline attachment.""" + workspace = _resolve_workspace(db, body.network) + if not workspace: + return json_response(ResponseCode.NOT_FOUND, "Network not found") + if not _verify_workspace_access(workspace, x_workspace_token, authorization): + return json_response(ResponseCode.UNAUTHORIZED, "Invalid workspace credentials") + + parsed = urlparse(body.url) + + # SSRF-safe streamed download: validates the URL (and every redirect hop) + # against internal/metadata addresses and caps the body size while reading. + try: + result = await safe_fetch( + body.url, + max_bytes=config.MAX_FILE_SIZE, + timeout=DOWNLOAD_TIMEOUT_SECONDS, + headers={"User-Agent": _DOWNLOAD_UA}, + truncate=False, # a file download must be complete, not truncated + ) + except UnsafeURLError as e: + code = e.code + if code == "RESPONSE_TOO_LARGE": + return json_response( + ResponseCode.BAD_REQUEST, + f"File too large. Maximum size: {config.MAX_FILE_SIZE // (1024*1024)}MB", + data={"error_code": code}, + ) + return json_response(ResponseCode.BAD_REQUEST, str(e), data={"error_code": code}) + except httpx.HTTPError as e: + return json_response( + ResponseCode.BAD_REQUEST, + f"Download failed: {e}", + data={"error_code": "DOWNLOAD_FAILED"}, + ) + + if result.status_code >= 400: + return json_response( + ResponseCode.BAD_REQUEST, + f"Download failed with HTTP {result.status_code}", + data={"error_code": "DOWNLOAD_FAILED", "status": result.status_code}, + ) + + data = result.content + content_type = result.content_type or "application/octet-stream" + if content_type.startswith("text/html"): + # An HTML response means the URL is a web page, not a file — saving + # it as an "image" would just produce a broken attachment. + return json_response( + ResponseCode.BAD_REQUEST, + "URL returned a web page, not a downloadable file", + data={"error_code": "NOT_A_FILE", "hint": "Use the page's direct image/file URL."}, + ) + + filename = body.filename or (parsed.path.rsplit("/", 1)[-1] or "download") + organized_filename = _organize_filename(filename, content_type) + + file_id = str(uuid.uuid4()) + store = get_file_store() + storage_name = organized_filename.rsplit("/", 1)[-1] if "/" in organized_filename else organized_filename + loop = asyncio.get_event_loop() + try: + storage_key = await loop.run_in_executor( + None, store.save, str(workspace.id), file_id, storage_name, data, + ) + except ValueError as exc: + return json_response(ResponseCode.BAD_REQUEST, str(exc)) + + record = FileRecord( + id=file_id, + workspace_id=str(workspace.id), + filename=organized_filename, + content_type=content_type, + size=len(data), + storage_key=storage_key, + uploaded_by=body.source or "human:user", + channel_name=body.channel_name, + ) + db.add(record) + + event = Event( + type="workspace.file.uploaded", + source=body.source or "human:user", + target=f"channel/{body.channel_name}" if body.channel_name else "core", + payload={ + "file_id": file_id, + "filename": organized_filename, + "content_type": content_type, + "size": len(data), + "source_url": body.url, + }, + ) + await _emit_event(event, workspace, db, token=x_workspace_token or workspace.password_hash) + + posted = False + if body.post_to_channel: + posted = await _post_attachment_message(db, workspace, record, body.caption, x_workspace_token) + + return success_response({ + "id": file_id, + "filename": organized_filename, + "content_type": content_type, + "size": len(data), + "uploaded_by": body.source or "human:user", + "source_url": body.url, + "posted_to_channel": posted, }) @@ -1408,9 +1607,13 @@ async def download_file( except FileNotFoundError: return json_response(ResponseCode.NOT_FOUND, "File data not found in storage") - # Use inline disposition for images and HTML so browsers can render them - ct = record.content_type or "" - disposition = "inline" if ct.startswith("image/") or ct == "text/html" else "attachment" + # Only render a narrow allowlist of raster image types inline. SVG (which + # can carry + +""".format("Lots of readable static content. " * 30) + +JS_SHELL_HTML = ( + "Notion" + "" + "
" + "" + "" +) + + +class TestFetchChain: + + @pytest.fixture(autouse=True) + def _skip_ssrf_dns(self): + # These tests use example hostnames and mock the fetch tiers; the SSRF + # guard's real DNS lookup is exercised separately in TestFetchSSRF. + with patch("app.routers.fetch.validate_public_url", new=AsyncMock()): + yield + + @patch("app.routers.fetch._static_fetch") + def test_static_page_served_without_browser(self, mock_static, client): + mock_static.return_value = {"html": STATIC_HTML, "final_url": "https://example.com/a", "status_code": 200} + workspace = _create_workspace(client) + + with patch("app.routers.fetch.BrowserManager") as mock_bm: + manager = MagicMock() + manager.render_page_text = AsyncMock() + mock_bm.get.return_value = manager + resp = _fetch(client, workspace, "https://example.com/a") + + assert resp.status_code == 200 + data = resp.json()["data"] + assert data["content_source"] == "static" + assert "A real article" in data["content"] + assert "console.log" not in data["content"] + assert data["title"] == "Article" + manager.render_page_text.assert_not_awaited() + + @patch("app.routers.fetch._resolve_bf_key", new_callable=AsyncMock, return_value=None) + @patch("app.routers.fetch._static_fetch") + def test_js_shell_escalates_to_browser_render(self, mock_static, _key, client): + mock_static.return_value = {"html": JS_SHELL_HTML, "final_url": "https://notion.site/x", "status_code": 200} + workspace = _create_workspace(client) + + with patch("app.routers.fetch.BrowserManager") as mock_bm: + manager = MagicMock() + manager.render_page_text = AsyncMock(return_value={ + "url": "https://notion.site/x", + "title": "Lunchbox", + "text": "Rendered Notion page content here. " * 100, + }) + mock_bm.get.return_value = manager + resp = _fetch(client, workspace, "https://notion.site/x") + + assert resp.status_code == 200 + data = resp.json()["data"] + assert data["content_source"] == "browser" + assert "Rendered Notion page" in data["content"] + manager.render_page_text.assert_awaited_once() + + @patch("app.routers.fetch._resolve_bf_key", new_callable=AsyncMock, return_value=None) + @patch("app.routers.fetch._static_fetch") + def test_login_wall_returns_auth_required(self, mock_static, _key, client): + mock_static.return_value = {"html": JS_SHELL_HTML, "final_url": "https://notion.site/p", "status_code": 200} + workspace = _create_workspace(client) + + with patch("app.routers.fetch.BrowserManager") as mock_bm: + manager = MagicMock() + manager.render_page_text = AsyncMock(return_value={ + "url": "https://notion.site/p", + "title": "Notion", + "text": "Please sign in to continue.", + }) + mock_bm.get.return_value = manager + resp = _fetch(client, workspace, "https://notion.site/p") + + assert resp.status_code == 400 + body = resp.json() + assert body["data"]["error_code"] == "AUTH_REQUIRED" + assert "hint" in body["data"] + + @patch("app.routers.fetch._static_fetch") + def test_static_mode_never_renders(self, mock_static, client): + mock_static.return_value = {"html": JS_SHELL_HTML, "final_url": "https://spa.example.com", "status_code": 200} + workspace = _create_workspace(client) + + with patch("app.routers.fetch.BrowserManager") as mock_bm: + manager = MagicMock() + manager.render_page_text = AsyncMock() + mock_bm.get.return_value = manager + resp = _fetch(client, workspace, "https://spa.example.com", mode="static") + + assert resp.status_code == 200 + manager.render_page_text.assert_not_awaited() + + @patch("app.routers.fetch._resolve_bf_key", new_callable=AsyncMock, return_value=None) + @patch("app.routers.fetch._static_fetch") + def test_render_timeout_surfaces_error_code(self, mock_static, _key, client): + mock_static.return_value = {"html": JS_SHELL_HTML, "final_url": "https://slow.example.com", "status_code": 200} + workspace = _create_workspace(client) + + with patch("app.routers.fetch.BrowserManager") as mock_bm: + manager = MagicMock() + manager.render_page_text = AsyncMock( + side_effect=BrowserNavigationError("NAV_TIMEOUT", "Timeout 30000ms exceeded") + ) + mock_bm.get.return_value = manager + resp = _fetch(client, workspace, "https://slow.example.com") + + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] == "JS_RENDER_TIMEOUT" + + def test_truncates_to_max_chars(self, client): + workspace = _create_workspace(client) + with patch("app.routers.fetch._static_fetch") as mock_static: + mock_static.return_value = {"html": STATIC_HTML, "final_url": "https://example.com", "status_code": 200} + resp = _fetch(client, workspace, "https://example.com", max_chars=1000) + data = resp.json()["data"] + assert len(data["content"]) <= 1000 + assert data["truncated"] is True + + @patch("app.routers.fetch._static_fetch") + def test_static_mode_reports_upstream_http_error(self, mock_static, client): + # A 404 in static mode must surface as an error, not a success body. + mock_static.return_value = {"html": "Not found", + "final_url": "https://example.com/missing", "status_code": 404} + workspace = _create_workspace(client) + resp = _fetch(client, workspace, "https://example.com/missing", mode="static") + assert resp.status_code == 400 + body = resp.json() + assert body["data"]["error_code"] == "UPSTREAM_HTTP_ERROR" + assert body["data"]["status"] == 404 + + +class TestFetchSSRF: + """The SSRF guard runs for real here (IP literals need no DNS).""" + + def test_rejects_non_http_scheme(self, client): + workspace = _create_workspace(client) + resp = _fetch(client, workspace, "file:///etc/passwd") + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] == "UNSUPPORTED_SCHEME" + + def test_blocks_loopback(self, client): + workspace = _create_workspace(client) + resp = _fetch(client, workspace, "http://127.0.0.1/admin") + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] == "BLOCKED_PRIVATE_ADDRESS" + + def test_blocks_cloud_metadata(self, client): + workspace = _create_workspace(client) + resp = _fetch(client, workspace, "http://169.254.169.254/latest/meta-data/") + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] == "BLOCKED_PRIVATE_ADDRESS" + + def test_blocks_url_credentials(self, client): + workspace = _create_workspace(client) + resp = _fetch(client, workspace, "http://user:pw@10.0.0.1/") + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] in ("URL_CREDENTIALS_NOT_ALLOWED", "BLOCKED_PRIVATE_ADDRESS") + + +class TestHeuristics: + + def test_extract_text_strips_scripts_and_keeps_title(self): + extracted = _extract_text(STATIC_HTML) + assert extracted["title"] == "Article" + assert "console.log" not in extracted["text"] + assert "A real article" in extracted["text"] + + def test_js_shell_detected_by_noscript_marker(self): + extracted = _extract_text(JS_SHELL_HTML) + assert _looks_like_js_shell(JS_SHELL_HTML, extracted) is True + + def test_rich_static_page_is_not_js_shell(self): + extracted = _extract_text(STATIC_HTML) + assert _looks_like_js_shell(STATIC_HTML, extracted) is False + + def test_wall_detection(self): + assert _detect_wall("Please sign in to continue.", "App") == "AUTH_REQUIRED" + assert _detect_wall("Checking your browser before accessing", "Just a moment") == "BOT_CHALLENGE" + assert _detect_wall("A long normal article. " * 200, "News") is None + # "sign in" link on a long page must not trigger the wall + assert _detect_wall("please sign in " + "content " * 400, "News") is None diff --git a/workspace/backend/tests/test_net_security.py b/workspace/backend/tests/test_net_security.py new file mode 100644 index 000000000..9ba24ccc3 --- /dev/null +++ b/workspace/backend/tests/test_net_security.py @@ -0,0 +1,109 @@ +# -*- coding: utf-8 -*- +"""Tests for the SSRF-safe fetch helpers (app/net_security.py).""" + +import asyncio +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from app import net_security +from app.net_security import UnsafeURLError, safe_fetch, validate_public_url + + +def _run(coro): + return asyncio.run(coro) + + +class TestValidatePublicUrl: + def test_rejects_non_http_scheme(self): + with pytest.raises(UnsafeURLError) as ei: + _run(validate_public_url("file:///etc/passwd")) + assert ei.value.code == "UNSUPPORTED_SCHEME" + + def test_rejects_embedded_credentials(self): + with pytest.raises(UnsafeURLError) as ei: + _run(validate_public_url("https://user:pass@example.com/")) + assert ei.value.code == "URL_CREDENTIALS_NOT_ALLOWED" + + def test_rejects_loopback(self): + with pytest.raises(UnsafeURLError) as ei: + _run(validate_public_url("http://127.0.0.1/admin")) + assert ei.value.code == "BLOCKED_PRIVATE_ADDRESS" + + def test_rejects_cloud_metadata(self): + with pytest.raises(UnsafeURLError) as ei: + _run(validate_public_url("http://169.254.169.254/latest/meta-data/")) + assert ei.value.code == "BLOCKED_PRIVATE_ADDRESS" + + def test_rejects_ipv4_mapped_ipv6_loopback(self): + with pytest.raises(UnsafeURLError) as ei: + _run(validate_public_url("http://[::ffff:127.0.0.1]/")) + assert ei.value.code == "BLOCKED_PRIVATE_ADDRESS" + + def test_rejects_hostname_resolving_to_private(self): + with patch.object(net_security, "_resolve_ips", new=AsyncMock(return_value={"127.0.0.1"})): + with pytest.raises(UnsafeURLError) as ei: + _run(validate_public_url("http://sneaky.internal.example/")) + assert ei.value.code == "BLOCKED_PRIVATE_ADDRESS" + + def test_allows_public_hostname(self): + with patch.object(net_security, "_resolve_ips", new=AsyncMock(return_value={"93.184.216.34"})): + _run(validate_public_url("https://example.com/page")) # no raise + + +def _stream_response(*, status=200, headers=None, chunks=(b"hello",), is_redirect=False): + resp = MagicMock() + resp.status_code = status + resp.headers = headers or {"content-type": "text/html"} + resp.is_redirect = is_redirect + + async def _aiter(): + for c in chunks: + yield c + resp.aiter_bytes = _aiter + + cm = MagicMock() + cm.__aenter__ = AsyncMock(return_value=resp) + cm.__aexit__ = AsyncMock(return_value=False) + return cm + + +class TestSafeFetch: + def _client_with(self, responses): + client = MagicMock() + client.__aenter__ = AsyncMock(return_value=client) + client.__aexit__ = AsyncMock(return_value=False) + client.stream = MagicMock(side_effect=responses) + return client + + def test_truncates_at_limit(self): + client = self._client_with([_stream_response(chunks=[b"a" * 100, b"b" * 100])]) + with patch.object(net_security, "validate_public_url", new=AsyncMock()), \ + patch.object(net_security.httpx, "AsyncClient", return_value=client): + result = _run(safe_fetch("https://example.com", max_bytes=150, timeout=5, truncate=True)) + assert result.truncated is True and len(result.content) == 150 + + def test_raises_over_limit_when_not_truncating(self): + client = self._client_with([_stream_response(chunks=[b"a" * 200])]) + with patch.object(net_security, "validate_public_url", new=AsyncMock()), \ + patch.object(net_security.httpx, "AsyncClient", return_value=client): + with pytest.raises(UnsafeURLError) as ei: + _run(safe_fetch("https://example.com", max_bytes=150, timeout=5, truncate=False)) + assert ei.value.code == "RESPONSE_TOO_LARGE" + + def test_revalidates_each_redirect_hop(self): + redirect = _stream_response(status=302, headers={"location": "http://169.254.169.254/"}, is_redirect=True) + client = self._client_with([redirect]) + calls = [] + + async def fake_validate(url): + calls.append(url) + if "169.254" in url: + raise UnsafeURLError("BLOCKED_PRIVATE_ADDRESS", "blocked") + + with patch.object(net_security, "validate_public_url", new=fake_validate), \ + patch.object(net_security.httpx, "AsyncClient", return_value=client): + with pytest.raises(UnsafeURLError) as ei: + _run(safe_fetch("https://example.com/redir", max_bytes=1000, timeout=5, truncate=True)) + assert ei.value.code == "BLOCKED_PRIVATE_ADDRESS" + assert any("169.254" in c for c in calls) diff --git a/workspace/backend/tests/test_search_images.py b/workspace/backend/tests/test_search_images.py new file mode 100644 index 000000000..6a836933c --- /dev/null +++ b/workspace/backend/tests/test_search_images.py @@ -0,0 +1,272 @@ +# -*- coding: utf-8 -*- +""" +Tests for image search (POST /v1/search/images) and URL file ingestion +(POST /v1/files/from_url) including post_to_channel attachment messages. + +Brave Search and the file download are mocked. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +from sqlalchemy import select + +from app.models import EventRecord, FileRecord +from tests.conftest import TestingSessionLocal + +PNG_BYTES = b"\x89PNG\r\n\x1a\n" + b"0" * 100 + + +def _create_workspace(client): + resp = client.post("/v1/workspaces", json={ + "name": "Image Test Workspace", + "agent_name": "agent-image", + "creator_email": "test@example.com", + }) + assert resp.status_code == 200 + data = resp.json()["data"] + channel = data["channel"] + channel_name = channel["name"] if isinstance(channel, dict) else channel + return {"id": data["workspaceId"], "token": data["token"], "channel": channel_name} + + +BRAVE_RESULTS = [ + { + "title": "Golden Gate at sunset", + "url": "https://photos.example.com/page1", + "source": "photos.example.com", + "thumbnail": {"src": "https://thumbs.example.com/1.jpg"}, + "properties": {"url": "https://images.example.com/1.jpg", "width": 1920, "height": 1080}, + }, + { + "title": "No image url — skipped", + "url": "https://photos.example.com/page2", + "thumbnail": {}, + "properties": {}, + }, +] + + +class TestImageSearch: + + @patch("app.routers.search._brave_image_search", new_callable=AsyncMock) + @patch.dict("os.environ", {"BRAVE_SEARCH_API_KEY": "test-key"}) + def test_search_returns_mapped_results(self, mock_search, client): + mock_search.return_value = [ + { + "title": "Golden Gate at sunset", + "image_url": "https://images.example.com/1.jpg", + "thumbnail_url": "https://thumbs.example.com/1.jpg", + "page_url": "https://photos.example.com/page1", + "source": "photos.example.com", + "width": 1920, + "height": 1080, + }, + ] + workspace = _create_workspace(client) + resp = client.post("/v1/search/images", json={ + "query": "golden gate", + "network": workspace["id"], + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 200 + data = resp.json()["data"] + assert data["total"] == 1 + assert data["results"][0]["image_url"] == "https://images.example.com/1.jpg" + + def test_search_without_key_returns_config_error(self, client): + workspace = _create_workspace(client) + with patch.dict("os.environ", {}, clear=False): + import os + os.environ.pop("BRAVE_SEARCH_API_KEY", None) + resp = client.post("/v1/search/images", json={ + "query": "anything", + "network": workspace["id"], + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 400 + body = resp.json() + assert body["data"]["error_code"] == "SEARCH_NOT_CONFIGURED" + assert "hint" in body["data"] + + def test_brave_result_mapping_skips_items_without_image(self): + import asyncio + from app.routers import search as search_mod + + response = MagicMock() + response.json.return_value = {"results": BRAVE_RESULTS} + response.raise_for_status.return_value = None + + client_cm = MagicMock() + client_cm.__aenter__ = AsyncMock(return_value=client_cm) + client_cm.__aexit__ = AsyncMock(return_value=False) + client_cm.get = AsyncMock(return_value=response) + + with patch.object(search_mod.httpx, "AsyncClient", return_value=client_cm): + results = asyncio.run(search_mod._brave_image_search("k", "q", 10, "strict")) + + assert len(results) == 1 + assert results[0]["image_url"] == "https://images.example.com/1.jpg" + assert results[0]["width"] == 1920 + + +def _fake_result(content=PNG_BYTES, content_type="image/png", status=200, final_url="https://images.example.com/pic.png"): + from app.net_security import SafeFetchResult + return SafeFetchResult( + content=content, + status_code=status, + headers={"content-type": content_type}, + final_url=final_url, + truncated=False, + ) + + +def _patch_safe_fetch(result): + from app.routers import files as files_mod + return patch.object(files_mod, "safe_fetch", new=AsyncMock(return_value=result)) + + +class TestFromUrl: + + def test_from_url_saves_file(self, client): + workspace = _create_workspace(client) + with _patch_safe_fetch(_fake_result()): + resp = client.post("/v1/files/from_url", json={ + "url": "https://images.example.com/pic.png", + "network": workspace["id"], + "source": "openagents:agent-image", + "channel_name": workspace["channel"], + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 200, resp.json() + data = resp.json()["data"] + assert data["content_type"] == "image/png" + assert data["posted_to_channel"] is False + + db = TestingSessionLocal() + record = db.get(FileRecord, data["id"]) + assert record is not None and record.size == len(PNG_BYTES) + db.close() + + def test_from_url_post_to_channel_emits_attachment_message(self, client): + workspace = _create_workspace(client) + with _patch_safe_fetch(_fake_result()): + resp = client.post("/v1/files/from_url", json={ + "url": "https://images.example.com/pic.png", + "network": workspace["id"], + "source": "openagents:agent-image", + "channel_name": workspace["channel"], + "post_to_channel": True, + "caption": "剧照来了", + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 200, resp.json() + data = resp.json()["data"] + assert data["posted_to_channel"] is True + + db = TestingSessionLocal() + events = db.execute( + select(EventRecord).where(EventRecord.type == "workspace.message.posted") + ).scalars().all() + message_events = [e for e in events if (e.payload or {}).get("attachments")] + assert len(message_events) == 1 + attachment = message_events[0].payload["attachments"][0] + assert attachment["file_id"] == data["id"] + assert attachment["content_type"] == "image/png" + assert message_events[0].payload["content"] == "剧照来了" + db.close() + + def test_from_url_rejects_html_pages(self, client): + workspace = _create_workspace(client) + result = _fake_result(content=b"a page", content_type="text/html; charset=utf-8") + with _patch_safe_fetch(result): + resp = client.post("/v1/files/from_url", json={ + "url": "https://example.com/page", + "network": workspace["id"], + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] == "NOT_A_FILE" + + def test_from_url_blocks_ssrf_without_mock(self, client): + # safe_fetch runs for real; an internal IP literal is blocked before + # any socket is opened (no DNS, no network). + workspace = _create_workspace(client) + resp = client.post("/v1/files/from_url", json={ + "url": "http://169.254.169.254/latest/meta-data/", + "network": workspace["id"], + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] == "BLOCKED_PRIVATE_ADDRESS" + + def test_from_url_rejects_non_http(self, client): + workspace = _create_workspace(client) + resp = client.post("/v1/files/from_url", json={ + "url": "ftp://example.com/file.png", + "network": workspace["id"], + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] == "UNSUPPORTED_SCHEME" + + +class TestBase64PostToChannel: + + def test_base64_upload_with_post_to_channel(self, client): + import base64 + workspace = _create_workspace(client) + resp = client.post("/v1/files/base64", json={ + "filename": "chart.png", + "content_base64": base64.b64encode(PNG_BYTES).decode(), + "content_type": "image/png", + "network": workspace["id"], + "channel_name": workspace["channel"], + "source": "openagents:agent-image", + "post_to_channel": True, + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 200, resp.json() + assert resp.json()["data"]["posted_to_channel"] is True + + +# --------------------------------------------------------------------------- +# Download hardening: no inline SVG/HTML (stored XSS), nosniff always set +# --------------------------------------------------------------------------- + +class TestDownloadDisposition: + + def _upload(self, client, workspace, filename, content_type, data=PNG_BYTES): + import base64 + resp = client.post("/v1/files/base64", json={ + "filename": filename, + "content_base64": base64.b64encode(data).decode(), + "content_type": content_type, + "network": workspace["id"], + "source": "openagents:agent-image", + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 200, resp.json() + return resp.json()["data"]["id"] + + def _download(self, client, workspace, file_id): + return client.get(f"/v1/files/{file_id}", + headers={"X-Workspace-Token": workspace["token"]}) + + def test_png_served_inline_with_nosniff(self, client): + workspace = _create_workspace(client) + fid = self._upload(client, workspace, "pic.png", "image/png") + resp = self._download(client, workspace, fid) + assert resp.status_code == 200 + assert resp.headers["content-disposition"].startswith("inline") + assert resp.headers["x-content-type-options"] == "nosniff" + + def test_svg_forced_to_attachment(self, client): + workspace = _create_workspace(client) + svg = b'' + fid = self._upload(client, workspace, "x.svg", "image/svg+xml", data=svg) + resp = self._download(client, workspace, fid) + assert resp.status_code == 200 + # SVG must NOT render inline (scriptable) — served as a download. + assert resp.headers["content-disposition"].startswith("attachment") + assert resp.headers["x-content-type-options"] == "nosniff" + + def test_html_forced_to_attachment(self, client): + workspace = _create_workspace(client) + html = b"" + fid = self._upload(client, workspace, "x.html", "text/html", data=html) + resp = self._download(client, workspace, fid) + assert resp.status_code == 200 + assert resp.headers["content-disposition"].startswith("attachment") + + From 160e859712986ebbfd7e8a341d09e1cb2f004392 Mon Sep 17 00:00:00 2001 From: QuanCheng <915158214@qq.com> Date: Wed, 29 Jul 2026 08:17:55 +0000 Subject: [PATCH 02/10] harden SSRF boundary for agent-driven fetch and browser navigation The render tier of POST /v1/fetch validated only the entry URL and then handed it to a headless browser that did its own DNS and followed redirects with no re-validation, so an attacker-controlled page could redirect into cloud metadata or the backend's own network. The shared browser tools had the same hole and no URL check at all. Put the boundary below the browser rather than inside the page. Chromium now launches behind a policy proxy (browser_egress) that sees every request it makes of every kind, resolves each name once, and refuses anything that is not a public address. Verified against a real Chromium that this single chokepoint catches 302 redirects, meta refresh, JS navigation, image subresources, iframes, XHR and WebSocket. Chromium bypasses a proxy for loopback unless launched with loopback removed from its bypass list, so that flag is part of the launch args and has its own regression test. Add a shared guard on open_tab, navigate and render_page_text so the shared browser tools get the same entry check, allowing only the blank page as a non-http target. The static tier now resolves once and connects to the pinned address while keeping the original Host header and TLS server name, closing the rebinding window between validation and connect. Each redirect hop gets its own connection pool so two hostnames sharing an address cannot reuse one another's connection and carry the wrong TLS name. Also restrict outbound ports, stop echoing the resolved internal address back to the caller, rate limit fetch per workspace, gate the Chromium sandbox behind an env flag, and correct the stale image search docstring. Browser Fabric mode is not covered by the proxy because the page runs on their infrastructure, and the Chromium sandbox default is unchanged because the image still runs as root. Both are noted in the code. --- workspace/backend/app/browser_egress.py | 319 ++++++++++ workspace/backend/app/net_security.py | 168 +++++- workspace/backend/app/routers/browser.py | 5 + workspace/backend/app/routers/fetch.py | 51 +- workspace/backend/app/routers/search.py | 15 +- workspace/backend/tests/test_ssrf_boundary.py | 551 ++++++++++++++++++ 6 files changed, 1069 insertions(+), 40 deletions(-) create mode 100644 workspace/backend/app/browser_egress.py create mode 100644 workspace/backend/tests/test_ssrf_boundary.py diff --git a/workspace/backend/app/browser_egress.py b/workspace/backend/app/browser_egress.py new file mode 100644 index 000000000..f2d317f14 --- /dev/null +++ b/workspace/backend/app/browser_egress.py @@ -0,0 +1,319 @@ +# -*- coding: utf-8 -*- +""" +Egress policy proxy for the local (Playwright) browser. + +Validating the URL a browser is *told* to open constrains nothing about where +it goes next: a 302, a meta-refresh, a JS navigation, an XHR, an iframe, an +image, or a WebSocket can all reach an internal address without any top-level +navigation happening. Intercepting inside the page (Playwright routing) means +enumerating every one of those mechanisms, still leaves service workers and +WebSockets needing their own handling, and cannot close the gap between the +interceptor's DNS lookup and Chromium's own. + +So the boundary is put below the browser instead. Chromium is launched with +`--proxy-server` pointing here, which turns every request it makes — of every +kind — into one chokepoint, and stops it from resolving DNS at all: the name +arrives here in the request line or CONNECT target, this proxy resolves it +exactly once via `resolve_and_validate`, and connects to that pinned address. + +Deny is the default: anything this proxy cannot parse, resolve, or prove +public gets a 403 and no connection is made. + +One gotcha this depends on: Chromium bypasses the proxy for loopback targets +unless it is launched with `--proxy-bypass-list=<-loopback>`. Without that +flag `http://127.0.0.1:...` goes direct and never reaches this code, which +would leave the backend's own API reachable. `chromium_proxy_args()` below is +the only supported way to build the flags, so the two can't drift apart. +""" + +import asyncio +import logging +from typing import Optional + +from app.net_security import UnsafeURLError, resolve_and_validate + +logger = logging.getLogger(__name__) + +_MAX_HEADER_BYTES = 64 * 1024 +_HEADER_TIMEOUT_SECONDS = 15.0 +_CONNECT_TIMEOUT_SECONDS = 15.0 + +_DENY_TEXT = b"Blocked by workspace egress policy: destination is not public" +_DENY_BODY = ( + b"HTTP/1.1 403 Forbidden\r\n" + b"Content-Type: text/plain; charset=utf-8\r\n" + b"Content-Length: " + str(len(_DENY_TEXT)).encode() + b"\r\n" + b"Connection: close\r\n" + b"\r\n" + _DENY_TEXT +) + + +def _bad_gateway(reason: str) -> bytes: + body = f"Egress proxy could not reach the destination: {reason}".encode("utf-8") + return ( + b"HTTP/1.1 502 Bad Gateway\r\n" + b"Content-Type: text/plain; charset=utf-8\r\n" + b"Content-Length: " + str(len(body)).encode() + b"\r\n" + b"Connection: close\r\n" + b"\r\n" + body + ) + + +def _split_host_port(authority: str, default_port: int) -> tuple: + """Split 'host:port' / '[v6]:port' / 'host' into (host, port).""" + authority = authority.strip() + if authority.startswith("["): + close = authority.find("]") + if close == -1: + raise ValueError("malformed IPv6 authority") + host = authority[1:close] + rest = authority[close + 1:] + port = int(rest[1:]) if rest.startswith(":") and rest[1:].isdigit() else default_port + return host, port + if ":" in authority: + host, _, port_str = authority.rpartition(":") + if port_str.isdigit(): + return host, int(port_str) + raise ValueError("malformed port") + return authority, default_port + + +def _target_from_absolute_url(target: str) -> tuple: + """Parse the absolute-form request target proxies receive for plain HTTP + ('GET http://host/path HTTP/1.1') into (host, port, path).""" + if "://" not in target: + raise ValueError("expected absolute-form request target") + scheme, _, rest = target.partition("://") + if scheme.lower() not in ("http", "https"): + raise ValueError(f"unsupported scheme '{scheme}'") + authority, slash, path = rest.partition("/") + default_port = 443 if scheme.lower() == "https" else 80 + host, port = _split_host_port(authority, default_port) + return host, port, (("/" + path) if slash else "/") + + +# Hop-by-hop headers, plus the proxy-specific ones. These are meaningful only +# between the browser and this proxy and must not be relayed upstream. +_HOP_BY_HOP = { + b"connection", b"proxy-connection", b"proxy-authorization", b"proxy-authenticate", + b"keep-alive", b"te", b"trailer", b"transfer-encoding", b"upgrade", +} + + +def _clean_headers(header_block: bytes) -> bytes: + """Drop hop-by-hop headers and force the upstream connection closed. + + Connection reuse has to be suppressed: an HTTP/1.1 proxy connection can + carry requests for several origins, and once this proxy has spliced the + socket to one upstream, later requests on it would ride to that upstream + without going back through the policy check. One request per connection + keeps every request individually validated. + """ + kept = [] + for line in header_block.split(b"\r\n"): + if not line: + continue + name = line.split(b":", 1)[0].strip().lower() + if name in _HOP_BY_HOP: + continue + kept.append(line) + kept.append(b"Connection: close") + return b"\r\n".join(kept) + b"\r\n" + + +async def _pipe(src: asyncio.StreamReader, dst: asyncio.StreamWriter) -> None: + try: + while True: + data = await src.read(65536) + if not data: + break + dst.write(data) + await dst.drain() + except (ConnectionError, asyncio.CancelledError, OSError): + pass + finally: + try: + dst.close() + except Exception: + pass + + +class EgressPolicyProxy: + """A loopback HTTP/HTTPS proxy that only forwards to public addresses.""" + + def __init__(self): + self._server: Optional[asyncio.AbstractServer] = None + self._port: Optional[int] = None + self.blocked_count = 0 + self.allowed_count = 0 + + @property + def port(self) -> Optional[int]: + return self._port + + async def start(self) -> int: + if self._server is not None: + return self._port + # Bind to an ephemeral port on loopback only: each uvicorn worker runs + # its own proxy for its own browser, and nothing off-box can use it. + self._server = await asyncio.start_server(self._handle, "127.0.0.1", 0) + self._port = self._server.sockets[0].getsockname()[1] + logger.info("Browser egress policy proxy listening on 127.0.0.1:%d", self._port) + return self._port + + async def stop(self) -> None: + if self._server is None: + return + self._server.close() + try: + await self._server.wait_closed() + except Exception: + pass + self._server = None + self._port = None + + def chromium_args(self) -> list: + """Launch flags that route this browser's traffic through the proxy. + + `<-loopback>` removes loopback from Chromium's implicit bypass list. + Without it, 127.0.0.1 and localhost are fetched directly and never + reach the policy check. + """ + if self._port is None: + return [] + return [ + f"--proxy-server=http://127.0.0.1:{self._port}", + "--proxy-bypass-list=<-loopback>", + ] + + # ------------------------------------------------------------------ + + async def _deny(self, writer: asyncio.StreamWriter, host: str, port: int, reason: str) -> None: + self.blocked_count += 1 + logger.warning("Egress policy blocked browser request to %s:%s (%s)", host, port, reason) + try: + writer.write(_DENY_BODY) + await writer.drain() + except (ConnectionError, OSError): + pass + writer.close() + + async def _handle(self, reader: asyncio.StreamReader, writer: asyncio.StreamWriter) -> None: + try: + await self._handle_inner(reader, writer) + except Exception: + logger.exception("Egress proxy connection failed") + try: + writer.close() + except Exception: + pass + + async def _handle_inner(self, reader: asyncio.StreamReader, writer: asyncio.StreamWriter) -> None: + try: + request_line = await asyncio.wait_for( + reader.readline(), timeout=_HEADER_TIMEOUT_SECONDS + ) + except (asyncio.TimeoutError, ConnectionError): + writer.close() + return + if not request_line: + writer.close() + return + + parts = request_line.decode("latin-1").strip().split(" ") + if len(parts) < 3: + await self._deny(writer, "?", 0, "malformed request line") + return + method, target = parts[0].upper(), parts[1] + + header_block = bytearray() + while True: + try: + line = await asyncio.wait_for(reader.readline(), timeout=_HEADER_TIMEOUT_SECONDS) + except (asyncio.TimeoutError, ConnectionError): + writer.close() + return + if not line or line in (b"\r\n", b"\n"): + break + header_block += line + if len(header_block) > _MAX_HEADER_BYTES: + await self._deny(writer, "?", 0, "header block too large") + return + + if method == "CONNECT": + # HTTPS and WebSocket-over-TLS. Policy is enforced on the tunnel + # target and the bytes are then relayed untouched, so the browser's + # own certificate verification is unaffected — no MITM here. + try: + host, port = _split_host_port(target, 443) + except ValueError as e: + await self._deny(writer, target, 0, str(e)) + return + await self._tunnel(reader, writer, host, port) + return + + try: + host, port, path = _target_from_absolute_url(target) + except ValueError as e: + await self._deny(writer, target, 0, str(e)) + return + await self._forward(reader, writer, method, host, port, path, bytes(header_block)) + + async def _pin(self, writer: asyncio.StreamWriter, host: str, port: int) -> Optional[str]: + """Return the pinned public IP, or None after writing a denial.""" + try: + return await resolve_and_validate(host, port) + except UnsafeURLError as e: + await self._deny(writer, host, port, e.code) + return None + except Exception as e: # fail closed on anything unexpected + await self._deny(writer, host, port, f"policy error: {type(e).__name__}") + return None + + async def _tunnel(self, reader, writer, host: str, port: int) -> None: + pinned = await self._pin(writer, host, port) + if pinned is None: + return + try: + up_reader, up_writer = await asyncio.wait_for( + asyncio.open_connection(pinned, port), timeout=_CONNECT_TIMEOUT_SECONDS + ) + except (OSError, asyncio.TimeoutError) as e: + writer.write(_bad_gateway(type(e).__name__)) + await writer.drain() + writer.close() + return + + self.allowed_count += 1 + writer.write(b"HTTP/1.1 200 Connection Established\r\n\r\n") + await writer.drain() + await asyncio.gather( + _pipe(reader, up_writer), _pipe(up_reader, writer), return_exceptions=True + ) + + async def _forward(self, reader, writer, method: str, host: str, port: int, + path: str, header_block: bytes) -> None: + pinned = await self._pin(writer, host, port) + if pinned is None: + return + try: + up_reader, up_writer = await asyncio.wait_for( + asyncio.open_connection(pinned, port), timeout=_CONNECT_TIMEOUT_SECONDS + ) + except (OSError, asyncio.TimeoutError) as e: + writer.write(_bad_gateway(type(e).__name__)) + await writer.drain() + writer.close() + return + + self.allowed_count += 1 + # Re-emit in origin form. The client's Host header is preserved so + # virtual-hosted origins still resolve correctly; only the request + # target is rewritten and hop-by-hop headers are dropped. + up_writer.write(f"{method} {path} HTTP/1.1\r\n".encode("latin-1")) + up_writer.write(_clean_headers(header_block)) + up_writer.write(b"\r\n") + await up_writer.drain() + await asyncio.gather( + _pipe(reader, up_writer), _pipe(up_reader, writer), return_exceptions=True + ) diff --git a/workspace/backend/app/net_security.py b/workspace/backend/app/net_security.py index 5b7694c92..468891a00 100644 --- a/workspace/backend/app/net_security.py +++ b/workspace/backend/app/net_security.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- """ -SSRF-safe outbound HTTP for agent-triggered fetches. +SSRF-safe outbound networking for agent-triggered fetches. Anything that fetches a URL chosen by a workspace agent (POST /v1/fetch, POST /v1/files/from_url) MUST go through here. A raw httpx.get would let an @@ -8,23 +8,29 @@ localhost, and private/internal services. Protections: - - scheme restricted to http/https + - scheme restricted to http/https, destination port restricted to an allowlist - URLs with embedded credentials rejected - every hostname is resolved and ALL resolved IPs must be public (private / loopback / link-local / reserved / multicast are blocked, including IPv4-mapped IPv6) - - redirects are followed manually and re-validated at every hop + - the validated IP is PINNED and connected to directly, so the name is + resolved exactly once — a DNS rebind between validation and connect can't + swing the destination to an internal address + - redirects are followed manually and re-validated (and re-pinned) per hop + - each hop gets its own connection pool, so two hostnames sharing an IP can + never reuse one another's connection (which would carry the wrong TLS SNI) - trust_env=False so backend proxy env vars can't redirect the request - streamed with an enforced byte cap so a huge body can't OOM the worker -Known residual: a DNS-rebinding window exists between validation and the -socket connect. The high-severity vectors (direct internal URLs, metadata -IPs, redirect-to-internal) are all closed. +The browser render path cannot use this module (Chromium does its own +networking); it is constrained by the egress proxy in app.browser_egress, +which enforces the same policy via `resolve_and_validate`. """ import asyncio import ipaddress import logging +import os from typing import Optional from urllib.parse import urljoin, urlparse @@ -36,6 +42,22 @@ DEFAULT_MAX_REDIRECTS = 4 +def _parse_port_allowlist() -> frozenset: + """Destination ports agents may reach. Restricting this stops the fetch + tools from being used to probe non-HTTP services (SSH, SMTP, Redis, ...) + on hosts that are technically public.""" + raw = os.environ.get("OUTBOUND_ALLOWED_PORTS", "80,443,8080,8443") + ports = set() + for part in raw.split(","): + part = part.strip() + if part.isdigit(): + ports.add(int(part)) + return frozenset(ports or {80, 443}) + + +ALLOWED_PORTS = _parse_port_allowlist() + + class UnsafeURLError(Exception): """A URL was rejected before or during fetching. Carries a stable code.""" @@ -57,18 +79,71 @@ def _ip_is_public(ip: ipaddress._BaseAddress) -> bool: ) -async def _resolve_ips(host: str, port: int) -> set: +async def _resolve_ips(host: str, port: int) -> list: + """Resolve `host` to a de-duplicated, order-preserving list of IP literals.""" loop = asyncio.get_event_loop() try: infos = await loop.getaddrinfo(host, port, proto=0, type=0) except OSError as e: raise UnsafeURLError("DNS_RESOLUTION_FAILED", f"Could not resolve host '{host}': {e}") from e - return {info[4][0] for info in infos} + ips = [] + for info in infos: + ip_str = info[4][0] + if ip_str not in ips: + ips.append(ip_str) + return ips + + +async def resolve_and_validate(host: str, port: int) -> str: + """Resolve `host` and return the single IP literal to connect to. + + Every address the name resolves to must be public — a name that returns + both a public and a private address is rejected outright rather than + having a "good" address picked out of it, since which one a later + resolution would pick is not ours to control. + The returned IP is what callers must actually connect to. Resolving here + and connecting elsewhere by name would reopen the rebinding window this + function exists to close. + """ + try: + literal = ipaddress.ip_address(host) + except ValueError: + literal = None + + if literal is not None: + if not _ip_is_public(literal): + logger.warning("Blocked SSRF attempt to non-public literal %s", host) + raise UnsafeURLError("BLOCKED_PRIVATE_ADDRESS", "Host resolves to a non-public address") + ips = [str(literal)] + else: + # Address policy is checked before port policy so a request aimed at an + # internal host reports BLOCKED_PRIVATE_ADDRESS — the code worth + # alerting on — rather than being masked by a port rejection. + ips = list(await _resolve_ips(host, port)) + if not ips: + raise UnsafeURLError("DNS_RESOLUTION_FAILED", f"Host '{host}' did not resolve") + for ip_str in ips: + if not _ip_is_public(ipaddress.ip_address(ip_str)): + # The resolved address is deliberately kept out of the + # caller-facing message: echoing it back turns this endpoint + # into an internal-DNS mapping oracle for the agent. + logger.warning("Blocked SSRF attempt: %s resolved to non-public %s", host, ip_str) + raise UnsafeURLError( + "BLOCKED_PRIVATE_ADDRESS", + f"Host '{host}' resolves to a non-public address", + ) -async def validate_public_url(url: str) -> None: - """Raise UnsafeURLError unless `url` is an http(s) URL that resolves only - to public IP addresses and carries no embedded credentials.""" + if port not in ALLOWED_PORTS: + raise UnsafeURLError( + "BLOCKED_PORT", + f"Port {port} is not allowed; permitted ports: {sorted(ALLOWED_PORTS)}", + ) + return ips[0] + + +def _split_url(url: str) -> tuple: + """Return (scheme, host, port) after scheme/credential/host checks.""" parsed = urlparse(url) if parsed.scheme not in ALLOWED_SCHEMES: raise UnsafeURLError("UNSUPPORTED_SCHEME", "Only http(s) URLs are supported") @@ -81,21 +156,43 @@ async def validate_public_url(url: str) -> None: port = parsed.port or (443 if parsed.scheme == "https" else 80) except ValueError: raise UnsafeURLError("INVALID_URL", "URL has an invalid port") + return parsed.scheme, host, port - try: - literal = ipaddress.ip_address(host) - ips = {str(literal)} - except ValueError: - ips = await _resolve_ips(host, port) - if not ips: - raise UnsafeURLError("DNS_RESOLUTION_FAILED", f"Host '{host}' did not resolve") - for ip_str in ips: - if not _ip_is_public(ipaddress.ip_address(ip_str)): - raise UnsafeURLError( - "BLOCKED_PRIVATE_ADDRESS", - f"Host '{host}' resolves to a non-public address ({ip_str})", - ) +async def validate_public_url(url: str) -> str: + """Raise UnsafeURLError unless `url` is an http(s) URL on an allowed port + that resolves only to public addresses and carries no embedded credentials. + + Returns the pinned IP, so a caller that is about to connect can use it + instead of resolving the name a second time. + """ + _scheme, host, port = _split_url(url) + return await resolve_and_validate(host, port) + + +class _PinnedTransport(httpx.AsyncHTTPTransport): + """Connects to a pre-validated IP while keeping the request's logical + identity: the Host header and the TLS SNI / certificate hostname stay the + original name, so HTTPS still verifies against the domain the caller asked + for — only the address the socket goes to is fixed. + """ + + def __init__(self, pinned_ip: str, **kwargs): + super().__init__(**kwargs) + self._pinned_ip = pinned_ip + + async def handle_async_request(self, request: httpx.Request): + original_host = request.url.host + original_authority = request.headers.get("host") or request.url.netloc.decode("ascii") + # Host header must survive the rewrite or virtual-hosted origins 404. + request.headers["Host"] = original_authority + # server_hostname for the TLS handshake: drives both SNI and the + # certificate hostname check. Without it the cert would be validated + # against the bare IP and every HTTPS fetch would fail. + request.extensions = dict(request.extensions or {}) + request.extensions["sni_hostname"] = original_host + request.url = request.url.copy_with(host=self._pinned_ip, port=request.url.port) + return await super().handle_async_request(request) class SafeFetchResult: @@ -122,21 +219,32 @@ async def safe_fetch( truncate: bool = False, max_redirects: int = DEFAULT_MAX_REDIRECTS, ) -> SafeFetchResult: - """SSRF-safe GET with manual, re-validated redirects and a streamed byte cap. + """SSRF-safe GET with manual, re-validated, IP-pinned redirects and a + streamed byte cap. truncate=True -> stop reading at max_bytes and flag `truncated` (text reads). truncate=False -> raise UnsafeURLError('RESPONSE_TOO_LARGE') past max_bytes. """ request_headers = dict(headers or {}) - async with httpx.AsyncClient(follow_redirects=False, trust_env=False, timeout=timeout) as client: - current = url - for _ in range(max_redirects + 1): - await validate_public_url(current) + current = url + for _ in range(max_redirects + 1): + pinned_ip = await validate_public_url(current) + # One client (one connection pool) per hop. Sharing a pool across hops + # would let a later hop reuse a connection opened for a different + # hostname that happens to share this IP, carrying the wrong TLS SNI. + async with httpx.AsyncClient( + transport=_PinnedTransport(pinned_ip, trust_env=False), + follow_redirects=False, + trust_env=False, + timeout=timeout, + ) as client: async with client.stream("GET", current, headers=request_headers) as resp: if resp.is_redirect: location = resp.headers.get("location") if not location: raise UnsafeURLError("NAVIGATION_FAILED", "Redirect without a Location header") + # Resolve the next hop against the logical URL, never the + # pinned-IP URL, so relative Locations keep the real host. current = urljoin(current, location) continue @@ -164,4 +272,4 @@ async def safe_fetch( final_url=current, truncated=truncated, ) - raise UnsafeURLError("TOO_MANY_REDIRECTS", f"Exceeded {max_redirects} redirects") + raise UnsafeURLError("TOO_MANY_REDIRECTS", f"Exceeded {max_redirects} redirects") diff --git a/workspace/backend/app/routers/browser.py b/workspace/backend/app/routers/browser.py index 1af3c59ec..e306b9774 100644 --- a/workspace/backend/app/routers/browser.py +++ b/workspace/backend/app/routers/browser.py @@ -37,6 +37,7 @@ resolve_tab_key, ) from app.database import get_db +from app.net_security import UnsafeURLError from app.models import BrowserContext, BrowserTab, BrowserUsage, Workspace from app.response import ResponseCode, json_response, success_response from app.routers.network import ( @@ -439,6 +440,8 @@ async def open_tab( try: result = await manager.open_tab(tab_id, body.url or "about:blank", bb_context_id=bb_context_id, api_key=bf_key) + except UnsafeURLError as e: + return json_response(ResponseCode.BAD_REQUEST, str(e), data={"error_code": e.code}) except RuntimeError as e: return json_response(ResponseCode.BAD_REQUEST, str(e)) except Exception as e: @@ -619,6 +622,8 @@ async def navigate_tab( manager = BrowserManager.get() try: result = await manager.navigate(tab_id, body.url) + except UnsafeURLError as e: + return json_response(ResponseCode.BAD_REQUEST, str(e), data={"error_code": e.code}) except KeyError: return json_response(ResponseCode.NOT_FOUND, "Browser tab not found in browser") except Exception as e: diff --git a/workspace/backend/app/routers/fetch.py b/workspace/backend/app/routers/fetch.py index 6ad5fcc60..7899e1b0f 100644 --- a/workspace/backend/app/routers/fetch.py +++ b/workspace/backend/app/routers/fetch.py @@ -18,7 +18,9 @@ """ import logging +import os import re +import time from html.parser import HTMLParser from typing import Optional @@ -168,6 +170,33 @@ class FetchRequest(BaseModel): max_chars: int = DEFAULT_MAX_CHARS +# Per-workspace throttle. Without one, an agent loop turns this endpoint into +# an unmetered outbound proxy running from the deployment's IP — useful for +# scanning or for amplifying traffic at a third party. +# +# The counter is per process, so with N uvicorn workers the effective ceiling +# is N x this value. That is deliberate: a shared counter would need Redis, +# and a loose in-process cap already removes the unbounded case. +FETCH_RATE_LIMIT_PER_MINUTE = int(os.environ.get("FETCH_RATE_LIMIT_PER_MINUTE", "60")) +_RATE_WINDOW_SECONDS = 60.0 +_fetch_hits: dict = {} # workspace_id -> list[timestamp] + + +def _rate_limited(workspace_id: str) -> bool: + now = time.monotonic() + cutoff = now - _RATE_WINDOW_SECONDS + hits = [t for t in _fetch_hits.get(workspace_id, []) if t > cutoff] + if len(hits) >= FETCH_RATE_LIMIT_PER_MINUTE: + _fetch_hits[workspace_id] = hits + return True + hits.append(now) + _fetch_hits[workspace_id] = hits + if len(_fetch_hits) > 1000: # bound the dict on a busy multi-tenant host + for key in [k for k, v in _fetch_hits.items() if not any(t > cutoff for t in v)]: + _fetch_hits.pop(key, None) + return False + + def _error(code: ResponseCode, message: str, error_code: str, **extra) -> object: return json_response(code, message, data={"error_code": error_code, **extra}) @@ -222,8 +251,17 @@ async def fetch_url( if not _verify_workspace_access(workspace, x_workspace_token, authorization): return json_response(ResponseCode.UNAUTHORIZED, "Invalid workspace credentials") - # SSRF guard — reject internal/metadata targets before any outbound call - # (covers both the static tier and the browser-render tier below). + if _rate_limited(str(workspace.id)): + return _error( + ResponseCode.BAD_REQUEST, + f"Fetch rate limit reached ({FETCH_RATE_LIMIT_PER_MINUTE}/min for this workspace)", + "FETCH_RATE_LIMITED", + hint="Wait a moment before fetching again.", + ) + + # SSRF guard — reject internal/metadata targets before any outbound call. + # This is the entry check only; the static tier re-validates and pins every + # redirect hop, and the browser tier is bounded by the egress proxy. try: await validate_public_url(body.url) except UnsafeURLError as e: @@ -280,9 +318,18 @@ async def fetch_url( return _error(ResponseCode.BAD_REQUEST, message, code) # ---- Tier 2: ephemeral browser render ---- + # + # The entry URL was validated above, but that constrains only the first + # request. Where the page goes next is constrained by the egress proxy the + # local browser is launched behind (app.browser_egress). In Browser Fabric + # mode the page runs on BF's infrastructure and this process cannot + # intercept its navigation, so the render tier there is only as confined + # as BF's own egress policy. bf_key = await _resolve_bf_key(workspace, db) try: rendered = await manager.render_page_text(body.url, api_key=bf_key) + except UnsafeURLError as e: + return _error(ResponseCode.BAD_REQUEST, str(e), e.code) except BrowserNavigationError as e: code = "JS_RENDER_TIMEOUT" if e.code == "NAV_TIMEOUT" else e.code return _error(ResponseCode.BAD_REQUEST, f"Browser render failed: {e}", code) diff --git a/workspace/backend/app/routers/search.py b/workspace/backend/app/routers/search.py index 25907bc15..073b665ab 100644 --- a/workspace/backend/app/routers/search.py +++ b/workspace/backend/app/routers/search.py @@ -2,10 +2,11 @@ """ Web image search for agents. -POST /v1/search/images — proxy to Brave Image Search. The API key is -resolved per workspace (settings.brave_search_api_key) with the -BRAVE_SEARCH_API_KEY env var as fallback, so a single deployment key can -serve all workspaces until they bring their own. +POST /v1/search/images — proxy to Brave Image Search. The API key comes from +the BRAVE_SEARCH_API_KEY env var only: one deployment-wide key serves every +workspace. It is deliberately NOT read from workspace settings, because +workspace settings are returned by the workspace API and the key would then +be readable by anyone who can read them. Agents display results by embedding markdown images (![title](image_url)) in their chat replies — the frontend renders those inline — or persist one @@ -41,9 +42,7 @@ class ImageSearchRequest(BaseModel): safesearch: str = "strict" # strict | off (Brave image search values) -def _resolve_search_key(workspace) -> Optional[str]: - # Deployment-wide key via env only. Deliberately NOT read from workspace - # settings so the secret never round-trips through workspace API responses. +def _resolve_search_key() -> Optional[str]: return os.environ.get("BRAVE_SEARCH_API_KEY") or None @@ -94,7 +93,7 @@ async def search_images( if not query: return json_response(ResponseCode.BAD_REQUEST, "Query must not be empty") - key = _resolve_search_key(workspace) + key = _resolve_search_key() if not key: return json_response( ResponseCode.BAD_REQUEST, diff --git a/workspace/backend/tests/test_ssrf_boundary.py b/workspace/backend/tests/test_ssrf_boundary.py new file mode 100644 index 000000000..ca4f813dc --- /dev/null +++ b/workspace/backend/tests/test_ssrf_boundary.py @@ -0,0 +1,551 @@ +# -*- coding: utf-8 -*- +""" +SSRF boundary tests. + +Two boundaries are covered: + + * the static tier (app.net_security) — every hop is resolved once, validated, + and connected to by pinned IP, so a name that rebinds after validation + cannot swing the connection to an internal address; + + * the browser tier (app.browser_egress) — every request Chromium makes, of + every kind, goes through a policy proxy. The browser integration tests + drive a real Chromium and assert on what the proxy was asked to reach, + because the thing worth regression-testing is the network destination, + not whether some interception hook fired. + +The Chromium tests skip when no browser binary is installed (the production +image does not ship one); the rest always run. +""" + +import asyncio +import threading +from http.server import BaseHTTPRequestHandler, HTTPServer +from unittest.mock import AsyncMock, patch + +import pytest + +from app import net_security +from app.browser import BLANK_PAGE, guard_browser_url +from app.browser_egress import EgressPolicyProxy, _split_host_port, _target_from_absolute_url +from app.net_security import UnsafeURLError, safe_fetch, validate_public_url + + +def _run(coro): + return asyncio.new_event_loop().run_until_complete(coro) + + +# --------------------------------------------------------------------------- +# Static tier: resolve once, connect to the pinned IP +# --------------------------------------------------------------------------- + +class TestResolveOnceAndPin: + """The rebinding case: validation sees a public address, and the socket + must go to *that* address rather than whatever a second lookup returns.""" + + def test_connects_to_first_resolved_ip_not_a_second_lookup(self): + """A name that answers public once and internal afterwards must still + be connected to at the address that was validated.""" + import httpx + + lookups = [] + connected_to = [] + + async def rebinding_resolve(host, port): + lookups.append(host) + # First lookup: public. Any later lookup: cloud metadata. + return ["93.184.216.34"] if len(lookups) == 1 else ["169.254.169.254"] + + async def capture(self, request): + connected_to.append(request.url.host) + return httpx.Response(200, content=b"ok", request=request) + + with patch.object(net_security, "_resolve_ips", new=rebinding_resolve): + with patch.object(httpx.AsyncHTTPTransport, "handle_async_request", new=capture): + result = _run(safe_fetch("http://rebind.example/", max_bytes=1000, timeout=5)) + + assert result.status_code == 200 + # One resolution for the one hop, and the socket went to the address + # that resolution returned — not to what a connect-time lookup says. + assert lookups == ["rebind.example"] + assert connected_to == ["93.184.216.34"] + + def test_redirect_hop_to_internal_is_blocked_after_a_public_first_hop(self): + """The first hop being legitimate must not carry the second one.""" + import httpx + + async def resolve(host, port): + return ["93.184.216.34"] if host == "public.example" else ["169.254.169.254"] + + async def redirect_once(self, request): + return httpx.Response( + 302, headers={"location": "http://metadata.example/creds"}, request=request + ) + + with patch.object(net_security, "_resolve_ips", new=resolve): + with patch.object(httpx.AsyncHTTPTransport, "handle_async_request", new=redirect_once): + with pytest.raises(UnsafeURLError) as ei: + _run(safe_fetch("http://public.example/", max_bytes=1000, timeout=5)) + assert ei.value.code == "BLOCKED_PRIVATE_ADDRESS" + + def test_pinned_transport_rewrites_host_but_keeps_identity(self): + import httpx + from app.net_security import _PinnedTransport + + transport = _PinnedTransport("93.184.216.34") + request = httpx.Request("GET", "https://example.com/page") + captured = {} + + async def fake_super(req): + captured["url"] = str(req.url) + captured["host_header"] = req.headers.get("host") + captured["sni"] = req.extensions.get("sni_hostname") + return httpx.Response(200) + + with patch.object(httpx.AsyncHTTPTransport, "handle_async_request", new=lambda self, r: fake_super(r)): + _run(transport.handle_async_request(request)) + + # Socket goes to the pinned IP; Host and TLS identity stay the domain, + # so certificate verification is still against example.com. + assert captured["url"] == "https://93.184.216.34/page" + assert captured["host_header"] == "example.com" + assert captured["sni"] == "example.com" + + def test_ipv6_pin_is_bracketed(self): + import httpx + from app.net_security import _PinnedTransport + + transport = _PinnedTransport("2606:2800:220:1:248:1893:25c8:1946") + request = httpx.Request("GET", "https://example.com/x") + captured = {} + + async def fake_super(req): + captured["url"] = str(req.url) + return httpx.Response(200) + + with patch.object(httpx.AsyncHTTPTransport, "handle_async_request", new=lambda self, r: fake_super(r)): + _run(transport.handle_async_request(request)) + assert captured["url"].startswith("https://[2606:2800:220:1:248:1893:25c8:1946]/") + + def test_mixed_public_and_private_answers_are_rejected(self): + """A name answering with both a public and an internal address is + refused outright — picking the 'good' one would leave which address a + later lookup returns up to the attacker.""" + async def resolve(host, port): + return ["93.184.216.34", "10.0.0.7"] + + with patch.object(net_security, "_resolve_ips", new=resolve): + with pytest.raises(UnsafeURLError) as ei: + _run(validate_public_url("http://mixed.example/")) + assert ei.value.code == "BLOCKED_PRIVATE_ADDRESS" + + def test_blocked_message_does_not_leak_the_resolved_address(self): + async def resolve(host, port): + return ["10.11.12.13"] + + with patch.object(net_security, "_resolve_ips", new=resolve): + with pytest.raises(UnsafeURLError) as ei: + _run(validate_public_url("http://internal.example/")) + # Knowing *that* it is internal is fine; knowing which address it maps + # to would let an agent map internal DNS one name at a time. + assert "10.11.12.13" not in str(ei.value) + + +class TestPortPolicy: + def test_blocks_non_http_port(self): + with pytest.raises(UnsafeURLError) as ei: + _run(validate_public_url("http://example.com:22/")) + assert ei.value.code == "BLOCKED_PORT" + + def test_internal_address_reports_private_not_port(self): + # Address policy must win, so the alertable code is the one emitted. + with pytest.raises(UnsafeURLError) as ei: + _run(validate_public_url("http://127.0.0.1:9999/")) + assert ei.value.code == "BLOCKED_PRIVATE_ADDRESS" + + +# --------------------------------------------------------------------------- +# Browser entry guard +# --------------------------------------------------------------------------- + +class TestBrowserUrlGuard: + def test_about_blank_is_allowed(self): + assert _run(guard_browser_url("about:blank")) == BLANK_PAGE + assert _run(guard_browser_url(None)) == BLANK_PAGE + assert _run(guard_browser_url("")) == BLANK_PAGE + + @pytest.mark.parametrize("url", [ + "http://169.254.169.254/latest/meta-data/", + "http://127.0.0.1/admin", + "http://[::ffff:127.0.0.1]/", + ]) + def test_internal_targets_rejected(self, url): + with pytest.raises(UnsafeURLError) as ei: + _run(guard_browser_url(url)) + assert ei.value.code == "BLOCKED_PRIVATE_ADDRESS" + + @pytest.mark.parametrize("url", [ + "file:///etc/passwd", + "chrome://settings", + "data:text/html,", + "view-source:http://example.com/", + ]) + def test_non_http_schemes_rejected(self, url): + # about:blank is allowlisted by exact match; nothing else non-http is. + with pytest.raises(UnsafeURLError): + _run(guard_browser_url(url)) + + +# --------------------------------------------------------------------------- +# Egress proxy policy +# --------------------------------------------------------------------------- + +class TestEgressProxyParsing: + def test_split_host_port_ipv6(self): + assert _split_host_port("[::1]:8443", 443) == ("::1", 8443) + assert _split_host_port("[::1]", 443) == ("::1", 443) + + def test_split_host_port_ipv4(self): + assert _split_host_port("example.com:8080", 443) == ("example.com", 8080) + assert _split_host_port("example.com", 80) == ("example.com", 80) + + def test_absolute_form_target(self): + assert _target_from_absolute_url("http://example.com/a/b?c=1") == ("example.com", 80, "/a/b?c=1") + assert _target_from_absolute_url("http://example.com") == ("example.com", 80, "/") + + def test_origin_form_target_is_refused(self): + # A proxy only ever receives absolute-form or CONNECT; anything else + # is malformed and must not be guessed at. + with pytest.raises(ValueError): + _target_from_absolute_url("/relative/path") + + +class TestEgressProxyPolicy: + """Drive the proxy directly over a socket — no browser needed.""" + + def _request(self, request_line: str) -> bytes: + async def run(): + proxy = EgressPolicyProxy() + port = await proxy.start() + try: + reader, writer = await asyncio.open_connection("127.0.0.1", port) + writer.write(f"{request_line}\r\nHost: x\r\n\r\n".encode()) + await writer.drain() + data = await asyncio.wait_for(reader.read(200), timeout=10) + writer.close() + return data, proxy + finally: + await proxy.stop() + + data, _proxy = _run(run()) + return data + + @pytest.mark.parametrize("request_line", [ + "GET http://169.254.169.254/latest/meta-data/ HTTP/1.1", + "GET http://127.0.0.1:8000/v1/workspaces HTTP/1.1", + "GET http://10.0.0.5/internal HTTP/1.1", + "GET http://192.168.1.1/router HTTP/1.1", + "CONNECT 169.254.169.254:443 HTTP/1.1", + "CONNECT 127.0.0.1:8000 HTTP/1.1", + "CONNECT [::1]:443 HTTP/1.1", + ]) + def test_internal_destinations_are_refused(self, request_line): + assert b"403 Forbidden" in self._request(request_line) + + @pytest.mark.parametrize("request_line", [ + "GET ftp://example.com/x HTTP/1.1", + "GET /origin-form HTTP/1.1", + "GARBAGE", + ]) + def test_unparseable_requests_fail_closed(self, request_line): + assert b"403 Forbidden" in self._request(request_line) + + def test_non_http_port_is_refused(self): + assert b"403 Forbidden" in self._request("CONNECT example.com:22 HTTP/1.1") + + +# --------------------------------------------------------------------------- +# Real-browser integration: the destinations Chromium actually tries to reach +# --------------------------------------------------------------------------- + +def _chromium_available() -> bool: + try: + import os + + from playwright.sync_api import sync_playwright + with sync_playwright() as p: + return os.path.exists(p.chromium.executable_path) + except Exception: + return False + + +CHROMIUM = pytest.mark.skipif( + not _chromium_available(), reason="no Chromium binary installed (production image ships none)" +) + +INTERNAL = "169.254.169.254" +INTERNAL_PRIVATE = "10.0.0.5" + +PAGES = { + "/redirect": ("302", f"http://{INTERNAL}/creds"), + "/meta": ("200", f''), + "/jsnav": ("200", f''), + "/xhr": ("200", f''), + "/iframe": ("200", f''), + "/img": ("200", f''), + "/ws": ("200", f''), + "/popup": ("200", f''), + "/public": ("200", "public content here"), +} + + +class _OriginHandler(BaseHTTPRequestHandler): + def do_GET(self): + path = self.path.split("?")[0] + kind, payload = PAGES.get(path, ("200", "ok")) + if kind == "302": + self.send_response(302) + self.send_header("Location", payload) + self.send_header("Content-Length", "0") + self.end_headers() + return + body = payload.encode() + self.send_response(200) + self.send_header("Content-Type", "text/html") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args): + pass + + +@pytest.fixture(scope="module") +def origin_server(): + server = HTTPServer(("127.0.0.1", 0), _OriginHandler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + yield server.server_address[1] + server.shutdown() + + +@CHROMIUM +class TestBrowserEgressIntegration: + """Launch a real Chromium behind the policy proxy and assert on every + destination it tried to reach. The origin page is served from loopback, + which the policy would normally refuse — so the policy is patched to allow + exactly that one port and nothing else, letting the test distinguish + 'reached the page' from 'escaped to an internal address'.""" + + def _drive(self, origin_port, path, wait_ms=1500): + attempts = [] + + async def policy(host, port): + attempts.append((host, port)) + if host in ("127.0.0.1", "localhost") and port == origin_port: + return "127.0.0.1" + raise UnsafeURLError("BLOCKED_PRIVATE_ADDRESS", "Host resolves to a non-public address") + + async def run(): + from playwright.async_api import async_playwright + + proxy = EgressPolicyProxy() + await proxy.start() + with patch("app.browser_egress.resolve_and_validate", new=policy): + async with async_playwright() as p: + browser = await p.chromium.launch( + headless=True, + args=["--no-sandbox"] + proxy.chromium_args(), + ) + page = await browser.new_page() + try: + await page.goto(f"http://127.0.0.1:{origin_port}{path}", timeout=15000) + await page.wait_for_timeout(wait_ms) + except Exception: + pass + body = "" + try: + body = await page.content() + except Exception: + pass + await browser.close() + await proxy.stop() + return attempts, proxy.blocked_count, body + + return _run(run()) + + @pytest.mark.parametrize("path,expected_host", [ + ("/redirect", INTERNAL), + ("/meta", INTERNAL), + ("/jsnav", INTERNAL), + ("/xhr", INTERNAL), + ("/img", INTERNAL), + ("/popup", INTERNAL), + ("/iframe", INTERNAL_PRIVATE), + ("/ws", INTERNAL_PRIVATE), + ]) + def test_internal_navigation_is_blocked_at_the_chokepoint(self, origin_server, path, expected_host): + attempts, blocked, _body = self._drive(origin_server, path) + hosts = [h for h, _p in attempts] + # The browser did try to reach the internal host (so the test is + # actually exercising the vector) and the proxy refused it. + assert expected_host in hosts, f"{path}: browser never attempted {expected_host}; got {hosts}" + assert blocked >= 1, f"{path}: proxy allowed the internal request" + + def test_public_page_still_loads(self, origin_server): + _attempts, _blocked, body = self._drive(origin_server, "/public", wait_ms=300) + assert "public content here" in body + + def test_loopback_is_not_bypassed_by_chromium(self, origin_server): + # The whole design rests on --proxy-bypass-list=<-loopback>: without it + # Chromium fetches loopback directly and never consults the policy. + attempts, _blocked, _body = self._drive(origin_server, "/public", wait_ms=300) + assert ("127.0.0.1", origin_server) in attempts, ( + "loopback request never reached the proxy — the bypass flag regressed" + ) + + +# --------------------------------------------------------------------------- +# Production wiring: the manager must actually launch behind the proxy +# --------------------------------------------------------------------------- + +class TestLocalBrowserLaunchWiring: + """The integration tests above build a proxy themselves, so they would + still pass if BrowserManager forgot to launch Chromium behind one. This + asserts the real launch path.""" + + def _captured_launch_args(self, env=None): + from app.browser import BrowserManager + + captured = {} + + class _FakeChromium: + async def launch(self, headless=True, args=None): + captured["args"] = args or [] + return object() + + class _FakePlaywright: + chromium = _FakeChromium() + + async def run(): + manager = BrowserManager() + manager._playwright = _FakePlaywright() + await manager._ensure_local_browser() + port = manager._egress_proxy.port + await manager._egress_proxy.stop() + return port + + port = _run(run()) + return captured["args"], port + + def test_launches_behind_the_egress_proxy(self): + args, port = self._captured_launch_args() + assert f"--proxy-server=http://127.0.0.1:{port}" in args + assert "--proxy-bypass-list=<-loopback>" in args + + def test_sandbox_flags_are_gated(self): + args, _port = self._captured_launch_args() + # Default keeps --no-sandbox because the image has no non-root user; + # the point of the assertion is that it is the gate deciding, so + # enabling BROWSER_SANDBOX genuinely changes the launch. + assert "--no-sandbox" in args + with patch("app.browser.BROWSER_SANDBOX", True): + args_sandboxed, _ = self._captured_launch_args() + assert "--no-sandbox" not in args_sandboxed + + +# --------------------------------------------------------------------------- +# Router surface: shared browser tools and the fetch render tier +# --------------------------------------------------------------------------- + +def _create_workspace(client): + resp = client.post("/v1/workspaces", json={ + "name": "SSRF Boundary Workspace", + "agent_name": "agent-ssrf", + "creator_email": "test@example.com", + }) + assert resp.status_code == 200 + data = resp.json()["data"] + return {"id": data["workspaceId"], "token": data["token"]} + + +class TestSharedBrowserRouterRejectsInternal: + """The shared-browser tools are agent-callable with an arbitrary URL, so + they need the same entry check as /v1/fetch — and it has to surface as a + stable 400, not a generic 500.""" + + @pytest.mark.parametrize("url", [ + "http://169.254.169.254/latest/meta-data/iam/security-credentials/", + "http://127.0.0.1:8000/v1/workspaces", + "file:///etc/passwd", + ]) + def test_open_tab_rejects_internal_url(self, client, url): + workspace = _create_workspace(client) + resp = client.post("/v1/browser/tabs", json={ + "url": url, + "network": workspace["id"], + "source": "openagents:agent-ssrf", + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] in ( + "BLOCKED_PRIVATE_ADDRESS", "UNSUPPORTED_SCHEME", "BLOCKED_PORT", + ) + + def test_open_tab_without_url_still_works(self, client): + # about:blank is the default a tab opens on; the guard must not break it. + workspace = _create_workspace(client) + with patch("app.browser.BrowserManager.open_tab", + new=AsyncMock(return_value={"url": "about:blank", "title": ""})): + resp = client.post("/v1/browser/tabs", json={ + "network": workspace["id"], + "source": "openagents:agent-ssrf", + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 200 + + +class TestFetchRenderTierRejectsInternal: + def test_render_mode_propagates_unsafe_url_as_400(self, client): + workspace = _create_workspace(client) + with patch("app.browser.BrowserManager.render_page_text", + new=AsyncMock(side_effect=UnsafeURLError( + "BLOCKED_PRIVATE_ADDRESS", "Host resolves to a non-public address"))): + with patch("app.routers.fetch.validate_public_url", new=AsyncMock(return_value="93.184.216.34")): + resp = client.post("/v1/fetch", json={ + "url": "http://attacker.example/redirect-to-metadata", + "network": workspace["id"], + "mode": "render", + "source": "openagents:agent-ssrf", + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] == "BLOCKED_PRIVATE_ADDRESS" + + def test_render_mode_blocks_internal_entry_url(self, client): + workspace = _create_workspace(client) + resp = client.post("/v1/fetch", json={ + "url": "http://169.254.169.254/latest/meta-data/", + "network": workspace["id"], + "mode": "render", + "source": "openagents:agent-ssrf", + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] == "BLOCKED_PRIVATE_ADDRESS" + + +class TestFetchRateLimit: + def test_rate_limit_kicks_in(self, client): + from app.routers import fetch as fetch_router + + workspace = _create_workspace(client) + fetch_router._fetch_hits.clear() + with patch.object(fetch_router, "FETCH_RATE_LIMIT_PER_MINUTE", 3): + codes = [] + for _ in range(5): + resp = client.post("/v1/fetch", json={ + "url": "http://169.254.169.254/", # rejected either way + "network": workspace["id"], + "source": "openagents:agent-ssrf", + }, headers={"X-Workspace-Token": workspace["token"]}) + codes.append(resp.json()["data"]["error_code"]) + fetch_router._fetch_hits.clear() + assert codes[-1] == "FETCH_RATE_LIMITED" + assert "BLOCKED_PRIVATE_ADDRESS" in codes From 0d2f846acac7e4d04aeda9d50ba2bedddce6cf5f Mon Sep 17 00:00:00 2001 From: QuanCheng <915158214@qq.com> Date: Wed, 29 Jul 2026 09:02:16 +0000 Subject: [PATCH 03/10] fix four defects found reviewing the SSRF boundary work The egress proxy spliced the browser socket to the upstream and relayed raw bytes until EOF, so a second request arriving on that connection was forwarded to the already-connected upstream without its own destination check. The injected close header did not prevent this because it only applied to the upstream request, never to the browser side. Reproduced with a raw client, where a request addressed to one host and carrying its cookie was delivered to a different host's server. The plain HTTP path now relays exactly one request and one response per connection, parsing the framing the headers declare rather than trusting the upstream to hang up, and stops reading from the browser after the request body. CONNECT tunnels keep raw relaying, which is correct there because the destination is fixed for the life of the tunnel. Upgrade is stripped so a 101 cannot turn the framed path back into a tunnel. The sandbox switch did nothing. Playwright defaults chromium_sandbox to false and injects the no-sandbox flag itself, so dropping that flag from the argument list left the sandbox off while looking like it had been enabled. The launch option now carries the decision, and the test reads the launch option instead of the argument list. Redirect cookies were lost. Building one client per hop for connection isolation also built one cookie jar per hop, breaking any flow that sets a session cookie on the redirect. Hops now share a real CookieJar, which httpx adopts by reference, while keeping separate transports and pools. Egress denials were reported as successful renders. A denial is an ordinary 403 and page.goto does not raise on error status, so the refusal text was scraped and returned as page content. Denials now carry a marker header, and a blocked main frame navigation raises. A blocked subresource deliberately does not, since the page still has real content worth returning. Replaces the two tests that asserted the wrong thing. The render test mocked an exception the real path never raises, so it now drives a real browser through a real redirect into a real refusal. Each fix was mutation checked by reverting it and confirming its test fails. --- workspace/backend/app/browser_egress.py | 182 ++++++++++-- workspace/backend/app/net_security.py | 10 + workspace/backend/tests/test_ssrf_boundary.py | 261 ++++++++++++++++-- 3 files changed, 399 insertions(+), 54 deletions(-) diff --git a/workspace/backend/app/browser_egress.py b/workspace/backend/app/browser_egress.py index f2d317f14..80649896e 100644 --- a/workspace/backend/app/browser_egress.py +++ b/workspace/backend/app/browser_egress.py @@ -37,11 +37,22 @@ _MAX_HEADER_BYTES = 64 * 1024 _HEADER_TIMEOUT_SECONDS = 15.0 _CONNECT_TIMEOUT_SECONDS = 15.0 +# An upstream that never closes must not be able to hold a relay task open +# indefinitely; the plain-HTTP path is bounded even when framing is absent. +_EXCHANGE_TIMEOUT_SECONDS = 120.0 + +# A denial is an ordinary 403, and page.goto() does not raise on HTTP error +# status — so without a marker the browser would simply render the refusal +# text and the caller would report it as a successful page load. This header +# is what lets BrowserManager tell "we refused this" apart from "the site +# returned 403". +DENY_MARKER_HEADER = "x-workspace-egress-blocked" _DENY_TEXT = b"Blocked by workspace egress policy: destination is not public" _DENY_BODY = ( b"HTTP/1.1 403 Forbidden\r\n" b"Content-Type: text/plain; charset=utf-8\r\n" + b"X-Workspace-Egress-Blocked: 1\r\n" b"Content-Length: " + str(len(_DENY_TEXT)).encode() + b"\r\n" b"Connection: close\r\n" b"\r\n" + _DENY_TEXT @@ -92,35 +103,109 @@ def _target_from_absolute_url(target: str) -> tuple: return host, port, (("/" + path) if slash else "/") -# Hop-by-hop headers, plus the proxy-specific ones. These are meaningful only -# between the browser and this proxy and must not be relayed upstream. -_HOP_BY_HOP = { +# Connection-management headers. These are meaningful only between two +# adjacent HTTP peers and must not be relayed. Transfer-Encoding is +# deliberately NOT in this set: the body framing it describes is relayed +# verbatim, so dropping the header would leave the receiver unable to parse +# what it is being sent. +# +# Upgrade is stripped on purpose. A 101 response would turn this path back +# into an unframed tunnel, which is exactly what must not happen here. +# Chromium tunnels ws:// through CONNECT anyway, so nothing legitimate needs +# an in-band upgrade. +_CONNECTION_HEADERS = { b"connection", b"proxy-connection", b"proxy-authorization", b"proxy-authenticate", - b"keep-alive", b"te", b"trailer", b"transfer-encoding", b"upgrade", + b"keep-alive", b"te", b"trailer", b"upgrade", } -def _clean_headers(header_block: bytes) -> bytes: - """Drop hop-by-hop headers and force the upstream connection closed. - - Connection reuse has to be suppressed: an HTTP/1.1 proxy connection can - carry requests for several origins, and once this proxy has spliced the - socket to one upstream, later requests on it would ride to that upstream - without going back through the policy check. One request per connection - keeps every request individually validated. - """ - kept = [] +def _parse_headers(header_block: bytes) -> dict: + """Parse a raw header block into {lowercased name: value}, both bytes.""" + parsed = {} for line in header_block.split(b"\r\n"): - if not line: + if not line or b":" not in line: continue - name = line.split(b":", 1)[0].strip().lower() - if name in _HOP_BY_HOP: - continue - kept.append(line) + name, _, value = line.partition(b":") + parsed[name.strip().lower()] = value.strip() + return parsed + + +def _rewrite_headers(header_block: bytes) -> bytes: + """Strip connection-management headers and pin the connection closed. + + Every request must be individually validated, which means this proxy can + never let a second request ride a connection it has already spliced to an + upstream. Closing after one exchange, in both directions, is what makes + that structurally true rather than a property of the upstream's manners. + """ + kept = [ + line for line in header_block.split(b"\r\n") + if line and line.split(b":", 1)[0].strip().lower() not in _CONNECTION_HEADERS + ] kept.append(b"Connection: close") return b"\r\n".join(kept) + b"\r\n" +async def _relay_exact(src: asyncio.StreamReader, dst: asyncio.StreamWriter, count: int) -> None: + remaining = count + while remaining > 0: + chunk = await src.read(min(65536, remaining)) + if not chunk: + return + dst.write(chunk) + await dst.drain() + remaining -= len(chunk) + + +async def _relay_chunked(src: asyncio.StreamReader, dst: asyncio.StreamWriter) -> None: + """Relay a chunked body, stopping at the terminal chunk rather than at EOF.""" + while True: + size_line = await src.readline() + if not size_line: + return + dst.write(size_line) + await dst.drain() + try: + size = int(size_line.split(b";")[0].strip(), 16) + except ValueError: + return + if size == 0: + while True: # trailers, terminated by a blank line + trailer = await src.readline() + if not trailer: + return + dst.write(trailer) + await dst.drain() + if trailer in (b"\r\n", b"\n"): + return + await _relay_exact(src, dst, size) + dst.write(await src.read(2)) # chunk terminator + await dst.drain() + + +async def _relay_until_eof(src: asyncio.StreamReader, dst: asyncio.StreamWriter) -> None: + while True: + data = await src.read(65536) + if not data: + return + dst.write(data) + await dst.drain() + + +async def _relay_body(src, dst, headers: dict, *, read_to_eof_if_unframed: bool) -> None: + """Relay exactly one message body, using the framing its headers declare.""" + if b"chunked" in headers.get(b"transfer-encoding", b"").lower(): + await _relay_chunked(src, dst) + elif b"content-length" in headers: + try: + length = int(headers[b"content-length"]) + except ValueError: + return + await _relay_exact(src, dst, length) + elif read_to_eof_if_unframed: + await _relay_until_eof(src, dst) + + async def _pipe(src: asyncio.StreamReader, dst: asyncio.StreamWriter) -> None: try: while True: @@ -307,13 +392,62 @@ async def _forward(self, reader, writer, method: str, host: str, port: int, return self.allowed_count += 1 + try: + await asyncio.wait_for( + self._exchange(reader, writer, up_reader, up_writer, method, path, header_block), + timeout=_EXCHANGE_TIMEOUT_SECONDS, + ) + except (asyncio.TimeoutError, ConnectionError, OSError): + pass + finally: + for w in (up_writer, writer): + try: + w.close() + except Exception: + pass + + async def _exchange(self, reader, writer, up_reader, up_writer, + method: str, path: str, header_block: bytes) -> None: + """Relay exactly one request and one response, then let the caller close. + + Nothing is read from the browser after this request's body. A second + request arriving on this connection is therefore never forwarded + anywhere — it cannot reach the upstream this socket is already spliced + to, which is what would let it skip the destination check entirely. + """ + request_headers = _parse_headers(header_block) # Re-emit in origin form. The client's Host header is preserved so # virtual-hosted origins still resolve correctly; only the request - # target is rewritten and hop-by-hop headers are dropped. + # target is rewritten and connection headers are replaced. up_writer.write(f"{method} {path} HTTP/1.1\r\n".encode("latin-1")) - up_writer.write(_clean_headers(header_block)) + up_writer.write(_rewrite_headers(header_block)) up_writer.write(b"\r\n") await up_writer.drain() - await asyncio.gather( - _pipe(reader, up_writer), _pipe(up_reader, writer), return_exceptions=True - ) + await _relay_body(reader, up_writer, request_headers, read_to_eof_if_unframed=False) + + status_line = await up_reader.readline() + if not status_line: + return + raw_headers = bytearray() + while True: + line = await up_reader.readline() + if not line or line in (b"\r\n", b"\n"): + break + raw_headers += line + if len(raw_headers) > _MAX_HEADER_BYTES: + return + response_headers = _parse_headers(bytes(raw_headers)) + + writer.write(status_line) + writer.write(_rewrite_headers(bytes(raw_headers))) + writer.write(b"\r\n") + await writer.drain() + + try: + status = int(status_line.split(b" ")[1]) + except (IndexError, ValueError): + status = 0 + # A body is absent by definition for these, regardless of headers. + if method == "HEAD" or status in (204, 304) or 100 <= status < 200: + return + await _relay_body(up_reader, writer, response_headers, read_to_eof_if_unframed=True) diff --git a/workspace/backend/app/net_security.py b/workspace/backend/app/net_security.py index 468891a00..334c63fb0 100644 --- a/workspace/backend/app/net_security.py +++ b/workspace/backend/app/net_security.py @@ -31,6 +31,7 @@ import ipaddress import logging import os +from http.cookiejar import CookieJar from typing import Optional from urllib.parse import urljoin, urlparse @@ -227,6 +228,14 @@ async def safe_fetch( """ request_headers = dict(headers or {}) current = url + # Cookies have to outlive the per-hop clients below: plenty of sites set a + # session cookie on the redirect and expect it back on the landing page, + # and httpx keeps its jar on the client. This must be a raw CookieJar -- + # handing httpx a Cookies instance makes it copy the cookies into a fresh + # jar per client, which would silently drop everything a later hop sets. + # A CookieJar is adopted by reference, and its domain and path rules still + # decide what actually gets sent. + cookie_jar = CookieJar() for _ in range(max_redirects + 1): pinned_ip = await validate_public_url(current) # One client (one connection pool) per hop. Sharing a pool across hops @@ -237,6 +246,7 @@ async def safe_fetch( follow_redirects=False, trust_env=False, timeout=timeout, + cookies=cookie_jar, ) as client: async with client.stream("GET", current, headers=request_headers) as resp: if resp.is_redirect: diff --git a/workspace/backend/tests/test_ssrf_boundary.py b/workspace/backend/tests/test_ssrf_boundary.py index ca4f813dc..b10b71483 100644 --- a/workspace/backend/tests/test_ssrf_boundary.py +++ b/workspace/backend/tests/test_ssrf_boundary.py @@ -263,6 +263,124 @@ def test_unparseable_requests_fail_closed(self, request_line): def test_non_http_port_is_refused(self): assert b"403 Forbidden" in self._request("CONNECT example.com:22 HTTP/1.1") + def test_denial_carries_the_marker_header(self): + # BrowserManager distinguishes "we refused this" from "the site said + # 403" by this header alone. + assert b"X-Workspace-Egress-Blocked" in self._request( + "GET http://169.254.169.254/ HTTP/1.1" + ) + + +class TestProxyConnectionIsNotReusable: + """A second request must never ride a connection already spliced to an + upstream. If it did, it would reach that upstream without its own + destination check — carrying whatever cookies and headers it was addressed + with, to a host that never validated for it.""" + + def test_second_request_never_reaches_the_first_upstream(self): + upstream_saw = [] + policy_checks = [] + + async def scenario(): + async def upstream(reader, writer): + while True: + line = await reader.readline() + if not line: + break + upstream_saw.append(line.decode("latin-1").strip()) + while True: + header = await reader.readline() + if not header or header in (b"\r\n", b"\n"): + break + body = b"FIRST" + # Ignore the injected close and keep the socket open, the + # way a malicious origin would. + writer.write( + b"HTTP/1.1 200 OK\r\nContent-Length: %d\r\n" + b"Connection: keep-alive\r\n\r\n%s" % (len(body), body) + ) + await writer.drain() + + server = await asyncio.start_server(upstream, "127.0.0.1", 0) + up_port = server.sockets[0].getsockname()[1] + + async def policy(host, port): + policy_checks.append(host) + return "127.0.0.1" + + with patch("app.browser_egress.resolve_and_validate", new=policy): + proxy = EgressPolicyProxy() + port = await proxy.start() + reader, writer = await asyncio.open_connection("127.0.0.1", port) + writer.write( + f"GET http://attacker.test:{up_port}/one HTTP/1.1\r\n" + f"Host: attacker.test\r\n\r\n".encode() + ) + await writer.drain() + await asyncio.sleep(0.5) + # A different origin, on the same proxy connection. + writer.write( + f"GET http://victim.test:{up_port}/two HTTP/1.1\r\n" + f"Host: victim.test\r\nCookie: session=SECRET\r\n\r\n".encode() + ) + await writer.drain() + await asyncio.sleep(0.7) + writer.close() + await proxy.stop() + server.close() + + _run(scenario()) + assert policy_checks == ["attacker.test"] + assert not any("victim.test" in seen or "/two" in seen for seen in upstream_saw), ( + f"second request leaked to the first upstream: {upstream_saw}" + ) + assert upstream_saw == ["GET /one HTTP/1.1"] + + +class TestRedirectCookies: + """Per-hop clients must not mean per-hop cookie jars — sites routinely set + a session cookie on the redirect and require it on the landing page.""" + + def test_cookie_set_on_a_redirect_is_sent_on_the_next_hop(self): + received = [] + + class _CookieHandler(BaseHTTPRequestHandler): + def do_GET(self): + if self.path == "/start": + self.send_response(302) + self.send_header("Set-Cookie", "sid=ok; Path=/") + self.send_header("Location", f"http://127.0.0.1:{self.server.server_address[1]}/final") + self.send_header("Content-Length", "0") + self.end_headers() + return + received.append(self.headers.get("Cookie")) + body = b"landed" + self.send_response(200) + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args): + pass + + server = HTTPServer(("127.0.0.1", 0), _CookieHandler) + threading.Thread(target=server.serve_forever, daemon=True).start() + port = server.server_address[1] + + async def allow_loopback(host, prt): + return "127.0.0.1" + + try: + with patch.object(net_security, "resolve_and_validate", new=allow_loopback): + result = _run(safe_fetch( + f"http://127.0.0.1:{port}/start", max_bytes=10000, timeout=10, truncate=True + )) + finally: + server.shutdown() + + assert result.status_code == 200 + assert received == ["sid=ok"] + # --------------------------------------------------------------------------- # Real-browser integration: the destinations Chromium actually tries to reach @@ -296,6 +414,10 @@ def _chromium_available() -> bool: "/ws": ("200", f''), "/popup": ("200", f''), "/public": ("200", "public content here"), + "/img-with-text": ( + "200", + f'

readable article body

', + ), } @@ -396,6 +518,58 @@ def test_public_page_still_loads(self, origin_server): _attempts, _blocked, body = self._drive(origin_server, "/public", wait_ms=300) assert "public content here" in body + def test_blocked_render_raises_instead_of_returning_the_denial_page(self, origin_server): + """The end-to-end path the mocked version of this test could not see. + + A real browser follows a real redirect into an internal address, the + real proxy refuses it with a real 403, and render_page_text has to turn + that into an error — page.goto does not raise on a 403, so the naive + version of this code scrapes the refusal text and reports success.""" + from app.browser import BrowserManager + + async def policy(host, port): + if host in ("127.0.0.1", "localhost") and port == origin_server: + return "127.0.0.1" + raise UnsafeURLError("BLOCKED_PRIVATE_ADDRESS", "Host resolves to a non-public address") + + async def run(): + manager = BrowserManager() + with patch("app.browser_egress.resolve_and_validate", new=policy): + with patch("app.browser.validate_public_url", new=AsyncMock(return_value="127.0.0.1")): + try: + return await manager.render_page_text( + f"http://127.0.0.1:{origin_server}/redirect" + ) + finally: + await manager.shutdown() + + with pytest.raises(UnsafeURLError) as ei: + _run(run()) + assert ei.value.code == "BLOCKED_PRIVATE_ADDRESS" + + def test_blocked_subresource_does_not_fail_the_whole_render(self, origin_server): + """A refused image must not cost the caller the page's real content.""" + from app.browser import BrowserManager + + async def policy(host, port): + if host in ("127.0.0.1", "localhost") and port == origin_server: + return "127.0.0.1" + raise UnsafeURLError("BLOCKED_PRIVATE_ADDRESS", "Host resolves to a non-public address") + + async def run(): + manager = BrowserManager() + with patch("app.browser_egress.resolve_and_validate", new=policy): + with patch("app.browser.validate_public_url", new=AsyncMock(return_value="127.0.0.1")): + try: + return await manager.render_page_text( + f"http://127.0.0.1:{origin_server}/img-with-text" + ) + finally: + await manager.shutdown() + + result = _run(run()) + assert "readable article body" in result["text"] + def test_loopback_is_not_bypassed_by_chromium(self, origin_server): # The whole design rests on --proxy-bypass-list=<-loopback>: without it # Chromium fetches loopback directly and never consults the policy. @@ -414,14 +588,15 @@ class TestLocalBrowserLaunchWiring: still pass if BrowserManager forgot to launch Chromium behind one. This asserts the real launch path.""" - def _captured_launch_args(self, env=None): + def _captured_launch(self): from app.browser import BrowserManager captured = {} class _FakeChromium: - async def launch(self, headless=True, args=None): + async def launch(self, headless=True, args=None, **kwargs): captured["args"] = args or [] + captured["kwargs"] = kwargs return object() class _FakePlaywright: @@ -436,22 +611,63 @@ async def run(): return port port = _run(run()) - return captured["args"], port + return captured, port def test_launches_behind_the_egress_proxy(self): - args, port = self._captured_launch_args() - assert f"--proxy-server=http://127.0.0.1:{port}" in args - assert "--proxy-bypass-list=<-loopback>" in args - - def test_sandbox_flags_are_gated(self): - args, _port = self._captured_launch_args() - # Default keeps --no-sandbox because the image has no non-root user; - # the point of the assertion is that it is the gate deciding, so - # enabling BROWSER_SANDBOX genuinely changes the launch. - assert "--no-sandbox" in args + captured, port = self._captured_launch() + assert f"--proxy-server=http://127.0.0.1:{port}" in captured["args"] + assert "--proxy-bypass-list=<-loopback>" in captured["args"] + + def test_sandbox_is_controlled_by_the_launch_option_not_the_args(self): + """Playwright defaults chromium_sandbox to False and adds --no-sandbox + itself, so asserting on args alone would pass while the sandbox stayed + off. The launch option is the thing that actually decides.""" + captured, _port = self._captured_launch() + assert captured["kwargs"].get("chromium_sandbox") is False + with patch("app.browser.BROWSER_SANDBOX", True): - args_sandboxed, _ = self._captured_launch_args() - assert "--no-sandbox" not in args_sandboxed + captured_on, _ = self._captured_launch() + assert captured_on["kwargs"].get("chromium_sandbox") is True + + def test_playwright_injects_no_sandbox_itself(self): + """The fact the gate rests on: Playwright adds --no-sandbox on its own + when chromium_sandbox is false. That is why leaving the flag out of + `args` does not enable sandboxing, and why the launch option has to + carry the decision. If a future Playwright stopped doing this, the + comment on the launch call would be wrong and this test says so.""" + import os + import sys + + if not _chromium_available(): + pytest.skip("no Chromium binary installed") + if not sys.platform.startswith("linux"): + pytest.skip("reads /proc to inspect the launched process") + + async def launched_cmdline(): + from playwright.async_api import async_playwright + async with async_playwright() as p: + browser = await p.chromium.launch( + headless=True, args=[], chromium_sandbox=False + ) + found = [] + for pid in os.listdir("/proc"): + if not pid.isdigit(): + continue + try: + with open(f"/proc/{pid}/cmdline", "rb") as fh: + argv = fh.read().split(b"\0") + except OSError: + continue + if argv and b"chrome" in argv[0].lower() and b"--headless" in b" ".join(argv): + found.append(argv) + await browser.close() + return found + + processes = _run(launched_cmdline()) + assert processes, "no Chromium process observed" + assert any(b"--no-sandbox" in b" ".join(argv) for argv in processes), ( + "Playwright no longer injects --no-sandbox when chromium_sandbox is false" + ) # --------------------------------------------------------------------------- @@ -504,21 +720,6 @@ def test_open_tab_without_url_still_works(self, client): class TestFetchRenderTierRejectsInternal: - def test_render_mode_propagates_unsafe_url_as_400(self, client): - workspace = _create_workspace(client) - with patch("app.browser.BrowserManager.render_page_text", - new=AsyncMock(side_effect=UnsafeURLError( - "BLOCKED_PRIVATE_ADDRESS", "Host resolves to a non-public address"))): - with patch("app.routers.fetch.validate_public_url", new=AsyncMock(return_value="93.184.216.34")): - resp = client.post("/v1/fetch", json={ - "url": "http://attacker.example/redirect-to-metadata", - "network": workspace["id"], - "mode": "render", - "source": "openagents:agent-ssrf", - }, headers={"X-Workspace-Token": workspace["token"]}) - assert resp.status_code == 400 - assert resp.json()["data"]["error_code"] == "BLOCKED_PRIVATE_ADDRESS" - def test_render_mode_blocks_internal_entry_url(self, client): workspace = _create_workspace(client) resp = client.post("/v1/fetch", json={ From 52fceaddcefe9604aacc20d818e808717cc61ec6 Mon Sep 17 00:00:00 2001 From: QuanCheng <915158214@qq.com> Date: Wed, 29 Jul 2026 09:32:48 +0000 Subject: [PATCH 04/10] fix cookie pinning, chunk framing, interim responses and marker forgery Four defects from the next review round, all reproduced before fixing. Cookies were still being lost. The pinning transport rewrote the request URL to the validated address and left it rewritten, so httpx attributed Set-Cookie to that address rather than to the hostname the caller asked for, and the next hop never sent the cookie back. The previous test used the same value for the logical host and the pinned address, which hid this completely. The transport now restores the logical URL before returning, and the test pins a named host to a different address so the two can never be confused again. The chunked relay could truncate a response. It read the two-byte chunk terminator with a call that is only required to return at most that many bytes, so a CR and LF split across TCP segments left a stray LF to be read as the next chunk size, ending the body early. Reproduced by delivering the terminator in two segments. It now reads exactly two bytes and validates them. Interim responses ended the exchange. Any 1xx was treated as final, so an upstream sending 103 Early Hints before its real response lost that response entirely. Interim responses are now relayed and reading continues, without announcing a close that the following response would contradict. A 101 is still refused, since accepting one would turn the framed path back into an opaque tunnel. Requests carrying an expectation of a continue are answered by the proxy rather than forwarded, which would otherwise deadlock the exchange. The denial marker was forgeable. BrowserManager checked only that the header was present, so any site the policy allows could set it and make its own page look like a blocked internal address. Over an encrypted tunnel the proxy cannot strip an upstream header, so the marker carries a per-proxy random token instead and is compared by value. Each fix was mutation checked by reverting it and confirming the new test fails. --- workspace/backend/app/browser_egress.py | 119 ++++++++---- workspace/backend/app/net_security.py | 16 +- workspace/backend/tests/test_ssrf_boundary.py | 178 +++++++++++++++++- 3 files changed, 270 insertions(+), 43 deletions(-) diff --git a/workspace/backend/app/browser_egress.py b/workspace/backend/app/browser_egress.py index 80649896e..f026d94cf 100644 --- a/workspace/backend/app/browser_egress.py +++ b/workspace/backend/app/browser_egress.py @@ -28,6 +28,7 @@ import asyncio import logging +import secrets from typing import Optional from app.net_security import UnsafeURLError, resolve_and_validate @@ -46,17 +47,27 @@ # text and the caller would report it as a successful page load. This header # is what lets BrowserManager tell "we refused this" apart from "the site # returned 403". +# +# Its VALUE is a per-proxy random token, not a fixed constant. Any site the +# policy allows could otherwise return this header and make an ordinary page +# look like a blocked internal address, turning a public page into a forged +# SSRF rejection. Over HTTPS the proxy cannot see, let alone strip, an +# upstream's headers, so the marker has to be unforgeable rather than merely +# stripped. DENY_MARKER_HEADER = "x-workspace-egress-blocked" _DENY_TEXT = b"Blocked by workspace egress policy: destination is not public" -_DENY_BODY = ( - b"HTTP/1.1 403 Forbidden\r\n" - b"Content-Type: text/plain; charset=utf-8\r\n" - b"X-Workspace-Egress-Blocked: 1\r\n" - b"Content-Length: " + str(len(_DENY_TEXT)).encode() + b"\r\n" - b"Connection: close\r\n" - b"\r\n" + _DENY_TEXT -) + + +def _deny_response(token: str) -> bytes: + return ( + b"HTTP/1.1 403 Forbidden\r\n" + b"Content-Type: text/plain; charset=utf-8\r\n" + b"X-Workspace-Egress-Blocked: " + token.encode("ascii") + b"\r\n" + b"Content-Length: " + str(len(_DENY_TEXT)).encode() + b"\r\n" + b"Connection: close\r\n" + b"\r\n" + _DENY_TEXT + ) def _bad_gateway(reason: str) -> bytes: @@ -130,20 +141,24 @@ def _parse_headers(header_block: bytes) -> dict: return parsed -def _rewrite_headers(header_block: bytes) -> bytes: +def _rewrite_headers(header_block: bytes, *, close: bool = True) -> bytes: """Strip connection-management headers and pin the connection closed. Every request must be individually validated, which means this proxy can never let a second request ride a connection it has already spliced to an upstream. Closing after one exchange, in both directions, is what makes that structurally true rather than a property of the upstream's manners. + + close=False is for interim 1xx responses, where announcing a close would + contradict the final response still to come on the same connection. """ kept = [ line for line in header_block.split(b"\r\n") if line and line.split(b":", 1)[0].strip().lower() not in _CONNECTION_HEADERS ] - kept.append(b"Connection: close") - return b"\r\n".join(kept) + b"\r\n" + if close: + kept.append(b"Connection: close") + return b"\r\n".join(kept) + b"\r\n" if kept else b"" async def _relay_exact(src: asyncio.StreamReader, dst: asyncio.StreamWriter, count: int) -> None: @@ -179,7 +194,16 @@ async def _relay_chunked(src: asyncio.StreamReader, dst: asyncio.StreamWriter) - if trailer in (b"\r\n", b"\n"): return await _relay_exact(src, dst, size) - dst.write(await src.read(2)) # chunk terminator + # readexactly, not read -- read(2) is free to return a single byte when + # the CR and LF land in different TCP segments, and the stray LF would + # then be consumed as the next chunk-size line and truncate the body. + try: + terminator = await src.readexactly(2) + except asyncio.IncompleteReadError: + return + if terminator != b"\r\n": + return + dst.write(terminator) await dst.drain() @@ -229,6 +253,9 @@ class EgressPolicyProxy: def __init__(self): self._server: Optional[asyncio.AbstractServer] = None self._port: Optional[int] = None + # Proves a denial came from this proxy. Regenerated per instance so a + # value seen by one workspace's page is useless anywhere else. + self.deny_token = secrets.token_urlsafe(24) self.blocked_count = 0 self.allowed_count = 0 @@ -277,7 +304,7 @@ async def _deny(self, writer: asyncio.StreamWriter, host: str, port: int, reason self.blocked_count += 1 logger.warning("Egress policy blocked browser request to %s:%s (%s)", host, port, reason) try: - writer.write(_DENY_BODY) + writer.write(_deny_response(self.deny_token)) await writer.drain() except (ConnectionError, OSError): pass @@ -423,31 +450,59 @@ async def _exchange(self, reader, writer, up_reader, up_writer, up_writer.write(_rewrite_headers(header_block)) up_writer.write(b"\r\n") await up_writer.drain() + + # Expect/100-continue is answered here rather than forwarded. Waiting + # for a body the browser will not send until it is told to continue, + # while the upstream waits for that body, deadlocks the exchange. + if b"100-continue" in request_headers.get(b"expect", b"").lower(): + writer.write(b"HTTP/1.1 100 Continue\r\n\r\n") + await writer.drain() await _relay_body(reader, up_writer, request_headers, read_to_eof_if_unframed=False) - status_line = await up_reader.readline() - if not status_line: + while True: + status_line, raw_headers, response_headers = await self._read_message_head(up_reader) + if status_line is None: + return + try: + status = int(status_line.split(b" ")[1]) + except (IndexError, ValueError): + status = 0 + + # 101 would hand the rest of the connection to the upstream as an + # opaque tunnel, which is the framing this path exists to keep. + # Upgrade is already stripped from the request, so an upstream + # offering one is answering something nobody asked for. + if status == 101: + logger.warning("Egress proxy refused an unrequested protocol upgrade") + return + + # 1xx is informational -- 103 Early Hints in particular precedes the + # real response. Relay it and keep reading rather than treating it + # as the end of the exchange. + interim = 100 <= status < 200 + writer.write(status_line) + writer.write(_rewrite_headers(raw_headers, close=not interim)) + writer.write(b"\r\n") + await writer.drain() + if interim: + continue + + if method == "HEAD" or status in (204, 304): + return + await _relay_body(up_reader, writer, response_headers, read_to_eof_if_unframed=True) return + + async def _read_message_head(self, reader) -> tuple: + """Read a status line plus header block. Returns (None, ...) at EOF.""" + status_line = await reader.readline() + if not status_line: + return None, b"", {} raw_headers = bytearray() while True: - line = await up_reader.readline() + line = await reader.readline() if not line or line in (b"\r\n", b"\n"): break raw_headers += line if len(raw_headers) > _MAX_HEADER_BYTES: - return - response_headers = _parse_headers(bytes(raw_headers)) - - writer.write(status_line) - writer.write(_rewrite_headers(bytes(raw_headers))) - writer.write(b"\r\n") - await writer.drain() - - try: - status = int(status_line.split(b" ")[1]) - except (IndexError, ValueError): - status = 0 - # A body is absent by definition for these, regardless of headers. - if method == "HEAD" or status in (204, 304) or 100 <= status < 200: - return - await _relay_body(up_reader, writer, response_headers, read_to_eof_if_unframed=True) + return None, b"", {} + return status_line, bytes(raw_headers), _parse_headers(bytes(raw_headers)) diff --git a/workspace/backend/app/net_security.py b/workspace/backend/app/net_security.py index 334c63fb0..f608153c9 100644 --- a/workspace/backend/app/net_security.py +++ b/workspace/backend/app/net_security.py @@ -183,8 +183,9 @@ def __init__(self, pinned_ip: str, **kwargs): self._pinned_ip = pinned_ip async def handle_async_request(self, request: httpx.Request): - original_host = request.url.host - original_authority = request.headers.get("host") or request.url.netloc.decode("ascii") + logical_url = request.url + original_host = logical_url.host + original_authority = request.headers.get("host") or logical_url.netloc.decode("ascii") # Host header must survive the rewrite or virtual-hosted origins 404. request.headers["Host"] = original_authority # server_hostname for the TLS handshake: drives both SNI and the @@ -192,8 +193,15 @@ async def handle_async_request(self, request: httpx.Request): # against the bare IP and every HTTPS fetch would fail. request.extensions = dict(request.extensions or {}) request.extensions["sni_hostname"] = original_host - request.url = request.url.copy_with(host=self._pinned_ip, port=request.url.port) - return await super().handle_async_request(request) + request.url = logical_url.copy_with(host=self._pinned_ip, port=logical_url.port) + try: + return await super().handle_async_request(request) + finally: + # The rewrite has to be undone before httpx sees the response. + # Everything above this layer reads identity off the request URL -- + # cookie ownership most of all, which would otherwise be filed + # under the pinned address and never sent back to the real host. + request.url = logical_url class SafeFetchResult: diff --git a/workspace/backend/tests/test_ssrf_boundary.py b/workspace/backend/tests/test_ssrf_boundary.py index b10b71483..d5716df3d 100644 --- a/workspace/backend/tests/test_ssrf_boundary.py +++ b/workspace/backend/tests/test_ssrf_boundary.py @@ -270,6 +270,113 @@ def test_denial_carries_the_marker_header(self): "GET http://169.254.169.254/ HTTP/1.1" ) + def test_deny_token_is_unguessable_and_per_proxy(self): + """A fixed marker value would let any allowed site forge a rejection, + so the value has to be a secret this proxy alone knows.""" + first, second = EgressPolicyProxy(), EgressPolicyProxy() + assert first.deny_token != second.deny_token + assert len(first.deny_token) >= 24 + + +class TestProxyHttpFraming: + """The plain-HTTP path parses framing itself, so it has to survive the + ways a real network delivers bytes rather than the way a test writes them.""" + + def _exchange(self, upstream_script, request_line="GET http://site.test/x HTTP/1.1"): + async def run(): + async def upstream(reader, writer): + await reader.readline() + while True: + header = await reader.readline() + if not header or header in (b"\r\n", b"\n"): + break + await upstream_script(writer) + + server = await asyncio.start_server(upstream, "127.0.0.1", 0) + up_port = server.sockets[0].getsockname()[1] + + async def policy(host, port): + return "127.0.0.1" + + with patch("app.browser_egress.resolve_and_validate", new=policy): + proxy = EgressPolicyProxy() + port = await proxy.start() + reader, writer = await asyncio.open_connection("127.0.0.1", port) + line = request_line.replace("site.test", f"site.test:{up_port}") + writer.write(f"{line}\r\nHost: site.test\r\n\r\n".encode()) + await writer.drain() + try: + data = await asyncio.wait_for(reader.read(-1), timeout=8) + except asyncio.TimeoutError: + data = b"" + writer.close() + await proxy.stop() + server.close() + return data + + return _run(run()) + + def test_chunk_terminator_split_across_packets(self): + """read(2) may return one byte; the stray LF would then be read as the + next chunk-size line and silently truncate everything after it.""" + async def script(writer): + writer.write(b"HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n") + await writer.drain() + writer.write(b"3\r\nabc\r") # chunk plus a lone CR + await writer.drain() + await asyncio.sleep(0.2) # force a separate segment + writer.write(b"\n") # the matching LF + await writer.drain() + writer.write(b"5\r\nWORLD\r\n0\r\n\r\n") + await writer.drain() + writer.close() + + data = self._exchange(script) + assert b"WORLD" in data, f"body truncated at the split terminator: {data!r}" + assert data.endswith(b"0\r\n\r\n") + + def test_interim_response_does_not_end_the_exchange(self): + """103 Early Hints is common in front of CDNs; treating it as the + response loses the page entirely.""" + async def script(writer): + writer.write(b"HTTP/1.1 103 Early Hints\r\nLink: ; rel=preload\r\n\r\n") + await writer.drain() + await asyncio.sleep(0.1) + body = b"REAL-PAGE" + writer.write(b"HTTP/1.1 200 OK\r\nContent-Length: %d\r\n\r\n%s" % (len(body), body)) + await writer.drain() + writer.close() + + data = self._exchange(script) + assert b"REAL-PAGE" in data + # Announcing a close on the interim response would contradict the + # final response still to come on the same connection. + interim, _, _final = data.partition(b"HTTP/1.1 200") + assert b"Connection: close" not in interim + + def test_upgrade_response_is_refused(self): + """A 101 would hand the rest of the connection to the upstream as an + opaque tunnel, undoing the per-request framing.""" + async def script(writer): + writer.write(b"HTTP/1.1 101 Switching Protocols\r\nUpgrade: h2c\r\n\r\n") + await writer.drain() + writer.write(b"RAW-TUNNEL-BYTES") + await writer.drain() + writer.close() + + data = self._exchange(script) + assert b"RAW-TUNNEL-BYTES" not in data + assert b"101" not in data + + def test_head_response_has_no_body_relayed(self): + async def script(writer): + writer.write(b"HTTP/1.1 200 OK\r\nContent-Length: 9\r\n\r\n") + await writer.drain() + writer.close() + + data = self._exchange(script, request_line="HEAD http://site.test/x HTTP/1.1") + assert data.endswith(b"\r\n\r\n") + class TestProxyConnectionIsNotReusable: """A second request must never ride a connection already spliced to an @@ -339,17 +446,23 @@ async def policy(host, port): class TestRedirectCookies: """Per-hop clients must not mean per-hop cookie jars — sites routinely set - a session cookie on the redirect and require it on the landing page.""" + a session cookie on the redirect and require it on the landing page. + + The logical hostname here is deliberately NOT the address connected to. + An earlier version of this test used 127.0.0.1 for both and passed while + cookies were in fact being filed under the pinned IP, where the real host + would never be sent them. + """ - def test_cookie_set_on_a_redirect_is_sent_on_the_next_hop(self): + def test_cookie_from_a_redirect_survives_under_a_pinned_address(self): received = [] class _CookieHandler(BaseHTTPRequestHandler): def do_GET(self): if self.path == "/start": self.send_response(302) - self.send_header("Set-Cookie", "sid=ok; Path=/") - self.send_header("Location", f"http://127.0.0.1:{self.server.server_address[1]}/final") + self.send_header("Set-Cookie", "sid=ok; Path=/") # host-only + self.send_header("Location", "/final") # relative self.send_header("Content-Length", "0") self.end_headers() return @@ -367,13 +480,15 @@ def log_message(self, *args): threading.Thread(target=server.serve_forever, daemon=True).start() port = server.server_address[1] - async def allow_loopback(host, prt): + async def pin_elsewhere(host, prt): + assert host == "cookie-host.test" # logical name, not the address return "127.0.0.1" try: - with patch.object(net_security, "resolve_and_validate", new=allow_loopback): + with patch.object(net_security, "resolve_and_validate", new=pin_elsewhere): result = _run(safe_fetch( - f"http://127.0.0.1:{port}/start", max_bytes=10000, timeout=10, truncate=True + f"http://cookie-host.test:{port}/start", + max_bytes=10000, timeout=10, truncate=True, )) finally: server.shutdown() @@ -381,6 +496,26 @@ async def allow_loopback(host, prt): assert result.status_code == 200 assert received == ["sid=ok"] + def test_request_url_is_left_logical_after_pinning(self): + """Cookie ownership is only one consumer of the request URL. The + transport must hand the request back the way it received it.""" + import httpx + from app.net_security import _PinnedTransport + + seen_during_send = {} + + async def fake_super(self, request): + seen_during_send["url"] = str(request.url) + return httpx.Response(200, request=request) + + transport = _PinnedTransport("93.184.216.34") + request = httpx.Request("GET", "https://example.com/page") + with patch.object(httpx.AsyncHTTPTransport, "handle_async_request", new=fake_super): + _run(transport.handle_async_request(request)) + + assert seen_during_send["url"] == "https://93.184.216.34/page" + assert str(request.url) == "https://example.com/page" + # --------------------------------------------------------------------------- # Real-browser integration: the destinations Chromium actually tries to reach @@ -418,6 +553,7 @@ def _chromium_available() -> bool: "200", f'

readable article body

', ), + "/forged-marker": ("200", "

not actually blocked

"), } @@ -434,6 +570,9 @@ def do_GET(self): body = payload.encode() self.send_response(200) self.send_header("Content-Type", "text/html") + if path == "/forged-marker": + # A hostile-but-allowed origin claiming to be an egress denial. + self.send_header("X-Workspace-Egress-Blocked", "1") self.send_header("Content-Length", str(len(body))) self.end_headers() self.wfile.write(body) @@ -547,6 +686,31 @@ async def run(): _run(run()) assert ei.value.code == "BLOCKED_PRIVATE_ADDRESS" + def test_a_site_cannot_forge_an_egress_denial(self, origin_server): + """The origin serves the marker header itself. Because the value is a + per-proxy secret, the page must still be returned as ordinary content + rather than reported as a blocked internal address.""" + from app.browser import BrowserManager + + async def policy(host, port): + if host in ("127.0.0.1", "localhost") and port == origin_server: + return "127.0.0.1" + raise UnsafeURLError("BLOCKED_PRIVATE_ADDRESS", "Host resolves to a non-public address") + + async def run(): + manager = BrowserManager() + with patch("app.browser_egress.resolve_and_validate", new=policy): + with patch("app.browser.validate_public_url", new=AsyncMock(return_value="127.0.0.1")): + try: + return await manager.render_page_text( + f"http://127.0.0.1:{origin_server}/forged-marker" + ) + finally: + await manager.shutdown() + + result = _run(run()) + assert "not actually blocked" in result["text"] + def test_blocked_subresource_does_not_fail_the_whole_render(self, origin_server): """A refused image must not cost the caller the page's real content.""" from app.browser import BrowserManager From d8888309971a2c79451e73972df60ced71c1005e Mon Sep 17 00:00:00 2001 From: QuanCheng <915158214@qq.com> Date: Wed, 29 Jul 2026 10:07:52 +0000 Subject: [PATCH 05/10] reapply browser SSRF guards on top of the rebased develop structure The rebase resolved every conflict to develop, which is whole-file, so the three security commits lost all of their changes to browser.py even where nothing actually conflicted. This puts them back, adapted to the structure develop now has rather than restored verbatim. Adapted rather than reverted because develop reworked the surrounding code. Cloud detection moved from a manager-wide flag to a per-tab check, so the navigate guard sits above the new call. Credential handling and the ephemeral tab quota are develop's, untouched. Restores the entry guard on open_tab, navigate and render_page_text, the blank-page allowlist, the egress proxy wiring, the sandbox launch option, the authenticated denial marker and the proxy shutdown. Also restores the fetch and search router registration, which the same whole-file resolution had dropped from main.py. Both endpoints were returning 404, so the feature this branch exists to add was absent entirely; only a test asserting a status code caught it. Adds the missing navigate coverage. Deleting that guard turned nothing red, because the shared browser tests only ever exercised open. Steering an already-open tab somewhere internal is the same capability as opening it there, so it needs its own test. Drops test_browser_quota.py. It covers the per-workspace cap and the idle reaper from the first commit, both superseded by develop's ephemeral quota and maintenance sweep, and neither symbol still exists. develop's own tab-leak suite covers the same behaviours. --- workspace/backend/app/browser.py | 113 +++++++++++++++++- workspace/backend/app/main.py | 4 +- workspace/backend/tests/test_browser_quota.py | 110 ----------------- workspace/backend/tests/test_ssrf_boundary.py | 30 +++++ 4 files changed, 141 insertions(+), 116 deletions(-) delete mode 100644 workspace/backend/tests/test_browser_quota.py diff --git a/workspace/backend/app/browser.py b/workspace/backend/app/browser.py index 67df8b857..feb71ca00 100644 --- a/workspace/backend/app/browser.py +++ b/workspace/backend/app/browser.py @@ -16,6 +16,8 @@ import httpx from app.browser_creds import redact +from app.browser_egress import DENY_MARKER_HEADER, EgressPolicyProxy +from app.net_security import UnsafeURLError, validate_public_url logger = logging.getLogger(__name__) @@ -31,6 +33,36 @@ RENDER_SETTLE_ATTEMPTS = int(os.environ.get("RENDER_SETTLE_ATTEMPTS", "3")) RENDER_MIN_TEXT_CHARS = int(os.environ.get("RENDER_MIN_TEXT_CHARS", "50")) +# Chromium's own sandbox is a second containment layer under the egress proxy: +# it is what keeps a renderer compromise (from a page an agent chose) inside +# the renderer process. It needs a non-root user plus unprivileged user +# namespaces in the container, which the current backend image does not +# provide, so it stays opt-in — flipping the default here without changing the +# image would make every browser launch fail. +BROWSER_SANDBOX = os.environ.get("BROWSER_SANDBOX", "").lower() in ("1", "true", "yes") + +# The only non-http(s) URL a tab may hold: the blank page a tab is opened on +# before anywhere real is navigated to. +BLANK_PAGE = "about:blank" + + +async def guard_browser_url(url: Optional[str]) -> str: + """Policy gate for every URL a browser is asked to load. + + Raises UnsafeURLError for anything that is not a public http(s) target. + `about:blank` is allowed through as an exact match only — it is the + default a tab opens on, and it reaches no network — while any other + non-http(s) URL (file://, chrome://, data:, view-source:, ...) is refused. + + This is the entry check. It is not the whole defence: once a page is + loaded, where it navigates next is constrained by the egress proxy, not + by this function. + """ + if not url or url.strip() == "" or url.strip() == BLANK_PAGE: + return BLANK_PAGE + await validate_public_url(url) + return url + class BrowserNavigationError(RuntimeError): """Navigation failure with a machine-readable code the agent can act on.""" @@ -66,6 +98,7 @@ def __init__(self): self._sessions: dict = {} # tab_id -> Browser Fabric session id self._live_urls: dict = {} # tab_id -> Browser Fabric share URL self._tab_keys: dict = {} # tab_id -> per-workspace BF API key + self._egress_proxy = None # EgressPolicyProxy (local mode only) @classmethod def get(cls) -> "BrowserManager": @@ -153,13 +186,26 @@ async def _ensure_playwright(self): from playwright.async_api import async_playwright self._playwright = await async_playwright().start() + async def _ensure_egress_proxy(self): + """Start (once) the policy proxy every local browser request goes through.""" + if self._egress_proxy is None: + self._egress_proxy = EgressPolicyProxy() + await self._egress_proxy.start() + return self._egress_proxy + async def _ensure_local_browser(self): if self._browser and self._browser.is_connected(): return await self._ensure_playwright() + proxy = await self._ensure_egress_proxy() + # chromium_sandbox is the switch that actually decides this: Playwright + # defaults it to False and injects --no-sandbox itself, so merely + # leaving that flag out of `args` would keep the sandbox off while + # looking like it had been enabled. self._browser = await self._playwright.chromium.launch( headless=True, - args=["--no-sandbox", "--disable-setuid-sandbox"], + args=list(proxy.chromium_args()), + chromium_sandbox=BROWSER_SANDBOX, ) # ------------------------------------------------------------------ @@ -184,7 +230,11 @@ async def _prune_dead_sessions(self) -> int: return len(dead) async def open_tab(self, tab_id: str, url: str = "about:blank", bb_context_id: str = None, api_key: str = None) -> dict: - """Create a new browser tab. Returns {url, title}.""" + """Create a new browser tab. Returns {url, title}. + + Raises UnsafeURLError if `url` is not a public http(s) target. + """ + url = await guard_browser_url(url) if api_key: self._tab_keys[tab_id] = api_key async with self._global_lock: @@ -252,7 +302,11 @@ async def open_tab(self, tab_id: str, url: str = "about:blank", bb_context_id: s return {"url": page.url, "title": title, "warnings": warnings} async def navigate(self, tab_id: str, url: str) -> dict: - """Navigate a tab to a URL. Returns {url, title}.""" + """Navigate a tab to a URL. Returns {url, title}. + + Raises UnsafeURLError if `url` is not a public http(s) target. + """ + url = await guard_browser_url(url) if self._is_cloud_tab(tab_id): session_id = self._get_session(tab_id) try: @@ -427,9 +481,19 @@ async def render_page_text(self, url: str, api_key: str = None) -> dict: snapshot, close. Never registers a tab, so it doesn't consume the workspace tab quota. Returns {url, title, text}. - Raises BrowserNavigationError on navigation failure. The caller - (/v1/fetch) has already validated the entry URL against private hosts. + Raises BrowserNavigationError on navigation failure, UnsafeURLError if + the entry URL is not a public http(s) target. + + Validating the entry URL constrains only the first request. Everything + the page does afterwards — redirects, meta-refresh, JS navigation, + XHR, iframes, subresources, WebSockets — is constrained in local mode + by the egress proxy (see app.browser_egress), which is the actual + boundary. In Browser Fabric mode the navigation happens on BF's + infrastructure and only this entry check applies. """ + url = await guard_browser_url(url) + if url == BLANK_PAGE: + raise BrowserNavigationError("NAVIGATION_FAILED", "No URL to render") if self.is_cloud_for(api_key): key = api_key or BROWSERFABRIC_API_KEY result = await self._bf_call("create_session", {"headless": True}, api_key=key) @@ -463,12 +527,45 @@ async def render_page_text(self, url: str, api_key: str = None) -> dict: async with self._global_lock: await self._ensure_local_browser() page = await self._browser.new_page() + + # A policy denial arrives as an ordinary 403 and page.goto() does + # not raise on HTTP error status, so without this the refusal page + # would be scraped and returned as if it were the article. Only + # main-frame navigations are treated as fatal: a page whose image + # or analytics call was refused still has real content worth + # returning. + blocked_navigations = [] + deny_token = self._egress_proxy.deny_token if self._egress_proxy else None + + def _note_blocked(response): + try: + # Compare the token, not just the header name. Any site the + # policy allows could set this header itself, and would + # otherwise be able to make its own page look like a + # blocked internal address. + if not deny_token or response.headers.get(DENY_MARKER_HEADER) != deny_token: + return + # response.frame is the frame that issued the request, so an + # image in the top document also reports the main frame. + # Only a navigation of the main frame replaces the content + # this function is about to return. + if response.frame is page.main_frame and response.request.is_navigation_request(): + blocked_navigations.append(response.url) + except Exception: + pass + + page.on("response", _note_blocked) try: try: await page.goto(url, wait_until="domcontentloaded", timeout=30000) except Exception as e: raise BrowserNavigationError(classify_navigation_error(e), str(e)[:500]) from e await page.wait_for_timeout(1500) # let client-side rendering settle + if blocked_navigations: + raise UnsafeURLError( + "BLOCKED_PRIVATE_ADDRESS", + "The page navigated to a non-public address, which was blocked", + ) text = await page.inner_text("body") return {"url": page.url, "title": await page.title(), "text": text} finally: @@ -493,6 +590,12 @@ async def shutdown(self) -> None: except Exception: pass self._playwright = None + if self._egress_proxy: + try: + await self._egress_proxy.stop() + except Exception: + pass + self._egress_proxy = None # ------------------------------------------------------------------ # Reconnection (serverless / cold-start recovery) diff --git a/workspace/backend/app/main.py b/workspace/backend/app/main.py index 042436322..5c083ab1c 100644 --- a/workspace/backend/app/main.py +++ b/workspace/backend/app/main.py @@ -17,7 +17,7 @@ from starlette.middleware.base import BaseHTTPMiddleware from app.config import config -from app.routers import account, browser, cloud_agents, devices, events, files, knowledge, network, notifications, routines, shares, timers, todos, workspaces +from app.routers import account, browser, cloud_agents, devices, events, fetch, files, knowledge, network, notifications, routines, search, shares, timers, todos, workspaces logging.basicConfig(level=logging.INFO) logger = logging.getLogger(__name__) @@ -501,11 +501,13 @@ async def _log_validation_errors(request: Request, exc: RequestValidationError): app.include_router(cloud_agents.router) app.include_router(devices.router) app.include_router(events.router) +app.include_router(fetch.router) app.include_router(files.router) app.include_router(knowledge.router) app.include_router(network.router) app.include_router(notifications.router) app.include_router(routines.router) +app.include_router(search.router) app.include_router(shares.router) app.include_router(todos.router) app.include_router(timers.router) diff --git a/workspace/backend/tests/test_browser_quota.py b/workspace/backend/tests/test_browser_quota.py deleted file mode 100644 index 7cfc66f5f..000000000 --- a/workspace/backend/tests/test_browser_quota.py +++ /dev/null @@ -1,110 +0,0 @@ -# -*- coding: utf-8 -*- -"""Tests for per-workspace browser tab quota and the idle-tab reaper.""" - -import asyncio -from datetime import datetime, timedelta, timezone -from unittest.mock import AsyncMock, MagicMock, patch - -from app.models import BrowserTab -from app.routers.browser import reap_idle_tabs -from tests.conftest import TestingSessionLocal - - -def _create_workspace(client, name="Quota Test Workspace"): - resp = client.post("/v1/workspaces", json={ - "name": name, "agent_name": "agent-quota", "creator_email": "test@example.com", - }) - assert resp.status_code == 200 - data = resp.json()["data"] - return {"id": data["workspaceId"], "token": data["token"]} - - -def _mock_manager(): - manager = MagicMock() - manager.is_cloud = False - manager.get_session_id.return_value = None - manager.get_live_url.return_value = None - manager.open_tab = AsyncMock(return_value={"url": "https://example.com", "title": "Example"}) - manager.close_tab = AsyncMock() - return manager - - -def _open_tab(client, workspace, url="https://example.com"): - return client.post("/v1/browser/tabs", json={ - "url": url, "network": workspace["id"], "source": "openagents:agent-quota", - }, headers={"X-Workspace-Token": workspace["token"]}) - - -class TestWorkspaceQuota: - @patch("app.routers.browser.MAX_TABS_PER_WORKSPACE", 2) - @patch("app.routers.browser.BrowserManager") - def test_quota_enforced_per_workspace(self, mock_bm, client): - mock_bm.get.return_value = _mock_manager() - mock_bm.provision_workspace_key = AsyncMock(return_value=None) - ws = _create_workspace(client) - assert _open_tab(client, ws).status_code == 200 - assert _open_tab(client, ws).status_code == 200 - resp = _open_tab(client, ws) - assert resp.status_code == 400 - data = resp.json()["data"] - assert data["error_code"] == "BROWSER_QUOTA_EXCEEDED" - assert len(data["open_tabs"]) == 2 - assert "hint" in data - - @patch("app.routers.browser.MAX_TABS_PER_WORKSPACE", 1) - @patch("app.routers.browser.BrowserManager") - def test_quota_does_not_leak_across_workspaces(self, mock_bm, client): - mock_bm.get.return_value = _mock_manager() - mock_bm.provision_workspace_key = AsyncMock(return_value=None) - ws_a = _create_workspace(client, "A") - ws_b = _create_workspace(client, "B") - assert _open_tab(client, ws_a).status_code == 200 - assert _open_tab(client, ws_a).status_code == 400 - assert _open_tab(client, ws_b).status_code == 200 # B unaffected - - @patch("app.routers.browser.MAX_TABS_PER_WORKSPACE", 1) - @patch("app.routers.browser.BrowserManager") - def test_closed_tab_frees_slot(self, mock_bm, client): - mock_bm.get.return_value = _mock_manager() - mock_bm.provision_workspace_key = AsyncMock(return_value=None) - ws = _create_workspace(client) - tab = _open_tab(client, ws).json()["data"] - assert _open_tab(client, ws).status_code == 400 - resp = client.delete(f"/v1/browser/tabs/{tab['id']}", headers={"X-Workspace-Token": ws["token"]}) - assert resp.status_code == 200 - assert _open_tab(client, ws).status_code == 200 - - -class TestIdleReaper: - @patch("app.routers.browser.BrowserManager") - def test_reaper_closes_idle_tabs_only(self, mock_bm, client): - manager = _mock_manager() - mock_bm.get.return_value = manager - mock_bm.provision_workspace_key = AsyncMock(return_value=None) - ws = _create_workspace(client) - idle = _open_tab(client, ws, "https://idle.example.com").json()["data"] - fresh = _open_tab(client, ws, "https://fresh.example.com").json()["data"] - - db = TestingSessionLocal() - db.get(BrowserTab, idle["id"]).last_active_at = datetime.now(timezone.utc) - timedelta(hours=1) - db.commit() - db.close() - - closed = asyncio.run(reap_idle_tabs(session_factory=TestingSessionLocal)) - assert closed == 1 - db = TestingSessionLocal() - assert db.get(BrowserTab, idle["id"]).status == "closed" - assert db.get(BrowserTab, fresh["id"]).status == "active" - db.close() - manager.close_tab.assert_awaited() - - @patch("app.routers.browser.BrowserManager") - def test_reaper_noop_when_nothing_idle(self, mock_bm, client): - manager = _mock_manager() - mock_bm.get.return_value = manager - mock_bm.provision_workspace_key = AsyncMock(return_value=None) - ws = _create_workspace(client) - _open_tab(client, ws) - closed = asyncio.run(reap_idle_tabs(session_factory=TestingSessionLocal)) - assert closed == 0 - manager.close_tab.assert_not_awaited() diff --git a/workspace/backend/tests/test_ssrf_boundary.py b/workspace/backend/tests/test_ssrf_boundary.py index d5716df3d..fbb139c33 100644 --- a/workspace/backend/tests/test_ssrf_boundary.py +++ b/workspace/backend/tests/test_ssrf_boundary.py @@ -871,6 +871,36 @@ def test_open_tab_rejects_internal_url(self, client, url): "BLOCKED_PRIVATE_ADDRESS", "UNSUPPORTED_SCHEME", "BLOCKED_PORT", ) + @pytest.mark.parametrize("url", [ + "http://169.254.169.254/latest/meta-data/iam/security-credentials/", + "http://127.0.0.1:8000/v1/workspaces", + "file:///etc/passwd", + ]) + def test_navigate_rejects_internal_url(self, client, url): + """Opening a tab on a public page and then steering it somewhere + internal is the same capability as opening it there directly, so + navigate needs its own guard rather than relying on open's.""" + workspace = _create_workspace(client) + with patch("app.browser.BrowserManager.open_tab", + new=AsyncMock(return_value={"url": "https://example.com/", "title": "ok"})): + opened = client.post("/v1/browser/tabs", json={ + "url": "https://example.com/", + "network": workspace["id"], + "source": "openagents:agent-ssrf", + }, headers={"X-Workspace-Token": workspace["token"]}) + assert opened.status_code == 200 + tab_id = opened.json()["data"]["id"] + + resp = client.post(f"/v1/browser/tabs/{tab_id}/navigate", json={ + "url": url, + "network": workspace["id"], + "source": "openagents:agent-ssrf", + }, headers={"X-Workspace-Token": workspace["token"]}) + assert resp.status_code == 400 + assert resp.json()["data"]["error_code"] in ( + "BLOCKED_PRIVATE_ADDRESS", "UNSUPPORTED_SCHEME", "BLOCKED_PORT", + ) + def test_open_tab_without_url_still_works(self, client): # about:blank is the default a tab opens on; the guard must not break it. workspace = _create_workspace(client) From 715589a14c7273e0fe31a28499caba04bf6e306c Mon Sep 17 00:00:00 2001 From: QuanCheng <915158214@qq.com> Date: Wed, 29 Jul 2026 10:39:52 +0000 Subject: [PATCH 06/10] strip a trailing blank line at the end of the image search tests Flagged by git diff --check as a new blank line at EOF. --- workspace/backend/tests/test_search_images.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/workspace/backend/tests/test_search_images.py b/workspace/backend/tests/test_search_images.py index 6a836933c..6558d55fb 100644 --- a/workspace/backend/tests/test_search_images.py +++ b/workspace/backend/tests/test_search_images.py @@ -268,5 +268,3 @@ def test_html_forced_to_attachment(self, client): resp = self._download(client, workspace, fid) assert resp.status_code == 200 assert resp.headers["content-disposition"].startswith("attachment") - - From 2d8a43204847b3596172b3cc078ad5cb107a7d5f Mon Sep 17 00:00:00 2001 From: QuanCheng <915158214@qq.com> Date: Fri, 31 Jul 2026 03:41:11 +0000 Subject: [PATCH 07/10] converge the outbound UA and stop advertising Linux MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /v1/fetch and /v1/files/from_url each carried their own copy of the User-Agent string, so a page read and the image download that follows it could drift apart. Both now use OUTBOUND_USER_AGENT from app.net_security, which lives next to safe_fetch because every agent-triggered outbound fetch has to send the same one. The platform token also changes from "X11; Linux x86_64" to Windows. mp.weixin.qq.com answers non-browser clients with a "当前环境异常" interstitial rather than the article, and it counts the Linux token as one of them. Measured four requests per UA from a single IP, the article came back 0/4 with the Linux token and 4/4 with Windows or macOS. The trailing OpenAgentsFetch/1.0 product token is not what triggers it (3/3 with it on Windows), so we keep it and stay honest about who is calling. --- workspace/backend/app/net_security.py | 17 +++++++++++++++++ workspace/backend/app/routers/fetch.py | 12 +++++++----- workspace/backend/app/routers/files.py | 8 +++----- workspace/backend/tests/test_fetch.py | 26 ++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 10 deletions(-) diff --git a/workspace/backend/app/net_security.py b/workspace/backend/app/net_security.py index f608153c9..faacbde46 100644 --- a/workspace/backend/app/net_security.py +++ b/workspace/backend/app/net_security.py @@ -42,6 +42,23 @@ ALLOWED_SCHEMES = {"http", "https"} DEFAULT_MAX_REDIRECTS = 4 +# The User-Agent for every agent-triggered outbound fetch. Defined here, next +# to safe_fetch, because both callers (/v1/fetch and /v1/files/from_url) must +# send the same one: a page read and the image download that follows it come +# from the same logical client, and two drifting strings mean a site can serve +# the article but refuse its images. +# +# The platform token is load-bearing, not cosmetic. mp.weixin.qq.com serves a +# "当前环境异常" interstitial instead of the article to non-browser agents, and +# it treats "X11; Linux x86_64" as one of them: measured over four requests +# each from the same IP, the Linux token was blocked 4/4 while the Windows and +# macOS tokens succeeded 4/4. The trailing product token is fine to keep (3/3 +# with it), so we stay honest about who is calling. +OUTBOUND_USER_AGENT = ( + "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) " + "Chrome/126.0.0.0 Safari/537.36 OpenAgentsFetch/1.0" +) + def _parse_port_allowlist() -> frozenset: """Destination ports agents may reach. Restricting this stops the fetch diff --git a/workspace/backend/app/routers/fetch.py b/workspace/backend/app/routers/fetch.py index 7899e1b0f..9625f5294 100644 --- a/workspace/backend/app/routers/fetch.py +++ b/workspace/backend/app/routers/fetch.py @@ -31,7 +31,12 @@ from app.browser import BrowserManager, BrowserNavigationError, classify_navigation_error from app.database import get_db -from app.net_security import UnsafeURLError, safe_fetch, validate_public_url +from app.net_security import ( + OUTBOUND_USER_AGENT, + UnsafeURLError, + safe_fetch, + validate_public_url, +) from app.response import ResponseCode, json_response, success_response from app.routers.browser import _resolve_bf_key from app.routers.network import _resolve_workspace, _verify_workspace_access @@ -43,10 +48,7 @@ STATIC_TIMEOUT_SECONDS = 15.0 MAX_RESPONSE_BYTES = 2 * 1024 * 1024 DEFAULT_MAX_CHARS = 20000 -USER_AGENT = ( - "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) " - "Chrome/126.0.0.0 Safari/537.36 OpenAgentsFetch/1.0" -) +USER_AGENT = OUTBOUND_USER_AGENT # A static fetch that yields less text than this (from a large HTML payload) # is treated as a JS shell and escalated to browser rendering. diff --git a/workspace/backend/app/routers/files.py b/workspace/backend/app/routers/files.py index cf6a696bd..4a004867a 100644 --- a/workspace/backend/app/routers/files.py +++ b/workspace/backend/app/routers/files.py @@ -34,7 +34,7 @@ from app.config import config from app.database import get_db from app.file_types import FILTER_GROUPS, KIND_GROUPS, kind_for -from app.net_security import UnsafeURLError, safe_fetch +from app.net_security import OUTBOUND_USER_AGENT, UnsafeURLError, safe_fetch from app.models import FileRecord, Workspace from app.response import ResponseCode, json_response, success_response from app.routers.network import ( @@ -559,10 +559,8 @@ async def upload_file_base64( # --------------------------------------------------------------------------- DOWNLOAD_TIMEOUT_SECONDS = 30.0 -_DOWNLOAD_UA = ( - "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) " - "Chrome/126.0.0.0 Safari/537.36 OpenAgentsFetch/1.0" -) +# Shared with /v1/fetch — see OUTBOUND_USER_AGENT for why the two must match. +_DOWNLOAD_UA = OUTBOUND_USER_AGENT @router.post("/files/from_url") diff --git a/workspace/backend/tests/test_fetch.py b/workspace/backend/tests/test_fetch.py index bbeed614e..900dc7ea0 100644 --- a/workspace/backend/tests/test_fetch.py +++ b/workspace/backend/tests/test_fetch.py @@ -11,6 +11,7 @@ import pytest from app.browser import BrowserNavigationError +from app.net_security import OUTBOUND_USER_AGENT from app.routers.fetch import _detect_wall, _extract_text, _looks_like_js_shell @@ -225,3 +226,28 @@ def test_wall_detection(self): assert _detect_wall("A long normal article. " * 200, "News") is None # "sign in" link on a long page must not trigger the wall assert _detect_wall("please sign in " + "content " * 400, "News") is None + + +class TestOutboundUserAgent: + """The UA is a compatibility contract with the sites we read, not a detail. + + Regression cover for two separate defects: the page reader and the file + downloader drifting apart, and the Linux platform token that mp.weixin.qq.com + answers with a "当前环境异常" interstitial instead of the article. + """ + + def test_fetch_and_download_send_the_same_ua(self): + from app.routers.files import _DOWNLOAD_UA + from app.routers.fetch import USER_AGENT + + assert USER_AGENT == _DOWNLOAD_UA == OUTBOUND_USER_AGENT + + def test_ua_does_not_advertise_linux(self): + assert "Linux" not in OUTBOUND_USER_AGENT + assert "X11" not in OUTBOUND_USER_AGENT + + def test_ua_looks_like_a_browser(self): + # Non-Mozilla prefixes (curl/..., python-requests/...) are refused + # outright by the same interstitial. + assert OUTBOUND_USER_AGENT.startswith("Mozilla/5.0 ") + assert "Chrome/" in OUTBOUND_USER_AGENT From 10fa801484b067b746d18df6d3ef5bf34cf4880d Mon Sep 17 00:00:00 2001 From: QuanCheng <915158214@qq.com> Date: Fri, 31 Jul 2026 04:03:34 +0000 Subject: [PATCH 08/10] return the images a page read saw /v1/fetch returned text only, so the direct image URLs were gone by the time an agent wanted them. /v1/files/from_url needs a direct URL to save one, which made the image path unreachable in practice no matter what the prompt said. The read now carries an assets[] of absolute image URLs with type, guessed mime, alt text and where each was found. Three shapes are needed to cover real pages, each verified against captured responses rather than guessed: - lazy-loading markup, where the URL is in data-src and there is no src at all (every mp.weixin.qq.com article image) - embedded JSON, where a gallery never reaches the DOM and the blob writes each path separator as an escape sequence (xiaohongshu keeps note images in imageList[].urlDefault inside a script tag) - the rendered tier, which now enumerates document.images so a browser read reports the same assets a static read extracts An img tag is treated as an image whatever its path looks like. Filtering markup by file extension is what dropped these two platforms outright, since neither puts one on its CDN URLs. The extension test survives only for the JSON sweep, where a bare "url" key on a host named fe-static was otherwise matching JavaScript bundles. og:title now fills in when title is empty, which is how mp.weixin.qq.com ships every article. --- workspace/backend/app/browser.py | 35 +++- workspace/backend/app/routers/fetch.py | 213 +++++++++++++++++++++++-- workspace/backend/tests/test_fetch.py | 164 ++++++++++++++++++- 3 files changed, 400 insertions(+), 12 deletions(-) diff --git a/workspace/backend/app/browser.py b/workspace/backend/app/browser.py index feb71ca00..7662760db 100644 --- a/workspace/backend/app/browser.py +++ b/workspace/backend/app/browser.py @@ -33,6 +33,17 @@ RENDER_SETTLE_ATTEMPTS = int(os.environ.get("RENDER_SETTLE_ATTEMPTS", "3")) RENDER_MIN_TEXT_CHARS = int(os.environ.get("RENDER_MIN_TEXT_CHARS", "50")) +# Enumerate the images the DOM actually resolved, so a rendered read can hand +# back the same assets a static read extracts from markup. currentSrc is what +# the browser picked out of any srcset; src is the fallback before layout. +# Bounded here rather than in the caller so a pathological page can't return a +# multi-megabyte list across the process boundary. +RENDER_IMAGE_JS = ( + "Array.from(document.images).slice(0, 60)" + ".map(function (i) { return {url: i.currentSrc || i.src || '', alt: i.alt || ''}; })" + ".filter(function (x) { return x.url; })" +) + # Chromium's own sandbox is a second containment layer under the egress proxy: # it is what keeps a renderer compromise (from a page an agent chose) inside # the renderer process. It needs a non-root user plus unprivileged user @@ -517,7 +528,22 @@ async def render_page_text(self, url: str, api_key: str = None) -> dict: break info = await self._bf_call("get_page_info", {}, session_id, api_key=key) page_info = info.get("result", {}) - return {"url": page_info.get("url", url), "title": page_info.get("title", ""), "text": text} + images = [] + try: + shot = await self._bf_call( + "evaluate_js", {"expression": RENDER_IMAGE_JS}, session_id, api_key=key + ) + images = shot.get("result", {}).get("result") or [] + except Exception as e: + # Images are a bonus on top of the text read; never fail + # the render because the page wouldn't enumerate them. + logger.debug("Image enumeration failed for %s: %s", url, e) + return { + "url": page_info.get("url", url), + "title": page_info.get("title", ""), + "text": text, + "images": images, + } finally: try: await self._bf_call("close_session", {}, session_id, api_key=key) @@ -567,7 +593,12 @@ def _note_blocked(response): "The page navigated to a non-public address, which was blocked", ) text = await page.inner_text("body") - return {"url": page.url, "title": await page.title(), "text": text} + try: + images = await page.evaluate(RENDER_IMAGE_JS) + except Exception as e: + logger.debug("Image enumeration failed for %s: %s", url, e) + images = [] + return {"url": page.url, "title": await page.title(), "text": text, "images": images} finally: try: await page.close() diff --git a/workspace/backend/app/routers/fetch.py b/workspace/backend/app/routers/fetch.py index 9625f5294..e7280995e 100644 --- a/workspace/backend/app/routers/fetch.py +++ b/workspace/backend/app/routers/fetch.py @@ -23,6 +23,7 @@ import time from html.parser import HTMLParser from typing import Optional +from urllib.parse import urljoin, urlparse import httpx from fastapi import APIRouter, Depends, Header @@ -83,9 +84,95 @@ ) +# --- image asset extraction ------------------------------------------------- +# +# Reading a page and downloading its images are two different endpoints, and +# only this one sees the markup. If the read returns text alone, the direct +# image URLs are gone by the time the agent wants them and +# POST /v1/files/from_url — which requires a direct URL — is unreachable in +# practice. So the read has to hand back what it saw. + +ASSET_LIMIT = 60 + +# src is the easy case. The data-* attributes are what lazy-loading pages use +# instead, and mp.weixin.qq.com is one of them: every article image sits in +# data-src with no src at all, so a src-only reader reports zero images for a +# page full of them. +_IMG_URL_ATTRS = ("src", "data-src", "data-original", "data-lazy-src", "data-actualsrc") + +_IMAGE_EXTENSIONS = (".jpg", ".jpeg", ".png", ".gif", ".webp", ".bmp", ".avif", ".ico", ".svg") + +# Checked before anything else. The host-shape arm below would otherwise +# accept a bundle off a CDN named "fe-static" as an image purely because its +# key was "url" — a real hit on xiaohongshu's note pages. +_NON_IMAGE_EXTENSIONS = ( + ".js", ".mjs", ".css", ".json", ".xml", ".txt", ".map", + ".woff", ".woff2", ".ttf", ".otf", ".eot", + ".mp4", ".webm", ".m3u8", ".ts", ".mp3", ".m4a", ".wav", + ".pdf", ".zip", ".gz", ".html", ".htm", +) + +_MIME_BY_EXTENSION = { + ".jpg": "image/jpeg", ".jpeg": "image/jpeg", ".png": "image/png", + ".gif": "image/gif", ".webp": "image/webp", ".bmp": "image/bmp", + ".avif": "image/avif", ".ico": "image/x-icon", ".svg": "image/svg+xml", +} + +# Some sites build the gallery from an embedded JSON blob and never emit an +# for it — xiaohongshu keeps its note images under +# imageList[].urlDefault, inside a " + ) + assets = _extract_text(html, "https://www.xiaohongshu.com/explore/x")["assets"] + assert [a["url"] for a in assets] == [ + "http://sns-webpic-qc.xhscdn.com/202607/abc/note!nd_dft" + ] + assert assets[0]["source"] == "embedded_json" + + def test_non_image_assets_from_json_are_rejected(self): + html = ( + '' + ) + assert _extract_text(html, "https://www.xiaohongshu.com/")["assets"] == [] + + def test_relative_and_protocol_relative_urls_are_absolute(self): + html = '' + urls = [a["url"] for a in _extract_text(html, "https://site.example/post/1")["assets"]] + assert urls == ["https://site.example/img/a.png", "https://cdn.example.com/b.jpg"] + + def test_inline_data_uris_and_duplicates_are_dropped(self): + html = ( + '' + '' + '' + ) + assets = _extract_text(html, "https://example.com/")["assets"] + assert [a["url"] for a in assets] == ["https://cdn.example.com/a.jpg"] + + def test_asset_list_is_capped(self): + html = "" + "".join( + f'' for i in range(200) + ) + "" + assert len(_extract_text(html, "https://example.com/")["assets"]) == ASSET_LIMIT + + def test_og_title_fills_in_for_an_empty_title_tag(self): + html = ( + '' + 'x' + ) + assert _extract_text(html, "https://mp.weixin.qq.com/s/x")["title"] == "真正的标题" + + def test_real_title_tag_wins_over_og_title(self): + html = ( + 'Real' + 'x' + ) + assert _extract_text(html, "https://example.com/")["title"] == "Real" + + +class TestRenderedAssets: + + def test_rendered_images_are_normalized_and_deduped(self): + assets = _normalize_rendered_assets( + [ + {"url": "https://cdn.example.com/a.png", "alt": "A"}, + {"url": "https://cdn.example.com/a.png", "alt": "dup"}, + {"url": "data:image/png;base64,AAA", "alt": ""}, + {"url": "/rel/b.jpg", "alt": ""}, + "not-a-dict", + {"alt": "no url"}, + ], + "https://site.example/page", + ) + assert [a["url"] for a in assets] == [ + "https://cdn.example.com/a.png", + "https://site.example/rel/b.jpg", + ] + assert all(a["source"] == "browser" for a in assets) + + def test_missing_image_list_is_not_an_error(self): + assert _normalize_rendered_assets(None, "https://site.example/") == [] From 3fa91f57df7611e1f35a10de0246d7494812d180 Mon Sep 17 00:00:00 2001 From: QuanCheng <915158214@qq.com> Date: Fri, 31 Jul 2026 04:48:58 +0000 Subject: [PATCH 09/10] classify refusals by platform instead of one marker list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every wall came back as AUTH_REQUIRED or BOT_CHALLENGE with the same hint, which produced advice that cannot work — telling a user to sign in past a block that ignores logins, or to re-send a link that was never the problem. _detect_wall now returns an error code with the hint that belongs to it, and four codes are added. CLIENT_ENV_BLOCKED mp.weixin.qq.com decided the caller is not a browser. No user action exists; it is ours. SHARE_TOKEN_REQUIRED xiaohongshu only serves a note to a link carrying xsec_token, which app share links already have. CONTENT_UNAVAILABLE the same xiaohongshu page, but the link did carry a token, so the note is deleted rather than under-parameterised. IP_OR_REGION_BLOCKED zhihu refused at the network level. The xiaohongshu pair matters because both states render the identical "你访问的页面不见了" body and only the URL distinguishes them. Treating that string as unconditional proof of a missing token would tell people to re-share links that will fail again. Matching is per host. The generic lists are substring-matched against every page we read, so a phrase added for one site would be free to misclassify another site's article. Tests run the classifier over captured responses rather than invented wording, under tests/fixtures/cn_walls. Zhihu is stored twice on purpose, because its static 403 body carries no refusal text at all and only the rendered body does — a fixture set with just the JSON would suggest static classification works for it, and it does not. Challenge values and trace ids are redacted; no cookies or share tokens are kept. --- workspace/backend/app/routers/fetch.py | 116 ++++++++++++++---- .../backend/tests/fixtures/cn_walls/README.md | 22 ++++ .../fixtures/cn_walls/weixin_env_blocked.html | 38 ++++++ .../cn_walls/xiaohongshu_no_token.html | 1 + .../cn_walls/zhihu_challenge_shell.html | 2 + .../fixtures/cn_walls/zhihu_rate_limited.json | 1 + workspace/backend/tests/test_fetch.py | 70 ++++++++++- 7 files changed, 225 insertions(+), 25 deletions(-) create mode 100644 workspace/backend/tests/fixtures/cn_walls/README.md create mode 100644 workspace/backend/tests/fixtures/cn_walls/weixin_env_blocked.html create mode 100644 workspace/backend/tests/fixtures/cn_walls/xiaohongshu_no_token.html create mode 100644 workspace/backend/tests/fixtures/cn_walls/zhihu_challenge_shell.html create mode 100644 workspace/backend/tests/fixtures/cn_walls/zhihu_rate_limited.json diff --git a/workspace/backend/app/routers/fetch.py b/workspace/backend/app/routers/fetch.py index e7280995e..2b4979a14 100644 --- a/workspace/backend/app/routers/fetch.py +++ b/workspace/backend/app/routers/fetch.py @@ -12,9 +12,12 @@ AUTH_REQUIRED so the agent can open a shared browser tab and let a human take over. -Errors carry a stable error_code: +Errors carry a stable error_code, so the caller can ask for the one action +that would actually help instead of a generic "fetch failed": JS_RENDER_TIMEOUT | AUTH_REQUIRED | BOT_CHALLENGE | DNS_OR_TLS_ERROR | - CONTENT_BLOCKED | NAVIGATION_FAILED | UNSUPPORTED_CONTENT + CONTENT_BLOCKED | NAVIGATION_FAILED | UNSUPPORTED_CONTENT | + CLIENT_ENV_BLOCKED | SHARE_TOKEN_REQUIRED | CONTENT_UNAVAILABLE | + IP_OR_REGION_BLOCKED """ import logging @@ -335,19 +338,94 @@ def _looks_like_js_shell(html: str, extracted: dict) -> bool: return len(html) > 10000 and len(extracted["text"]) < JS_SHELL_MIN_TEXT_CHARS -def _detect_wall(text: str, title: str) -> Optional[str]: - """Return AUTH_REQUIRED / BOT_CHALLENGE if the page is a wall, else None. +# One line per code, so the caller can tell the user what happened without +# reading the hint, and so "login wall or bot challenge" stops being the +# message for refusals that are neither. +_WALL_MESSAGES = { + "AUTH_REQUIRED": "The page is behind a login wall", + "BOT_CHALLENGE": "The page is behind a bot challenge", + "CLIENT_ENV_BLOCKED": "The site rejected this client and served a verification page", + "SHARE_TOKEN_REQUIRED": "The page needs the share token that comes with the app link", + "CONTENT_UNAVAILABLE": "The content is deleted, private, or no longer public", + "IP_OR_REGION_BLOCKED": "The site refused this request at the network level", +} + +_GENERIC_BOT_HINT = ( + "Open the URL in the shared browser (workspace_browser_open) and ask a human " + "to complete the challenge there." +) +_GENERIC_AUTH_HINT = ( + "Open the URL in the shared browser (workspace_browser_open) and ask a human " + "to sign in there." +) + + +def _host_matches(host: str, domains: tuple) -> bool: + return any(host == d or host.endswith("." + d) for d in domains) + + +def _detect_platform_wall(host: str, sample: str, url: str) -> Optional[tuple]: + """Classify refusals whose wording only means something on one platform. + + Kept off the generic marker lists on purpose. Those are substring-matched + against every page we read, so a phrase added here for one site would be + free to misclassify another site's perfectly good article. + """ + if _host_matches(host, ("weixin.qq.com",)) and any( + m in sample for m in ("当前环境异常", "请在微信客户端打开") + ): + # Nothing a user can do: WeChat decided the caller isn't a browser. + return ("CLIENT_ENV_BLOCKED", ( + "WeChat served an environment check instead of the article. This is an " + "outbound-client problem, not a user action — check that " + "OUTBOUND_USER_AGENT still presents a desktop browser." + )) + + if _host_matches(host, ("xiaohongshu.com", "xhslink.com")) and "你访问的页面不见了" in sample: + # Same page for "you didn't bring a share token" and "this note is + # gone", so the URL decides which. Asking a user to re-share a link + # that already had a token would just fail again. + if "xsec_token=" in url: + return ("CONTENT_UNAVAILABLE", ( + "The note is deleted or private. The link already carried a share " + "token, so this is not a missing-parameter problem — do not ask the " + "user to send it again." + )) + return ("SHARE_TOKEN_REQUIRED", ( + "Xiaohongshu only serves a note to a link carrying xsec_token. Ask the " + "user for the full link shared from the app, not a trimmed " + "/explore/ URL." + )) + + if _host_matches(host, ("zhihu.com",)) and any( + m in sample for m in ("请求存在异常", "安全验证") + ): + return ("IP_OR_REGION_BLOCKED", ( + "Zhihu refused this request at the network level, which signing in does " + "not by itself fix. Do not prompt the user to log in until an egress " + "path has been verified." + )) + + return None + + +def _detect_wall(text: str, title: str, url: str = "") -> Optional[tuple]: + """Return (error_code, hint) if the page is a refusal rather than content. Markers alone are too false-positive-prone (any page with a login link mentions "sign in"), so only classify short pages as walls. """ if len(text) > 1500: return None - sample = (title + " " + text[:1500]).lower() - if any(marker in sample for marker in _BOT_MARKERS): - return "BOT_CHALLENGE" - if any(marker in sample for marker in _AUTH_MARKERS): - return "AUTH_REQUIRED" + sample = title + " " + text[:1500] + platform = _detect_platform_wall(urlparse(url).netloc.lower(), sample, url) + if platform: + return platform + lowered = sample.lower() + if any(marker in lowered for marker in _BOT_MARKERS): + return ("BOT_CHALLENGE", _GENERIC_BOT_HINT) + if any(marker in lowered for marker in _AUTH_MARKERS): + return ("AUTH_REQUIRED", _GENERIC_AUTH_HINT) return None @@ -482,7 +560,7 @@ async def fetch_url( if static_result is not None: extracted = _extract_text(static_result["html"], static_result["final_url"]) - wall = _detect_wall(extracted["text"], extracted["title"]) + wall = _detect_wall(extracted["text"], extracted["title"], static_result["final_url"]) upstream_error = static_result["status_code"] >= 400 needs_render = ( upstream_error @@ -491,12 +569,8 @@ async def fetch_url( ) if body.mode == "static" or not needs_render: if wall: - return _error( - ResponseCode.BAD_REQUEST, - "The page is behind a login wall or bot challenge", - wall, - hint="Open the URL in the shared browser (workspace_browser_open) and ask a human to complete the login there.", - ) + code, hint = wall + return _error(ResponseCode.BAD_REQUEST, _WALL_MESSAGES[code], code, hint=hint) # In static mode we don't escalate to a browser, so an upstream # 4xx/5xx must be reported as an error, not returned as success. if body.mode == "static" and upstream_error: @@ -542,14 +616,10 @@ async def fetch_url( return _error(ResponseCode.INTERNAL_ERROR, "Browser render failed", "NAVIGATION_FAILED") # ---- Tier 3: wall detection on the rendered page ---- - wall = _detect_wall(rendered["text"], rendered["title"]) + wall = _detect_wall(rendered["text"], rendered["title"], rendered["url"]) if wall: - return _error( - ResponseCode.BAD_REQUEST, - "The page is behind a login wall or bot challenge", - wall, - hint="Open the URL in the shared browser (workspace_browser_open) and ask a human to complete the login there.", - ) + code, hint = wall + return _error(ResponseCode.BAD_REQUEST, _WALL_MESSAGES[code], code, hint=hint) return _success(rendered["text"], "browser", rendered["url"], rendered["title"], max_chars, _normalize_rendered_assets(rendered.get("images"), rendered["url"])) diff --git a/workspace/backend/tests/fixtures/cn_walls/README.md b/workspace/backend/tests/fixtures/cn_walls/README.md new file mode 100644 index 000000000..bfc1fe51f --- /dev/null +++ b/workspace/backend/tests/fixtures/cn_walls/README.md @@ -0,0 +1,22 @@ +# Captured refusal pages + +Real responses, trimmed and redacted, from a datacenter IP with no cookies. +They exist so the wall classifier is tested against what these sites actually +send rather than against wording we imagined. + +| file | how it was produced | classifies as | +|---|---|---| +| `weixin_env_blocked.html` | `mp.weixin.qq.com` article requested with `curl/8.5.0` | `CLIENT_ENV_BLOCKED` | +| `xiaohongshu_no_token.html` | `xiaohongshu.com/explore/` with no `xsec_token` | `SHARE_TOKEN_REQUIRED` | +| `zhihu_challenge_shell.html` | `zhihu.com` question page, HTTP 403 static body | nothing — see below | +| `zhihu_rate_limited.json` | same URL through headless Chromium | `IP_OR_REGION_BLOCKED` | + +Zhihu is deliberately represented twice. Its static 403 body carries no +refusal wording at all — the visible text is a tagline — so the static tier +cannot classify it and correctly reports the upstream 403. The Chinese +refusal only appears once a browser runs the challenge, which is the body the +render tier sees. A classifier tested only against the JSON would look like it +handles Zhihu statically, and it does not. + +Redactions: the `zh-zse-ck` challenge value and per-request trace ids are +replaced with placeholders. No cookies or share tokens are stored here. diff --git a/workspace/backend/tests/fixtures/cn_walls/weixin_env_blocked.html b/workspace/backend/tests/fixtures/cn_walls/weixin_env_blocked.html new file mode 100644 index 000000000..4fd443797 --- /dev/null +++ b/workspace/backend/tests/fixtures/cn_walls/weixin_env_blocked.html @@ -0,0 +1,38 @@ +ws\sPhone/i.test(ua) || /(Android)/i.test(ua)); + setTimeout(() => { + noMobile && document.title === '' && (document.title = '微信公众平台'); + }, 1000); + })(); + + + + + + + + + + +
+ +
+ +
+
+

环境异常

+

当前环境异常,完成验证后即可继续访问。

+
+
+

+ 去验证 +

+
+
+ + + +