mosaicstack/stack
1
0
Fork 0
Code Issues 36 Pull Requests 2 Actions Packages Projects Releases 15 Wiki Activity

feat(#462): add federation list verb #682

Merged
jason.woltje merged 1 commits from feat/federation-m3-verb-list into main 2026-06-25 02:15:18 +00:00
Conversation 5 Commits 1 Files Changed 6 +1228 -1
jason.woltje commented 2026-06-24 23:42:24 +00:00
Owner
Copy Link

Summary

  • Implements FED-M3-05: POST /api/federation/v1/list/:resource
  • Wires FederationAuthGuard → merged M3-04 FederationScopeService → read-only list query layer
  • Applies grant/native-RBAC scope intersection, row cap, keyset cursor pagination, and _source: "local" tags
  • Keeps read audit writes deferred to M4 per federation plan; no request/response persistence

Depends on merged M3-04 scope service (#672).

Refs #462

Verification

  • pnpm typecheck — PASS (41/41)
  • pnpm lint — PASS (23/23)
  • pnpm format:check — PASS
  • pnpm --filter @mosaicstack/gateway test -- list.controller.spec.ts list-query.service.spec.ts — PASS (9 tests)
  • Codex code review — PASS after remediation
  • Codex security review — PASS after cursor remediation

Notes

  • Local full gateway test still depends on local PostgreSQL at localhost:5433; scoped FED-M3-05 tests pass and repo gates are green.
## Summary - Implements FED-M3-05: `POST /api/federation/v1/list/:resource` - Wires `FederationAuthGuard` → merged M3-04 `FederationScopeService` → read-only list query layer - Applies grant/native-RBAC scope intersection, row cap, keyset cursor pagination, and `_source: "local"` tags - Keeps read audit writes deferred to M4 per federation plan; no request/response persistence Depends on merged M3-04 scope service (#672). Refs #462 ## Verification - `pnpm typecheck` — PASS (41/41) - `pnpm lint` — PASS (23/23) - `pnpm format:check` — PASS - `pnpm --filter @mosaicstack/gateway test -- list.controller.spec.ts list-query.service.spec.ts` — PASS (9 tests) - Codex code review — PASS after remediation - Codex security review — PASS after cursor remediation ## Notes - Local full gateway test still depends on local PostgreSQL at `localhost:5433`; scoped FED-M3-05 tests pass and repo gates are green.
jason.woltje added 1 commit 2026-06-24 23:42:24 +00:00
feat(#462): add federation list verb
Some checks failed
ci/woodpecker/push/ci Pipeline was canceled
Details
ci/woodpecker/pr/ci Pipeline was successful
Details
6e37ab00b9
jason.woltje commented 2026-06-24 23:45:58 +00:00
Author
Owner
Copy Link

REVIEW-OF-RECORD — REQUEST CHANGES (#682, head 6e37ab00b9). BLOCKER: notes list can leak other users' mission_tasks.notes for team-scoped missions. In FederationListQueryService.listNotes(), when filter.teamIds is non-empty, the query adds missionTasks.missionId IN (team mission ids) as an OR branch without also constraining missionTasks.userId to filter.subjectUserId. The schema explicitly treats mission_tasks as user-scoped for multi-tenant RBAC isolation, and the normal gateway mission-task path filters by mission AND user. As written, a grant with include_teams for notes can return every user's notes on accessible team missions, widening beyond the subject user. Please keep missionTasks.userId = subjectUserId on all missionTasks note rows (or otherwise prove/document a distinct team-owned notes model) and add regression tests for: (1) team-scoped notes do not include another user's missionTasks rows, and (2) include_personal:false does not return subject personal notes unless an explicitly allowed team-owned note model exists.

Reviewed positives: controller route/auth metadata is correct; request limit/cursor validation happens before scope/query; ScopeService deny maps to federation error envelope and does not query storage; row cap/pagination uses limit+1 and opaque keyset cursors; _source: local tagging is applied; sensitive credentials/api_keys are denied by native RBAC for M3. Reviewer verification: scoped tests list.controller.spec.ts list-query.service.spec.ts pass 9/9; gateway typecheck pass; gateway lint pass; prettier check pass. CI may continue running, but merge should wait on the notes isolation fix.

REVIEW-OF-RECORD — REQUEST CHANGES (#682, head 6e37ab00b99ca76b4bcb2fe0565d016b5051b18b). BLOCKER: `notes` list can leak other users' `mission_tasks.notes` for team-scoped missions. In `FederationListQueryService.listNotes()`, when `filter.teamIds` is non-empty, the query adds `missionTasks.missionId IN (team mission ids)` as an OR branch without also constraining `missionTasks.userId` to `filter.subjectUserId`. The schema explicitly treats `mission_tasks` as user-scoped for multi-tenant RBAC isolation, and the normal gateway mission-task path filters by mission AND user. As written, a grant with `include_teams` for notes can return every user's notes on accessible team missions, widening beyond the subject user. Please keep `missionTasks.userId = subjectUserId` on all missionTasks note rows (or otherwise prove/document a distinct team-owned notes model) and add regression tests for: (1) team-scoped notes do not include another user's missionTasks rows, and (2) `include_personal:false` does not return subject personal notes unless an explicitly allowed team-owned note model exists. Reviewed positives: controller route/auth metadata is correct; request `limit`/`cursor` validation happens before scope/query; ScopeService deny maps to federation error envelope and does not query storage; row cap/pagination uses limit+1 and opaque keyset cursors; `_source: local` tagging is applied; sensitive `credentials`/`api_keys` are denied by native RBAC for M3. Reviewer verification: scoped tests `list.controller.spec.ts list-query.service.spec.ts` pass 9/9; gateway typecheck pass; gateway lint pass; prettier check pass. CI may continue running, but merge should wait on the notes isolation fix.
jason.woltje force-pushed feat/federation-m3-verb-list from 6e37ab00b9 to b38b9846c1 2026-06-24 23:55:54 +00:00 Compare
jason.woltje commented 2026-06-24 23:58:45 +00:00
Author
Owner
Copy Link

INDEPENDENT REVIEW-OF-RECORD — REQUEST-CHANGES (reviewed by an independent code-review agent dispatched by Mos; this review applies to commit 6e37ab00 — note a newer commit b38b9846 has since been pushed and needs the same scrutiny / may already address some of these).

This PR has BLOCKING issues; CI being green does not make the diff sound. Do not merge until resolved.

CRITICAL (must fix):

  1. Cross-scope data leak in listNotes (list-query.service.ts) — when includePersonal is true, the personal clause eq(missionTasks.userId, subjectUserId) is OR'd with the mission-scoped inArray(missionTasks.missionId, missionIds). The OR means "all notes created by the subject ANYWHERE, OR all notes in authorized missions" — a federation peer can enumerate notes the subject created in missions outside the authorized grant scope. The personal clause must be INTERSECTED with the authorized mission set (AND), not OR'd — or removed entirely in favor of project/mission scope only.
  2. listMemory pagination is incorrect across pages — two parallel sub-queries each apply the same keyset cursor against separate tables, merged + re-sorted in memory; the emitted cursor cannot be deterministically resumed, so rows are duplicated/skipped at page boundaries (equal timestamps interleave). Either paginate the two resources separately, or return a single deterministic capped block.
  3. Missing notes test coverage — the resource with the most complex scoping (and the leak above) has ZERO tests. Add specs proving unauthorized-mission notes are excluded and includePersonal does not over-share.

IMPORTANT (fix concurrently):
4. list.controller.ts missing-context path throws a plain Error → 500 leaking internal message; use a federation HttpException envelope (FederationUnauthorizedError/InvalidRequest).
5. encodeCursor returns undefined for non-Date createdAt while hasMore is true → silent pagination truncation (peer sees _truncated with no nextCursor, cannot continue). Throw instead.
6. listAllRows switch has no default → returns undefined for unknown resource → paginate(undefined) crash. Add default: throw.

The federation auth guard, scope delegation (per #672), and sensitive-resource (credentials/api_keys) denial are all sound. The blocker is the notes scope leak. Please address #1–#3 (and ideally #4–#6) and re-push; I'll re-review and shepherd on a clean solo green CI run.

INDEPENDENT REVIEW-OF-RECORD — REQUEST-CHANGES (reviewed by an independent code-review agent dispatched by Mos; this review applies to commit 6e37ab00 — note a newer commit b38b9846 has since been pushed and needs the same scrutiny / may already address some of these). This PR has BLOCKING issues; CI being green does not make the diff sound. Do not merge until resolved. CRITICAL (must fix): 1. **Cross-scope data leak in `listNotes` (list-query.service.ts)** — when `includePersonal` is true, the personal clause `eq(missionTasks.userId, subjectUserId)` is OR'd with the mission-scoped `inArray(missionTasks.missionId, missionIds)`. The OR means "all notes created by the subject ANYWHERE, OR all notes in authorized missions" — a federation peer can enumerate notes the subject created in missions outside the authorized grant scope. The personal clause must be INTERSECTED with the authorized mission set (AND), not OR'd — or removed entirely in favor of project/mission scope only. 2. **`listMemory` pagination is incorrect across pages** — two parallel sub-queries each apply the same keyset cursor against separate tables, merged + re-sorted in memory; the emitted cursor cannot be deterministically resumed, so rows are duplicated/skipped at page boundaries (equal timestamps interleave). Either paginate the two resources separately, or return a single deterministic capped block. 3. **Missing `notes` test coverage** — the resource with the most complex scoping (and the leak above) has ZERO tests. Add specs proving unauthorized-mission notes are excluded and `includePersonal` does not over-share. IMPORTANT (fix concurrently): 4. `list.controller.ts` missing-context path throws a plain `Error` → 500 leaking internal message; use a federation HttpException envelope (FederationUnauthorizedError/InvalidRequest). 5. `encodeCursor` returns `undefined` for non-Date createdAt while `hasMore` is true → silent pagination truncation (peer sees _truncated with no nextCursor, cannot continue). Throw instead. 6. `listAllRows` switch has no `default` → returns `undefined` for unknown resource → `paginate(undefined)` crash. Add `default: throw`. The federation auth guard, scope delegation (per #672), and sensitive-resource (credentials/api_keys) denial are all sound. The blocker is the notes scope leak. Please address #1–#3 (and ideally #4–#6) and re-push; I'll re-review and shepherd on a clean solo green CI run.
jason.woltje commented 2026-06-24 23:59:43 +00:00
Author
Owner
Copy Link

REVIEW-OF-RECORD — APPROVE (#682 FED-M3-05 list verb, head b38b9846c1). Re-review confirms the prior notes isolation blocker is closed: listNotes() now constrains every mission_tasks note row by missionTasks.userId = subjectUserId AND accessible mission IDs, so team scope narrows allowed missions but does not widen to other users' mission task notes. Added PGlite regressions cover both another-user team mission note exclusion and includePersonal:false excluding subject personal mission notes.

Trust-boundary review: route is POST /api/federation/v1/list/:resource under FederationAuthGuard; request limit/cursor validation happens before storage access; FederationScopeService remains the allow/deny gate with query service as native RBAC evaluator; scope deny returns federation error envelopes and does not query storage; sensitive credentials/api_keys are denied in M3 native RBAC; rows are capped/paginated with opaque keyset cursor and tagged _source: local; no request/response persistence or audit writes (M4-deferred by plan). Reviewer verification on head b38b9846: scoped list tests 11/11 pass; gateway typecheck pass; gateway lint pass; prettier check pass. APPROVE.

REVIEW-OF-RECORD — APPROVE (#682 FED-M3-05 list verb, head b38b9846c170da1fb64eb49feebd6acacd2b2117). Re-review confirms the prior notes isolation blocker is closed: `listNotes()` now constrains every mission_tasks note row by `missionTasks.userId = subjectUserId` AND accessible mission IDs, so team scope narrows allowed missions but does not widen to other users' mission task notes. Added PGlite regressions cover both another-user team mission note exclusion and `includePersonal:false` excluding subject personal mission notes. Trust-boundary review: route is POST `/api/federation/v1/list/:resource` under `FederationAuthGuard`; request limit/cursor validation happens before storage access; `FederationScopeService` remains the allow/deny gate with query service as native RBAC evaluator; scope deny returns federation error envelopes and does not query storage; sensitive `credentials`/`api_keys` are denied in M3 native RBAC; rows are capped/paginated with opaque keyset cursor and tagged `_source: local`; no request/response persistence or audit writes (M4-deferred by plan). Reviewer verification on head b38b9846: scoped list tests 11/11 pass; gateway typecheck pass; gateway lint pass; prettier check pass. APPROVE.
jason.woltje commented 2026-06-25 00:03:37 +00:00
Author
Owner
Copy Link

ORCHESTRATOR — re-review delta + status (head 6e37ab00 -> b38b9846)

Reconciling the independent review (15945, vs 6e37ab00) against the newer head b38b9846 so you can re-review only the delta:

Already changed in b38b9846 (author-reported, addresses original 15940):

  • listNotes team branch now constrains missionTasks.userId = subjectUserId AND missionId IN (accessible mission ids).
  • +2 regression tests: (a) team-scoped notes exclude another user's missionTasks rows; (b) includePersonal:false excludes subject personal mission note.

Still OUTSTANDING from 15945 (author addressing locally now):

  • CRITICAL #1 — listNotes includePersonal personal clause OR'd with mission scope -> intersect (AND) or drop.
  • CRITICAL #2 — listMemory two-table merged keyset pagination non-deterministic -> paginate separately or single capped block.
  • IMPORTANT #4 — controller missing-fed-context plain Error -> HttpException envelope.
  • IMPORTANT #5 — encodeCursor silent-undefined truncation -> throw.
  • IMPORTANT #6 — listAllRows switch missing default -> add default throw.

Process: this PR is NOT mergeable. Author fixes #1-#6 locally; re-push is HELD — the shared Woodpecker runner is OOM-serialized (every push to an open-PR branch fires 2 pipelines that OOM-kill ci-postgres), so re-push happens only on a clean SOLO run once the orchestrator confirms the queue is idle. Will ping for re-review on the new head.

ORCHESTRATOR — re-review delta + status (head 6e37ab00 -> b38b9846) Reconciling the independent review (15945, vs 6e37ab00) against the newer head b38b9846 so you can re-review only the delta: Already changed in b38b9846 (author-reported, addresses original 15940): - listNotes team branch now constrains `missionTasks.userId = subjectUserId AND missionId IN (accessible mission ids)`. - +2 regression tests: (a) team-scoped notes exclude another user's missionTasks rows; (b) `includePersonal:false` excludes subject personal mission note. Still OUTSTANDING from 15945 (author addressing locally now): - CRITICAL #1 — listNotes `includePersonal` personal clause OR'd with mission scope -> intersect (AND) or drop. - CRITICAL #2 — listMemory two-table merged keyset pagination non-deterministic -> paginate separately or single capped block. - IMPORTANT #4 — controller missing-fed-context plain Error -> HttpException envelope. - IMPORTANT #5 — encodeCursor silent-undefined truncation -> throw. - IMPORTANT #6 — listAllRows switch missing `default` -> add default throw. Process: this PR is NOT mergeable. Author fixes #1-#6 locally; re-push is HELD — the shared Woodpecker runner is OOM-serialized (every push to an open-PR branch fires 2 pipelines that OOM-kill ci-postgres), so re-push happens only on a clean SOLO run once the orchestrator confirms the queue is idle. Will ping for re-review on the new head.
jason.woltje force-pushed feat/federation-m3-verb-list from b38b9846c1 to e483d976e4 2026-06-25 01:51:59 +00:00 Compare
jason.woltje commented 2026-06-25 01:54:30 +00:00
Author
Owner
Copy Link

REVIEW-OF-RECORD — APPROVE (#682, head e483d976e4). Re-review confirms the prior blockers are resolved.

Trust boundary: listNotes now constrains note rows with missionTasks.userId = subjectUserId AND missionTasks.missionId in authorized missionIds; there is no remaining OR branch that widens team visibility into another user's notes or into missions outside the grant-visible project/mission set. The new PGlite coverage includes both another-user notes and subject notes on an unauthorized mission, and the scoped tests prove those rows are excluded.

Pagination: listMemory now uses deterministic table-block pagination (insights first, preferences second) and encodes the cursor source for memory rows, so page boundaries do not reapply one table's keyset cursor to the other table. The regression with equal timestamps across insights/preferences returns two non-overlapping pages with no duplicate/skip. encodeCursor also now fails closed on non-Date rows instead of silently omitting nextCursor.

Other hardening verified: missing federation context returns a structured 401 unauthorized federation envelope; unsupported resources hit an explicit default throw; request validation still prevents malformed cursor/limit before storage access.

Reviewer verification on head e483d976: scoped gateway list tests 16/16 pass; gateway typecheck pass; gateway lint pass; prettier check pass for changed list/module/scratchpad files. APPROVE.

REVIEW-OF-RECORD — APPROVE (#682, head e483d976e40b8f35eae173f7e342af6d870f6e51). Re-review confirms the prior blockers are resolved. Trust boundary: listNotes now constrains note rows with `missionTasks.userId = subjectUserId` AND `missionTasks.missionId in authorized missionIds`; there is no remaining OR branch that widens team visibility into another user's notes or into missions outside the grant-visible project/mission set. The new PGlite coverage includes both another-user notes and subject notes on an unauthorized mission, and the scoped tests prove those rows are excluded. Pagination: listMemory now uses deterministic table-block pagination (insights first, preferences second) and encodes the cursor source for memory rows, so page boundaries do not reapply one table's keyset cursor to the other table. The regression with equal timestamps across insights/preferences returns two non-overlapping pages with no duplicate/skip. encodeCursor also now fails closed on non-Date rows instead of silently omitting nextCursor. Other hardening verified: missing federation context returns a structured 401 unauthorized federation envelope; unsupported resources hit an explicit default throw; request validation still prevents malformed cursor/limit before storage access. Reviewer verification on head e483d976: scoped gateway list tests 16/16 pass; gateway typecheck pass; gateway lint pass; prettier check pass for changed list/module/scratchpad files. APPROVE.
jason.woltje merged commit 86e106fcc9 into main 2026-06-25 02:15:18 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
jason.woltje
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#682
Delete Branch "feat/federation-m3-verb-list"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?