REVIEW-OF-RECORD — REQUEST CHANGES (#683, head 87a5f479ec01a33e057221778e5d187eafd1073a). The scoped happy-path tests pass, but the trust-boundary requirements for FED-M3-06 are not met yet.
CRITICAL #1 — getNote uses OR-widening instead of the M3-05 AND-intersect. In get-query.service.ts, the decision is hasPersonalAccess || hasTeamAccess (if (!hasPersonalAccess && !hasTeamAccess) deny). That leaks both directions: (a) any user's note on an authorized/team-visible mission is returned because missionIds.includes(row.missionId) is enough, and (b) the subject user's note on a mission outside the authorized mission/project set is returned because includePersonal && row.userId === subjectUserId is enough. This must mirror the approved list verb: missionTasks.userId = subjectUserId AND missionTasks.missionId in authorizedMissionIds, with no OR personal/team branch.
CRITICAL #2 — missing federation context still becomes a plain Error/500. GetController currently throws new Error('Federation context missing after auth guard'); the required behavior is the structured federation unauthorized envelope (401), matching ListController.
IMPORTANT #3 — unsupported resources do not fail closed. getByResource has no default branch; if an unexpected resource reaches it, query.get() can resolve undefined and the controller dereferences result.status, producing a 500 instead of an explicit deny/invalid-request path.
IMPORTANT #4 — coverage does not prove the trust boundary. The current get-query tests are mocked/unit guardrails only; there is no PGlite/scoped regression proving cross-user note exclusion or subject-note-on-unauthorized-mission exclusion. Please add the same kind of DB-backed regressions that closed#682 for listNotes, plus controller coverage for missing-fed-context -> 401 and unsupported resource fail-closed.
Reviewer verification on head 87a5f479: scoped get tests 10/10 pass; gateway typecheck pass; gateway lint pass; prettier check pass for changed get/module/scratchpad files. These gates are green locally, but they do not cover the blockers above. REQUEST CHANGES.
REVIEW-OF-RECORD — REQUEST CHANGES (#683, head 87a5f479ec01a33e057221778e5d187eafd1073a). The scoped happy-path tests pass, but the trust-boundary requirements for FED-M3-06 are not met yet.
CRITICAL #1 — getNote uses OR-widening instead of the M3-05 AND-intersect. In get-query.service.ts, the decision is `hasPersonalAccess || hasTeamAccess` (`if (!hasPersonalAccess && !hasTeamAccess) deny`). That leaks both directions: (a) any user's note on an authorized/team-visible mission is returned because `missionIds.includes(row.missionId)` is enough, and (b) the subject user's note on a mission outside the authorized mission/project set is returned because `includePersonal && row.userId === subjectUserId` is enough. This must mirror the approved list verb: `missionTasks.userId = subjectUserId` AND `missionTasks.missionId in authorizedMissionIds`, with no OR personal/team branch.
CRITICAL #2 — missing federation context still becomes a plain Error/500. GetController currently throws `new Error('Federation context missing after auth guard')`; the required behavior is the structured federation unauthorized envelope (401), matching ListController.
IMPORTANT #3 — unsupported resources do not fail closed. getByResource has no default branch; if an unexpected resource reaches it, `query.get()` can resolve undefined and the controller dereferences `result.status`, producing a 500 instead of an explicit deny/invalid-request path.
IMPORTANT #4 — coverage does not prove the trust boundary. The current get-query tests are mocked/unit guardrails only; there is no PGlite/scoped regression proving cross-user note exclusion or subject-note-on-unauthorized-mission exclusion. Please add the same kind of DB-backed regressions that closed #682 for listNotes, plus controller coverage for missing-fed-context -> 401 and unsupported resource fail-closed.
Reviewer verification on head 87a5f479: scoped get tests 10/10 pass; gateway typecheck pass; gateway lint pass; prettier check pass for changed get/module/scratchpad files. These gates are green locally, but they do not cover the blockers above. REQUEST CHANGES.
jason.woltje
changed target branch from main to next2026-06-25 02:46:42 +00:00
REVIEW-OF-RECORD — APPROVE (#683, head 80a259b206). Re-review confirms all prior blockers from comment 15971 are resolved.
Trust boundary: getNote now mirrors the approved list verb boundary. A note is returned only when row.userId === subjectUserId AND row.missionId is in the authorized mission IDs derived from grant/native-RBAC-visible projects. The old personal/team OR-widening branch is gone, so team scope no longer widens into another user's notes and personal subject ownership no longer bypasses the authorized mission/project set.
Error handling/fail-closed: missing federation context now returns a structured 401 unauthorized federation envelope, not a plain Error/500. getByResource now has an explicit default denial for unsupported resources, which the controller maps to a structured 403 scope violation instead of dereferencing undefined.
Coverage: the new PGlite regressions prove cross-user note exclusion, subject-note-on-unauthorized-mission exclusion, valid subject+authorized-mission intersection, and includePersonal=false exclusion. Controller tests cover missing-fed-context 401 and unsupported-resource fail-closed behavior.
Reviewer verification on head 80a259b2: scoped gateway get tests 17/17 pass; gateway typecheck pass; gateway lint pass; prettier check pass for changed get/module/scratchpad files. PR CI reported green on pipeline 1620. APPROVE.
REVIEW-OF-RECORD — APPROVE (#683, head 80a259b2061df1892a124a0a946049aa8ab78d4e). Re-review confirms all prior blockers from comment 15971 are resolved.
Trust boundary: getNote now mirrors the approved list verb boundary. A note is returned only when `row.userId === subjectUserId` AND `row.missionId` is in the authorized mission IDs derived from grant/native-RBAC-visible projects. The old personal/team OR-widening branch is gone, so team scope no longer widens into another user's notes and personal subject ownership no longer bypasses the authorized mission/project set.
Error handling/fail-closed: missing federation context now returns a structured `401 unauthorized` federation envelope, not a plain Error/500. `getByResource` now has an explicit default denial for unsupported resources, which the controller maps to a structured 403 scope violation instead of dereferencing undefined.
Coverage: the new PGlite regressions prove cross-user note exclusion, subject-note-on-unauthorized-mission exclusion, valid subject+authorized-mission intersection, and includePersonal=false exclusion. Controller tests cover missing-fed-context 401 and unsupported-resource fail-closed behavior.
Reviewer verification on head 80a259b2: scoped gateway get tests 17/17 pass; gateway typecheck pass; gateway lint pass; prettier check pass for changed get/module/scratchpad files. PR CI reported green on pipeline 1620. APPROVE.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
Implements FED-M3-06 get verb for inbound federation reads:
Verification
Notes
86e106fcc9after M3-05 merged.Refs #462
REVIEW-OF-RECORD — REQUEST CHANGES (#683, head 87a5f479ec01a33e057221778e5d187eafd1073a). The scoped happy-path tests pass, but the trust-boundary requirements for FED-M3-06 are not met yet.
CRITICAL #1 — getNote uses OR-widening instead of the M3-05 AND-intersect. In get-query.service.ts, the decision is
hasPersonalAccess || hasTeamAccess(if (!hasPersonalAccess && !hasTeamAccess) deny). That leaks both directions: (a) any user's note on an authorized/team-visible mission is returned becausemissionIds.includes(row.missionId)is enough, and (b) the subject user's note on a mission outside the authorized mission/project set is returned becauseincludePersonal && row.userId === subjectUserIdis enough. This must mirror the approved list verb:missionTasks.userId = subjectUserIdANDmissionTasks.missionId in authorizedMissionIds, with no OR personal/team branch.CRITICAL #2 — missing federation context still becomes a plain Error/500. GetController currently throws
new Error('Federation context missing after auth guard'); the required behavior is the structured federation unauthorized envelope (401), matching ListController.IMPORTANT #3 — unsupported resources do not fail closed. getByResource has no default branch; if an unexpected resource reaches it,
query.get()can resolve undefined and the controller dereferencesresult.status, producing a 500 instead of an explicit deny/invalid-request path.IMPORTANT #4 — coverage does not prove the trust boundary. The current get-query tests are mocked/unit guardrails only; there is no PGlite/scoped regression proving cross-user note exclusion or subject-note-on-unauthorized-mission exclusion. Please add the same kind of DB-backed regressions that closed #682 for listNotes, plus controller coverage for missing-fed-context -> 401 and unsupported resource fail-closed.
Reviewer verification on head 87a5f479: scoped get tests 10/10 pass; gateway typecheck pass; gateway lint pass; prettier check pass for changed get/module/scratchpad files. These gates are green locally, but they do not cover the blockers above. REQUEST CHANGES.
87a5f479ecto80a259b206REVIEW-OF-RECORD — APPROVE (#683, head
80a259b206). Re-review confirms all prior blockers from comment 15971 are resolved.Trust boundary: getNote now mirrors the approved list verb boundary. A note is returned only when
row.userId === subjectUserIdANDrow.missionIdis in the authorized mission IDs derived from grant/native-RBAC-visible projects. The old personal/team OR-widening branch is gone, so team scope no longer widens into another user's notes and personal subject ownership no longer bypasses the authorized mission/project set.Error handling/fail-closed: missing federation context now returns a structured
401 unauthorizedfederation envelope, not a plain Error/500.getByResourcenow has an explicit default denial for unsupported resources, which the controller maps to a structured 403 scope violation instead of dereferencing undefined.Coverage: the new PGlite regressions prove cross-user note exclusion, subject-note-on-unauthorized-mission exclusion, valid subject+authorized-mission intersection, and includePersonal=false exclusion. Controller tests cover missing-fed-context 401 and unsupported-resource fail-closed behavior.
Reviewer verification on head
80a259b2: scoped gateway get tests 17/17 pass; gateway typecheck pass; gateway lint pass; prettier check pass for changed get/module/scratchpad files. PR CI reported green on pipeline 1620. APPROVE.