diff --git a/packages/api/src/domains/approval-hub/ApprovalIngress.ts b/packages/api/src/domains/approval-hub/ApprovalIngress.ts index 597b661495..92893aeb07 100644 --- a/packages/api/src/domains/approval-hub/ApprovalIngress.ts +++ b/packages/api/src/domains/approval-hub/ApprovalIngress.ts @@ -2,6 +2,7 @@ import type { ApprovalEnvelope, ApprovalOriginRef, ApprovalProducerId, CatId, Ri import { approvalProducerMeta, validateApprovalEnvelope, validateApprovalOriginRef } from '@cat-cafe/shared'; import type { SocketManager } from '../../infrastructure/websocket/index.js'; import type { IMessageStore, StoredMessage } from '../cats/services/stores/ports/MessageStore.js'; +import { isSystemUserMessage } from '../cats/services/stores/visibility.js'; import { approvalCardIdempotencyKey, buildApprovalCardBlock } from './buildApprovalCardBlock.js'; import type { ApprovalPublicationStore } from './ports/ApprovalPublicationStore.js'; @@ -218,7 +219,45 @@ export class ApprovalIngress { const origin = await this.deps.messageStore.getById(draft.originRef.messageId); if (!origin || origin.deletedAt || origin._tombstone) throw new Error('Approval origin message not found'); if (origin.threadId !== draft.originRef.threadId) throw new Error('Approval origin message thread mismatch'); - if (origin.userId !== draft.ownerUserId) throw new Error('Approval origin message owner mismatch'); + // Cross-tenant isolation here is a CONJUNCTION of two parts, and neither part + // is sufficient on its own (@codex-luna's review, rounds 2-4): + // + // (1) CALLER: originRef.threadId / .messageId and ownerUserId must be bound + // to an authenticated record. This class CANNOT verify that binding. + // (2) THIS CLASS: the assertion above checks that the origin message really + // does live in the thread the draft names. + // + // (2) alone proves nothing about tenancy — a draft naming a thread of its own + // choosing satisfies the comparison trivially. It carries weight only because + // (1) fixes which thread may be named. On the F225 session-handoff path (1) + // holds: that producer derives all three from an authenticated + // InvocationRecord, and a request body cannot rewrite them. + // + // ApprovalIngress serves several producers, so (1) is a property of the F225 + // path and NOT of this class. Any producer deriving a draft from untrusted + // input must itself establish and validate that binding before calling this + // ingress; the exemption below assumes (1) rather than proving it. + // + // Given (1) AND (2), what the comparison below still decides is narrower: who + // may have authored a row inside an already-verified thread — and a system + // pseudo-user speaking in that thread is not another tenant. + // + // Without the exemption this rejected precisely the sessions that need it most. + // A session-handoff proposal anchors on the message that triggered the + // invocation, and for any long-running session that message IS the scheduler's + // wake row ("持球唤醒"), persisted as userId='scheduler' / catId=null. So a + // timer-woken cat could never hand off, while the same proposal from an + // A2A-triggered turn (authored by the real owner) went through — the failure + // was invisible except as a 500 with no actionable text. + // + // isSystemUserMessage is the store layer's existing predicate for exactly this + // distinction, and it is deliberately reused rather than re-derived here: it + // requires BOTH a system userId AND a system/null catId, so a cat-authored row + // wearing a system userId stays rejected. A second, private definition of "is + // this the system" is how the two drift apart. + if (origin.userId !== draft.ownerUserId && !isSystemUserMessage(origin)) { + throw new Error('Approval origin message owner mismatch'); + } } private async findPersistedCard( diff --git a/packages/api/test/approval-hub/approval-ingress.test.js b/packages/api/test/approval-hub/approval-ingress.test.js index bb1c4f97a6..1a86460946 100644 --- a/packages/api/test/approval-hub/approval-ingress.test.js +++ b/packages/api/test/approval-hub/approval-ingress.test.js @@ -41,7 +41,7 @@ class FakePublicationStore { } } -function appendOrigin(messageStore) { +function appendOrigin(messageStore, authorOverrides = {}) { messageStore.append({ userId: ownerUserId, catId: null, @@ -49,6 +49,7 @@ function appendOrigin(messageStore) { mentions: [], timestamp: 1_721_111_110_000, threadId: originRef.threadId, + ...authorOverrides, }); const stored = messageStore.getByThread(originRef.threadId, 1)[0]; stored.id = originRef.messageId; @@ -76,9 +77,9 @@ function makeDraft(overrides = {}) { }; } -function makeHarness() { +function makeHarness(originAuthorOverrides = {}) { const messageStore = new MessageStore(); - appendOrigin(messageStore); + appendOrigin(messageStore, originAuthorOverrides); const broadcasts = []; const userEvents = []; const socketManager = { @@ -100,6 +101,48 @@ function findApprovalCard(messageStore) { } describe('ApprovalIngress', () => { + // F167 — a session-handoff proposal anchors on the message that triggered the + // invocation, and for a long-running session that message IS the scheduler's + // wake row ("持球唤醒"), persisted under the `scheduler` system pseudo-user with + // `catId: null`. A bare `origin.userId !== ownerUserId` check therefore rejected + // exactly the sessions most in need of a handoff: every attempt driven by a + // timer wake failed with "Approval origin message owner mismatch", while the + // same proposal from an A2A-triggered turn (userId = the real owner) succeeded. + // + // The cross-user protection does not live in this comparison — the preceding + // threadId assertion already pins the origin to the caller's own thread. What + // this one adds is a rule about WHO may author a row inside that thread, and a + // system pseudo-user speaking in your own thread is not another tenant. + it('accepts a scheduler-authored origin in the owner thread', async () => { + const harness = makeHarness({ userId: 'scheduler', catId: null }); + const store = new FakePublicationStore(); + + const envelope = await harness.ingress.publish(makeDraft(), store); + + assert.equal(store.publication.state, 'anchored'); + assert.equal(envelope.approvalCardRef.threadId, 'source-thread'); + }); + + // The exemption must not become a hole. A different HUMAN owner is a genuine + // cross-tenant anchor and stays rejected — without this, the fix above would be + // indistinguishable from deleting the check. + it('still rejects an origin authored by another human user', async () => { + const harness = makeHarness({ userId: 'user-2', catId: null }); + const store = new FakePublicationStore(); + + await assert.rejects(() => harness.ingress.publish(makeDraft(), store), /Approval origin message owner mismatch/); + }); + + // isSystemUserMessage requires BOTH a system userId AND a system/null catId, so + // a cat-authored row wearing a system userId is not a system message. Pinning it + // here keeps the exemption tied to that predicate rather than to the userId alone. + it('rejects a system userId carried by a cat-authored message', async () => { + const harness = makeHarness({ userId: 'scheduler', catId: 'codex-sol' }); + const store = new FakePublicationStore(); + + await assert.rejects(() => harness.ingress.publish(makeDraft(), store), /Approval origin message owner mismatch/); + }); + it('persists one card, commits its exact envelope, then broadcasts', async () => { const harness = makeHarness(); const store = new FakePublicationStore();