Promote dev to master: Mail app, Browser redesign, optional Store apps, agent screenshot, canvas + restart fixes#952
Conversation
…n feed (#939) * feat(notifications): wire desktop bell to backend notification feed The desktop notification bell read only from a client-only zustand store, so the persistent backend feed (worker join/leave, backend up/down, training, app install, disk quota) never reached the UI. Add server-notifications lib (fetch/map/mark-read against /api/notifications), a mergeServerNotifications store action that upserts server rows while preserving local read state and keeping client items, a polling hook (30s + on-open refresh) mounted in App for both shells, and wire mark-read / mark-all-read to also persist on the backend. Server rows are mapped to the frontend shape with an srv- id prefix and seconds-to-millis timestamps; unknown levels fall back to info. Fetch failures yield an empty list and never throw. * fix(notifications): keep dismissed server items dismissed; pause poll when tab hidden Address gitar review on #939: - dismiss()/clearAll() record server ids so the next poll does not resurrect them (the merge filters dismissedServerIds). - pause the 30s poll while the tab is backgrounded; resync on return.
…ew task backlog #57-62
) Map backend event sources to a target app (and optional launch prop) so clicking a notification in the centre opens the right place: - system.update / system.lifecycle -> Settings (updates) - disk_quota -> Settings (storage) - worker.* / backend.* -> Cluster - training.* -> Agents - app.installed / app.failed -> Store NotificationCentre opens the target via openWindow (passing meta as launch props), marks the item read, and closes. Action-less items keep the existing mark-read-in-place behaviour. SettingsApp accepts an optional section launch prop, mirroring the FilesApp rootPath mechanism.
…h prop)
Double-clicking a Desktop folder passed {location,path} but FilesApp only read
rootPath, so it opened at the workspace root. FilesApp now takes a path prop that
seeds currentPath + shows the browser pane; DesktopIcons passes the folder path.
Phase 1 of the approved Projects redesign: - Project-list sidebar (248px) restyled to the mock with project marks, token-driven surfaces, and the empty state preserved. - Project header with a static-but-real presence row: avatars of the owner plus the real member roster (agents vs humans), derived from existing member/agent data. Live multiplayer presence is deferred to phase 2. - New 8-tab bar (Workspace default) restyled with the token underline chrome. - New Workspace hero tab: a split pane. Left reuses the project-scoped MessagesApp (real thread + composer + A2A bus). Right is a preview pane with a Preview | Code | Canvas segmented toggle and a toolbar; Canvas embeds the real project canvas, Preview/Code show honest placeholders (a true streamed live build preview is deferred to phase 2/3, marked with TODOs). - All other tabs keep their behavior under the new header/tab chrome. - Tokens used throughout so dark and light both work; split pane stacks on narrow widths. Existing ProjectsApp tests pass; added Workspace default + tab-switch tests. Board internals unchanged (already token-styled).
The controller registers itself as worker 'local' on every boot; that is not a cluster event worth notifying about. Only remote workers joining or returning online now fire a notification.
…witching) Collapse the 248px project list to a 56px rail (mark avatars stay clickable so you can still switch projects) to give the workspace/chat area more room. Desktop-only; mobile keeps its split view.
…n light theme) The mobile chat sidebar's Projects and Archived sections hardcoded white-alpha text colors, which vanish on the light theme (project/channel names invisible). Use shell-text tokens so they adapt to the active theme.
…reply view) The fullscreen/side thread panel used bg-shell-surface (~4% alpha), so the chat list behind bled through and the parent message rendered garbled/overlapping. Use the opaque bg-shell-bg, tokenize the author/content text (was white-only, invisible in light theme), and wrap long content.
…ck drain) (#942) * fix(lifespan): bound background-task cancellation so shutdown never strands The controller restart was slow (~51s) because the FastAPI lifespan SHUTDOWN phase gathered background tasks with an unbounded asyncio.gather. If any long-lived loop (event brokers, health/reap loops, provider refresh, A2A poller, LiteLLM bringup, base-image prefetch, the store_popularity warmer, or the local-heartbeat loop) did not unwind promptly on cancel, the lifespan context manager blocked until systemd SIGKILLed the process at TimeoutStopUSec (~45s). The new process then booted in ~5s, so the entire cost was SHUTDOWN. Make shutdown deterministic and bounded: - Add task_utils.cancel_and_wait: cancels tracked tasks and waits with asyncio.wait(timeout=...). Unlike wait_for+gather it does not await the cancellation of still-pending tasks, so a task that refuses to unwind cannot hang the call. Stragglers are logged by name and we proceed. - Lifespan shutdown now cancels the supervised _background_tasks set plus the local-heartbeat task under one 5s budget instead of an unbounded gather, and the separate unbounded await of the heartbeat task is removed. Also fix the 401 that made the systemd graceful-stop drain a no-op: the taos-graceful-stop hook POSTs /api/system/prepare-shutdown from localhost with no session cookie. AuthMiddleware now exempts POST to that exact path for loopback callers only (127.0.0.1 / ::1, checked via request.client, not X-Forwarded-For), so the local drain works while remote callers still hit the session gate. TimeoutStopUSec is left unchanged (install-script concern); shutdown is just fast enough that the timeout is never reached. Tests: cancel_and_wait cancels tracked loops within the budget and reports an uncancellable straggler without blocking; prepare-shutdown is reachable from loopback (v4 and v6) without a session, rejected from a remote address, and the exemption is path-scoped so other APIs stay gated on loopback. * fix(auth): correct loopback-drain docstring (controller binds 0.0.0.0) Address gitar security finding: the rationale wrongly claimed taOS binds 127.0.0.1. It binds 0.0.0.0, so safety comes from request.client.host being the immediate TCP peer (a remote caller cannot make it loopback) and from NOT trusting X-Forwarded-For, not from the bind address. Logic unchanged.
The split-pane divider showed a col-resize cursor but did nothing. Wire pointer drag (with pointer capture + min-width clamps) so the chat and preview panes resize.
…ang 45s The real restart-hang cause: uvicorn had no timeout_graceful_shutdown, so on SIGTERM it waited indefinitely for long-lived connections (SSE, heartbeats) to close, never reaching the lifespan shutdown, until systemd SIGKILLed at the 45s stop timeout. Set timeout_graceful_shutdown=5 on all uvicorn configs. Follows the #942 background-task cancel (necessary but not sufficient on its own).
…45s-restart fix) The two uvicorn servers each installed their own SIGTERM handler and the second overrode the first, so on SIGTERM only the proxy server got should_exit; the main server was then force-cancelled mid-serve, which hung the 45s stop. Neuter uvicorn's per-server signal capture and install ONE handler that flips should_exit on both, then await the survivor gracefully (bounded) instead of force-cancelling.
…wn (THE 45s-restart fix) py-spy on the hung process proved the real cause: the asyncio loop exits fine, but Python's threading._shutdown() blocks on a non-daemon aiosqlite connection worker thread. github_identities_store (a BaseStore) was never closed, so its thread kept the interpreter alive until the 45s SIGKILL. Close it, and add an idempotent app.state BaseStore close-all backstop so a future omission cannot reintroduce the hang. (The prior shutdown-bounding + dual-server-signal changes stay as hardening; this is the actual fix.)
…der resize + chat fixes; tasks #66/67/68
The client POSTs mark-read with no body to mark the whole channel read, which made request.json() raise JSONDecodeError and return 500. Tolerate an empty or non-dict body.
Self-host tldraw assets, fix shape validation, and fill container. - CSP/offline: pass assetUrls from getAssetUrlsByMetaUrl (@tldraw/assets/urls) to <Tldraw> so fonts/translations/icons/embed-icons load same-origin via Vite-bundled hashed URLs instead of cdn.tldraw.com, which the offline-first CSP blocks. Added @tldraw/assets 4.5.12 (matches tldraw). - Validation: the geo/unknown fallback was the built-in geo shape receiving custom taos_* props, throwing ValidationError. Route the fallback to a new taos-generic custom shape that declares only geometry and carries taos_* in shape.meta. note/link/image keep taos_* in their own declared props. - Layout: CanvasView height calc(100vh - 100px) -> 100% so the board fills its container inside the Workspace right pane and the standalone Canvas tab. - Extracted elementToShape/shapeType into element-to-shape.ts and added a unit test covering each kind plus the meta-based fallback.
… in CSP Two remaining canvas console errors after #943: (1) store.put rejected taos-generic because createTLStore had no custom shape schema (shapeUtils were only on <Tldraw>, which governs rendering not validation) -- pass shapeUtils to createTLStore. (2) tldraw's bundled translation data: URI was blocked by connect-src -- add data: to the CSP connect-src.
- add_element did a plain INSERT, so re-sending an existing element (hydrate then nudge) hit a UNIQUE constraint and returned 500. Make it an ON CONFLICT upsert. - tldraw renders images via blob: URLs, blocked by img-src; add blob:.
…mobile - MobileAppWindow content bg was hardcoded rgba(15,15,35) which read as an indigo flash on the dark theme; use var(--color-shell-bg) (graphite). - The Projects list sidebar kept its fixed 248px inside the mobile full-width list screen, leaving an empty strip showing the window bg; make .sidebar 100% width under the mobile media query.
… (#945) * feat(mail): Phase 1 Mail app, multi-account IMAP/SMTP client (#60) Frontend (desktop/src/apps/MailApp): - New "mail" platform app in the registry (envelope icon, singleton, 1200x800 default), built from the approved mock with theme tokens (dark + light) on the Store/Images/GitHub visual bar. - Account switcher, folder list (Inbox/Sent/Drafts/Archive/Trash plus the server's own mailboxes), message list with search and All/Unread/Flagged filters, reading pane (headers + body + attachments + reply/forward/archive/delete actions), and a Compose overlay (To/Cc/Subject/body, send/discard). - Add-account form, mobile-adaptive layout (stacks list and reading pane), and a Share / Send to entry-point stub for task #69. - The Agent accounts group and the send-as From switcher are shown disabled as a Phase 2 affordance, not wired. Backend (tinyagentos): - MailAccountStore: per-user account metadata over aiosqlite. The password is never stored here; only a SecretsStore pointer is kept. Store is initialised in the app lifespan and closed on shutdown so it does not leak a connection thread. - mail_client: IMAP folder/message listing, single-message fetch, and SMTP send over stdlib imaplib/smtplib (matching the existing email connector), with all blocking IO run via asyncio.to_thread. Envelope and detail parsing are pure functions for testing. - /api/mail routes (accounts CRUD, folders, messages list, message get, send), per-user scoped via the auth_context current_user dependency. Passwords are written to and read from the SecretsStore. Tests: - test_mail_store (account CRUD, user scoping, secret indirection, no plaintext password column) and test_mail_client (envelope/detail parsing, flags, encoded headers, attachments). Frontend MailApp vitest covers list/filter/open/compose/share/empty. Deferred to Phase 2 (clean TODOs): agent send-as, OAuth, full share sheet (#69), and push/IDLE new-mail. * fix(mail): use stable IMAP UIDs, validate folder names, bound list size Address Gitar review findings on the Mail app: - list/get used IMAP sequence numbers as the message id, which the server renumbers on expunge; switch to UID SEARCH/FETCH so the id stays stable - folder arrived untrusted and was interpolated into SELECT; reject names containing quotes or CR/LF (MailFolderError -> 400) to block IMAP injection - limit was unbounded; clamp to MAX_MESSAGE_LIMIT (200) so one request cannot tie up the connection with thousands of per-id fetches - add_account now rolls back the account row if the secret write or re-point fails, instead of orphaning a row that points at a missing secret Add folder-validation tests.
* feat(browser): redesign chrome to the approved design bar Restyle the Browser app chrome to the approved mock (task #66): a tab strip with a Proxy/Streamed segmented engine toggle, a unified toolbar with back/forward/reload/home plus a pill omnibox (lock + security state), an agent-presence pill, and a profile chip whose dropdown lists profiles with avatar rows. Tokenize against the real taOS design tokens (dark + light), matching the Store/Images bar. Behavior is unchanged: tabs, navigation, the URL-rewriting proxy browser, the streamed WebRTC Chromium session, profiles/profile-switching, agent sessions, and settings all keep their existing state and handlers. - New BrowserModeToggle surfaces the proxy-vs-streamed engine as a segmented control. Streamed drives the existing escalate lifecycle (POST /api/browser/sessions, poll, setTabLiveSession); Proxy clears the liveSession. Covered by BrowserModeToggle.test.tsx. - Chrome becomes the unified toolbar (adds a Home button, wraps AddressBar in a pill omnibox with a connection-security lock); the engine toggle moves to the tab strip. - TabStrip, ProfileSwitcher, SettingsPanel, AddressBar, AgentPresencePill retokenized to live tokens (replacing the dead shell-hover / shell-border-subtle classes in touched files). - BrowserApp reorders desktop layout to tab strip over toolbar and gives the mobile address bar an omnibox pill. * fix(browser): clean up poll timer on unmount + honour proxy cancellation Gitar flagged two bugs in BrowserModeToggle: - the re-poll setTimeout was never cleared on unmount, so a closed tab kept fetching and calling setPhase on a dead component - goStreamed ignored a mid-flight Proxy click: after the POST resolved it forced the tab into a streamed session even though the user had cancelled Add an unmount cleanup effect and re-check cancelledRef after the POST. * fix(browser): re-check cancellation after the session JSON parse Gitar follow-up: the post-POST guard left a residual window during the await resp.json() parse. Re-check cancelledRef after the parse (and on the parse-error path) so a Proxy click or unmount mid-parse cannot commit a live session or setPhase on an unmounted component.
#946) * feat(apps): backend install-state routes for optional frontend apps Reddit/YouTube/GitHub/X become optional Store installs instead of always-on desktop apps. Add /api/apps/optional/installed + per-app install/uninstall, backed by installed_apps (kind=frontend-app, no service spawned), gated to an allowlist so the endpoint can't write arbitrary rows. * feat(apps): make Reddit/YouTube/GitHub/X optional Store installs These four were always-on platform apps. Mark them optional in the registry so the desktop launcher (launchpad, search, mobile home) only shows them once installed. Add a 'taOS Apps' section to the Store with instant one-click Install/Remove (no device target or progress -- they are frontend-only), wired to /api/apps/optional/* and an APP_OPTIONAL_CHANGED event so the launcher updates immediately. Update the Launchpad and mobile-home tests for the new default (optional apps absent until installed). * test(apps): cover optional frontend-app install/uninstall routes Allowlist rejection (404), install->listed, uninstall->removed, and that installed optional apps stay out of the proxy-services list. * fix(mobile): surface installed optional apps on the home grid Gitar⚠️ on #946: the mobile home grid (the only launcher on phones) excluded optional apps from the default and never re-added them, so an installed Reddit/YouTube/GitHub/X could never appear. Append any installed optional app not already placed to the last home page at render.
…14a1e3 Freshness sweep: #944/#945/#946 merged + deployed/verified on Pi. #946 made the four social apps optional Store installs, so split them out of the always-on platform list (22 platform + 4 optional). Roll STATUS to the current dev tip with the merged set, the two open PRs (#947 browser, #948 agent screenshot), and the new tasks #70/#71/#72.
Standing rule (Jay): strip em dashes from docs when found; replace with --.
…#947) * feat(browser): taos.my homepage + dark/light scheme for proxied sites Homepage: the browser opens to https://taos.my (initial tab) and the Home button navigates there; new blank tabs still open about:blank. Colour scheme: proxied sites that support dark/light now render to match the taOS theme. The active scheme rides the redeem URL (cs=) -> a taos_cs cookie on the proxy origin -> proxy_get injects a <meta name=color-scheme> plus a taos-color-scheme meta that copilot.js reads to emulate prefers-color-scheme (matchMedia) before page scripts run. Switching the taOS theme re-mints the redeem URL so pages re-theme on next load. Pure CSS @media(prefers-color-scheme) still follows the host browser (an iframe's UA preference cannot be overridden from the parent); the streamed engine will get full control via CDP once an ARM-capable node exists (separate task). * fix(csp): allow framing the browser-proxy origin (frame-src) The desktop CSP had no frame-src, so default-src 'self' blocked the BrowserApp from framing the cross-origin proxy (separate port) -- proxied pages could not render. Add frame-src naming the proxy origin (same host, proxy port), derived per-request; 'self' still covers single-port mode. Surfaced by the new taos.my homepage, which frames the proxy on first load. * fix(csp): validate Host before using it in frame-src (header injection) The Host header is attacker-controllable; interpolating it into the CSP unvalidated allowed CSP-directive injection. Restrict to a bare hostname/IP charset (no spaces, ';', quotes) and fall back to no proxy frame-src on a non-conforming Host. Add regression tests.
* feat(desktop): agent-callable screenshot with capture flash effect Adds an agent screenshot round-trip on the existing desktop-command channel: POST /api/desktop/screenshot emits a 'screenshot' command to every open desktop for the user and awaits the result; the desktop rasterises its viewport (modern-screenshot) and POSTs it back to /api/desktop/screenshot-result, which resolves the request and returns a PNG. A subtle shutter flash (white veil + inset frame, reduced-motion aware) plays on capture, excluded from the image. This lets an agent visually verify the OS it drives -- the missing piece next to open_app / arrange_windows. DOM rasterisation cannot read cross-origin iframes (the Browser's proxied page); the desktop chrome + native apps capture fully. Full-pixel capture incl. web content stays on the user-gesture getDisplayMedia path in the assistant panel. * harden(screenshot): owner-scope result, cap payload size, get_running_loop Gitar review follow-ups on the screenshot round-trip: track the owner user per request and re-check it in screenshot-result (defence in depth over the unguessable, user-scoped request_id); reject image payloads over ~24 MB base64 (413); use asyncio.get_running_loop(). * feat(screenshot): full-fidelity capture via a persistent getDisplayMedia grant Answer to: can the PWA capture full screenshots client-side? Yes -- one getDisplayMedia grant (one gesture, one prompt) yields a persistent MediaStream; agent screenshots then grab frames from it on demand with no re-prompt, and the capture includes cross-origin iframes (the Browser's proxied page) that DOM rasterisation cannot read. The agent screenshot handler prefers a live capture frame and falls back to DOM raster when no grant is active. A MonitorUp toggle in the assistant panel grants/stops the share; the browser shows its own sharing indicator and the user can stop anytime. A fully invisible/persistent grant (survives reload, no indicator) needs a native wrapper or extension -- noted. * fix(screenshot): flash after capture + sync grant button on native stop Gitar review on #948: - the shutter flash was dispatched before capture, so it leaked into the full-fidelity getDisplayMedia frame (that path captures the real composited screen, where the overlay is not excludable like the DOM-raster filter). Capture first, then flash. - the grant button went stale when the user stopped sharing from the browser's native bar. Emit a SCREEN_CAPTURE_CHANGED event on grant/revoke and have the button re-read state from it.
The taOS agent panel hardcoded rgba(21,22,37,0.92), which read as purple on the graphite/light themes. Use var(--color-shell-bg-glass) so it matches the active theme.
On reload the whole server feed arrives at once (all unread), and the toast layer popped every unread item -- spamming toasts on every load. Toasts now fire only for genuinely fresh notifications (created within 20s) and at most once per session; the backlog still fills the bell, it just does not pop.
… open canvas+browser issues #947+#948 merged + deployed; agent-screenshot demo verified live via the control API (no Playwright). Record the agent-panel + notification fixes, the Tailscale private-HTTPS setup + the 6970 port-conflict lesson, #73 single-port follow-up, and the open Projects-canvas frontend crash.
… Projects (#949) Jay hit a Projects canvas crash (frontend tldraw error; backend healthy). Two defences: - hydrateEditor isolates each element in try/catch, so one malformed shape (bad payload, props the tldraw validator rejects, a version-skewed tldraw_shape) is skipped instead of throwing out of editor.run and crashing the whole canvas. - wrap CanvasBoard in AppErrorBoundary (keyed by project) so any remaining render crash shows a fallback rather than taking down the Projects app.
…ng console error)
…xact queries (#950) Gitar 💡 follow-up on #947: the proxied-site matchMedia patch matched ANY query containing prefers-color-scheme and keyed only on the dark/light term, so a compound query like (min-width:600px) and (prefers-color-scheme: dark) -- or a valueless (prefers-color-scheme) -- returned a wrong result. Only override a query that is EXACTLY (prefers-color-scheme: dark|light); everything else falls through to the real matchMedia.
…render (#951) The Projects canvas renders note/link/image elements through tldraw custom shapes whose props schema is strict (every field type-checked at editor.createShape). The backend only validates an element's kind, not its payload, so an agent can store a note/link/image whose payload is missing a field, has the wrong type (e.g. font_size as the string "14"), or is empty. Feeding that raw payload into props throws a ValidationError, so the element silently vanishes from the canvas (and previously took the whole board down before the per-element guard). elementToShape now coerces every payload field to its declared type with a sensible default, and coerces geometry and author_kind too, so imperfect agent-authored content renders as a proper note/link/image instead of disappearing. Reproduced live: notes written with a missing/string/empty font_size produced 'ValidationError: ... font_size: Expected number, got undefined/a string' and were skipped. With coercion they render with defaults.
… tldraw->Konva decision, new Store-app studios/office/guided-mode tasks
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
👋 Thanks for the PR! This one targets See CONTRIBUTING.md for the branch model. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds a Mail app and mail APIs, optional app install flows, browser and Projects updates, server notification syncing, agent-triggered screenshots, proxied theme propagation, bounded shutdown and auth/security hardening, plus documentation and UI styling refreshes. ChangesBrowser shell and proxied theme flow
Mail app and mail service
Projects workspace and canvas resilience
Optional apps and desktop surfaces
Notifications, screenshots, and desktop capture
Runtime shutdown, auth, security, and docs polish
Sequence Diagram(s)sequenceDiagram
participant Client
participant ScreenshotRoute
participant DesktopBroker
participant DesktopUI
participant ScreenshotResultRoute
Client->>ScreenshotRoute: POST /api/desktop/screenshot
ScreenshotRoute->>DesktopBroker: register_result + publish screenshot command
DesktopBroker-->>DesktopUI: screenshot request_id
DesktopUI->>ScreenshotResultRoute: POST /api/desktop/screenshot-result
ScreenshotResultRoute->>DesktopBroker: resolve_result(request_id)
DesktopBroker-->>ScreenshotRoute: image or error
ScreenshotRoute-->>Client: image/png or HTTP error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Comment on lines
+281
to
+294
| def _get_message_blocking( | ||
| cfg: MailAccountConfig, folder: str, uid: str | ||
| ) -> MessageDetail | None: | ||
| _validate_folder(folder) | ||
| conn = _imap_connect(cfg) | ||
| try: | ||
| conn.select(f'"{folder}"', readonly=True) | ||
| # Fetch by UID to match the stable id handed out by list_messages. | ||
| typ, msg_data = conn.uid("FETCH", uid, "(RFC822)") | ||
| if typ != "OK" or not msg_data or not msg_data[0]: | ||
| return None | ||
| raw_email = msg_data[0][1] | ||
| msg = email.message_from_bytes(raw_email) | ||
| return parse_detail(uid, msg) |
There was a problem hiding this comment.
⚠️ Security: IMAP message UID not validated before FETCH (injection gap)
_get_message_blocking (tinyagentos/mail_client.py:281-294) passes the client-supplied uid path parameter straight into conn.uid("FETCH", uid, "(RFC822)"). The route GET /api/mail/accounts/{account_id}/messages/{uid} (mail.py:197-220) takes uid from the URL and forwards it unmodified. imaplib does not quote or sanitize command arguments, so a uid containing spaces or CR/LF can append extra IMAP protocol tokens on the connection — exactly the class of attack the module guards against for folder names via _validate_folder (mail_client.py:40-49). The folder argument is validated but the equally-untrusted UID is not, leaving an inconsistent gap.
While the connection is the user's own authenticated session (limiting blast radius), the code already treats this exact vector as worth defending for folders, so the UID should get the same treatment. Add a validator that rejects anything that is not a bare IMAP UID (digits, or digit ranges/sets like 1:5), e.g. if not re.fullmatch(r"[0-9:,*]+", uid): raise MailFolderError(...), and call it in _get_message_blocking before conn.uid(...).
Validate the UID the same way folder names are validated (requires import re).:
def _validate_uid(uid: str) -> str:
"""Reject UID values that are not a bare IMAP UID/UID-set."""
if not uid or not re.fullmatch(r"[0-9][0-9:,*]*", uid):
raise MailFolderError(f"invalid message uid: {uid!r}")
return uid
# in _get_message_blocking, before conn.select(...):
_validate_folder(folder)
_validate_uid(uid)
Was this helpful? React with 👍 / 👎
Comment on lines
+299
to
+313
| def _build_outgoing( | ||
| cfg: MailAccountConfig, | ||
| to: str, | ||
| subject: str, | ||
| body: str, | ||
| cc: str = "", | ||
| attachments: list[tuple[str, bytes, str]] | None = None, | ||
| ) -> MIMEMultipart: | ||
| msg = MIMEMultipart() | ||
| msg["From"] = cfg.email_address or cfg.username | ||
| msg["To"] = to | ||
| if cc: | ||
| msg["Cc"] = cc | ||
| msg["Subject"] = subject | ||
| msg.attach(MIMEText(body, "plain")) |
There was a problem hiding this comment.
💡 Security: SMTP send allows header injection via subject/to/cc
_build_outgoing (tinyagentos/mail_client.py:299-323) assigns user-supplied values directly to MIME headers: msg["To"] = to, msg["Subject"] = subject, msg["Cc"] = cc. These come from the unvalidated SendBody fields in the send route (mail.py:43-48, 223-242). Depending on the active email policy, embedded CR/LF in these values can fold/inject additional headers (e.g. Bcc, extra recipients) into the outgoing message. The recipient list is also derived by naive comma-splitting of to/cc, so injected newlines or stray addresses propagate to sendmail.
The credentials are the user's own, so impact is limited, but it is good practice to strip CR/LF from header values (or rely on the modern email.message.EmailMessage policy which raises on illegal header characters). Sanitize to/subject/cc by rejecting/stripping \r and `` before building the message.
Strip CR/LF from header-bound fields before assignment.:
def _clean_header(value: str) -> str:
# Drop CR/LF so a crafted value cannot inject extra headers.
return value.replace("\r", " ").replace("
", " ").strip()
# in _build_outgoing:
msg["To"] = _clean_header(to)
if cc:
msg["Cc"] = _clean_header(cc)
msg["Subject"] = _clean_header(subject)
Was this helpful? React with 👍 / 👎
Comment on lines
+35
to
+49
| def _strip_port(host: str) -> str: | ||
| return host.rsplit(":", 1)[0] if ":" in host else host | ||
|
|
||
|
|
||
| def _proxy_frame_origin(request: Request) -> str: | ||
| """The browser-proxy origin (same host, proxy port) to allow in frame-src, | ||
| or "" when single-port (proxy served from the main origin, already 'self').""" | ||
| state = request.app.state | ||
| main_port = getattr(state, "main_port", None) | ||
| proxy_port = getattr(state, "browser_proxy_port", 0) | ||
| if not main_port or not proxy_port or main_port == proxy_port: | ||
| return "" | ||
| host = _strip_port(request.headers.get("host") or "") | ||
| if not host or not _SAFE_HOST_RE.fullmatch(host): | ||
| return "" |
There was a problem hiding this comment.
💡 Bug: _strip_port corrupts bare IPv6 hosts in CSP frame-src
_strip_port in security_headers.py (lines 35-36) does host.rsplit(":", 1)[0] if ":" in host else host. For a bracketed IPv6 host without a port (e.g. [::1]), the host contains :, so it is wrongly truncated to [::1 (and any [2001:db8::1] to [2001:db8::1). _SAFE_HOST_RE permits [ and ], so such a malformed host then gets interpolated into the frame-src origins (line 54), producing a broken/invalid CSP source for IPv6 deployments. Only host:port forms strip correctly. Consider only stripping when the trailing segment is numeric, or detect the ] bracket for IPv6 before splitting.
Handle bracketed IPv6 literals and only strip a numeric trailing port.:
def _strip_port(host: str) -> str:
# IPv6 literal: only a port lives after the closing bracket.
if host.startswith("["):
end = host.find("]")
return host[: end + 1] if end != -1 else host
# IPv4 / hostname: strip a trailing :port only.
head, sep, tail = host.rpartition(":")
return head if sep and tail.isdigit() else host
Was this helpful? React with 👍 / 👎
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
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.
Promotes dev to master (40 commits since #938). No new release version bump in this PR; cuts the next beta state.
Highlights
Notes
All merged via per-PR review (Gitar approved, CI green) under the severity gate.
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Bug Fixes