Skip to content

Commit 03a1e8c

Browse files
committed
fix: correct session-ownership check
AdminForth/1871/https-claude.aicodeartifacteff
1 parent 9750aaf commit 03a1e8c

2 files changed

Lines changed: 61 additions & 4 deletions

File tree

application/runTurnUseCase.ts

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,10 @@ export class RunTurnUseCase {
178178
return this.pendingInterrupts.get(sessionId) ?? [];
179179
}
180180

181+
private requiresSessionOwnership(input: BaseAgentTurnInput) {
182+
return !input.chatSurface;
183+
}
184+
181185
/** Verify the session exists and belongs to the requesting admin user. */
182186
private async assertSessionOwnership(sessionId: string, adminUser: BaseAgentTurnInput["adminUser"]) {
183187
const s = this.deps.sessionResource;
@@ -193,9 +197,9 @@ export class RunTurnUseCase {
193197
}
194198

195199
/**
196-
* Prepare an edit/branch turn: validate ownership, resolve the fork checkpoint from
197-
* the previous turn, truncate the conversation after the edited turn, and reuse the
198-
* edited turn's id for the regenerated response.
200+
* Prepare an edit/branch turn: resolve the fork checkpoint from the previous turn,
201+
* truncate the conversation after the edited turn, and reuse the edited turn's id
202+
* for the regenerated response. Ownership is already asserted by `prepareTurn`.
199203
*/
200204
private async prepareEditTurn(input: RunAndPersistAgentResponseInput): Promise<PreparedTurn> {
201205
const sequenceDebugSink = this.deps.createDebugSink();
@@ -205,7 +209,6 @@ export class RunTurnUseCase {
205209
if (!this.deps.turnCheckpointsEnabled) {
206210
throw new Error("Edit/fork requires checkpointIdField to be configured on turnResource.");
207211
}
208-
await this.assertSessionOwnership(input.sessionId, input.adminUser);
209212

210213
const agentTurns = await this.deps.sessions.getAgentTurns(input.sessionId);
211214
const targetIndex = agentTurns.findIndex((turn) => turn.id === editTurnId);
@@ -266,6 +269,12 @@ export class RunTurnUseCase {
266269
const approvalDecision = getApprovalDecision(input);
267270
const shouldResume = Boolean(approvalDecision);
268271

272+
console.log(`Preparing turn for session "${input.sessionId}" (resume=${shouldResume})`);
273+
if (this.requiresSessionOwnership(input)) {
274+
console.log(`Asserting session ownership for session "${input.sessionId}"`);
275+
await this.assertSessionOwnership(input.sessionId, input.adminUser);
276+
}
277+
269278
if (input.editTurnId && !shouldResume) {
270279
return this.prepareEditTurn(input);
271280
}

tests/turn_flow.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,54 @@ describe('RunTurnUseCase.handleTurn', () => {
394394
await expect(useCase.handleTurn(input as any)).rejects.not.toThrow('No pending approval');
395395
});
396396

397+
it('rejects a normal turn on a session the user does not own, before creating a turn', async () => {
398+
const { useCase, sessions, llm } = buildUseCase({
399+
sessionOwnerPk: 'someone-else',
400+
streamFor: () => streamOf(text('secret')),
401+
});
402+
const { input } = makeInput();
403+
404+
await expect(useCase.handleTurn(input as any)).rejects.toThrow(/does not belong/);
405+
// Nothing about the victim's session may be touched or streamed back.
406+
expect(sessions.calls.createNewTurn).toHaveLength(0);
407+
expect(llm.calls).toHaveLength(0);
408+
});
409+
410+
it('rejects a turn on a session that does not exist', async () => {
411+
const { useCase } = buildUseCase({ sessionOwnerPk: null });
412+
const { input } = makeInput();
413+
414+
await expect(useCase.handleTurn(input as any)).rejects.toThrow(/not found/);
415+
});
416+
417+
it('rejects an approval resume on a session the user does not own, before resolving interrupts', async () => {
418+
const { useCase, llm } = buildUseCase({
419+
sessionOwnerPk: 'someone-else',
420+
hasPersistentCheckpointer: true,
421+
pendingInterrupts: [{ id: 'int-1', count: 1 }],
422+
});
423+
const { input } = makeInput({ prompt: '', approvalDecision: 'approve' });
424+
425+
// An attacker must not be able to approve a dangerous tool call pending in
426+
// someone else's session.
427+
await expect(useCase.handleTurn(input as any)).rejects.toThrow(/does not belong/);
428+
expect(llm.getPendingInterruptsCalls).toHaveLength(0);
429+
expect(llm.calls).toHaveLength(0);
430+
});
431+
432+
it('allows a chat-surface turn whose session is shared with other linked users', async () => {
433+
const { useCase, llm } = buildUseCase({
434+
sessionOwnerPk: 'someone-else',
435+
streamFor: () => streamOf(text('ok')),
436+
});
437+
const { input } = makeInput({ chatSurface: 'telegram' });
438+
439+
const result = await useCase.handleTurn(input as any);
440+
441+
expect(result.text).toBe('ok');
442+
expect(llm.calls).toHaveLength(1);
443+
});
444+
397445
it('rejects (does not swallow) an approval with no pending interrupt, after emitting turn-started', async () => {
398446
const { useCase } = buildUseCase();
399447
const { input, events } = makeInput({ prompt: '', approvalDecision: 'approve' });

0 commit comments

Comments
 (0)