From 9d07184e814be17e7dcd6efc32982ad0fa4957b2 Mon Sep 17 00:00:00 2001 From: bwerapol Date: Fri, 31 Jul 2026 19:41:29 +0000 Subject: [PATCH] fix(core): align the two V2 scope helpers; cover the untested one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pass over the v2.3.0 -> v2.5.2 arc turned up three gaps, all introduced by that arc. 1. buildKnowledgeWhereV2 and buildRawProjectFilterV2 DISAGREED on the no-activeProjectId case. The raw helper admits scope IN ('user','global') unconditionally there — added 2026-05-12, on the reasoning that with no active project there is no boundary to enforce. The Prisma twin gated it on the new opt-in flag. They back the same visibility rule on different query surfaces, so a caller's results depended on which one their code path happened to hit. Same inconsistency class CodeRabbit caught inside the raw helper on #179 — fixed there, and introduced between the two. 2. buildKnowledgeWhereV2 had ZERO test coverage despite backing the knowledge listing route and action-items, and #177 added an unreachable branch to it (no caller passes the flag; kra/oracle use the raw helper). Six tests now pin: closed by default under an active project, opened when opted in, applied to scope="all", unconditional with no active project — asserted against the raw helper in the same test so they cannot drift apart again — and the disjunct pinned to ownerUserId. 3. Documented the behaviour change the parity fix causes at a security-reviewed call site. action-items.ts passes projectId as string | null; with null it now admits the caller's own user-scoped rows. Acceptable — accessibleProjectIds stays [], the query ANDs type: action_item, it is pinned to ownerUserId, and action items are project-scoped by contract so the set is empty — but it is a real change at a spot with an explicit isolation note, so it is stated rather than left to be rediscovered. Also notes on Limit.max that 0 blocks everything and is not a "disabled" sentinel. Before the atomic rewrite the fresh-bucket path returned ok:true unconditionally, so max:0 leaked one request per window; that inconsistency is gone and nothing depended on it. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9 --- .../core/src/__tests__/scope-filter.test.ts | 58 +++++++++++++++++++ packages/core/src/action-items.ts | 10 ++++ packages/core/src/rate-limit.ts | 7 +++ packages/core/src/scope-filter.ts | 14 ++++- 4 files changed, 88 insertions(+), 1 deletion(-) diff --git a/packages/core/src/__tests__/scope-filter.test.ts b/packages/core/src/__tests__/scope-filter.test.ts index 1935929..5d35541 100644 --- a/packages/core/src/__tests__/scope-filter.test.ts +++ b/packages/core/src/__tests__/scope-filter.test.ts @@ -10,6 +10,7 @@ import { buildProposalWhere, buildRawProjectFilter, buildRawProjectFilterV2, + buildKnowledgeWhereV2, } from "../scope-filter.js"; const USER = "user_abc"; @@ -256,3 +257,60 @@ describe("buildRawProjectFilterV2 — cross-project reach of user-scope rows", ( expect(on.params).toEqual(off.params); }); }); + +// --------------------------------------------------------------------------- +// buildKnowledgeWhereV2 — the Prisma-side twin of buildRawProjectFilterV2. +// Had no coverage at all before 2026-07-31 despite backing the knowledge +// listing route and action-items. +// --------------------------------------------------------------------------- + +describe("buildKnowledgeWhereV2 — user-scope reach", () => { + const args = ( + activeProjectId: string | null, + accessible: string[] = [], + optIn = false, + scope: "project" | "all" = "project", + ) => ({ + userId: USER, + activeProjectId, + activeOrgId: null, + accessibleProjectIds: accessible, + scope, + includeUserScopeAcrossProjects: optIn, + }); + + /** The user-scope disjunct, as it appears in a serialised Prisma where. */ + const hasUserScope = (where: object) => + JSON.stringify(where).includes('{"scope":{"in":["user","global"]}}'); + + it("does NOT widen under an active project unless opted in", () => { + expect(hasUserScope(buildKnowledgeWhereV2(args(PROJECT)))).toBe(false); + expect(hasUserScope(buildKnowledgeWhereV2(args(PROJECT, ["p2"])))).toBe(false); + }); + + it("widens under an active project when opted in", () => { + expect(hasUserScope(buildKnowledgeWhereV2(args(PROJECT, [], true)))).toBe(true); + expect(hasUserScope(buildKnowledgeWhereV2(args(PROJECT, ["p2"], true)))).toBe(true); + }); + + it('applies the opt-in to scope="all" too', () => { + expect(hasUserScope(buildKnowledgeWhereV2(args(PROJECT, ["p2"], false, "all")))).toBe(false); + expect(hasUserScope(buildKnowledgeWhereV2(args(PROJECT, ["p2"], true, "all")))).toBe(true); + }); + + // Parity with buildRawProjectFilterV2: with no active project there is no + // boundary to enforce, so user/global rows are admitted regardless of the + // flag. The raw helper has done this since 2026-05-12; the two must agree. + it("admits user/global rows with no active project, even when not opted in", () => { + expect(hasUserScope(buildKnowledgeWhereV2(args(null)))).toBe(true); + const raw = buildRawProjectFilterV2(args(null), 3).sql; + expect(raw).toMatch(/scope IN \('user',\s*'global'\)/); + }); + + it("pins the user-scope disjunct to ownerUserId", () => { + const where = buildKnowledgeWhereV2(args(PROJECT, [], true)); + const json = JSON.stringify(where); + const idx = json.indexOf('{"scope":{"in":["user","global"]}}'); + expect(json.slice(idx, idx + 120)).toContain(`"ownerUserId":"${USER}"`); + }); +}); diff --git a/packages/core/src/action-items.ts b/packages/core/src/action-items.ts index 10ae723..73018bf 100644 --- a/packages/core/src/action-items.ts +++ b/packages/core/src/action-items.ts @@ -36,6 +36,16 @@ function baseWhere(opts: { // content into other projects' Oracle context. accessibleProjectIds: [], scope: "project", + // Deliberately NOT opting into includeUserScopeAcrossProjects (#174): + // that finding was about personal rules following the user, which is + // the opposite of what tasks want. One case still admits + // `scope:'user'|'global'` rows — `projectId === null`, where there is + // no boundary left to enforce (the raw filter has behaved that way + // since 2026-05-12 and the two helpers must agree). Acceptable here: + // the query still ANDs `type: action_item` and stays pinned to + // `ownerUserId`, and action items are created project-scoped by + // contract, so that set is empty in practice. Revisit if action items + // ever become user-scoped. }), { type: ACTION_ITEM_TYPE }, // Decay is the abandonment path (spec §3): fully-decayed items are diff --git a/packages/core/src/rate-limit.ts b/packages/core/src/rate-limit.ts index 350873d..a375d89 100644 --- a/packages/core/src/rate-limit.ts +++ b/packages/core/src/rate-limit.ts @@ -17,6 +17,13 @@ export interface Bucket { export interface Limit { name: string; + /** + * Requests allowed per window. `0` blocks everything — it is NOT a + * "disabled" sentinel, and no caller treats it as one. (Before the atomic + * rewrite the fresh-bucket path returned `ok: true` unconditionally, so + * `max: 0` leaked one request per window; that inconsistency is gone.) To + * disable a limit, don't route the path through the limiter. + */ max: number; windowMs: number; } diff --git a/packages/core/src/scope-filter.ts b/packages/core/src/scope-filter.ts index e98ebed..abf6a19 100644 --- a/packages/core/src/scope-filter.ts +++ b/packages/core/src/scope-filter.ts @@ -218,11 +218,18 @@ export interface VisibilityScopeArgs { * OR (visibility="org" AND ownerProjectId IN accessibleProjectIds) * OR (visibility="private" AND ownerUserId=userId AND ownerProjectId=activeProjectId) * OR (ownerProjectId IS NULL AND ownerUserId=userId) // legacy/personal rows + * OR (scope IN ('user','global') AND ownerUserId=userId) + * — only when `includeUserScopeAcrossProjects` is set, OR when there is + * no activeProjectId (no boundary to enforce). See #174. * * scope="all": * (visibility IN ('project','org') AND ownerProjectId IN accessibleProjectIds) * OR (visibility="private" AND ownerUserId=userId) * OR (ownerProjectId IS NULL AND ownerUserId=userId) + * OR (scope IN ('user','global') AND ownerUserId=userId) // opt-in only + * + * Mirrors `buildRawProjectFilterV2` clause for clause — the two are one policy + * on two query surfaces, so a change here needs the same change there. */ export function buildKnowledgeWhereV2(args: VisibilityScopeArgs): object { const { userId, activeProjectId, accessibleProjectIds, scope } = args; @@ -299,7 +306,12 @@ export function buildKnowledgeWhereV2(args: VisibilityScopeArgs): object { } orClauses.push(legacyBranch); - if (crossProject) orClauses.push(userScopeBranch); + // Parity with buildRawProjectFilterV2: with no active project there is no + // boundary to enforce, so cross-project rows are admitted regardless of the + // flag (that branch has behaved this way since 2026-05-12). With an active + // project, the widening is opt-in. The two helpers must express one policy — + // they back the same visibility rule on different query surfaces. + if (crossProject || !activeProjectId) orClauses.push(userScopeBranch); return { AND: [