fix(browser): GET-form search through the proxy (query-clobber)#569
Merged
Conversation
A GET form submit replaces the action's query string with the form's own fields, wiping the query-encoded profile_id/url and 422-ing the proxy. Route GET forms via the bare proxy path with routing in reserved hidden inputs (__taos_pid/__taos_url) that survive the submit; the proxy resolves those and merges the remaining fields into the target URL (SSRF-revalidated, host unchanged). POST forms keep the query-encoded action (body carries fields). Rewriter now owns all <form> actions; _rewrite_attributes does href/src only. Tests: 4 rewriter + 4 proxy-route cases.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis pull request routes GET-form submissions through the desktop-browser proxy by rewriting forms to include reserved hidden routing inputs ( ChangesGET-form proxy routing
Sequence DiagramsequenceDiagram
participant Browser
participant Rewriter
participant ProxyGET
participant TargetServer
Browser->>Rewriter: fetches proxied page (HTML)
Rewriter->>Browser: returns rewritten HTML with hidden __taos_* inputs for GET forms
Browser->>ProxyGET: submits form (hidden routing inputs + user fields)
ProxyGET->>ProxyGET: extract/validate __taos_* or query params
ProxyGET->>ProxyGET: merge non-reserved user fields into target URL
ProxyGET->>TargetServer: forward GET to merged target URL
TargetServer-->>ProxyGET: respond
ProxyGET-->>Browser: return (possibly rewritten) response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tinyagentos/routes/desktop_browser/proxy.py (1)
73-83: ⚡ Quick winExtract shared routing constants to avoid drift.
The reserved field names (
__taos_pid,__taos_url) and proxy path are duplicated between this file andrewriter.py. If one side changes without the other, GET-form routing silently breaks.Consider extracting to a shared module (e.g.,
routing_constants.py) that both files import.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/desktop_browser/proxy.py` around lines 73 - 83, The reserved GET-form routing constants (_FORM_PID_FIELD, _FORM_URL_FIELD, _FORM_TAB_FIELD, and _RESERVED_QUERY_KEYS) are duplicated between proxy.py and rewriter.py; extract these symbols into a new shared module (e.g., routing_constants.py) and have both files import them instead of defining them locally so they cannot drift—create routing_constants.py exporting the four identifiers, update tinyagentos/routes/desktop_browser/proxy.py to import those names (remove the local definitions), and update rewriter.py to import the same names; ensure any references (e.g., usages of _FORM_PID_FIELD, _FORM_URL_FIELD, _FORM_TAB_FIELD, _RESERVED_QUERY_KEYS) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/routes/desktop_browser/test_proxy_post.py`:
- Around line 130-169: Add a regression test ensuring unprefixed site fields
named "url" and "profile_id" are not dropped by the proxy's reserved-key
filtering: create a test (e.g., test_preserves_site_fields_url_and_profile_id)
mirroring the existing GET tests that calls the endpoint
"/api/desktop/browser/proxy" with params including "__taos_pid"/"__taos_url"
plus additional unprefixed "url" and "profile_id" fields, use respx to mock the
upstream target and assert route.calls.last.request.url contains the forwarded
"url" and "profile_id" query values; this verifies behavior around the
_RESERVED_QUERY_KEYS handling in tinyagentos/routes/desktop_browser/proxy.py and
prevents collisions between prefixed hidden fields and real site fields.
---
Nitpick comments:
In `@tinyagentos/routes/desktop_browser/proxy.py`:
- Around line 73-83: The reserved GET-form routing constants (_FORM_PID_FIELD,
_FORM_URL_FIELD, _FORM_TAB_FIELD, and _RESERVED_QUERY_KEYS) are duplicated
between proxy.py and rewriter.py; extract these symbols into a new shared module
(e.g., routing_constants.py) and have both files import them instead of defining
them locally so they cannot drift—create routing_constants.py exporting the four
identifiers, update tinyagentos/routes/desktop_browser/proxy.py to import those
names (remove the local definitions), and update rewriter.py to import the same
names; ensure any references (e.g., usages of _FORM_PID_FIELD, _FORM_URL_FIELD,
_FORM_TAB_FIELD, _RESERVED_QUERY_KEYS) remain unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb1de7f1-83e8-48aa-94b4-f120da91c853
📒 Files selected for processing (4)
tests/routes/desktop_browser/test_proxy_post.pytests/routes/desktop_browser/test_rewriter.pytinyagentos/routes/desktop_browser/proxy.pytinyagentos/routes/desktop_browser/rewriter.py
…lide Address CodeRabbit: when a GET form submit carries the reserved __taos_* routing fields, treat ANY plain url/profile_id/tab_id in the query as the site's own form data (merge into target) instead of consuming them as taOS routing or stripping them. Direct navigation still uses the plain params. Drop the now-unused _RESERVED_QUERY_KEYS. Adds a collision regression test (site form fields literally named url + profile_id are forwarded intact).
The proxy route's profile_id/url are now Optional (to allow the __taos_* GET-form fallback), so missing routing params return a clear 400 from the route's own validation rather than FastAPI's generic 422. Update the two TestParameterValidation cases to the 400 contract + assert the error body.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
After the POST fix (#566), searching (e.g. Google) returned
422 — profile_id / url required. A GET form submit replaces the action URL's query string with the form's own fields (?q=…), wiping the query-encodedprofile_id/urlthe proxy needs.Fix
<form>actions (split out of the generic href/src pass):__taos_pid/__taos_url, prefixed so they can't collide with a site field namedurl/q) that survive the submit.q=cats) into the target URL. Only the query is touched, so the per-hop SSRF re-check sees the same host.Tests
4 rewriter cases (GET hidden-inputs, no-method default, POST query action, no-action→current page) + 4 proxy-route cases (merge, preserve existing target query, missing-profile 400, missing-url 400). Targeted suites green.
Note
JS/SPA-driven search submits via
fetch()from inside the sandboxed iframe and isn't covered by HTML-form rewriting — that's the real-Chromium escalation tier's job (#567).Summary by CodeRabbit
New Features
Bug Fixes
Tests