diff --git a/.changeset/warm-peaches-occur.md b/.changeset/warm-peaches-occur.md new file mode 100644 index 0000000..6d43d9d --- /dev/null +++ b/.changeset/warm-peaches-occur.md @@ -0,0 +1,5 @@ +--- +"sideshow": patch +--- + +Comment authors are now derived from the session's agent name — the `author` parameter is removed from the CLI (`sideshow comment --author`), MCP (`reply_to_user`), and stdio MCP tools. Only same-origin browser requests (the viewer composer) can mint `author: "user"`, closing a feedback-label forgery gap where programmatic callers could inject commands into the agent's user-feedback stream. The reserved `"user"` label is also blocked as a session agent name. diff --git a/bin/sideshow.js b/bin/sideshow.js index db4dc95..4f2bc71 100755 --- a/bin/sideshow.js +++ b/bin/sideshow.js @@ -108,7 +108,6 @@ usage: sideshow comment [options] reply to the user on a post --post post to attach the comment to (required; --surface is a deprecated alias) - --author defaults to agent name sideshow list [--session |--all] list posts sideshow sessions list sessions sideshow demo seed two example sessions to explore the viewer @@ -1248,8 +1247,6 @@ const commands = { post: { type: "string" }, surface: { type: "string" }, // deprecated alias snippet: { type: "string" }, // legacy alias - author: { type: "string" }, - agent: { type: "string" }, }, }); const text = positionals.join(" ").trim(); @@ -1258,14 +1255,12 @@ const commands = { // body key is the wire field `surface`, kept as-is. const post = flags.post ?? flags.surface ?? flags.snippet; if (!post) fail("a comment must target a post — pass --post "); + // The server derives the author from the session's agent name — the CLI + // never sends one, so the reserved "user" label can't be forged here. out( await api("/api/comments", { method: "POST", - body: JSON.stringify({ - text, - surface: post, - author: flags.author ?? agentName(flags), - }), + body: JSON.stringify({ text, surface: post }), }), ); }, @@ -1320,8 +1315,11 @@ const commands = { }); } if (step.comment) { + // The demo seeds a realistic human/agent dialogue, so simulate + // viewer-origin requests (the only path that may mint author:"user"). await api("/api/comments", { method: "POST", + headers: { "sec-fetch-site": "same-origin" }, body: JSON.stringify({ snippet: snippet.id, ...step.comment }), }); } diff --git a/mcp/server.ts b/mcp/server.ts index 1782265..a7115fb 100644 --- a/mcp/server.ts +++ b/mcp/server.ts @@ -202,7 +202,7 @@ server.registerTool( const created = JSON.parse( await api("/api/comments", { method: "POST", - body: JSON.stringify({ surface: postId ?? surfaceId, text: message, author: AGENT }), + body: JSON.stringify({ surface: postId ?? surfaceId, text: message }), }), ); return text(created); diff --git a/server/app.ts b/server/app.ts index c6c9adf..10fee66 100644 --- a/server/app.ts +++ b/server/app.ts @@ -455,7 +455,11 @@ export function createApp({ async function createComment(input: { text: string; surface?: string; - author: string; + // Only the viewer (same-origin REST) may set this — "user" for the composer, + // "surface" for the send-prompt bridge. Programmatic callers (MCP, CLI, + // cross-origin REST) omit it and the author is derived from session.agent, + // so an agent can never mint the reserved "user" trust label. + author?: string; }): Promise< { comment: Comment; userFeedback?: Feedback[] } | { error: string; status: 400 | 404 } > { @@ -464,10 +468,13 @@ export function createApp({ if (!input.surface) return { error: 'provide a "surface" id', status: 400 }; const surface = await store.getPost(input.surface); if (!surface) return { error: "surface not found", status: 404 }; + const session = await store.getSession(surface.sessionId); + if (!session) return { error: "session not found", status: 404 }; + const author = input.author ?? session.agent; const comment = await store.createComment({ sessionId: surface.sessionId, postId: surface.id, - author: input.author, + author, text: input.text.trim().slice(0, MAX_COMMENT_TEXT), }); if (!comment) return { error: "session not found", status: 404 }; @@ -480,8 +487,7 @@ export function createApp({ }); // agent replies are writes too — piggyback pending feedback on them, but // never on the user's own comments - const userFeedback = - input.author === "user" ? undefined : await collectFeedback(comment.sessionId); + const userFeedback = author === "user" ? undefined : await collectFeedback(comment.sessionId); return { comment, userFeedback }; } @@ -901,10 +907,21 @@ export function createApp({ return c.json({ error: 'body must include non-empty "text" string' }, 400); } const surface = typeof body.surface === "string" ? body.surface : body.snippet; + // Only same-origin browser requests (the viewer page) may declare the + // author — the composer sends "user", the send-prompt bridge sends "surface". + // Programmatic callers (MCP, CLI) never send Sec-Fetch-Site and get the author + // derived from session.agent, so the reserved "user" trust label can't be + // forged through the supported tool surfaces. A sandboxed iframe has an opaque + // origin (not same-origin), so contained content can't forge it either. + // Note: a raw HTTP client can set this header freely (it's only browser- + // enforced), but that requires knowing to do so — the trivial one-liner + // forgery is closed, and MCP/CLI (the natural agent paths) are blocked. + const isViewerOrigin = c.req.header("sec-fetch-site") === "same-origin"; + const author = isViewerOrigin && typeof body.author === "string" ? body.author : undefined; const result = await createComment({ text: body.text, surface: typeof surface === "string" ? surface : undefined, - author: typeof body.author === "string" ? body.author : "user", + author, }); if ("error" in result) return c.json({ error: result.error }, result.status); return c.json( diff --git a/server/mcpHttp.ts b/server/mcpHttp.ts index b208da5..6d8b33c 100644 --- a/server/mcpHttp.ts +++ b/server/mcpHttp.ts @@ -35,7 +35,7 @@ export interface McpDeps { createComment(input: { text: string; surface?: string; - author: string; + author?: string; }): Promise<{ comment: Comment; userFeedback?: Feedback[] } | { error: string; status: number }>; waitForComments(q: CommentWait): Promise<{ comments: Comment[]; lastSeq: number }>; uploadAsset(input: { @@ -147,15 +147,12 @@ export function registerMcp(app: Hono, deps: McpDeps) { ); } case "reply_to_user": { - // "user" is the reserved trust label, minted only by the viewer's - // composer (genuine human keystrokes). The agent may name itself - // anything else, but never the user — that would forge feedback. - const named = typeof args.author === "string" ? args.author.trim() : ""; - const author = named && named !== "user" ? named : "agent"; + // The author is derived from session.agent by createComment — the agent + // never gets to choose it, and "user" (the reserved human label) is + // unreachable from MCP entirely. const result = await deps.createComment({ text: String(args.message ?? ""), surface: String(args.postId ?? args.surfaceId ?? ""), - author, }); if ("error" in result) throw new Error(result.error); return JSON.stringify( diff --git a/server/mcpSpec.ts b/server/mcpSpec.ts index e14b652..ce06081 100644 --- a/server/mcpSpec.ts +++ b/server/mcpSpec.ts @@ -305,11 +305,6 @@ export const HTTP_MCP_TOOLS = [ postId: { type: "string", description: "Post whose comment thread to reply in" }, surfaceId: { type: "string", description: "Deprecated alias of postId" }, message: { type: "string", description: d.replyMessage }, - author: { - type: "string", - description: - 'Your agent name (default "agent"; "user" is reserved and coerced to "agent")', - }, }, required: ["message"], }, diff --git a/server/sqlStore.ts b/server/sqlStore.ts index d632ac4..b368121 100644 --- a/server/sqlStore.ts +++ b/server/sqlStore.ts @@ -13,6 +13,7 @@ import { htmlSurface, MAX_WORKSPACE_ASSET_BYTES, newId, + reservedAgent, selectEvictions, type Session, type SqlStorage, @@ -238,7 +239,7 @@ export class SqlStore implements Store { const now = new Date().toISOString(); const session: Session = { id: newId(), - agent: stripNul(input.agent).trim() || "agent", + agent: reservedAgent(stripNul(input.agent).trim() || "agent"), title: stripNul(input.title)?.trim() || null, cwd: stripNul(input.cwd ?? null), createdAt: now, diff --git a/server/storage.ts b/server/storage.ts index 0920041..dcf0224 100644 --- a/server/storage.ts +++ b/server/storage.ts @@ -15,6 +15,7 @@ import { htmlSurface, MAX_WORKSPACE_ASSET_BYTES, newId, + reservedAgent, selectEvictions, type Session, stripNul, @@ -253,7 +254,7 @@ export class JsonFileStore implements Store { const now = new Date().toISOString(); const session: Session = { id: newId(), - agent: stripNul(input.agent).trim() || "agent", + agent: reservedAgent(stripNul(input.agent).trim() || "agent"), title: stripNul(input.title)?.trim() || null, cwd: stripNul(input.cwd ?? null), createdAt: now, diff --git a/server/types.ts b/server/types.ts index e4ad7a0..0478678 100644 --- a/server/types.ts +++ b/server/types.ts @@ -330,6 +330,14 @@ export interface WorkspaceSnapshot { export const HISTORY_LIMIT = 20; +// "user" is the reserved trust label for genuine human comments (minted only by +// the viewer's composer). An agent that names itself "user" at session creation +// would have its derived comments read as human feedback — so coerce it to the +// default. The check lives here so both stores enforce it identically. +export function reservedAgent(name: string): string { + return name === "user" ? "agent" : name; +} + // SQLite terminates a TEXT value at the first embedded NUL byte, while the JSON // store preserves it — so the two stores would diverge on a NUL. A NUL has no // place in a title/comment/label anyway, so both stores strip it from stored diff --git a/test/api.test.ts b/test/api.test.ts index 4a6deb9..5ec2397 100644 --- a/test/api.test.ts +++ b/test/api.test.ts @@ -40,6 +40,14 @@ const authedJson = (body: unknown, token = "secret") => ({ headers: { "content-type": "application/json", authorization: `Bearer ${token}` }, }); +// Simulates a request from the viewer page (same-origin) — the only access +// point that may declare `author`. The browser sets Sec-Fetch-Site +// automatically; a sandboxed iframe is opaque-origin so it can't forge this. +const viewerJson = (body: unknown) => ({ + ...json(body), + headers: { "content-type": "application/json", "sec-fetch-site": "same-origin" }, +}); + test("publish without session auto-creates one", async () => { const app = makeApp(); const res = await app.request( @@ -647,7 +655,10 @@ test("comments attach to snippets and filter by author/after", async () => { const s = (await ( await app.request("/api/snippets", json({ html: "

x

", title: "Sketch" })) ).json()) as any; - await app.request("/api/comments", json({ snippet: s.id, text: "love it", author: "user" })); + await app.request( + "/api/comments", + viewerJson({ snippet: s.id, text: "love it", author: "user" }), + ); await app.request("/api/comments", json({ snippet: s.id, text: "thanks", author: "claude" })); const all = (await (await app.request(`/api/comments?session=${s.sessionId}`)).json()) as any; @@ -667,6 +678,37 @@ test("comments attach to snippets and filter by author/after", async () => { assert.equal(later.comments.length, 0); }); +test('programmatic POST cannot forge author: "user" — derived from session.agent', async () => { + const app = makeApp(); + // Session created with an explicit agent name so we can distinguish it + const s = (await ( + await app.request("/api/snippets", json({ html: "

x

", agent: "my-agent" })) + ).json()) as any; + + // A curl-style POST (no Sec-Fetch-Site) sends author:"user" — the server + // must ignore it and derive from session.agent instead. + const forged = (await ( + await app.request("/api/comments", json({ snippet: s.id, text: "fake user", author: "user" })) + ).json()) as any; + assert.equal(forged.author, "my-agent"); + + // A same-origin POST (the viewer) may mint "user" — the composer's path + const real = (await ( + await app.request( + "/api/comments", + viewerJson({ snippet: s.id, text: "real user", author: "user" }), + ) + ).json()) as any; + assert.equal(real.author, "user"); + + // Only the same-origin "user" comment is delivered as feedback + const feedback = (await ( + await app.request(`/api/comments?session=${s.sessionId}&author=user&after=0`) + ).json()) as any; + assert.equal(feedback.comments.length, 1); + assert.equal(feedback.comments[0].text, "real user"); +}); + test("a comment must target a surface", async () => { const app = makeApp(); const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any; @@ -683,7 +725,7 @@ test("a comment must target a surface", async () => { test("author=user reads resume from the agent's server-side cursor", async () => { const app = makeApp(); const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any; - await app.request("/api/comments", json({ snippet: s.id, text: "first", author: "user" })); + await app.request("/api/comments", viewerJson({ snippet: s.id, text: "first", author: "user" })); // no cursor given: delivered once... const first = (await ( @@ -706,7 +748,10 @@ test("author=user reads resume from the agent's server-side cursor", async () => test("piggyback delivery advances the cursor seen by author=user waits", async () => { const app = makeApp(); const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any; - await app.request("/api/comments", json({ snippet: s.id, text: "tweak it", author: "user" })); + await app.request( + "/api/comments", + viewerJson({ snippet: s.id, text: "tweak it", author: "user" }), + ); // an agent write piggybacks the pending feedback... const updated = (await ( @@ -731,7 +776,7 @@ test("author=user lastSeq reflects the last comment overall, not the last user c // the next call re-reads the agent comment and wastes a round-trip. const app = makeApp(); const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any; - await app.request("/api/comments", json({ snippet: s.id, text: "first", author: "user" })); + await app.request("/api/comments", viewerJson({ snippet: s.id, text: "first", author: "user" })); await app.request("/api/comments", json({ snippet: s.id, text: "reply", author: "agent" })); const res = (await ( @@ -804,7 +849,7 @@ test("long-poll resolves when a comment arrives", async () => { const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any; const pending = app.request(`/api/comments?session=${s.sessionId}&wait=5`); setTimeout(() => { - app.request("/api/comments", json({ snippet: s.id, text: "feedback!", author: "user" })); + app.request("/api/comments", viewerJson({ snippet: s.id, text: "feedback!", author: "user" })); }, 50); const start = Date.now(); const result = (await (await pending).json()) as any; @@ -1127,7 +1172,10 @@ test("mcp endpoint: initialize, tools/list, publish round trip", async () => { assert.equal(JSON.parse(second.result.content[0].text).sessionId, payload.sessionId); // feedback loop through the mcp tool - await app.request("/api/comments", json({ snippet: payload.id, text: "nice", author: "user" })); + await app.request( + "/api/comments", + viewerJson({ snippet: payload.id, text: "nice", author: "user" }), + ); const feedback = (await ( await app.request( "/mcp", @@ -1204,8 +1252,14 @@ test("agent writes piggyback unseen user comments, delivered once", async () => assert.equal(s.userFeedback, undefined); // the user comments while the agent works on something else - await app.request("/api/comments", json({ snippet: s.id, text: "wrong color", author: "user" })); - await app.request("/api/comments", json({ snippet: s.id, text: "also add a key" })); + await app.request( + "/api/comments", + viewerJson({ snippet: s.id, text: "wrong color", author: "user" }), + ); + await app.request( + "/api/comments", + viewerJson({ snippet: s.id, text: "also add a key", author: "user" }), + ); // the agent's next write carries the feedback const updated = (await ( @@ -1224,9 +1278,12 @@ test("agent writes piggyback unseen user comments, delivered once", async () => assert.equal(again.userFeedback, undefined); // agent replies piggyback too; the user's own comments never do - await app.request("/api/comments", json({ snippet: s.id, text: "more", author: "user" })); + await app.request("/api/comments", viewerJson({ snippet: s.id, text: "more", author: "user" })); const userPost = (await ( - await app.request("/api/comments", json({ snippet: s.id, text: "and more", author: "user" })) + await app.request( + "/api/comments", + viewerJson({ snippet: s.id, text: "and more", author: "user" }), + ) ).json()) as any; assert.equal(userPost.userFeedback, undefined); const reply = (await ( @@ -1243,7 +1300,7 @@ test("a consumed wait is not re-delivered as piggyback", async () => { const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any; await app.request( "/api/comments", - json({ snippet: s.id, text: "seen via wait", author: "user" }), + viewerJson({ snippet: s.id, text: "seen via wait", author: "user" }), ); // the agent receives it through the long-poll... @@ -1259,7 +1316,7 @@ test("a consumed wait is not re-delivered as piggyback", async () => { assert.equal(updated.userFeedback, undefined); // the viewer's unfiltered reads do NOT consume the cursor - await app.request("/api/comments", json({ snippet: s.id, text: "fresh", author: "user" })); + await app.request("/api/comments", viewerJson({ snippet: s.id, text: "fresh", author: "user" })); await app.request(`/api/comments?session=${s.sessionId}`); // viewer-style read const next = (await ( await app.request(`/api/snippets/${s.id}`, { ...json({ html: "

v3

" }), method: "PUT" }) @@ -1287,7 +1344,10 @@ test("feedback consumed via the MCP wait is not re-delivered through REST channe ) ).json()) as any; const p = JSON.parse(published.result.content[0].text); - await app.request("/api/comments", json({ snippet: p.id, text: "via mcp", author: "user" })); + await app.request( + "/api/comments", + viewerJson({ snippet: p.id, text: "via mcp", author: "user" }), + ); // the agent drains it through the MCP tool... const feedback = (await ( @@ -1319,7 +1379,10 @@ test("feedback consumed via the MCP wait is not re-delivered through REST channe test("feedback consumed via a REST wait is not re-delivered through the MCP wait", async () => { const app = makeApp(); const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any; - await app.request("/api/comments", json({ snippet: s.id, text: "via rest", author: "user" })); + await app.request( + "/api/comments", + viewerJson({ snippet: s.id, text: "via rest", author: "user" }), + ); // the agent drains it through a REST author=user read... const restWait = (await ( @@ -1345,7 +1408,7 @@ test("feedback consumed via a REST wait is not re-delivered through the MCP wait // and a fresh comment still flows to the MCP channel — the cursor advanced, // it didn't wedge - await app.request("/api/comments", json({ snippet: s.id, text: "later", author: "user" })); + await app.request("/api/comments", viewerJson({ snippet: s.id, text: "later", author: "user" })); const next = (await ( await app.request( "/mcp", @@ -1373,7 +1436,10 @@ test("mcp publish result carries userFeedback", async () => { ) ).json()) as any; const first = JSON.parse(published.result.content[0].text); - await app.request("/api/comments", json({ snippet: first.id, text: "neat", author: "user" })); + await app.request( + "/api/comments", + viewerJson({ snippet: first.id, text: "neat", author: "user" }), + ); const second = (await ( await app.request( @@ -1427,7 +1493,7 @@ test("comment text and titles are capped before they ride the feedback channel", await app.request( "/api/comments", - json({ snippet: s.id, text: "x".repeat(20000), author: "user" }), + viewerJson({ snippet: s.id, text: "x".repeat(20000), author: "user" }), ); const all = (await (await app.request(`/api/comments?session=${s.sessionId}`)).json()) as any; assert.equal(all.comments[0].text.length, 8000); @@ -1697,8 +1763,8 @@ test("GET /api/comments?surface= filters to that surface, not the whole session" ).json()) as any; // A comment on each surface. - await app.request("/api/comments", json({ surface: a.id, text: "on A", author: "user" })); - await app.request("/api/comments", json({ surface: b.id, text: "on B", author: "user" })); + await app.request("/api/comments", viewerJson({ surface: a.id, text: "on A", author: "user" })); + await app.request("/api/comments", viewerJson({ surface: b.id, text: "on B", author: "user" })); // Filtering by surface must return only that surface's comment. Regression // guard for the app→store query mapping (q.surfaceId → CommentQuery.postId): @@ -1975,7 +2041,7 @@ test("reply_to_user MCP tool accepts postId (and legacy surfaceId)", async () => "/mcp", mcpCall(2, "tools/call", { name: "reply_to_user", - arguments: { postId: id, message: "ack", author: "test-agent" }, + arguments: { postId: id, message: "ack" }, }), ) ).json()) as any; diff --git a/test/cli.test.ts b/test/cli.test.ts index 159d20d..ed50f41 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -59,7 +59,7 @@ function serveApp() { const post = (url: string, body: unknown) => fetch(url, { method: "POST", - headers: { "content-type": "application/json" }, + headers: { "content-type": "application/json", "sec-fetch-site": "same-origin" }, body: JSON.stringify(body), }).then((r) => r.json() as Promise); diff --git a/test/feedbackSqlStore.test.ts b/test/feedbackSqlStore.test.ts index e08a14f..28fa3b4 100644 --- a/test/feedbackSqlStore.test.ts +++ b/test/feedbackSqlStore.test.ts @@ -27,10 +27,17 @@ const json = (body: unknown) => ({ body: JSON.stringify(body), }); +// Simulates a request from the viewer page (same-origin) — the only access +// point that may declare `author`. +const viewerJson = (body: unknown) => ({ + ...json(body), + headers: { "content-type": "application/json", "sec-fetch-site": "same-origin" }, +}); + test("SqlStore: author=user feedback delivers exactly once; the viewer read is unaffected", async () => { const app = makeSqlApp(); const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any; - await app.request("/api/comments", json({ snippet: s.id, text: "first", author: "user" })); + await app.request("/api/comments", viewerJson({ snippet: s.id, text: "first", author: "user" })); // cursor-less read delivers it once... const first = (await ( @@ -53,7 +60,10 @@ test("SqlStore: author=user feedback delivers exactly once; the viewer read is u test("SqlStore: piggybacked feedback on a write advances the cursor; only author=user is delivered", async () => { const app = makeSqlApp(); const s = (await (await app.request("/api/snippets", json({ html: "

x

" }))).json()) as any; - await app.request("/api/comments", json({ snippet: s.id, text: "tweak it", author: "user" })); + await app.request( + "/api/comments", + viewerJson({ snippet: s.id, text: "tweak it", author: "user" }), + ); // the agent's write piggybacks the pending feedback... const updated = (await ( @@ -71,7 +81,7 @@ test("SqlStore: piggybacked feedback on a write advances the cursor; only author // a surface-authored comment is never delivered as user feedback await app.request( "/api/comments", - json({ session: s.sessionId, text: "auto", author: "surface" }), + viewerJson({ snippet: s.id, text: "auto", author: "surface" }), ); const afterSurface = (await ( await app.request(`/api/comments?session=${s.sessionId}&author=user`) diff --git a/test/storeContract.ts b/test/storeContract.ts index 3860fa2..3df07c1 100644 --- a/test/storeContract.ts +++ b/test/storeContract.ts @@ -81,6 +81,13 @@ export function runStoreContract(name: string, makeStore: () => Store | Promise< assert.equal(await store.getSetting("other"), null); }); + contract('reserves "user" as an agent name — coerced to "agent"', async (store) => { + const s = await store.createSession({ agent: "user" }); + assert.equal(s.agent, "agent"); + const got = (await store.getSession(s.id))!; + assert.equal(got.agent, "agent"); + }); + contract("renames sessions; blank title clears it; unknown id is null", async (store) => { const session = await store.createSession({ agent: "pi", title: "Old" }); const renamed = await store.renameSession(session.id, " New ");