diff --git a/src/resources/extensions/sf/sm-client.js b/src/resources/extensions/sf/sm-client.js index 0f2b89739..13166adda 100644 --- a/src/resources/extensions/sf/sm-client.js +++ b/src/resources/extensions/sf/sm-client.js @@ -202,13 +202,69 @@ export async function querySmMemories(query, opts = {}) { } const data = await response.json(); - return data.memories || []; + const memories = data.memories || []; + // Defense-in-depth tenant gate: even though the request body + // pins tenantId, a misconfigured SM server could echo memories + // from other tenants. Drop any returned memory whose tenant + // claim doesn't match what we asked for. Memories without a + // tenant field at all are treated as legacy SF rows and + // allowed through (matches the local DB's "NULL tenant = + // legacy" rule from schema v68). Anything with a NON-matching + // claim is dropped and a one-line warning is logged so + // operators see misconfigured-SM symptoms instead of silent + // cross-tenant injection. + return filterSmMemoriesToTenant(memories, scope.tenantId); } catch { // Network error or timeout — fail open return []; } } +/** + * Filter SM-returned memories to those that either claim the expected + * tenant OR carry no tenant field at all (legacy). Exported for the + * dedicated unit test. + * + * The forgiving "no tenant = allow" rule is deliberate: SM is the + * upstream and may legitimately store legacy unmigrated entries. + * If you want hard-fail-closed semantics, set + * SM_REQUIRE_TENANT_CLAIM=true. + */ +export function filterSmMemoriesToTenant(memories, expectedTenantId) { + if (!Array.isArray(memories)) return []; + if (!expectedTenantId) { + // Caller didn't supply a tenant — preserve fail-open semantics + // (caller is responsible for refusing to query in that case). + return memories; + } + const requireClaim = process.env.SM_REQUIRE_TENANT_CLAIM === "true"; + const allowed = []; + let droppedForeign = 0; + let droppedMissingClaim = 0; + for (const m of memories) { + const claim = m?.tenantId ?? m?.tenant ?? null; + if (claim === null || claim === undefined) { + if (requireClaim) { + droppedMissingClaim++; + continue; + } + allowed.push(m); + continue; + } + if (claim === expectedTenantId) { + allowed.push(m); + } else { + droppedForeign++; + } + } + if (droppedForeign > 0 || droppedMissingClaim > 0) { + console.warn( + `[sm-client] dropped ${droppedForeign} foreign-tenant and ${droppedMissingClaim} unclaimed memories (expected tenantId=${expectedTenantId})`, + ); + } + return allowed; +} + /** * Get SM client status for doctor checks. */ diff --git a/src/resources/extensions/sf/tests/sm-tenant-filter.test.mjs b/src/resources/extensions/sf/tests/sm-tenant-filter.test.mjs new file mode 100644 index 000000000..2dd48cf01 --- /dev/null +++ b/src/resources/extensions/sf/tests/sm-tenant-filter.test.mjs @@ -0,0 +1,130 @@ +/** + * Defense-in-depth tenant filter on SM responses. + * + * Even though querySmMemories pins tenantId in the request body, + * filterSmMemoriesToTenant re-checks every returned memory against + * the expected tenant. Misconfigured SM servers (or compromised + * ones) can't leak cross-tenant content into SF prompts. + * + * Documented behaviour: + * - same-tenant memories pass through + * - foreign-tenant memories are dropped + logged + * - memories with no tenant claim default to allow (legacy) + * - SM_REQUIRE_TENANT_CLAIM=true flips legacy rule to drop + * - empty input → empty output + * - non-array input → empty output (defensive) + * - missing expectedTenantId → preserve fail-open + */ + +import assert from "node:assert/strict"; +import { afterEach, beforeEach, test } from "vitest"; +import { filterSmMemoriesToTenant } from "../sm-client.js"; + +const originalRequireClaim = process.env.SM_REQUIRE_TENANT_CLAIM; +const originalWarn = console.warn; + +beforeEach(() => { + console.warn = () => {}; +}); + +afterEach(() => { + if (originalRequireClaim === undefined) { + delete process.env.SM_REQUIRE_TENANT_CLAIM; + } else { + process.env.SM_REQUIRE_TENANT_CLAIM = originalRequireClaim; + } + console.warn = originalWarn; +}); + +test("same-tenant memories pass through", () => { + const memories = [ + { id: "m1", tenantId: "T" }, + { id: "m2", tenantId: "T" }, + ]; + const out = filterSmMemoriesToTenant(memories, "T"); + assert.equal(out.length, 2); +}); + +test("foreign-tenant memories are dropped", () => { + const memories = [ + { id: "m1", tenantId: "T" }, + { id: "m2", tenantId: "other-tenant" }, + { id: "m3", tenantId: "T" }, + ]; + const out = filterSmMemoriesToTenant(memories, "T"); + assert.deepEqual( + out.map((m) => m.id), + ["m1", "m3"], + ); +}); + +test("legacy memories with no tenant claim default to allow", () => { + const memories = [ + { id: "m1", tenantId: "T" }, + { id: "m2" }, // legacy — no tenant at all + { id: "m3", tenantId: null }, + ]; + const out = filterSmMemoriesToTenant(memories, "T"); + assert.deepEqual( + out.map((m) => m.id), + ["m1", "m2", "m3"], + ); +}); + +test("SM_REQUIRE_TENANT_CLAIM flips legacy rule to drop", () => { + process.env.SM_REQUIRE_TENANT_CLAIM = "true"; + const memories = [ + { id: "m1", tenantId: "T" }, + { id: "m2" }, // no tenant — must be dropped under strict mode + { id: "m3", tenantId: "other" }, + ]; + const out = filterSmMemoriesToTenant(memories, "T"); + assert.deepEqual( + out.map((m) => m.id), + ["m1"], + ); +}); + +test("memory.tenant is accepted as an alias for memory.tenantId", () => { + const memories = [ + { id: "m1", tenant: "T" }, + { id: "m2", tenant: "other" }, + ]; + const out = filterSmMemoriesToTenant(memories, "T"); + assert.deepEqual( + out.map((m) => m.id), + ["m1"], + ); +}); + +test("empty input → empty output, non-array → empty output", () => { + assert.deepEqual(filterSmMemoriesToTenant([], "T"), []); + assert.deepEqual(filterSmMemoriesToTenant(null, "T"), []); + assert.deepEqual(filterSmMemoriesToTenant(undefined, "T"), []); + assert.deepEqual(filterSmMemoriesToTenant("not an array", "T"), []); +}); + +test("missing expectedTenantId → pass-through (caller's responsibility)", () => { + const memories = [ + { id: "m1", tenantId: "T" }, + { id: "m2", tenantId: "other" }, + ]; + const out = filterSmMemoriesToTenant(memories, ""); + assert.equal(out.length, 2); + const out2 = filterSmMemoriesToTenant(memories, null); + assert.equal(out2.length, 2); +}); + +test("emits a warning when dropping foreign or unclaimed memories", () => { + const messages = []; + console.warn = (msg) => messages.push(msg); + const memories = [ + { id: "m1", tenantId: "T" }, + { id: "m2", tenantId: "other" }, + { id: "m3", tenantId: "different" }, + ]; + const out = filterSmMemoriesToTenant(memories, "T"); + assert.equal(out.length, 1); + assert.equal(messages.length, 1); + assert.match(messages[0], /dropped 2 foreign-tenant/); +});