From b1f8b1605f5010b45137738829d74c1b3ebd08f5 Mon Sep 17 00:00:00 2001 From: khustup2 Date: Tue, 26 May 2026 00:29:32 +0000 Subject: [PATCH] fix(notifications): match goal owner by canonical forms, not '%user%' substring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The open-goals SessionStart banner filtered goals with `owner LIKE '%${userName}%'`, which has two real failure modes (the same ones CodeRabbit flagged on PR #203 for the context block): 1. Substring collision — `'%ali%'` matches `malice@…`, leaking another user's goals into the banner. 2. Reverse-alias miss — a full-email userName never matches a row whose owner is the short form. The context-renderer already solved this with `listOpenGoals` (exact full / exact short / `short@%` alias), but the banner was never ported. Reuse that reader instead of duplicating a worse query; the loose bidirectional `includes()` JS guard goes away with it. Note: `listOpenGoals` also carries a `version = MAX(version)` filter, but `version` is vestigial (always 1, see deeplake-fs.ts), so it does NOT dedup the duplicate-row "N goals" double-count from Deeplake's UPDATE-coalescing quirk. That needs a separate `DISTINCT ON (goal_id)` fix in both readers — out of scope here. --- src/notifications/sources/open-goals.ts | 60 +++++++------------ .../notifications-open-goals-source.test.ts | 21 ++++--- 2 files changed, 34 insertions(+), 47 deletions(-) diff --git a/src/notifications/sources/open-goals.ts b/src/notifications/sources/open-goals.ts index d31d187d..8ccf64f7 100644 --- a/src/notifications/sources/open-goals.ts +++ b/src/notifications/sources/open-goals.ts @@ -2,9 +2,12 @@ * Open-goals SessionStart summary. * * Reads the user's open goals from the dedicated hivemind_goals - * table (owner + status filter on indexed columns, no LIKE scan) - * and produces a short one-line summary the primary banner appends - * to its body. + * table via the shared `listOpenGoals` reader (canonical owner-form + * matching + version=MAX dedup) and produces a short one-line summary + * the primary banner appends to its body. Sharing that reader keeps + * the banner and the SessionStart context block in agreement — both + * count one row per goal_id and never leak another user's goals via a + * substring collision. * * Returns null when: * - creds are missing @@ -19,7 +22,7 @@ import type { Credentials } from "../../commands/auth-creds.js"; import { DeeplakeApi } from "../../deeplake-api.js"; -import { sqlIdent, sqlStr } from "../../utils/sql.js"; +import { listOpenGoals } from "../../hooks/shared/context-renderer.js"; import { log as _log } from "../../utils/debug.js"; const log = (msg: string) => _log("notifications-open-goals", msg); @@ -49,44 +52,23 @@ export async function fetchOpenGoals( creds.workspaceId ?? "default", goalsTableName, ); - const safe = sqlIdent(goalsTableName); - // Latest version per goal_id, filtered to current owner + - // non-closed status. The (goal_id, version) and (owner, status) - // indexes both apply, so this stays a cheap point lookup even - // on a large hivemind_goals table. - // - // We tolerate userName in either short ("emanuele.fenocchi") or - // full-email form ("emanuele.fenocchi@activeloop.ai") by using - // a LIKE substring match on the owner column. Different agents - // historically populated this field with one or the other; the - // skill instructs the agent to use whatever userName the - // credentials carry, but staleness in older rows is real. - const sql = - // One row per goal_id (UPDATE-or-INSERT model), so a direct - // WHERE on owner+status is the cheap and correct path. - `SELECT goal_id, owner, status, content FROM "${safe}" ` + - `WHERE owner LIKE '%${sqlStr(creds.userName)}%' ` + - ` AND status IN ('opened', 'in_progress') ` + - `ORDER BY created_at DESC LIMIT 25`; - const rows = (await api.query(sql)) as Array<{ - goal_id?: string; - owner?: string; - status?: string; - content?: string; - }>; - if (!Array.isArray(rows) || rows.length === 0) return null; + // Reuse the canonical goal reader: it matches the owner by exact + // full form, exact short form, and `short@%` alias (never a + // `'%user%'` substring scan, which collides — e.g. 'ali' matching + // 'malice@…') and keeps only the latest version per goal_id, so + // multiple stored versions of one goal count exactly once. + const rows = await listOpenGoals( + sql => api.query(sql) as Promise>>, + goalsTableName, + creds.userName, + { limit: 25 }, + ); + if (rows.length === 0) return null; const goals: Array<{ label: string }> = []; for (const r of rows) { - const owner = String(r.owner ?? ""); - const content = String(r.content ?? ""); - if (!owner || !content) continue; - // Bi-directional substring match — same userName variant - // problem as the previous LIKE narrowing did at the SQL - // layer. Belt-and-braces guard for older rows. - const u = creds.userName; - if (owner !== u && !owner.includes(u) && !u.includes(owner)) continue; - goals.push({ label: firstLine(content) }); + if (!r.content) continue; + goals.push({ label: firstLine(r.content) }); } if (goals.length === 0) return null; return { diff --git a/tests/claude-code/notifications-open-goals-source.test.ts b/tests/claude-code/notifications-open-goals-source.test.ts index a90c344c..ba0c9776 100644 --- a/tests/claude-code/notifications-open-goals-source.test.ts +++ b/tests/claude-code/notifications-open-goals-source.test.ts @@ -9,7 +9,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; * `query` method) and capture the exact SQL the banner caused, so any * regression in: * - SQL injection escaping of `creds.userName` - * - the owner-LIKE matching that tolerates short vs full-email forms + * - the canonical owner-form matching that tolerates short vs + * full-email forms without admitting substring collisions * - the status filter (must not surface closed goals) * - the limit (banner is bounded — never lets a flood of goals stretch) * fails fast. @@ -72,26 +73,30 @@ describe("fetchOpenGoals — auth gating", () => { // ── SQL shape ─────────────────────────────────────────────────────────────── describe("fetchOpenGoals — SQL shape", () => { - it("issues a single SELECT scoped to current owner with LIKE + non-closed status filter + limit 25", async () => { + it("issues a single SELECT scoped to current owner with canonical-form match + non-closed status filter + version dedup + limit 25", async () => { queryMock.mockResolvedValue([]); await fetchOpenGoals(BASE_CREDS as any, "hivemind_goals_test"); expect(queryMock).toHaveBeenCalledTimes(1); const sql = queryMock.mock.calls[0][0] as string; expect(sql).toMatch(/^SELECT goal_id, owner, status, content FROM "hivemind_goals_test"/); - // owner LIKE is intentional — tolerates short vs full-email forms - expect(sql).toContain(`WHERE owner LIKE '%alice@activeloop.ai%'`); + // Canonical owner forms (exact full / exact short / `short@%` alias) + // instead of a `'%user%'` substring scan, which collides across users. + expect(sql).toContain(`(owner = 'alice@activeloop.ai' OR owner = 'alice' OR owner LIKE 'alice@%')`); + expect(sql).not.toContain(`LIKE '%alice`); expect(sql).toContain(`status IN ('opened', 'in_progress')`); // 'closed' status must never show up in the banner — that would // surface user's already-done work as if it were still open. expect(sql).not.toMatch(/closed/); - expect(sql).toContain("ORDER BY created_at DESC LIMIT 25"); + // One row per goal_id — multiple stored versions count once. + expect(sql).toContain("version = (SELECT MAX(version)"); + expect(sql).toContain("LIMIT 25"); }); it("escapes single quotes in userName (SQL injection guard)", async () => { queryMock.mockResolvedValue([]); await fetchOpenGoals({ ...BASE_CREDS, userName: "O'Brien" } as any, "hivemind_goals"); const sql = queryMock.mock.calls[0][0] as string; - expect(sql).toContain(`'%O''Brien%'`); + expect(sql).toContain(`owner = 'O''Brien'`); }); it("refuses to issue a query when goalsTableName is not a valid SQL identifier", async () => { @@ -133,10 +138,10 @@ describe("fetchOpenGoals — result mapping", () => { expect(summary!.sample).toEqual(["Ship search", "Land memory", "Onboard team"]); }); - it("filters out rows where owner doesn't substring-match userName (defense-in-depth past the LIKE)", async () => { + it("filters out rows whose owner isn't a canonical form of userName (defense-in-depth past the SQL gate)", async () => { queryMock.mockResolvedValue([ { goal_id: "g1", owner: "alice@activeloop.ai", status: "opened", content: "mine" }, - // A stray row that the LIKE allowed through but logically isn't this user + // A stray row that slipped past the SQL gate but logically isn't this user { goal_id: "g2", owner: "bob@activeloop.ai", status: "opened", content: "his" }, ]); const summary = await fetchOpenGoals(BASE_CREDS as any, "hivemind_goals");