diff --git a/changelog.d/fixes/notion-thread-idor-hardening.md b/changelog.d/fixes/notion-thread-idor-hardening.md new file mode 100644 index 0000000000..24909ac462 --- /dev/null +++ b/changelog.d/fixes/notion-thread-idor-hardening.md @@ -0,0 +1 @@ +- Harden the Notion web provider thread-session cache against cross-tenant access: the cache is now namespaced per caller (hash of the caller's cookie) instead of only by Notion space id, and client-supplied thread ids are validated as real Notion UUIDs before use — closing an IDOR where two users of the same Notion space could pin/read each other's thread (#7900 security review). diff --git a/open-sse/executors/notion-web.ts b/open-sse/executors/notion-web.ts index 30475b8ef3..72a3f3c8b7 100644 --- a/open-sse/executors/notion-web.ts +++ b/open-sse/executors/notion-web.ts @@ -43,6 +43,7 @@ import { notionThreadSessionLookup, notionThreadSessionStore, readClientThreadId, + hashNotionCallerCookie, resolveNotionThreadBinding, type NotionMessage, } from "../services/notionThreadSessions.ts"; @@ -553,8 +554,13 @@ export class NotionWebExecutor extends BaseExecutor { | Record | undefined); const clientThreadId = readClientThreadId(requestBody, inboundHeaders ?? undefined); - // Namespace thread cache by custom agent so default AI and agents never share threads. - const threadSpaceKey = agent.workflowId ? `${spaceId}|wf:${agent.workflowId}` : spaceId; + // Namespace the thread cache PER CALLER (hash of the caller's cookie) AND by custom + // agent, so (a) two users of the same Notion space never share a cached thread + // (cross-tenant IDOR, #7900 review) and (b) default AI and agents never share threads. + const callerScope = hashNotionCallerCookie(cookie); + const threadSpaceKey = agent.workflowId + ? `caller:${callerScope}|${spaceId}|wf:${agent.workflowId}` + : `caller:${callerScope}|${spaceId}`; const binding = resolveNotionThreadBinding(threadSpaceKey, messages, clientThreadId); let { threadId, createThread, rootKey } = binding; diff --git a/open-sse/services/notionThreadSessions.ts b/open-sse/services/notionThreadSessions.ts index de6378e745..4f6032ae34 100644 --- a/open-sse/services/notionThreadSessions.ts +++ b/open-sse/services/notionThreadSessions.ts @@ -400,6 +400,16 @@ export function notionThreadSessionStore( /** Client-supplied thread continuity pin: body (`notion_thread_id`/`thread_id`) or * the `X-Notion-Thread-Id` header (case-insensitive). */ +/** + * A Notion page/thread id is a UUID (32 hex chars, dashed or undashed). Reject + * anything else so a client cannot pin/poison the session cache with an arbitrary + * string (defense against cross-tenant thread-id injection — see #7900 review). + */ +export function isValidNotionThreadId(id: string): boolean { + const t = id.trim().replace(/-/g, ""); + return /^[0-9a-f]{32}$/i.test(t); +} + export function readClientThreadId( body: NotionThreadRequestBody, headers?: Record @@ -408,12 +418,31 @@ export function readClientThreadId( (typeof body.notion_thread_id === "string" && body.notion_thread_id.trim()) || (typeof body.thread_id === "string" && body.thread_id.trim()) || ""; - if (fromBody) return fromBody; + if (fromBody) return isValidNotionThreadId(fromBody) ? fromBody : ""; if (!headers) return ""; for (const [k, v] of Object.entries(headers)) { if (k.toLowerCase() === "x-notion-thread-id" && typeof v === "string" && v.trim()) { - return v.trim(); + const h = v.trim(); + return isValidNotionThreadId(h) ? h : ""; } } return ""; } + +/** + * Short FNV-1a hash of a caller's Notion cookie, used to namespace the thread-session + * cache PER CALLER. Without this, two users of the SAME Notion space share one cache + * key (spaceId is space-, not user-scoped), so one user's thread could be served to + * another (cross-tenant IDOR, #7900 review). The raw cookie is never stored — only this + * non-reversible digest. + */ +export function hashNotionCallerCookie(cookie: string): string { + const raw = (cookie || "").trim(); + if (!raw) return "anon"; + let hash = 0x811c9dc5; + for (let i = 0; i < raw.length; i++) { + hash ^= raw.charCodeAt(i); + hash = Math.imul(hash, 0x01000193) >>> 0; + } + return hash.toString(16).padStart(8, "0"); +} diff --git a/tests/unit/notion-thread-idor-7900.test.ts b/tests/unit/notion-thread-idor-7900.test.ts new file mode 100644 index 0000000000..1ccd4b8c29 --- /dev/null +++ b/tests/unit/notion-thread-idor-7900.test.ts @@ -0,0 +1,47 @@ +// Regression guard for the cross-tenant IDOR flagged in the #7900 security review: +// the Notion thread-session cache was keyed only by spaceId (space-, not user-scoped) +// and accepted an arbitrary client-supplied thread id, so one user of a shared Notion +// space could pin/read another user's thread. Fix: validate the id is a real Notion +// UUID, and namespace the cache per caller (hash of the caller's cookie). + +import test from "node:test"; +import assert from "node:assert/strict"; +import { + isValidNotionThreadId, + readClientThreadId, + hashNotionCallerCookie, +} from "../../open-sse/services/notionThreadSessions.ts"; + +test("isValidNotionThreadId accepts real Notion UUIDs (dashed + undashed), rejects junk", () => { + assert.equal(isValidNotionThreadId("11111111-2222-3333-4444-555555555555"), true); + assert.equal(isValidNotionThreadId("11111111222233334444555555555555"), true); + assert.equal(isValidNotionThreadId("../../etc/passwd"), false); + assert.equal(isValidNotionThreadId("not-a-uuid"), false); + assert.equal(isValidNotionThreadId(""), false); + assert.equal(isValidNotionThreadId("11111111-2222-3333-4444-55555555555"), false); // too short +}); + +test("readClientThreadId rejects a non-UUID client-supplied thread id (body + header)", () => { + // A malformed id must NOT be accepted into the session cache. + assert.equal(readClientThreadId({ notion_thread_id: "attacker-pinned-value" }), ""); + assert.equal(readClientThreadId({ thread_id: "'; DROP TABLE" }), ""); + assert.equal( + readClientThreadId({}, { "x-notion-thread-id": "../../secret" }), + "" + ); + // A well-formed id is still accepted. + const good = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; + assert.equal(readClientThreadId({ notion_thread_id: good }), good); + assert.equal(readClientThreadId({}, { "x-notion-thread-id": good }), good); +}); + +test("hashNotionCallerCookie namespaces per caller — different cookies never collide", () => { + const a = hashNotionCallerCookie("token_v2=USER_A_SESSION; space=X"); + const b = hashNotionCallerCookie("token_v2=USER_B_SESSION; space=X"); + assert.notEqual(a, b, "two users of the same space must get different cache namespaces"); + // Stable for the same caller. + assert.equal(a, hashNotionCallerCookie("token_v2=USER_A_SESSION; space=X")); + // Never stores the raw cookie. + assert.ok(!a.includes("USER_A_SESSION")); + assert.equal(hashNotionCallerCookie(""), "anon"); +});