From 1f01f6f48de1abf0b7490227046e104c65ddfafe Mon Sep 17 00:00:00 2001 From: devinmlowe Date: Mon, 21 Sep 2026 01:20:04 -0500 Subject: [PATCH] fix(mcp): intersect per-call scope params with env-pinned tenant scoping (#108) Client-supplied read_scopes replaced ENGRAM_READ_SCOPES and scope replaced ENGRAM_SCOPE, so a stdio child pinned to one Hermes profile could read and write any tenant with one tool argument. resolveCallScoping now treats the env read scopes as the ceiling when they are set: read_scopes is intersected (empty intersection throws) and scope must be one of them. With the env unset (shared HTTP daemon) the params remain the tenant identity, unchanged. Tests flipped where they asserted the old escape; docs and changelog say which deployment pins the tenant where. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014FJcVdAMkcEH6U3ZMKrEwv --- CHANGELOG.md | 3 ++ docs/integrate-your-agent.md | 27 +++++++++------ src/interfaces/mcp/scoping.ts | 33 +++++++++++++----- tests/core/tenant-scoping.test.ts | 34 +++++++++++++++---- .../mcp/per-request-scoping.test.ts | 10 ++++-- 5 files changed, 79 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73a2713..65739ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ All notable changes to engram are documented here. The format follows ## [Unreleased] +### Fixed +- 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. - MCP server trims (#119, part of the #114 over-engineering burn-down). Every tool's `inputSchema` is now generated from its zod schema (`z.toJSONSchema`, io `input`), replacing about 700 lines of hand-written JSON Schema; `tests/interfaces/mcp/tool-schemas.test.ts` snapshots the full ListTools surface of all 16 tools. Names, descriptions and annotations are unchanged. Visible differences: unbounded integer fields (`show.startLine`/`endLine`, `recall_drill.result_index`, `fetch_snippets` ranges, `ingest_turn.turn_index`) advertise `"integer"` instead of `"number"` (the validator always rejected fractions), `ingest_turn` `tool_calls[]` items no longer claim `additionalProperties: false` (never enforced), and the defaults the handlers already applied are now on the schema (`recall` budget 1500 / dateBasis `filed` / depth `shallow` / sources / reinforce, `recall_session` budget 3000, `commitments` status / limit / budget, `ingest_turn` source). `src/graph/format.ts` renders explore and reflect results once with an `xml` (MCP) / `text` (CLI) switch — output is byte-identical. `recall_session` returns `sessionId` on its result for worker affinity instead of the dispatcher scraping it out of the XML. The worker pool keeps the per-call timeout, the kill-and-respawn at 2× the timeout and respawn-on-exit, and loses the never-set `hangMultiplier` / `readyTimeoutMs` options, the generation counters and its three Error subclasses (same messages, plain `Error`). diff --git a/docs/integrate-your-agent.md b/docs/integrate-your-agent.md index a7f0ee2..8e6cb49 100644 --- a/docs/integrate-your-agent.md +++ b/docs/integrate-your-agent.md @@ -186,17 +186,22 @@ Scoping has a per-process default and a per-call override. 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; }