feat(sm-client): defense-in-depth tenant filter on SM responses
Even though querySmMemories pins tenantId in the request body sent
to the Singularity Memory server, SF used to accept whatever came
back without verifying. A misconfigured or compromised SM server
could echo memories from other tenants and SF would inject them
into the next execute-task prompt — cross-customer leak.
filterSmMemoriesToTenant() now re-checks every returned memory:
- same-tenant memories pass through
- foreign-tenant memories (memory.tenantId OR memory.tenant !=
expectedTenantId) are dropped, with a one-line warning so the
misconfigured-SM symptom is visible rather than silent
- memories with no tenant claim at all default to allow — matches
the local DB's "NULL tenant = legacy row" rule from schema v68
- SM_REQUIRE_TENANT_CLAIM=true flips the legacy rule to drop
(hard fail-closed mode for operators who want it)
Defensive guards against non-array inputs, missing expectedTenantId
(returns input unchanged so caller-side fail-open semantics are
preserved), and the dual tenantId/tenant field naming.
Tests: 8 cases — same-tenant pass-through, foreign drop, legacy
allow, strict mode drop, tenantId/tenant alias, empty/non-array
defensiveness, missing-expected pass-through, warning emission.
Resolves the cross-project tenant-leak feedback row filed via the
new headless feedback CLI.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
7c3d9bd3bf
commit
671b2c8628
2 changed files with 187 additions and 1 deletions
|
|
@ -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.
|
||||
*/
|
||||
|
|
|
|||
130
src/resources/extensions/sf/tests/sm-tenant-filter.test.mjs
Normal file
130
src/resources/extensions/sf/tests/sm-tenant-filter.test.mjs
Normal file
|
|
@ -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/);
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue