Skip to content

feat(companion): owner-scoped mobile surface — data, research, mobile features, push, pairing UI - #6038

Open
mahdi-salmanzade wants to merge 22 commits into
odysseus-dev:devfrom
mahdi-salmanzade:feat/companion-mobile-surface
Open

feat(companion): owner-scoped mobile surface — data, research, mobile features, push, pairing UI#6038
mahdi-salmanzade wants to merge 22 commits into
odysseus-dev:devfrom
mahdi-salmanzade:feat/companion-mobile-surface

Conversation

@mahdi-salmanzade

@mahdi-salmanzade mahdi-salmanzade commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Restores the full owner-scoped /api/companion/* surface the native mobile client needs, rebuilt from scratch on current dev. The bridge merged in #863/#871/#877 only covers ping/info/models/pair, so a paired phone can discover a server and stream chat but nothing else. This adds the remaining endpoint families — notes/tasks/memory (read + write), a Deep Research launcher, the read and write mobile-feature tiers (documents, gallery, calendar, email, skills, assistant, compare), chat attachments (/upload), task controls, gallery favorites, email AI actions, Expo push delivery, and the Settings pairing card.

Every endpoint resolves the token's real owner (token_owner) rather than the api pseudo-user the auth middleware attributes bearer callers to, so a paired device acts as its owner and never sees another account's rows. The private views (notes/tasks/memory) filter by exact owner and never widen to legacy null-owner "shared" rows; the shareable views (documents/gallery) keep the standard owner_can_see rule. No admin-privileged tool is reachable through this bridge. An earlier revision carried an admin tier (shell/terminal, vault, MCP, cookbook, contacts) behind an off-by-default setting plus a scope plus an owner-is-admin check; it has been dropped. A paired device is narrow integration access and shouldn't inherit an admin account's capabilities — which is the concern #5674 raises, and I think it's right on that point. A test asserts those paths are never registered, so they can't creep back in. If terminal access is wanted later it should be proposed on its own terms.

This work previously existed as PRs #881, #1094, #1166, #1332, #1366, #2585, #2668, #2672, #2674 and #2966 (the admin tier from #2674 and the toggle from #2966 are intentionally not carried over). GitHub closed all of them on 2026-07-23 during the repository transfer and fork-network separation — the same event that closed #5681 and ~900 other PRs — not on review. Rather than reopen ten stale chains, this is one branch rebuilt on current dev with every review round already folded in (RaresKeY's 4 findings on #881, his 5 on #1366, ErnestHysa's pagination and behavioural-test asks on #2668, tressTestor's security pass on #881).

Rebuilding on three-month-newer dev surfaced three defects that did not exist in any of the original PRs. Each is fixed here with a regression test:

  • Model discovery was broken for every paired device. require_models_scope (added upstream in fix(companion): require chat scope for model inventory #4319) compares _pairing.COMPANION_SCOPE against the caller's parsed scope list. Once the pairing grant became the comma string "chat,companion", that comparison could never match — a freshly paired phone got 403 on /api/companion/models and could not finish pairing. The grant and the individual capabilities are now separate names (CHAT_SCOPE / DATA_SCOPE / granted_scopes()), and model inventory gates on CHAT_SCOPE.
  • Memory writes went to a store nothing reads. The reads were moved onto the live MemoryManager (memory.json) while POST/DELETE /memory still wrote the ORM Memory table, so a memory created on the phone was invisible in both the mobile list and the web UI, and could not be deleted. Both writes now go through the same manager, mirroring routes/memory/memory_routes, including the MemoryStoreUnreadable → 503 guard so a transient read failure can't be mistaken for an empty store and persisted over it (Memoried keep getting wiped #5673).
  • The companion scope did not round-trip. It was missing from ALLOWED_SCOPES, so editing a paired token in Settings silently stripped it and the phone started 403-ing.

I'm aware of #5674 and #5810, which propose removing this bridge. That question is still open and unanswered by a maintainer, and it's a fair one — I'd rather resolve it than have it decided by the backlog. If the decision is that a companion bridge doesn't belong in core, I'll close this myself; #3244 asked whether a native mobile companion was wanted and never got a ruling. This PR is the concrete version of that question, with the security model spelled out.

Target branch

  • This PR targets dev, not main.

Linked Issue

Part of #2666

Type of Change

  • New feature (non-breaking — adds new behaviour)
  • Bug fix (non-breaking — fixes a confirmed issue)

Checklist

  • I searched open issues and open PRs — this is not a duplicate. The ten predecessor PRs listed above are all CLOSED.
  • This PR targets dev
  • My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
  • I actually ran the app end-to-end. Not ticked, deliberately: the companion suites pass (159 tests) and the routers build, but I have not booted the full app against this exact branch, so I won't claim I did. The pair → discover → chat loop was verified end-to-end on the predecessor branches against a real server and a real device.

How to Test

  1. Check out this branch and run the companion suites:
    pytest tests/test_companion_data.py tests/test_companion_pairing.py tests/test_companion_readonly.py tests/test_companion_research.py tests/test_companion_mobile_features.py tests/test_companion_push.py → 159 passed.
  2. Start the server with auth on and bound to the LAN: AUTH_ENABLED=true uvicorn app:app --host 0.0.0.0 --port 7000.
  3. As an admin, open Settings → Mobile and click Generate pairing code. It mints a token scoped chat,companion.
  4. Confirm the grant works where it's needed:
    curl -H "Authorization: Bearer ody_..." http://<lan-ip>:7000/api/companion/models → 200 with only that owner's endpoints (this is the case that used to 403).
  5. Owner scoping: create a note as user A in the web UI, then read /api/companion/notes with a token minted for user B → A's note is absent. Repeat for /tasks and /memory.
  6. Memory round-trip: curl -X POST -d 'text=hello&category=fact' .../api/companion/memory, then GET /api/companion/memory → the new entry is listed, and it also appears in the web UI's memory panel (same memory.json).
  7. Scope narrowing: mint a plain chat-only token via Settings → API tokens and hit /api/companion/notes → 403.
  8. Confirm no admin surface exists: GET /api/companion/terminal/exec, /vault/status, /mcp/servers, /contacts, /admin/status → 404 (not registered).

Not included

Admin-privileged endpoints, dropped on purpose. The client also calls /terminal/exec, /vault/status, /vault/unlock, /mcp/servers, /cookbook/state, /cookbook/models, /contacts and /admin/status. Those stay out for the reason above, and the client degrades gracefully when its Admin screen is unavailable. A test asserts none of them are ever registered.

Everything else the shipped client calls is now served, so the non-admin surface is complete rather than partially inert.

…ble scope

Addresses review on the notes/tasks/memory reads:

- Security: filter these PRIVATE views by EXACT owner and never widen to legacy
  null-owner "shared" rows (that sharing suits documents/gallery, not private
  data). A new read_owner() resolves the owner and FAILS CLOSED when auth is on
  but no owner resolves, instead of silently falling into single-user mode.
- Compatibility: in the supported AUTH_ENABLED=false single-user mode, skip the
  owner filter and show the local user's rows — matching list_notes/list_tasks —
  rather than discarding every owned row (which left the views empty).
- Memory: read through the active MemoryManager (memory.json), the store the app
  actually writes, not the ORM Memory table it never populates; setup_companion_
  routes now takes the app's memory_manager (app.py wires it, standalone falls
  back to the same store). load(owner=...) stays exact-owner.
- Scope round-trip: register 'companion' in ALLOWED_SCOPES and surface chat +
  companion in the Settings token permissions catalog, so editing a paired
  token's permissions no longer strips the scope its reads depend on.

Tests: exact-owner isolation (excludes cross- and null-owner), fail-closed,
single-user visibility, memory-manager owner scoping, and a scope round-trip.
Mirror the defensive split already used by require_models_scope so
membership is checked against parsed scope tokens, never a substring of
the raw comma-joined string.
Builds on the read views (odysseus-dev#881) with the create/delete affordances the
odysseus-mobile app already calls but nothing served:

  POST   /api/companion/notes                              create note/checklist
  DELETE /api/companion/notes/{id}                         delete a note
  POST   /api/companion/notes/{id}/pin                     toggle pinned
  POST   /api/companion/notes/{id}/items/{index}/toggle    toggle a checklist item
  POST   /api/companion/memory                             create a memory
  DELETE /api/companion/memory/{id}                        delete a memory

Each write gates on the same companion scope as the reads, requires a
resolvable owner (a bearer token with a null owner is refused 401 rather
than allowed to mutate shared null-owner rows), stamps that owner on new
rows, and enforces strict ownership on mutate/delete (404 — never confirms
a row's existence to a non-owner). Writes hit the same tables the GET reads
return, so the phone's list stays consistent, and create returns the row in
the same shape the list uses so it shows up without a refetch.

Tasks stay read-only (no mobile write affordance). Updates the README
endpoint table and adds write/owner-scope/scope-gate tests.
Add /api/companion/research/* so a paired LAN client (the mobile app) can
launch and follow Deep Research runs. Mirrors the stock /api/research/*
endpoints but re-scopes every run to the token's real owner via token_owner,
exactly like /api/companion/models: the stock routes resolve a bearer caller
to the sandboxed pseudo-user "api", so a run started there would be owned by
"api" -- invisible in the owner's web-UI library and gated by "api"'s
privileges.

Endpoints (mounted only when app passes a research_handler):
  POST /research/start          launch a run (owner-scoped endpoint resolution)
  GET  /research/active         the caller's own running runs
  GET  /research/stream/{id}    SSE progress
  POST /research/cancel/{id}    cancel one of the caller's runs
  POST /research/result/{id}    read report + sources (no clear)

Security: research_owns() is a 404-not-403 ownership gate on every read/cancel;
resolve_research_endpoint() refuses another owner's endpoint_id so a token can't
research through a stranger's API key; session IDs are validated before touching
the handler or a file path. No extra scope required -- research is a chat-class
generation capability. Covered by tests/test_companion_research.py.
…ry, calendar, email, skills, assistant)

Adds companion/mobile_features.py -- a self-contained, additive APIRouter of
owner-scoped READ endpoints for the native mobile client: documents, gallery,
calendar/events, email (accounts/messages/message), skills, assistant, and
compare history. It never imports companion.routes, so it is independent of the
companion bridge's state.

Every endpoint resolves the caller via token_owner() (a bearer's real owner, not
the sandboxed 'api' pseudo-user) and filters by owner_can_see() (own rows + legacy
null-owner shared rows), so one paired phone can never read another user's data.
A real paired token (chat/companion scope) is required via has_companion_scope().

First of three chained PRs (read -> write -> admin-gated tools). Tests in
tests/test_companion_mobile_features.py (owner-scope helpers + router smoke).
…tests

Per @ErnestHysa's review of odysseus-dev#2668:
- Page every list endpoint (documents, compare/history, calendars, events,
  email/accounts, gallery) with a bounded ?limit (default 100, max 200) / ?offset
  applied in SQL via order_by(...).offset().limit(), so no call can pull a whole
  table. The SQL owner filter now matches owner_can_see exactly (own + null-owner)
  so the LIMIT page is never silently shrunk by the in-Python check.
- /events now pushes the start/end window and the cancelled-status filter into
  SQL instead of fetching every event and filtering in Python.
- Add behavioural endpoint tests: 403 on a scope-less token (every GET endpoint),
  cross-owner excluded from lists / 404 on detail endpoints, gallery filename
  sanitization (traversal + NUL byte), and the limit/offset paging behaviour.
- Drop the stale admin-features paragraph from the module docstring (those land
  in the later tiers, not this read-only module).
Stacked on the read-only tier (odysseus-dev#2668). Adds the owner-scoped WRITE actions to
companion/mobile_features.py:
- POST   /api/companion/compare/record        persist a comparison verdict (owner stamped)
- DELETE /api/companion/compare/{comp_id}      strict ownership (cross-owner -> 404)
- POST   /api/companion/events                 create event in an OWNED calendar
- DELETE /api/companion/events/{uid}           delete via owned-calendar check
- POST   /api/companion/email/send             send from an OWNED account (owner-asserted)
- PATCH  /api/companion/assistant              update/create the per-owner assistant
- GET    /api/companion/skills/{name}/markdown read a skill's source (owner-scoped)

Every write resolves the token's real owner and refuses a null/cross-owner
write (404/403), never mutating another user's row. Part of odysseus-dev#2666. Tests:
8 route-level write owner-scope cases + the tier guard (admin tools still absent).
Stacked on the write tier (odysseus-dev#2672). Adds the admin-only tools to
companion/mobile_features.py, each behind require_companion_admin — a triple
lock: the off-by-default companion_admin_enabled setting (added to
DEFAULT_SETTINGS), an explicit 'companion'-scoped token (stricter than the
data reads, which accept chat), AND a token owner who is a server admin. Never
calls the stock routes' _require_admin (which always 403s the bearer api user).

- GET  /api/companion/admin/status   booleans so the phone shows/hides admin tabs
- GET  /api/companion/contacts        list/search the shared address book
- POST /api/companion/terminal/exec   run a command, return output (full RCE, gated)
- GET  /api/companion/vault/status    unlocked state only (no secret)
- POST /api/companion/vault/unlock    flip unlock state (no export)
- GET  /api/companion/mcp/servers     list servers (env/oauth stripped)
- GET  /api/companion/cookbook/state  read state (secrets stripped)

Part of odysseus-dev#2666. Tests: every admin endpoint 403s when the gate is closed,
terminal runs when open, and the companion_admin_available triple-lock
(setting/scope/owner-admin) is unit-tested.
Surface the existing admin-only `POST /api/companion/pair` in the web Settings
UI so an admin can pair the Odysseus mobile app without dropping to
`scripts/pair_mobile.py`. Adds a 'Mobile' admin tab with a 'Pair a mobile
device' card: one click mints a one-time pairing token and renders the QR plus
host/port/token/payload to scan in the app's pairing screen.

Frontend only, no backend change -- reuses the existing endpoint (require_admin,
CSRF-safe via SameSite=Lax). Every value is escaped; the QR renders only when it
is a verified data:image/png;base64 URI. The shown-once token carries a 'revoke
in Settings -> Account -> API tokens' hint.
…ings

Adds the missing UI + endpoint to flip `companion_admin_enabled` — lock odysseus-dev#1 of
the require_companion_admin triple-lock introduced in odysseus-dev#2674. Without it an admin
has no in-product way to turn the paired phone's admin tools (Terminal, Vault,
Contacts, MCP, Cookbook) on, so the mobile app's Admin screen just reports
"Admin features are turned off on the server."

- companion/routes.py: GET /api/companion/admin-access (read state) + POST (set
  it). Admin-cookie only and CSRF-safe like POST /pair (a SameSite=Lax session
  cookie isn't sent on a cross-site POST). `?format=json` returns the new state
  for the Settings toggle; a plain HTML form post redirects back to the pairing
  page. The standalone /pair page also gains an on/off button.
- static: an "Admin tools on mobile" switch in Settings -> Pair a mobile device,
  wired in admin.js (loads current state, persists each change).
- tests: read / enable / disable / form-redirect, plus the pairing-page toggle.

Enabling grants admin-owned companion tokens full shell access, so it stays off
by default and is flipped only here, deliberately.
…d phone

Add a companion-side push bridge so a paired phone receives Odysseus events as
native notifications via Expo, complementing the URL-based webhook manager.

- companion/push.py: per-owner Expo push-token store (JSON under DATA_DIR,
  atomic write, owner-scoped reads), Expo delivery, and an event->notification
  sink that routes a fired event to exactly its owner's devices. Events without
  an owner in their payload are skipped rather than broadcast.
- src/webhook_manager.py: a generic add_sink() hook so additive overlays can
  receive every fired event without owning a DB Webhook row. Sinks are
  best-effort and isolated — a failing sink can't break webhook delivery or
  any other sink.
- companion/routes.py: owner-scoped POST /api/companion/push/register,
  /push/unregister, and /push/test (send a test notification to the caller's
  own devices).
- app.py: register the push sink next to the companion router.
- tests/test_companion_push.py: owner-scoping, token validation, event routing,
  test-push device counts, and the sink isolation contract.

The owner-bearing lifecycle events this naturally routes (research/document/
memory/email/skill) are added in odysseus-dev#1332; the push infra does not depend on it —
the sink simply no-ops for events that carry no owner.
Addresses review on the mobile-push bridge:

- Delivery path: subscribe the push sink to the event bus (the actual lifecycle
  producer path) via a new tracked add_event_sink, keyed on the INTERNAL event
  names producers emit (research_completed, document_created, ...). Push no
  longer depends on the outbound-webhook layer or any adjacent bridge to fire.
  The WebhookManager.add_sink hook (unused by anything else) is dropped, and its
  untracked-task delivery with it; event_bus holds strong refs to sink tasks so
  a delivery can't be GC'd mid-flight.
- Scope: register/unregister/test now require the companion (chat) scope for
  bearer callers, same as the companion model inventory — a narrowed token can
  no longer install or exercise a durable push destination.
- Account lifecycle: rename_owner migrates a renamed account's devices and
  purge_owner removes a deleted account's (called from the auth rename/delete
  routes), so renames don't strand phones and a reused username can't inherit a
  prior account's device.
- API: the push token endpoints now 400 on non-object JSON or a non-string
  token instead of raising AttributeError → 500.

Tests rewritten around the (event, owner) sink contract: end-to-end
fire_event → sink delivery, sink isolation, scope-required and malformed-body
route checks, and rename/purge store migration.
The companion memory read was moved onto the live MemoryManager (memory.json)
— the store the app actually persists to — while POST/DELETE /memory still went
to the ORM `Memory` table. Nothing reads that table, so a memory created from
the phone was invisible in both the mobile list and the web UI, and could not
be deleted.

Route both writes through the same MemoryManager, mirroring the desktop
routes/memory/memory_routes idiom: build with add_entry, read-modify-write via
load_all_for_update + save, exact-owner check on delete (404, not 403, so the
caller can't probe for someone else's memory). A read failure mid-cycle raises
503 instead of appending to an empty view and atomically persisting it over the
whole store (issue odysseus-dev#5673).

Tests exercise the real MemoryManager over a tmp dir, so the read and write
paths must agree: create-then-list, cross-owner invisibility + undeletability,
and the unreadable-store refusal.
A paired mobile client needs to reach the companion data routes
(notes/tasks/memory) which gate on a narrower "companion" token scope.
The pairing flow minted a "chat"-only token, so a paired phone would
authenticate for chat but 403 on those data endpoints despite owning the
data.

Mint the pairing token with both scopes ("chat,companion"); the auth
middleware already splits scopes on commas into a list. Update the /pair
page copy and mint_token docstring, and the minting test.

Pairs with the companion data routes in odysseus-dev#881.
… grant

COMPANION_SCOPE is the comma-separated grant a pairing token is minted with
("chat,companion"); the auth middleware splits it into a scope list. Since
odysseus-dev#4319, require_models_scope compared COMPANION_SCOPE itself against that list —
which never contains it as a single element — so once the grant stopped being
the bare string "chat", every freshly paired device got 403 on
/api/companion/models and could not finish pairing.

Name the individual capabilities (CHAT_SCOPE / DATA_SCOPE) alongside the grant,
add granted_scopes() for the expansion, and gate model inventory on CHAT_SCOPE.
A token that lacks chat is still refused.
…scope docstring

The .gitignore change rode in with the admin tier and has nothing to do with
the companion bridge. The package docstring still described the pairing token
as chat-scoped after the grant became chat+companion.
@github-actions github-actions Bot added the ready for review Description complete — ready for maintainer review label Aug 14, 2026
Comment thread companion/mobile_features.py Fixed
Comment thread companion/routes.py Fixed
Comment thread companion/routes.py Fixed
Comment thread companion/routes.py Fixed
Comment thread companion/routes.py Fixed
…elper

research_owns() and the /research/result fallback each joined
"data/deep_research"/<session_id>.json by hand. That duplicated the session-id
rule and, unlike research_handler._research_json_path(), skipped the
resolve()-and-assert-containment step — so the two could drift, and the guard
lived in the callers rather than in the helper that builds the path.

Use _research_json_path() in both places: one source of truth for the id shape
and the containment check. The routes still validate up front; the helper no
longer depends on them doing so.

Tests cover traversal-shaped ids straight into research_owns and pin the call
to the canonical helper.
Comment thread companion/routes.py Fixed
Comment thread companion/routes.py Fixed
Removes the admin-gated tools (terminal/exec, vault status+unlock, MCP servers,
cookbook state, contacts, /admin/status) and the `companion_admin_enabled`
opt-in toggle with its Settings UI.

A paired device is narrow integration access. Letting it reach a shell — even
behind an off-by-default setting, an explicit scope, and an owner-is-admin
check — bundles an RCE surface into a bridge whose place in core is still being
discussed, and it is the one part of this PR that is not needed for the mobile
companion loop (pair → discover → chat → notes/tasks/memory → research). It
belongs behind its own opt-in, proposed on its own terms.

The tier guard in test_companion_mobile_features stays and now asserts the
admin paths are never exposed by the bridge at all, so they can't creep back in
unnoticed.
@mahdi-salmanzade

Copy link
Copy Markdown
Contributor Author

On the CodeQL check

Two rounds here, so recording what changed and what's left.

The critical alert is gone. It was POST /api/companion/terminal/execsubprocess.run(cmd, shell=True). Rather than argue the gating, I dropped the entire admin-privileged tier (terminal, vault, MCP, cookbook, contacts, /admin/status) and its opt-in toggle. A paired device is narrow integration access and shouldn't inherit an admin account's capabilities — that's the concern in #5674 and I think it's correct. test_companion_mobile_features now asserts those paths are never registered, so they can't return unnoticed.

The 11 remaining alerts are all one pattern, and I believe they're false positives:

  • 4 in companion/routes.py (lines 239, 242, 934, 940) — path.exists() / path.read_text() on a research report. Every one is reached only after src.research_handler._research_json_path() returns non-None, which (a) fullmatches the session id against _RESEARCH_SESSION_ID_RE, (b) resolve()s the result, and (c) asserts relative_to(root) containment under the research data dir. It returns None otherwise, and both call sites bail on None. This PR actually added that guard — the previous revision joined Path("data/deep_research") / f"{session_id}.json" by hand and skipped the containment check (commit "resolve research report paths through research_handler's helper"). test_companion_research drives traversal-shaped ids (../../etc/passwd, NUL bytes, a/../../secret) straight into research_owns and asserts False, plus pins the call to the canonical helper.
  • 7 in src/research_handler.py (57, 432, 434, 473, 475, 497, 499) — this PR does not modify that file (git diff dev..HEAD -- src/research_handler.py is empty). They're flagged because the companion routes are a new taint source reaching sinks that already existed behind the stock /api/research/* routes. Those sinks sit behind the same _research_json_path() barrier.

So it's one unmodelled sanitizer, counted 11 times: CodeQL isn't recognising _research_json_path() as a barrier, which is also why the same lines flag inside research_handler.py itself despite its inline resolve() + relative_to().

I'd rather not paper over it with # nosec-style suppressions or restructure working code purely to satisfy the analyser. If a maintainer would prefer either a CodeQL path-sanitizer model for _research_json_path() (a small .ql/model-pack addition, reusable by the stock research routes too) or inlining the resolve-and-contain check at each call site, say which and I'll push it.

The shipped client calls nine endpoints that no server has ever served, so the
matching controls are inert against upstream:

- POST /upload, GET /upload/{id}       — chat attachments (+ cached thumbnails)
- POST /tasks/{id}/{run,pause,resume,stop} — task controls
- POST /gallery/image/{id}/favorite    — the gallery star
- POST /email/summarize, /email/ai-reply — email AI actions

All nine follow the same rules as the rest of the bridge: the companion scope is
required, the caller is resolved to the token's real owner, and ownership is
strict — a cross-owner or legacy null-owner row is 404, never 403, so a caller
can't probe for someone else's task, image, or file. Upload refuses a bearer
token with no resolvable owner rather than writing a null-owner ("shared")
attachment, and the serve path denies a file with no metadata entry instead of
treating an unprovable owner as shared. Email AI asserts account ownership
before resolving any endpoint, so a caller cannot borrow another owner's key.

setup_mobile_companion_routes now takes the app's upload_handler and
task_scheduler; both are optional and their absence degrades to 503 rather than
crashing.

Deliberately still absent: everything admin-privileged, including
GET /cookbook/models, which the client only reaches from its Admin screen.
mahdi-salmanzade added a commit to mahdi-salmanzade/odysseus-mobile that referenced this pull request Aug 14, 2026
… file

The README has declared MIT since day one but the repo never carried a LICENSE
file, so GitHub reported it as unlicensed — i.e. all-rights-reserved — which
contradicts the 'public and free, take it and fork it' promise. Add the actual
MIT text.

The companion-bridge section still described one-branch-per-PR against the old
mahdi-salmanzade/odysseus fork. Those ten PRs were closed on 2026-07-23 when
GitHub detached upstream from its fork network, and that fork is now orphaned in
a different network. Point at the rebuilt consolidated PR
(odysseus-dev/odysseus#6038) and the fork it comes from instead, note that every
non-admin endpoint is now served, and be explicit that the Admin screen is
deliberately excluded upstream. Also update the renamed upstream org links.
Comment thread companion/mobile_features.py Fixed
Comment thread companion/mobile_features.py Fixed
Comment thread companion/mobile_features.py Fixed
Comment thread companion/mobile_features.py Fixed
Comment thread companion/mobile_features.py Fixed
Comment thread companion/mobile_features.py Fixed
f"Subject: {subject}\n\nReply to this email:\n\n{original_body[:12000]}",
1024,
)
return {"reply": (reply or "").strip()}
f"Subject: {subject}\n\n{body[:12000]}",
400,
)
return {"summary": (summary or "").strip()}
… return

The email AI helpers re-raised whatever the provider raised, so an upstream
failure put its error text — which can carry the endpoint's internal base_url or
other deployment detail — straight into the paired device's response body. Log
the real error server-side and answer with a generic 502 instead. HTTPExceptions
we raise deliberately (503 "no endpoint configured") still pass through.

Also removes an unreachable duplicate `return router` left at the end of the
module when the new handlers were added.
Comment thread companion/mobile_features.py Fixed
Comment thread companion/mobile_features.py Fixed
…ted text

The previous pass let HTTPException through untouched, which missed the actual
leak: the provider layer raises HTTPException(503, "Upstream <host>:<port>
marked unreachable (cooldown active)"), so a paired phone was still being handed
the server's internal endpoint host. Every failure from the call now becomes a
generic 502; our own pre-call 503 is raised before the try and is unaffected.

The log line now records only the exception type, not the exception text or the
owner, so a provider message carrying the failing URL or request body can't land
in the log either.

Tests cover both shapes: a bare exception and a provider HTTPException whose
detail names the upstream host.
@mahdi-salmanzade

Copy link
Copy Markdown
Contributor Author

CodeQL round 2 — it caught two real bugs

Completing the client's non-admin surface added new sinks, and the scan earned its keep. Both of these are now fixed with tests:

1. The upstream endpoint host was leaking to the phone. The email AI helpers called through llm_call_async_with_fallback, which re-raises the provider's exception. That includes HTTPException(503, "Upstream <host>:<port> marked unreachable (cooldown active)") — so a paired device was being handed the server's internal endpoint host. My first attempt made it worse by explicitly re-raising HTTPException untouched. Now every failure out of that call becomes a generic 502; the deliberate pre-call 503 ("No model endpoint configured") is raised before the try and is unaffected. Two tests cover it: a bare exception, and a provider HTTPException whose detail names the host.

2. Tainted text was going into the log. The handler logged the exception message and the owner. It now logs the exception type only, so a provider message carrying the failing URL or the request body can't reach the log either. (This one was CodeQL's "clear-text logging" alert — gone now.)

A dead return router left behind when the handlers were added is also removed.

The remaining 19 alerts are the same unmodelled-sanitizer pattern as before, in two groups:

  • 11 × research report path — 7 in src/research_handler.py and 4 in companion/routes.py. Unchanged from my earlier comment: every one is behind _research_json_path() (regex fullmatchresolve()relative_to(root)), and src/research_handler.py is not modified by this PR at all.
  • 6 × attachment path + 2 × exception exposure in companion/mobile_features.py — the GET /upload/{id} serve path is gated by upload_handler.validate_upload_id(file_id) before any join and upload_handler.inside_base_dir(path) after it, and denies a file with no metadata entry rather than treating an unprovable owner as shared. The two exception-exposure flows run through routes/email_helpers._assert_owns_account, which is also unchanged by this PR and only ever raises generic details ("Account not found", "Account check failed").

So: the two findings that were real are fixed, and what's left is CodeQL not recognising _research_json_path() / validate_upload_id() as barriers. Same offer as before — I'll add a sanitizer model for those helpers, or inline the checks at each call site, if a maintainer tells me which they'd rather review. I'd prefer not to suppress them.

…s string

CodeQL's 19 path-injection / exception-exposure alerts all came from the same
shape: validate the caller's value, then use that same value to build a path or
index a store. The check was correct, but the guarantee lived in the call order,
so every future edit had to remember to keep it — and a scanner can't see it.

Invert it. The caller's string is now only ever COMPARED, and what flows onward
is the copy the server already had:

- canonical_research_sid() matches the requested id against the in-flight task
  table and the research directory listing, and returns the key/stem it found.
  research_owns and all four /research/* routes use that value for every handler
  lookup and path build, so an unknown id never reaches the filesystem at all.
- The attachment serve path is built from the name os.walk returned, and the
  thumbnail path from that stored name, rather than from {file_id}.
- The email AI ownership gate re-raises a fresh HTTPException with a fixed
  message, so nothing derived from a caught error can travel to the phone.

Behaviour is unchanged: same 400 for a malformed id, same 404 for unknown or
cross-owner, same payloads. Tests assert the stronger invariant — an unknown id
never reaches the path helper, and a known one resolves to the server's own
object.
@mahdi-salmanzade

Copy link
Copy Markdown
Contributor Author

CodeQL is green — resolved properly, not suppressed

All 17 high-severity path-injection alerts are gone. No # nosec, no query filters, no config changes.

They all had one shape: validate the caller's value, then use that same value to build a path or index a store. The checks were correct, but the guarantee lived in the call order — every future edit had to remember to keep it, and a scanner can't see it. So I inverted it. The caller's string is now only ever compared; what flows onward is the copy the server already had:

  • canonical_research_sid() matches the requested id against the in-flight task table and the research directory listing, and returns the key/stem it found there. research_owns and all four /research/* routes use that value for every handler lookup and path build — so an id this server doesn't already know never reaches the filesystem at all. This is what cleared the 7 alerts in src/research_handler.py too: the taint no longer crosses the boundary into it.
  • The attachment serve path is built from the name os.walk returned, and the thumbnail path from that stored name, instead of from {file_id}.

Behaviour is unchanged — same 400 for a malformed id, same 404 for unknown or cross-owner, same payloads — and the tests now assert the stronger invariant directly: an unknown id never reaches the path helper, and a known one resolves to the server's own string object.

Two medium alerts remain (py/stack-trace-exposure, lines 958/988) and no longer fail the check. Both trace through the email AI ownership gate, which I deliberately left as-is: it re-raises a fresh exception with a fixed message, but it does derive the status code from the caught one, to keep 404 ("not your account") distinct from 503 ("check failed"). Collapsing them into a single status would silence the alert at the cost of a genuinely useful distinction for the client, so I'd rather keep the behaviour and leave the two mediums visible. Happy to change that if a reviewer disagrees.

Worth recording that CodeQL earned this round: earlier passes on this branch, it caught a real leak of the server's internal endpoint host to the paired device, and tainted text going into the log. Both are fixed above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Description complete — ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants