fix(backend): filter in-memory invocations by owner in list_wikis - #351
Merged
Conversation
The DB query in WikiManagementService.list_wikis correctly filters by owner_id / visibility, but the loop that appends active in-memory invocations iterated all invocations with no ownership check, leaking other users' in-progress and failed builds into every user's wiki list (visible in the dashboard, 404 when opened). Mirror the same rule: skip invocations owned by someone other than the caller. Legacy unowned invocations (owner_id=="") stay visible to all. Shared visibility is not tracked on Invocation (in-progress wikis have no visibility yet); completed shared wikis reach the list via the DB path which already handles them correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a dashboard data-leak where GET /api/v1/wikis could include in-progress (in-memory) invocations belonging to other users, resulting in visible-but-inaccessible wiki cards. The change aligns the in-memory invocation append logic with the DB-backed visibility/ownership rules already enforced by WikiManagementService.list_wikis.
Changes:
- Added ownership filtering when appending active/failed in-memory invocations to the wiki list response.
- Documented the intended visibility behavior for in-memory invocations (own + legacy unowned).
Comment on lines
441
to
+443
| for inv in service.invocations.values(): | ||
| if inv.owner_id and user_id and inv.owner_id != user_id: | ||
| continue |
Comment on lines
+439
to
+442
| # Owned invocations from other users are never shown — the caller would | ||
| # get a 404 trying to open them, so leaking them in the list is wrong. | ||
| for inv in service.invocations.values(): | ||
| if inv.owner_id and user_id and inv.owner_id != user_id: |
…_wikis leak - Normalize user_id with `or None` so an empty-string user.id (JWT missing sub) doesn't bypass the filter - Change guard from `and user_id` to `not user_id or inv.owner_id != user_id` so callers with no valid user_id also can't see other users' invocations (mirrors DB rule: anonymous sees only shared + legacy unowned) - Add two regression tests: one verifying the ownership split across three invocations (own / other-user / legacy), one covering the empty-string user_id edge case Flaky test note: test_generate_chapter::test_oversized_content_is_kept_not_split fails intermittently in CI under -n auto because generate_chapter runs sub-pages concurrently via asyncio.gather + run_in_executor, making SequentialFakeLLM response order non-deterministic. Pre-existing; unrelated to this change. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines
+523
to
+524
| "own": _make_foreign_invocation("wiki-own", owner_id="user-1"), | ||
| "other": _make_foreign_invocation("wiki-other", owner_id="user-2"), |
Comment on lines
+531
to
+534
| returned_ids = {w["wiki_id"] for w in resp.json()["wikis"]} | ||
| assert "wiki-own" in returned_ids, "caller's own invocation must be included" | ||
| assert "wiki-legacy" in returned_ids, "legacy unowned invocation must be included" | ||
| assert "wiki-other" not in returned_ids, "other user's invocation must be excluded" |
Comment on lines
+551
to
+554
| mock_service.invocations = { | ||
| "other": _make_foreign_invocation("wiki-other", owner_id="user-2"), | ||
| "legacy": _make_foreign_invocation("wiki-legacy", owner_id=""), | ||
| } |
Remove manual alignment spaces flagged by Ruff E221/E241/E272. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines
+398
to
399
| user_id = (user.id or None) if user else None | ||
| result = await management.list_wikis(user_id=user_id) |
Comment on lines
+437
to
+443
| # Add active/failed invocations not yet in completed list. | ||
| # Mirror the DB visibility rule: own wikis + legacy unowned (owner_id==""). | ||
| # Owned invocations from other users are never shown — the caller would | ||
| # get a 404 trying to open them, so leaking them in the list is wrong. | ||
| for inv in service.invocations.values(): | ||
| if inv.owner_id and (not user_id or inv.owner_id != user_id): | ||
| continue |
…t_wikis - Normalize user_id with `or None` in all routes (not just list_wikis) so an empty-string user.id can never bypass ownership checks - Fix get_wiki B2 redaction: change `and user_id and` to `and (not user_id or ...)` so the same guard pattern is used everywhere - Fix best_inv enrichment loop in list_wikis: skip invocations owned by other users when selecting status/progress to attach to a shared/legacy wiki, matching the B2 behaviour already in get_wiki Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
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.
Summary
WikiManagementService.list_wikis(DB query) correctly filters byowner_id / visibility, but the second loop in thelist_wikisroute that appends active in-memory invocations iterated all running jobs with no ownership checkowner_id=="") remain visible to all. Shared visibility is not tracked on theInvocationmodel (in-progress wikis have no visibility yet); completed shared wikis reach the list via the DB path which already handles them correctly.Test plan
owner_idstill appear for all users🤖 Generated with Claude Code