feat(browser): SSRF guard + proxy endpoint shell (PR 2/10)#301
Conversation
Three fixes from the whole-branch security review: - proxy.py / ssrf.py: 403 responses no longer echo the resolved IP back to the client. Reason was logged server-side instead. Without this, a malicious site could DNS-pin its hostname to internal IPs and read them from taOS's 403 responses, enumerating the user's LAN. Tightens the SSRF guard's purpose end-to-end. - ssrf.py: add fec0::/10 (RFC 3879 deprecated IPv6 site-local) to _BLOCKED_NETWORKS — Python's ipaddress doesn't classify it but it's still routable on legacy/embedded gear. - csp.py: add explicit connect-src / worker-src / frame-src 'self'. Browser fallback to default-src for these has historically been inconsistent — same justification we used for object-src 'none'.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds a secure browser proxy endpoint ( ChangesDesktop Browser Proxy with SSRF & CSP Security
Sequence DiagramsequenceDiagram
actor Client
participant Proxy as Proxy Endpoint<br/>(proxy.py)
participant Auth as Auth Gate<br/>(get_current_user)
participant SSRF as SSRF Validator<br/>(ssrf.py)
participant CSP as CSP Builder<br/>(csp.py)
participant Resp as Response
Client->>Proxy: GET /api/desktop/browser/proxy?url=...&profile_id=...
Proxy->>Auth: Check taos_session cookie
alt No Valid Session
Auth-->>Proxy: Unauthenticated
Proxy-->>Client: 401 Unauthorized
else Valid Session
Auth-->>Proxy: current_user dict
Proxy->>SSRF: validate_url_or_raise(url)
alt SSRF Blocked (scheme/hostname/IP)
SSRF-->>Proxy: raise SsrfBlockedError
Proxy-->>Client: 403 {error: "URL blocked"}
else URL Passes SSRF
SSRF-->>Proxy: validation success
Proxy->>CSP: proxied_response_csp()
CSP-->>Proxy: CSP header string
Proxy-->>Resp: Build 501 response
Resp-->>Client: 501 {error: "not yet implemented"}
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The SSRF validation logic is dense, requiring careful verification of IPv4/IPv6 parsing, IP range blocklists (loopback, RFC1918, link-local, multicast, CGNAT, deprecated site-local), DNS resolution handling, and encoded IP detection. The heterogeneous changes span authentication wiring, exception handling, CSP policy construction, and route registration. Test coverage is comprehensive but each test cohort validates distinct SSRF scenarios that demand independent reasoning.
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 35 minutes and 56 seconds.Comment |
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments: (None) Files Reviewed (7 files)
Fix these issues in Kilo Cloud Reviewed by grok-code-fast-1:optimized:free · 145,449 tokens |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if not parsed.hostname: | ||
| raise SsrfBlockedError("URL has no hostname") | ||
|
|
||
| host = parsed.hostname.lower() |
There was a problem hiding this comment.
WARNING: Hostname is not stripped of leading/trailing whitespace, which could allow potential bypasses if DNS resolves unexpected hostnames with spaces.
| host = parsed.hostname.lower() | |
| host = parsed.hostname.lower().strip() |
|
|
||
|
|
||
| # Hostname-suffix blocklist — applied before DNS resolution. | ||
| _BLOCKED_TLDS = (".local", ".onion", ".internal") |
There was a problem hiding this comment.
SUGGESTION: Consider expanding the blocked TLD list to include other common internal TLDs such as .home, .corp, .lan, etc., to enhance SSRF protection against internal domains.
| # that happen to be parseable as ints but aren't valid IPv4 (e.g. | ||
| # negative numbers, numbers > 0xFFFFFFFF). | ||
| try: | ||
| as_int = int(host, 0) |
There was a problem hiding this comment.
WARNING: If host is an extremely long string, int() may consume significant memory and time, potentially leading to DoS. Consider adding a length check before parsing.
| # not executed. | ||
| "img-src 'self' data: https:", | ||
| # Stylesheets may use data: for inline font references. | ||
| "style-src 'self' 'unsafe-inline' data:", |
There was a problem hiding this comment.
WARNING: Allowing 'unsafe-inline' for styles increases XSS risk, even though CSS is less dangerous than JS. Ensure this is strictly necessary for functionality.
| @router.get("/api/desktop/browser/proxy") | ||
| async def proxy_get( | ||
| profile_id: str, | ||
| url: str, |
There was a problem hiding this comment.
SUGGESTION: Consider adding validation for url parameter length to prevent potential DoS from extremely long URLs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tinyagentos/routes/desktop_browser/csp.py`:
- Around line 27-34: The CSP currently allows direct third-party fetches via the
directives "img-src 'self' data: https:" (and the similar "font-src ... https:"
occurrence around lines 55-57); remove the bare https: source from img-src and
font-src so both only allow 'self' and data: (and keep style-src as-is if it
only needs 'self' 'unsafe-inline' data:), thereby forcing external assets to be
rewritten/routed through the proxy rewriter; update both occurrences of the
offending strings in tinyagentos/routes/desktop_browser/csp.py accordingly.
In `@tinyagentos/routes/desktop_browser/proxy.py`:
- Around line 55-58: The log currently records the full user-supplied URL
(logging.getLogger(__name__).info line with url=%r), which can leak secrets;
update the block where url and exception e are available to parse the URL (use
urllib.parse.urlparse on the variable url), extract parsed.scheme and
parsed.hostname (and parsed.port only if needed), and log only those pieces plus
the reason (e) instead of the raw url; locate the
logging.getLogger(__name__).info call in proxy.py and replace the message to
include scheme and host (and optional port) rather than url=%r.
In `@tinyagentos/routes/desktop_browser/ssrf.py`:
- Around line 93-98: The code currently uses socket.gethostbyname_ex(host) in
the SSRF check (see the try block that sets _hostname, _aliases, addr_list and
raises SsrfBlockedError) which only returns IPv4 addresses; replace this with
socket.getaddrinfo(host, None, family=socket.AF_UNSPEC, type=socket.SOCK_STREAM)
(or SOCK_DGRAM) to retrieve both A and AAAA records, extract the IP addresses
from the returned tuples, and run your existing per-address validation on every
unique IP returned (instead of only addr_list); ensure you still catch
socket.gaierror and raise SsrfBlockedError with the error, and add tests for
IPv4-only, IPv6-only, and dual-stack hostnames to confirm all returned addresses
are validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f1e4d72-8f23-4962-ae7b-6de849f4daf5
📒 Files selected for processing (7)
tests/routes/desktop_browser/test_csp.pytests/routes/desktop_browser/test_proxy_shell.pytests/routes/desktop_browser/test_ssrf.pytinyagentos/routes/desktop_browser/__init__.pytinyagentos/routes/desktop_browser/csp.pytinyagentos/routes/desktop_browser/proxy.pytinyagentos/routes/desktop_browser/ssrf.py
CodeRabbit review feedback on the security-gate PR: - csp.py (Major): drop `https:` from img-src and font-src. Allowing bare https: lets the proxied page fetch external assets directly, leaking the user's real IP. External assets are rewritten by PR 3's rewriter to flow back through the proxy. - proxy.py (Major): SSRF block log no longer records the full URL — query strings can carry secrets. Log scheme + hostname only. - ssrf.py (Major): switch DNS resolution from gethostbyname_ex to getaddrinfo. The former returns A records only (IPv4); AAAA was silently ignored, so a hostname with public IPv4 + private IPv6 could bypass the guard. New test_rejects_when_only_ipv6_resolves _to_private case proves the dual-stack path is now checked. - ssrf.py (Warning): strip whitespace from hostnames before suffix check (defensive — urlparse usually does, but explicit is safer). - ssrf.py (Warning): expand _BLOCKED_TLDS with .home / .corp / .lan / .intranet (common internal-network conventions). Test renamed to test_rejects_internal_network_tlds; parametrize cases added for the new entries.
1 participant
Second of ten PRs implementing BrowserApp v2 per the design doc.
Summary
ssrf.py) — parses URL, resolves DNS, blocks RFC1918 / link-local / loopback / IPv6 alts (incl.::ffff:mapped) / multicast /.local/.onion/.internal/ decimal+octal+hex IP encodings / RFC 6598 CGNAT (100.64/10) / RFC 3879 IPv6 site-local (fec0::/10). Multi-record hostnames must pass ALL resolved addresses (defends against DNS pinning).csp.py) — builder for the strict Content-Security-Policy applied to proxied responses by PR 3. Explicitdefault-src 'self',script-src 'self'(no inline/eval),connect-src 'self',worker-src 'self',frame-src 'self',object-src 'none',base-uri 'self',form-action 'self',frame-ancestors 'self',upgrade-insecure-requests. Pragmaticstyle-src 'unsafe-inline'(CSS isn't an XSS vector and inline styles are universal in real-world HTML)./api/desktop/browser/proxyendpoint shell (proxy.py) — auth via existingDepends(get_current_user), SSRF check on every URL, returns 501 Not Implemented for valid URLs (the actual fetch + rewriter + cookie jar pipeline lands in PR 3). 403 responses do not leak resolved internal IPs back to the client (LAN-enumeration defence) — the detailed reason is logged server-side instead.What this does not land
Distinct-origin deferral
The original spec called for serving the proxy on a distinct origin (
browser.<host>) from v1. During PR 2 planning we found that the project does not currently have HTTPS / TLS / mDNS-wildcard infrastructure — adding all three to makebrowser.taos.localreachable would expand PR 2 well past its intended size and is genuinely a platform-wide change, not browser-specific. Decision: defer distinct-origin to a dedicated HTTPS + DNS Foundations PR (queued from the audit). PRs 2–9 ship at the same origin as the rest of the desktop; the strict CSP injection fromcsp.proxied_response_csp()provides partial defence in the meantime. PR 4 (frontend chrome) does not depend on origin separation.Test Plan
pytest tests/routes/desktop_browser/test_ssrf.py -v— 34 cases (parametrized) covering schemes, IPv4 blocklist (incl. CGNAT), IPv6 blocklist (incl. fec0::/10), hostname suffixes, encoded IP forms, DNS resolution failure, multi-record DNS-pinningpytest tests/routes/desktop_browser/test_csp.py -v— 10 tests verifying every strict directive plus the script-src-scoped no-inline assertionpytest tests/routes/desktop_browser/test_proxy_shell.py -v— 8 tests covering auth gate, SSRF gate, parameter validation, 501 stub, and the LAN-enumeration regression test (asserts 403 body does NOT contain resolved IPs)pytest tests/routes/desktop_browser/— 26 PR 1 tests + 52 new PR 2 tests = 78 total, all greenpytest tests/routes/ tests/test_secrets.py— 160 tests pass, no regressions outside scopeSpec
docs/superpowers/specs/2026-05-03-browser-app-v2-design.md§4.3, §9. (Local spec amended in §3.1 with the deferral note.)Cumulative shipping arc:
Notes for reviewers
ipaddress.IPv4Address.is_privatedoesn't include RFC 6598 (100.64/10), andipaddress.IPv6Addressdoesn't include RFC 3879 (fec0::/10). Both are explicitly listed in_BLOCKED_NETWORKSbecause they're routable on real consumer/enterprise gear.socket.gethostbyname_exis sync inside an async route. Acceptable for the gate-only PR 2; PR 3 should wrap withasyncio.to_thread(...)when serving real traffic.Summary by CodeRabbit
Release Notes
New Features
Tests