3 Commits

Author	SHA1	Message	Date
xiaodu55	16f154f32b	fix(gateway): preserve owner isolation when thread metadata is missing (#5484) ... * fix(gateway): preserve owner isolation when thread metadata is missing Follow-up to the #5448 review P1 (post-merge finding): owner_check=True also authorizes threads whose meta row is missing (legacy compatibility) or NULL-owner (shared/pre-auth data). _run_scope_user_id returned None for every trusted internal caller, which dropped the only remaining per-user filter on those threads and let an internal caller acting for owner A read owner B's persisted runs. _run_scope_user_id now takes the thread_id and consults the thread meta store: when an existing meta row establishes ownership, the authorized thread's runs are still read unfiltered (merged #5448 semantics, including owner-header-less internal callers); when the meta row is missing or NULL-owner, the filter falls back to the acting owner's raw stamp (the exact value start_run writes) — or the synthetic "default" identity without an owner header — so cross-user runs stay hidden. Isolation coverage uses the real MemoryThreadMetaStore with no metadata row (and a NULL-owner row) plus another user's persisted run: /runs and /runs/page must be empty and /runs/{run_id} must 404 for internal callers, while an established-ownership thread keeps the unfiltered read. * fix(gateway): gate run-scoped sub-resource reads for internal callers Review follow-up on #5484: the P1 owner-isolation class remained reachable through run-scoped sibling reads that apply no per-user filter at all — /runs/{run_id}/messages, /events, /join, /stream and /workspace-changes query by (thread_id, run_id) directly, so on missing/NULL-owner threads an internal caller acting for owner A could still read owner B's run content by id (verified 200 at the previous head). - Extract _thread_ownership_established (shared meta-row check) and add _require_run_visible_to_scope: for internal callers on threads without established ownership, the run's own user_id stamp must match the acting owner's raw value (or the legacy "default" stamp) or the read 404s. Established-ownership threads and every non-internal caller keep their existing thread-scoped semantics. - Wire the gate into join, stream, messages, events and workspace-changes; reword the now-stale messages comment to track the new scoping semantics. Regression tests: sub-resource reads 404 for a mismatched internal owner while the matching owner reads them normally, and the owner-less fallback branch (synthetic "default" filter on missing-meta threads) is pinned. Red confirmed against the pre-gate head. * fix(gateway): gate cancel and artifact archive for internal callers Review follow-up on #5484 round 2: POST /cancel resolved runs unscoped (require_existing=True only closes the missing-meta case — NULL-owner meta rows still pass), so an internal caller acting for a different owner could interrupt another owner's active run on a shared thread while /join and /stream were already gated. The archive manifest and download pair likewise leaked the other owner's delivered-file count and a 200-vs-409 delivery oracle on NULL-owner threads (missing-meta threads were already denied by require_existing=True). All three routes now call _require_run_visible_to_scope; its docstring records the extended coverage. NULL-owner-thread regression tests pin: a mismatched internal owner gets 404 from cancel, manifest and archive download, while the acting owner reaches the real conflict path (409 on a terminal run) and reads the manifest (file_count 2). * fix(gateway): tolerate state-less request stand-ins in the scope helpers The new owner-isolation gate and _run_scope_user_id read request.state directly, which crashed the FakeRequest-based unit suites for the run events, workspace-changes and scope endpoints (backend-unit-tests shards 1/2/4 on #5484). Read the state object defensively first: a request without state is simply not an internal caller, so those paths keep their pre-gate semantics. * fix(gateway): scope the thread token-usage aggregate by owner Review follow-up on #5484 round 4: GET /{thread_id}/token-usage called aggregate_tokens_by_thread(thread_id) with no user filter at all, so on missing/NULL-owner threads an internal caller acting for owner A read owner B's spend, model names, run count and (with include_active=true) live activity; the NULL-owner variant reached browser sessions too. build_context_usage's latest-model lookup was unfiltered as well. aggregate_tokens_by_thread gains an optional user_id (mirroring list_by_thread: explicit None = unfiltered, AUTO resolves the contextvar) in the memory store, the SQL repository and the store base; build_context_usage/_resolve_thread_model_name thread the scope through the latest-run lookup; the token-usage endpoint passes _run_scope_user_id's value. Established-ownership threads aggregate unfiltered as before; shared/missing-meta threads narrow to the acting identity. Stale helper-test comment reworded after the #5482 merge adaptation. * test(gateway): pin the unfiltered aggregate on established-ownership threads Review follow-up on #5484 round 5: the established-ownership branch of the token-usage scoping (store receives user_id=None) was the only unpinned half of the contract — the round-4 call-assertions never set app.state.thread_store, so their None came from the user-less stand-in path. test_token_usage_unfiltered_on_established_ownership_for_ internal_callers seeds an established meta row plus runs stamped by two different identities and asserts the totals fold (166 = 111 + 55); together with the isolation tests it now catches both failure modes (always-stamp narrowing and always-None leak).	2026-09-18 09:39:14 +08:00
xiaodu55	e493390aea	fix(gateway): scope edit/regenerate helper fallbacks by data identity (#5483) ... The message edit/regenerate prepare chain resolves source runs through _resolve_run_id_for_message, _require_successful_source_run and _find_interrupted_target_run_id, which filtered by the authorization identity (get_current_user) — the same conflation fixed for the runs and messages read endpoints in #5448, left out of scope there. For trusted internal callers the authorization identity never matches the raw owner-stamped run rows, so regenerate/edit-regenerate prepare fails with 409 on threads the caller is authorized on (#5482). The three helpers now resolve their filter id through _run_scope_user_id as well; both prepare endpoints keep their owner_check=True authorization and browser/API sessions keep the per-user filter. Regression tests extend test_thread_runs_internal_scope.py to the helper fallback paths: internal callers resolve raw-owner-stamped runs (including the interrupted-run and status-409 paths), browser sessions keep the per-user filter and 409 on cross-user runs.	2026-09-16 23:32:57 +08:00
xiaodu55	0745fb268f	fix(gateway): scope runs read endpoints by data identity, not authorization identity (#5448) ... * fix(gateway): scope runs read endpoints by data identity, not authorization identity Trusted internal callers are authorized as a synthetic internal user (id "default", or the make_safe_user_id-normalized owner with an owner header), while start_run stamps run rows with the raw trusted-owner value. list_runs / get_run / list_runs_page filtered by the authorization identity, so the store-side user filter never matched and internal callers always saw an empty runs list (or 404) for threads they are authorized to read. The three read endpoints now resolve their filter id through _run_scope_user_id: internal-role callers skip the per-user filter (thread visibility is already authorized by owner_check=True), and browser/API sessions keep the existing per-user filter unchanged. Regression tests cover both identities across the three endpoints (list, keyset page, single get) with a MemoryRunStore seeded with mixed-owner rows; without the fix the four internal-caller cases fail while the browser-session isolation case passes. * fix(gateway): route the message read endpoints through the same data-identity scoping Review follow-up on #5448: list_thread_messages and list_thread_messages_page resolved get_current_user and passed it as the data filter to the event-store scan, hidden-run lookups, turn-duration injection and the feedback queries — the same authorization-vs-data identity conflation fixed for the runs endpoints, leaving the #5437 empty-read symptom in place for lossy owner values. Both endpoints now resolve their filter id through _run_scope_user_id as well. Regression tests extend to the two message endpoints, asserting the resolved filter identity at the runs-store and feedback-repo boundaries (None for internal callers, the session user id for browser sessions). * fix(feedback): deterministic per-run collapse for unfiltered feedback reads Review follow-up on #5448: with _run_scope_user_id returning None for internal callers, the feedback lookups now receive an explicit-None user id, which skips the user_id WHERE in FeedbackRepository. On shared/NULL-owner threads several browser users can hold feedback on the same run, and list_by_thread_grouped / list_by_run_ids collapsed rows per run_id via a dict comprehension over unordered results — the feedback attached to the last AI message would be an arbitrary user's row. Both methods now order by created_at ASC with feedback_id as the tie-break, so the collapse deterministically keeps the most recently created feedback. _run_scope_user_id's docstring now documents that the resolved id also scopes feedback and event-store reads, not just run rows. Regression test seeds multi-user feedback on one run and asserts the collapse outcome is stable across repeated unfiltered reads. * docs(feedback): the collapse keeps the most recently written feedback created_at is refreshed on upsert, so the surviving row per run is the most recently written (created or updated), not the most recently created — align both docstrings with the ordering key's actual semantics.	2026-09-16 15:56:25 +08:00