feat(browser): PR 9 — bookmarks + site permissions + downloads (backends)#309
Conversation
Adds GET/POST/DELETE routes for /api/desktop/browser/bookmarks with per-user/per-profile isolation, 204 info-hide on DELETE, and 16 tests.
Adds GET /api/desktop/browser/download that proxies an upstream file to the client as a browser save-dialog download. Mirrors the security pattern from proxy.py and extract.py: auth guard, SSRF validation on the initial URL, manual redirect walk with per-hop SSRF re-validation (blocks redirect-bypass attacks), and cookie-jar integration so authenticated downloads from sites the user is logged into work. Sets Content-Disposition: attachment with RFC 5987 filename* encoding. Filename sanitisation strips path-traversal (os.path.basename) and characters that would break Content-Disposition header parsing. When no filename is provided it is inferred from the URL path. 16 new tests; full desktop_browser suite 427/427 green.
|
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 (6)
📝 WalkthroughWalkthroughThe PR introduces bookmark management and site permission controls for a desktop browser, adding frontend UI in AddressBar with a star button, TypeScript API clients, FastAPI endpoints for bookmark CRUD (GET/POST/DELETE), site permission management (GET/POST/DELETE), and a download proxy endpoint with SSRF protection and redirect handling. Database schema and store methods support bookmarks indexed by ChangesBookmark and Permission Management
Download Endpoint with SSRF Protection
Sequence DiagramsequenceDiagram
participant User
participant AddressBar
participant bookmarksAPI as Bookmarks API<br/>(Client)
participant Server as FastAPI<br/>Backend
participant DB as BrowserStore<br/>(SQLite)
User->>AddressBar: Page loads / Tab changes
AddressBar->>bookmarksAPI: listBookmarks(profileId)
bookmarksAPI->>Server: GET /api/desktop/browser/bookmarks
Server->>DB: list_bookmarks_for_profile(user_id, profile_id)
DB-->>Server: [bookmark_id, url, title, created_at]
Server-->>bookmarksAPI: {bookmarks: [...]}
bookmarksAPI-->>AddressBar: Bookmark[] or []
AddressBar->>AddressBar: Sync bookmarked state for active URL
User->>AddressBar: Click star button
alt Bookmarked
AddressBar->>bookmarksAPI: removeBookmark(profileId, bookmarkId)
bookmarksAPI->>Server: DELETE /api/desktop/browser/bookmarks/{id}
Server->>DB: delete_bookmark(user_id, profile_id, bookmark_id)
DB-->>Server: true
Server-->>bookmarksAPI: 204 No Content
else Not bookmarked
AddressBar->>bookmarksAPI: addBookmark(profileId, url, title)
bookmarksAPI->>Server: POST /api/desktop/browser/bookmarks
Server->>DB: create_bookmark(user_id, profile_id, url, title)
DB-->>Server: bookmark_id
Server-->>bookmarksAPI: {bookmark_id: "..."}
end
bookmarksAPI-->>AddressBar: Success
AddressBar->>AddressBar: Update bookmarked state + aria-pressed
sequenceDiagram
participant User
participant Frontend
participant permAPI as Site Permissions API<br/>(HTTP)
participant Server as FastAPI<br/>Backend
participant DB as BrowserStore<br/>(SQLite)
User->>Frontend: Navigate to settings / Change site permission
Frontend->>permAPI: POST /api/desktop/browser/site-permissions
permAPI->>Server: {profile_id, host_pattern, permission, state}
Server->>DB: set_site_permission(user_id, profile_id, host_pattern, permission, state)
DB->>DB: Validate permission + state
DB->>DB: UPSERT into site_permissions
DB-->>Server: Success / ValueError
Server-->>permAPI: 200 {granted: true} or 400 {error}
permAPI-->>Frontend: Confirmation
Browser->>Server: Request to restricted host
Server->>DB: check_site_permission(user_id, profile_id, host, permission)
DB->>DB: Match host against patterns (* wildcard, *.domain)
DB-->>Server: "allow" or "deny" or None
Server->>Server: Enforce permission state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
Code Review SummaryStatus: Issues Resolved | Recommendation: Merge Overview
Resolved Issues
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (6 files)
Reviewed by grok-code-fast-1:optimized:free · 160,826 tokens |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const id = await addBookmark(win.profileId, activeTab.url, activeTab.title || activeTab.url); | ||
| if (id) { | ||
| bookmarksRef.current.set(activeTab.url, id); | ||
| setBookmarked({ id }); |
There was a problem hiding this comment.
WARNING: The bookmark cache (bookmarksRef) is local to each AddressBar component instance. This means toggling a bookmark in one tab will not update the star button state in other open tabs until the component remounts. This could lead to stale UI state across multiple tabs.
Consider making the bookmark cache global (e.g., via a React context or Zustand store) or refreshing the cache when switching tabs to ensure consistency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
desktop/src/lib/browser-bookmarks-api.test.ts (1)
135-144: ⚡ Quick winCover the real success status for DELETE here.
The route contract for bookmark deletion is
204 No Content, but this happy-path test only exercises200. Adding a204success case will keep the client wrapper aligned with the backend behavior.🤖 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 `@desktop/src/lib/browser-bookmarks-api.test.ts` around lines 135 - 144, Add a test exercising the actual backend success status 204 for removeBookmark: update or add an it block that mocks global.fetch to resolve with { ok: true, status: 204 } and asserts removeBookmark("profile-1", "bm-1") returns true, and still verifies the request URL and opts.method === "DELETE" (referencing the removeBookmark call and the existing fetchMock usage to locate where to change/add the test).
🤖 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 `@desktop/src/apps/BrowserApp/AddressBar.tsx`:
- Around line 123-138: The toggleBookmark flow can trigger duplicate mutations
because bookmarked remains null while addBookmark is pending; add a local
pending state (e.g., isBookmarkPending) and use it to short-circuit early in
toggleBookmark and to disable the bookmark button; set isBookmarkPending = true
before calling addBookmark or removeBookmark and clear it in finally, and update
bookmarksRef and setBookmarked only after the mutation succeeds; update any UI
that reads bookmarked (the button render) to also disable when isBookmarkPending
is true so double-clicks cannot send concurrent requests (apply same pattern for
the remove path and the similar code at the other location referenced).
- Around line 95-109: The bookmarks cache is never cleared when the profile
changes, so bookmarksRef.current (keyed by URL) can retain entries from a
previous profile and cause setBookmarked to report bookmarks for the wrong
profile; modify the useEffect that calls listBookmarks (the effect watching
win?.profileId) to clear bookmarksRef.current (e.g.,
bookmarksRef.current.clear()) before repopulating, and add a
cancellation/sequence guard so late responses from an earlier listBookmarks call
are ignored (e.g., capture a local requestId or use an aborted flag and verify
it before writing to bookmarksRef.current or calling setBookmarked), referencing
the existing useEffect, listBookmarks, bookmarksRef, win?.profileId, activeTab,
and setBookmarked symbols.
In `@tinyagentos/routes/desktop_browser/download.py`:
- Around line 138-141: Move the filename resolution to after the final HTTP
response (after redirects) so that upstream headers and the final URL can
influence the name: call _safe_filename(...) only once you have the response
object, prefer a Content-Disposition filename* value, then Content-Disposition
filename, then derive from response.url using _filename_from_url, and finally
fall back to the incoming filename parameter if provided; update the code that
assigns final_name (currently using _safe_filename and _filename_from_url) to
run after the fetch/response and use response.headers and response.url for this
logic.
- Around line 76-81: The current code uses await http.get(fetch_url) which
buffers the entire body; change it to use http.stream("GET", fetch_url, ...) so
the response body is not read into memory. Replace the await http.get(...) call
inside the for _hop loop with an async with http.stream("GET", fetch_url,
follow_redirects=False, timeout=_FETCH_TIMEOUT, cookies=cookies) as response and
then feed response.aiter_bytes() (or response.aiter_raw()) into the
StreamingResponse instead of accessing response.content; keep using the same
variables (http, fetch_url, response, StreamingResponse) and preserve existing
hop/redirect handling and headers.
In `@tinyagentos/routes/desktop_browser/store.py`:
- Around line 1007-1020: check_site_permission currently returns the first
matching rule from list_site_permissions (oldest-first), letting older wildcards
override newer specific rules; fix by collecting matches while iterating the
permissions (iterate rows in reverse to prefer newest on ties) and pick by
specificity: prefer exact matches (row["host_pattern"] == host), then subdomain
wildcard matches (pattern starting with "*." where host equals domain or
endswith ".domain"), then global "*" matches; return the state of the
highest-specificity match (use the reversed iteration as the tie-breaker when
multiple rules have same specificity). Ensure you update the logic in the
function that calls list_site_permissions (check_site_permission) and reference
row["host_pattern"], row["state"], list_site_permissions, and host/permission
variables when implementing.
---
Nitpick comments:
In `@desktop/src/lib/browser-bookmarks-api.test.ts`:
- Around line 135-144: Add a test exercising the actual backend success status
204 for removeBookmark: update or add an it block that mocks global.fetch to
resolve with { ok: true, status: 204 } and asserts removeBookmark("profile-1",
"bm-1") returns true, and still verifies the request URL and opts.method ===
"DELETE" (referencing the removeBookmark call and the existing fetchMock usage
to locate where to change/add the test).
🪄 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: d951098d-3400-425b-aed8-1a966eb0a37a
📒 Files selected for processing (14)
desktop/src/apps/BrowserApp/AddressBar.test.tsxdesktop/src/apps/BrowserApp/AddressBar.tsxdesktop/src/lib/browser-bookmarks-api.test.tsdesktop/src/lib/browser-bookmarks-api.tstests/routes/desktop_browser/test_bookmark_routes.pytests/routes/desktop_browser/test_download.pytests/routes/desktop_browser/test_site_permission_routes.pytests/routes/desktop_browser/test_site_permissions.pytinyagentos/routes/desktop_browser/__init__.pytinyagentos/routes/desktop_browser/bookmark_routes.pytinyagentos/routes/desktop_browser/download.pytinyagentos/routes/desktop_browser/schema.pytinyagentos/routes/desktop_browser/site_permission_routes.pytinyagentos/routes/desktop_browser/store.py
| rows = await self.list_site_permissions(user_id=user_id, profile_id=profile_id) | ||
| for row in rows: | ||
| if row["permission"] != permission: | ||
| continue | ||
| pattern = row["host_pattern"] | ||
| if pattern == "*": | ||
| matched = True | ||
| elif pattern.startswith("*."): | ||
| domain = pattern[2:] | ||
| matched = host == domain or host.endswith("." + domain) | ||
| else: | ||
| matched = host == pattern | ||
| if matched: | ||
| return row["state"] |
There was a problem hiding this comment.
Don't let wildcard site-permission rules win just because they were inserted first.
check_site_permission() returns the first match from list_site_permissions(), and that list is ordered oldest-first. An older * -> allow therefore beats a newer example.com -> deny, so the result depends on insertion order instead of match specificity. Prefer exact > subdomain wildcard > *, with newest grant as the tie-breaker.
🤖 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/store.py` around lines 1007 - 1020,
check_site_permission currently returns the first matching rule from
list_site_permissions (oldest-first), letting older wildcards override newer
specific rules; fix by collecting matches while iterating the permissions
(iterate rows in reverse to prefer newest on ties) and pick by specificity:
prefer exact matches (row["host_pattern"] == host), then subdomain wildcard
matches (pattern starting with "*." where host equals domain or endswith
".domain"), then global "*" matches; return the state of the highest-specificity
match (use the reversed iteration as the tie-breaker when multiple rules have
same specificity). Ensure you update the logic in the function that calls
list_site_permissions (check_site_permission) and reference row["host_pattern"],
row["state"], list_site_permissions, and host/permission variables when
implementing.
…reaming + filename; site-permissions specificity (CR feedback)
|
@coderabbitai full review — please re-review at |
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
1 participant
Summary
Three browser conveniences delivered as backends + the highest-impact frontend (bookmark star toggle in URL bar). Frontend management UX (BookmarksBar, SitePermissionsPanel) is deferred to follow-ups since the data model + APIs land here and the existing AgentCapabilitiesPanel pattern from PR 7 already proves the management surface works.
What ships
GET/POST/DELETE /api/desktop/browser/bookmarkswith per-(user, profile) keying, info-hide DELETE, opaque IDs. URL-bar star button toggles bookmark for current tab; cache populated on mount vialistBookmarks.site_permissionstable keyed on(user_id, profile_id, host_pattern, permission)withstate ∈ {allow, deny}. Permissions: notifications / clipboard-read/write / geolocation / camera / microphone.check_site_permissionreuses the same host_pattern matching as agent capabilities (*/*.example.com/ exact). HTTP CRUD endpoints + 35 store/route tests.GET /api/desktop/browser/downloadendpoint. Auth + per-hop SSRF (matches extract.py post-fix) + cookie jar (so authenticated downloads work). Inferred filename from URL path;Content-Disposition: attachment; filename="…"; filename*=UTF-8''…so the browser shows save dialog. Path-traversal sanitisation on caller-supplied filename.Files (8 changed, ~1900 lines)
Backend (7 files): new
bookmark_routes.py+site_permission_routes.py+download.py; extensions tostore.py(4 bookmark methods + 4 site-permission methods);schema.py(newsite_permissionstable);__init__.pyregistrations.Frontend (3 files): new
browser-bookmarks-api.ts; modifiedAddressBar.tsx(star toggle +Staricon, hidden onabout:blank); extended tests.Test plan
pytest tests/routes/desktop_browser/ -q→ 427 passedcd desktop && npx vitest run→ 715 passed across 103 filescd desktop && npx tsc --noEmit→ zero errorsAtomic invariants
os.path.basename+ control-char/quote strippingDeferred to follow-ups
Notification.requestPermission()) — pattern matches PR 7 capability flowSpec / plan references
Summary by CodeRabbit
Release Notes