diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index dff68dbd..a7cf01e6 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -18,6 +18,36 @@ entry. See `CONTRIBUTING.md` § Releases & changelog. ## [Unreleased] +### Added — the audience floor now guards attachment-handle resolution (#575) + +- **The floor's third and last guard**, completing the trio the spec names. A + storage key is a handle: it is minted in one turn and can be redeemed later, + potentially in a different room. +- **The check rides with the handle, not with the call sites.** Enforcement + lives in a wrapper around `AttachmentReader` applied at its single + construction site, so `read_attachment`, the orchestrator's own + `ingestAttachments` and any future resolution path are covered by + construction rather than by remembering to add a call. +- **This closes a path the egress guard did not.** `read_attachment` is a tool + and so already passed the first guard, but `ingestAttachments` resolves + storage keys straight off the inbound turn with no tool call involved — the + path a caller actually controls. +- **A refusal is indistinguishable from "unknown key" to the caller, on + purpose.** Confirming that a key exists but is off-limits would leak the + document's existence to a room that may not know it. The real reason goes to + the operator log, where it is actionable and not a side channel. The inner + reader is never reached, so a refused redemption does not even touch the + store. +- Redeeming a handle and invoking the read tool are separate capabilities; + neither grants the other. + +> **Remaining gap, named rather than assumed closed.** This checks the floor at +> *redemption*, not at *minting*. It stops a room from redeeming a handle that +> room may not read, but cannot yet stop a handle minted in a narrow room from +> being redeemed in a wider one that happens to hold the capability. Binding the +> minting audience to the handle needs the attachment store to persist it, and +> that store lives in the channel plugins. + ### Added — the audience floor now guards context recall (#575) - **The floor's second guard**, at the single context-assembly call site. In a diff --git a/middleware/packages/harness-orchestrator/src/attachmentReaderFactory.ts b/middleware/packages/harness-orchestrator/src/attachmentReaderFactory.ts index f15e77c2..b72ccd2f 100644 --- a/middleware/packages/harness-orchestrator/src/attachmentReaderFactory.ts +++ b/middleware/packages/harness-orchestrator/src/attachmentReaderFactory.ts @@ -16,6 +16,7 @@ import type { Readable } from 'node:stream'; import type { AttachmentReader } from './tools/readAttachmentTool.js'; +import { guardAttachmentRead } from './audienceFloorGuard.js'; /** Minimal structural view of the kernel's `tigrisStore` service. */ export interface AttachmentByteStore { @@ -40,10 +41,53 @@ function fileNameFromKey(key: string): string | undefined { return seg && seg.length > 0 ? seg : undefined; } +/** + * #575 — wrap a reader so every handle redemption passes the audience floor. + * + * The check rides with the handle rather than sitting at each call site, which + * is the point spec §5.2 makes: a storage key outlives the turn that minted it, + * and a resolution site added later would otherwise silently escape the guard. + * Wrapping the reader covers `read_attachment`, the orchestrator's own + * `ingestAttachments`, and anything added next, by construction. + * + * A refusal returns `undefined` — the reader's existing "unavailable" contract — + * rather than throwing, so no caller needs new error handling. That does mean a + * denial is indistinguishable from "unknown key" *to the model*, and that is + * deliberate rather than sloppy: a message confirming that the key exists but + * is off-limits would leak the document's existence to a room that may not know + * it. The real reason goes to the operator log, where it is actionable and not + * a side channel. + * + * Inert when no audience source is installed, like every other guard here. + */ +export function audienceGuardedAttachmentReader(inner: AttachmentReader): AttachmentReader { + return { + async readByStorageKey(storageKey) { + const refusal = await guardAttachmentRead(); + if (refusal !== undefined) { + console.warn(`[harness-orchestrator] attachment read refused by audience floor: ${refusal}`); + return undefined; + } + return inner.readByStorageKey(storageKey); + }, + async readByUrl(url) { + const refusal = await guardAttachmentRead(); + if (refusal !== undefined) { + console.warn(`[harness-orchestrator] attachment fetch refused by audience floor: ${refusal}`); + return undefined; + } + return inner.readByUrl(url); + }, + }; +} + /** * Construct an {@link AttachmentReader}. When `store` is `undefined` * (bucket env not set), `readByStorageKey` always resolves to `undefined` * and the feature is inert; `readByUrl` still works via `fetch`. + * + * Unguarded on its own — `plugin.ts` wraps it in + * {@link audienceGuardedAttachmentReader} at the single construction site. */ export function createAttachmentReader( store: AttachmentByteStore | undefined, diff --git a/middleware/packages/harness-orchestrator/src/audienceFloorGuard.ts b/middleware/packages/harness-orchestrator/src/audienceFloorGuard.ts index a9a184c4..556e4723 100644 --- a/middleware/packages/harness-orchestrator/src/audienceFloorGuard.ts +++ b/middleware/packages/harness-orchestrator/src/audienceFloorGuard.ts @@ -76,6 +76,65 @@ export function toolCapability(name: string): Capability { */ export const MEMORY_RECALL_CAPABILITY: Capability = 'memory:recall'; +/** + * The capability resolving a stored attachment handle requires. + * + * Separate from `tool:read_attachment` on purpose. That one asks "may this room + * invoke the read tool"; this one asks "may this room redeem a storage handle", + * and the handle is redeemable from paths that are not tool calls at all — see + * {@link guardAttachmentRead}. + */ +export const ATTACHMENT_READ_CAPABILITY: Capability = 'attachment:read'; + +/** + * #575 — the third guard: file / credential handle resolution. + * + * ## Why this is not already covered by the egress guard + * + * `read_attachment` is a tool, so it passes `dispatchTool` and Guard 1 does + * bound it. But that is not the only way a handle gets redeemed: the + * orchestrator's own `ingestAttachments` resolves storage keys straight off the + * inbound turn, with no tool call in sight. Guarding only the tool would leave + * the path a caller actually controls wide open. + * + * ## Why the check rides with the handle rather than sitting at call sites + * + * Spec §5.2 says the check "must ride with" the handle, because a handle + * outlives the turn that minted it. A storage key issued in a private chat is + * just a string, and a string can be pasted into a group chat. Adding a call to + * every resolution site would work exactly until somebody adds the next site + * and forgets — so the enforcement lives in a wrapper around `AttachmentReader` + * itself (`attachmentReaderFactory.ts`). Every consumer, present and future, is + * covered by construction. + * + * ## What this version does NOT do + * + * It checks the floor **at redemption**, not the floor **at minting**. So it + * stops a room from redeeming a handle that room may not read — but it cannot + * yet stop a handle minted in a narrow room from being redeemed in a room that + * happens to hold the capability. Binding the minting audience to the handle + * needs the attachment store to persist it, and that store lives in the channel + * plugins rather than here. Stated so the remaining gap is visible rather than + * assumed closed. + */ +export async function guardAttachmentRead(): Promise { + const provider = turnContext.current()?.audienceFloor; + if (!provider) return undefined; + + let floor: AudienceFloor; + try { + floor = await provider(); + } catch (err) { + return `audience unresolvable (${err instanceof Error ? err.message : String(err)})`; + } + + if (floorPermits(floor, ATTACHMENT_READ_CAPABILITY)) return undefined; + + return floor.outcome === 'closed' + ? floor.reason + : 'not every participant in this conversation may read stored attachments'; +} + /** * #575 — the second guard: context / memory recall. * diff --git a/middleware/packages/harness-orchestrator/src/plugin.ts b/middleware/packages/harness-orchestrator/src/plugin.ts index 218d981a..f4080791 100644 --- a/middleware/packages/harness-orchestrator/src/plugin.ts +++ b/middleware/packages/harness-orchestrator/src/plugin.ts @@ -63,6 +63,7 @@ import { type OrchestratorDeps, } from './buildOrchestrator.js'; import { + audienceGuardedAttachmentReader, createAttachmentReader, type AttachmentByteStore, } from './attachmentReaderFactory.js'; @@ -554,7 +555,13 @@ export async function activate( // tool; harness-orchestrator stays free of any @aws-sdk dependency. const attachmentByteStore = ctx.services.get('tigrisStore'); - const attachmentReader = createAttachmentReader(attachmentByteStore); + // #575 — every attachment-handle redemption passes the audience floor. Wrapped + // here, at the ONE construction site, so the check rides with the handle rather + // than depending on each resolution site remembering to ask. Inert unless an + // audience source is installed. + const attachmentReader = audienceGuardedAttachmentReader( + createAttachmentReader(attachmentByteStore), + ); // Phase-1 of the Kemia integration. Late-bound `responseGuard@1` getter — // the orchestrator generally activates BEFORE its tool plugins, so a // bind-at-activate lookup would always miss the responseGuard provider diff --git a/middleware/test/audienceFloorGuard.test.ts b/middleware/test/audienceFloorGuard.test.ts index 9287aff0..a4c3fc7f 100644 --- a/middleware/test/audienceFloorGuard.test.ts +++ b/middleware/test/audienceFloorGuard.test.ts @@ -13,11 +13,14 @@ import { describe, it } from 'node:test'; import assert from 'node:assert/strict'; import { + ATTACHMENT_READ_CAPABILITY, MEMORY_RECALL_CAPABILITY, + guardAttachmentRead, guardContextRecall, guardToolEgress, toolCapability, } from '../packages/harness-orchestrator/src/audienceFloorGuard.js'; +import { audienceGuardedAttachmentReader } from '../packages/harness-orchestrator/src/attachmentReaderFactory.js'; import { turnContext } from '../packages/harness-orchestrator/src/turnContext.js'; import type { AudienceFloor } from '../packages/harness-channel-sdk/src/audienceFloor.js'; @@ -180,6 +183,90 @@ describe('the context guard', () => { }); }); +// ─── guard 3: file / credential handle resolution ────────────────────────── + +describe('the handle guard', () => { + it('an unconfigured deployment resolves handles exactly as before', async () => { + assert.equal(await withFloor(undefined, () => guardAttachmentRead()), undefined); + }); + + it('permits redemption when the whole room may read stored attachments', async () => { + assert.equal( + await withFloor(async () => open(ATTACHMENT_READ_CAPABILITY), () => guardAttachmentRead()), + undefined, + ); + }); + + it('refuses redemption when someone present may not', async () => { + const refusal = await withFloor(async () => open('tool:t'), () => guardAttachmentRead()); + assert.match(refusal ?? '', /not every participant/); + }); + + it('the read-attachment TOOL capability does not grant handle redemption', async () => { + // Being allowed to invoke the tool is a different question from being + // allowed to redeem a storage handle — the handle is redeemable from paths + // that are not tool calls at all. + const refusal = await withFloor( + async () => open(toolCapability('read_attachment')), + () => guardAttachmentRead(), + ); + assert.ok(refusal); + }); +}); + +describe('the check rides with the handle, not with the call site', () => { + const inner = { + readByStorageKey: async () => ({ bytes: Buffer.from('secret'), contentType: 'text/plain' }), + readByUrl: async () => ({ bytes: Buffer.from('secret'), contentType: 'text/plain' }), + }; + + it('a wrapped reader serves bytes when the floor permits', async () => { + const reader = audienceGuardedAttachmentReader(inner); + const got = await withFloor( + async () => open(ATTACHMENT_READ_CAPABILITY), + () => reader.readByStorageKey('k'), + ); + assert.equal(got?.bytes.toString(), 'secret'); + }); + + it('BOTH resolution methods are guarded, not just the storage-key one', async () => { + // `ingestAttachments` prefers `readByStorageKey` but falls back to + // `readByUrl`; guarding only the first would leave the fallback open. + const reader = audienceGuardedAttachmentReader(inner); + await withFloor(async () => closed('nope'), async () => { + assert.equal(await reader.readByStorageKey('k'), undefined); + assert.equal(await reader.readByUrl('https://x/y'), undefined); + }); + }); + + it('a refusal is indistinguishable from "unknown key" TO THE CALLER', async () => { + // Deliberate: confirming the key exists but is off-limits would leak the + // document's existence. The reason goes to the operator log instead. + const reader = audienceGuardedAttachmentReader(inner); + const got = await withFloor(async () => closed('nope'), () => reader.readByStorageKey('k')); + assert.equal(got, undefined); + }); + + it('the inner reader is never even reached on a refusal', async () => { + // The bytes must not be fetched and then discarded — that would still hit + // the store, and a store hit is itself observable. + let touched = false; + const spy = { + readByStorageKey: async () => { + touched = true; + return undefined; + }, + readByUrl: async () => { + touched = true; + return undefined; + }, + }; + const reader = audienceGuardedAttachmentReader(spy); + await withFloor(async () => closed('nope'), () => reader.readByStorageKey('k')); + assert.equal(touched, false); + }); +}); + describe('re-evaluation, not a snapshot', () => { it('the provider is consulted on EVERY dispatch', async () => { // Spec §5.2: a turn-start snapshot is a TOCTOU hole — somebody can join