diff --git a/CHANGELOG.md b/CHANGELOG.md index 685c763..16e0c9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ All notable changes to engram are documented here. The format follows ### Fixed - A stdio start no longer bridges its per-process scoping away (#87, P1). `engram mcp` / `dist/interfaces/mcp/server.js` with `ENGRAM_SCOPE` or `ENGRAM_READ_SCOPES` in the environment — every Hermes profile child, or any host following `docs/integrate-your-agent.md` — used to bridge to a healthy daemon, which ran each forwarded call under *its* env: every profile wrote `global` and read every scope, silently. Such a start now runs inline without probing, with the reason on stderr (`running inline (ENGRAM_SCOPE=hermes:career is per-process; the daemon would ignore it)`). The daemon's `/health` gained `dbPath`; with `ENGRAM_DB_PATH` set the bridge is used only when it matches the daemon's, otherwise inline (`ENGRAM_DB_PATH=… but the daemon serves …`). A daemon that does not report `dbPath` (older build) keeps bridging as before. - `forget` no longer honours `scope: "global"` as a scope override when the server's own env pins reads (`ENGRAM_SCOPE` / `ENGRAM_READ_SCOPES`): an env-pinned stdio child (the Hermes memory-provider transport) could pass it to delete, or search for and delete, another tenant's memories (#109). The override still works from an unrestricted server. The tool description says so; the `tool-schemas` snapshot is re-recorded for that sentence only. - +- Per-call `scope` / `read_scopes` could escape an env-pinned tenant (#108). When `ENGRAM_SCOPE` / `ENGRAM_READ_SCOPES` are set (a stdio child scoped by the Hermes plugin), `resolveCallScoping` now intersects `read_scopes` with the env read scopes (empty intersection is an error) and rejects a `scope` that is not one of them — a tenant may only write where it may read — instead of letting the params replace the env. The shared HTTP daemon, where the env is unset and the params are the tenant identity, is unchanged. ### Changed - Test scaffolding shared instead of copied (#126, part of the #114 over-engineering burn-down). No behaviour change and no test lost: `tests/mocks/embeddings.ts` replaces eleven copies of the hash-based embeddings `vi.mock` factory, `tests/mocks/llm-fetch.ts` the four `stubFetch` / `makeAnthropicMock` copies in the `*-factory` suites, `tests/mocks/fake-worker.ts` the two `FakeWorker` classes; `tests/helpers.ts` gains the raw row inserts (`insertEntity`, `insertRelationship`, `insertCluster`, `insertBridgeScore`, `insertConversation`, `insertExchange`, `insertMemory`), `createTestMemory`, `makeExchanges` and the built-CLI guard (`builtCli`, `itBuilt`), and loses its `cosineSimilarity` copy (tests import `src/_core/search/vector.ts`) and the one-caller `createTestFixture` / `createSyntheticToolCall`. `vitest.config.ts` sets `test.unstubEnvs`, so suites isolate env with `vi.stubEnv(key, undefined)` instead of hand-rolled save/restore loops. diff --git a/docs/integrate-your-agent.md b/docs/integrate-your-agent.md index cd45413..279495c 100644 --- a/docs/integrate-your-agent.md +++ b/docs/integrate-your-agent.md @@ -191,17 +191,22 @@ the `scope` / `read_scopes` parameters instead. Per call, `recall` and `recall_session` accept `scope` (reads then default to `global` + that scope) and `read_scopes` (explicit list); `remember` and -`remember_batch` accept `scope`; `ingest_turn` requires `scope`. A parameter -overrides the environment for that call only, so one HTTP daemon can serve -several tenants — the Hermes plugin sends `hermes:` on every write. -With nothing set the server is single-tenant: writes land in `global`, reads -see everything. Convention: name scopes `:`, for example -`codex:work` or `hermes:career`. - -Because the model can pass `scope` / `read_scopes` itself, the integration -layer (your hook or plugin), not the tool schema, is what pins a tenant: set -the params from your side and do not expose them to the model if it must not -choose its own scope. +`remember_batch` accept `scope`; `ingest_turn` requires `scope`. With the +environment unset (the shared HTTP daemon) the params are the tenant identity +for that call, so one daemon can serve several tenants — the Hermes plugin +sends `hermes:` on every write. With `ENGRAM_SCOPE` / +`ENGRAM_READ_SCOPES` set (a stdio child pinned to one tenant) the params can +only narrow: `read_scopes` is intersected with the env read scopes and `scope` +must be one of them; anything else is rejected, so a prompt-injected model +cannot read or write another tenant. With nothing set the server is +single-tenant: writes land in `global`, reads see everything. Convention: name +scopes `:`, for example `codex:work` or `hermes:career`. + +On the HTTP daemon the model can pass `scope` / `read_scopes` itself, so the +integration layer (your hook or plugin), not the tool schema, is what pins a +tenant there: set the params from your side and do not expose them to the +model if it must not choose its own scope. Pin the env on a stdio child +instead when the model must be sandboxed. **Caveat:** scope filtering applies to the **semantic** memory table only. Episodic conversation exchanges and graph entities are not scope-filtered. diff --git a/src/interfaces/mcp/scoping.ts b/src/interfaces/mcp/scoping.ts index 16ff16d..dccf125 100644 --- a/src/interfaces/mcp/scoping.ts +++ b/src/interfaces/mcp/scoping.ts @@ -62,28 +62,45 @@ function requireScope(value: string, param: string): string { } /** - * Resolve the scoping for a single tool call: request params win over env, - * env is the default when params are absent. + * Resolve the scoping for a single tool call. * - * - `scope` overrides ENGRAM_SCOPE. When ENGRAM_READ_SCOPES is unset it also - * re-derives the read default to global + own, mirroring the env rule. - * - `read_scopes` overrides ENGRAM_READ_SCOPES (and the derived default). + * When the env does not restrict (no ENGRAM_SCOPE / ENGRAM_READ_SCOPES — the + * shared HTTP daemon), params are the tenant identity: `scope` sets the write + * scope and re-derives reads to global + own; `read_scopes` is taken as given. + * + * When the env restricts (a stdio child pinned to one tenant, #108), params + * may only narrow it: `read_scopes` is intersected with the env read scopes + * (empty intersection throws) and `scope` must be one of them — a tenant may + * only write where it may read. The env read scopes stay the read default. */ export function resolveCallScoping( env: Record, params: CallScopeParams, ): TenantScoping { const base = getTenantScoping(env); + const allowed = base.readScopes; - const writeScope = params.scope !== undefined ? requireScope(params.scope, "scope") : base.writeScope; + let writeScope = base.writeScope; + if (params.scope !== undefined) { + writeScope = requireScope(params.scope, "scope"); + if (allowed && !allowed.includes(writeScope)) { + throw new Error(`scope "${writeScope}" is outside this server's ENGRAM_READ_SCOPES (${allowed.join(",")})`); + } + } - let readScopes = base.readScopes; + let readScopes = allowed; if (params.read_scopes !== undefined) { if (!Array.isArray(params.read_scopes) || params.read_scopes.length === 0) { throw new Error("read_scopes must contain at least one scope"); } readScopes = params.read_scopes.map((s) => requireScope(s, "read_scopes")); - } else if (params.scope !== undefined && !env.ENGRAM_READ_SCOPES?.trim()) { + if (allowed) { + readScopes = readScopes.filter((s) => allowed.includes(s)); + if (readScopes.length === 0) { + throw new Error(`read_scopes has no scope inside this server's ENGRAM_READ_SCOPES (${allowed.join(",")})`); + } + } + } else if (params.scope !== undefined && !allowed) { readScopes = ["global", writeScope as string]; } diff --git a/tests/core/tenant-scoping.test.ts b/tests/core/tenant-scoping.test.ts index fadfa48..ed926cd 100644 --- a/tests/core/tenant-scoping.test.ts +++ b/tests/core/tenant-scoping.test.ts @@ -57,20 +57,42 @@ describe("resolveCallScoping (per-request override)", () => { expect(s.readScopes).toEqual(["global", "hermes:career"]); }); - it("scope param overrides ENGRAM_SCOPE but keeps an explicit ENGRAM_READ_SCOPES", () => { - const env = { ENGRAM_SCOPE: "hermes:pmp", ENGRAM_READ_SCOPES: "global,hermes:pmp" }; + it("scope param may pick another env read scope and keeps ENGRAM_READ_SCOPES as the read default", () => { + const env = { ENGRAM_SCOPE: "hermes:pmp", ENGRAM_READ_SCOPES: "global,hermes:pmp,hermes:career" }; const s = resolveCallScoping(env, { scope: "hermes:career" }); expect(s.writeScope).toBe("hermes:career"); - expect(s.readScopes).toEqual(["global", "hermes:pmp"]); + expect(s.readScopes).toEqual(["global", "hermes:pmp", "hermes:career"]); }); - it("read_scopes param overrides env read scopes and leaves writeScope alone", () => { - const env = { ENGRAM_SCOPE: "hermes:pmp" }; - const s = resolveCallScoping(env, { read_scopes: [" hermes:career "] }); + // #108: an env-pinned child must not escape its tenant through params. + it("scope param outside the env read scopes throws", () => { + expect(() => resolveCallScoping({ ENGRAM_SCOPE: "hermes:career" }, { scope: "hermes:personal" })).toThrow( + /outside this server's ENGRAM_READ_SCOPES/, + ); + expect(() => + resolveCallScoping({ ENGRAM_READ_SCOPES: "global,hermes:career" }, { scope: "hermes:personal" }), + ).toThrow(/outside/); + }); + + it("read_scopes param is intersected with the env read scopes", () => { + const env = { ENGRAM_SCOPE: "hermes:pmp", ENGRAM_READ_SCOPES: "global,hermes:pmp,hermes:career" }; + const s = resolveCallScoping(env, { read_scopes: [" hermes:career ", "hermes:personal"] }); expect(s.writeScope).toBe("hermes:pmp"); expect(s.readScopes).toEqual(["hermes:career"]); }); + it("read_scopes param with no scope inside the env read scopes throws", () => { + expect(() => resolveCallScoping({ ENGRAM_SCOPE: "hermes:pmp" }, { read_scopes: ["hermes:personal"] })).toThrow( + /no scope inside/, + ); + }); + + it("without env restriction read_scopes is taken as given", () => { + const s = resolveCallScoping({}, { read_scopes: [" hermes:career "] }); + expect(s.writeScope).toBeUndefined(); + expect(s.readScopes).toEqual(["hermes:career"]); + }); + it("rejects empty / whitespace scope strings and empty read_scopes", () => { expect(() => resolveCallScoping({}, { scope: "" })).toThrow(/scope/); expect(() => resolveCallScoping({}, { scope: " " })).toThrow(/scope/); diff --git a/tests/interfaces/mcp/per-request-scoping.test.ts b/tests/interfaces/mcp/per-request-scoping.test.ts index 5f3ff71..26c5b56 100644 --- a/tests/interfaces/mcp/per-request-scoping.test.ts +++ b/tests/interfaces/mcp/per-request-scoping.test.ts @@ -159,9 +159,13 @@ describe("per-request scope / read_scopes params", () => { // env read default = global + own, so the career memory is hidden… const hidden = await handleToolCall("recall", { query: "interview", sources: ["semantic"] }); expect(hidden.content[0].text as string).not.toContain("Databricks"); - // …unless the call overrides read_scopes - const shown = await handleToolCall("recall", { query: "interview", sources: ["semantic"], read_scopes: ["hermes:career"] }); - expect(shown.content[0].text as string).toContain("Databricks"); + // …and the call cannot widen past the env (#108): read_scopes intersects, scope must be readable + const escape = await handleToolCall("recall", { query: "interview", sources: ["semantic"], read_scopes: ["hermes:career"] }); + expect(escape.isError).toBe(true); + expect(escape.content[0].text as string).toContain("no scope inside"); + const write = await handleToolCall("remember", { content: "leak", scope: "hermes:career" }); + expect(write.isError).toBe(true); + expect(write.content[0].text as string).toContain("outside"); } finally { delete process.env.ENGRAM_SCOPE; }