From 605d453e3a0ad019ed8674784b69ae4164907bd1 Mon Sep 17 00:00:00 2001 From: Thor Haaland Date: Tue, 6 Oct 2026 21:28:54 +0000 Subject: [PATCH] fix: retain user file ownership beyond session expiry --- service/src/agent-run.test.ts | 36 +++++++++++++++++++++++++++++++++- service/src/agent-run.ts | 7 ++++++- service/src/file-server.ts | 18 ++++++++--------- service/src/middleware/auth.ts | 17 ++++++++-------- service/src/types/service.ts | 5 ++--- 5 files changed, 61 insertions(+), 22 deletions(-) diff --git a/service/src/agent-run.test.ts b/service/src/agent-run.test.ts index e2c84cf7..452fa2a9 100644 --- a/service/src/agent-run.test.ts +++ b/service/src/agent-run.test.ts @@ -21,6 +21,8 @@ import { CodeApiJwtAuthError, verifyLibreChatJwt } from './auth/librechat-jwt'; import { applyPrincipal } from './auth/principal'; import { OWNER_BINDING_HEADER, + OWNER_BINDING_PATTERN, + sessionOwnerBindingValue, agentRunSessionKey, ownerBindingFromHeader, ownerBindingValue, @@ -136,7 +138,7 @@ function uploadedRef(result: { body: unknown }): { sid: string; fid: string } { /** The new file server's owner-checked delete, applied to the stub's objects. */ const newFileServerOwnerDelete = (req: { headers: Record }, key: string) => { const expected = req.headers['x-codeapi-owner-expect']; - if (typeof expected !== 'string' || !/^agent_run\.[0-9a-f]{64}$/.test(expected)) return { status: 400, body: { error: 'Expected owner is required' } }; + if (typeof expected !== 'string' || !OWNER_BINDING_PATTERN.test(expected)) return { status: 400, body: { error: 'Expected owner is required' } }; if (!harness.objects.has(key)) return { status: 200, body: { outcome: 'absent' } }; const stored = bindings.get(key); if (!stored || stored !== expected) return { status: 403, body: { error: 'Owner binding does not match' } }; @@ -163,6 +165,10 @@ beforeAll(async () => { const owner = ownerBindingFromHeader(headers[OWNER_BINDING_HEADER.toLowerCase()], sid, fid); if (!owner.ok) return { status: 400, body: { error: owner.error } }; if (owner.binding) bindings.set(`${sid}/${fid}`, owner.binding); + else { + const sessionKey = harness.redis.entries.get(`session:${sid}`)?.value; + if (sessionKey?.includes(':user:')) bindings.set(`${sid}/${fid}`, sessionOwnerBindingValue(sessionKey)); + } return undefined; }); harness.setOwnerDeleteHandler(newFileServerOwnerDelete); @@ -177,6 +183,34 @@ beforeEach(() => { delete process.env.CODEAPI_TENANT_ISOLATION_STRICT; }); +describe('durable user deletion', () => { + test('expired user session keeps bytes private and lets only the owner delete', async () => { + const { sid, fid } = uploadedRef(await agentUpload({ kind: 'user', id: USER }, userToken())); + expect(harness.objects.get(`${sid}/${fid}`)?.bytes.toString()).toBe('agent input'); + await harness.redis.del(`session:${sid}`); + const path = `/v1/files/${sid}/${fid}?kind=user&id=${USER}`; + for (const token of [userToken({ sub: 'other-user' }), userToken({ tenant_id: OTHER_TENANT })]) { + expect((await call(harness.baseUrl, token, 'DELETE', path)).status).toBe(403); + expect(harness.objects.has(`${sid}/${fid}`)).toBe(true); + } + expect((await call(harness.baseUrl, userToken(), 'GET', `/v1/download/${sid}/${fid}?kind=user&id=${USER}`)).status).toBe(403); + expect((await call(harness.baseUrl, userToken(), 'DELETE', path)).status).toBe(200); + expect(harness.objects.has(`${sid}/${fid}`)).toBe(false); + expect((await call(harness.baseUrl, userToken(), 'DELETE', path)).status).toBe(404); + }); + + test('unbound old objects and conflicting live sessions cannot use the fallback', async () => { + const { sid, fid } = uploadedRef(await agentUpload({ kind: 'user', id: USER }, userToken())); + const path = `/v1/files/${sid}/${fid}?kind=user&id=${USER}`; + await harness.redis.set(`session:${sid}`, `${OTHER_TENANT}:user:${USER}`); + expect((await call(harness.baseUrl, userToken(), 'DELETE', path)).status).toBe(403); + await harness.redis.del(`session:${sid}`); + bindings.delete(`${sid}/${fid}`); + expect((await call(harness.baseUrl, userToken(), 'DELETE', path)).status).toBe(403); + expect(harness.objects.has(`${sid}/${fid}`)).toBe(true); + }); +}); + describe('agent_run claim grammar', () => { test('a well-formed agent_run token verifies to an Agent principal with no user', () => { const principal = verifyLibreChatJwt(agentToken()); diff --git a/service/src/agent-run.ts b/service/src/agent-run.ts index f3e6e88e..962f037d 100644 --- a/service/src/agent-run.ts +++ b/service/src/agent-run.ts @@ -64,7 +64,7 @@ export const OWNER_BINDING_HEADER = 'X-CodeAPI-Owner-Binding'; /** Internal api → file_server header: delete only if the stored binding equals this. */ export const OWNER_EXPECT_HEADER = 'X-CodeAPI-Owner-Expect'; /** Shape of a stored binding (see `ownerBindingValue`). */ -export const OWNER_BINDING_PATTERN = /^agent_run\.[0-9a-f]{64}$/; +export const OWNER_BINDING_PATTERN = /^(?:agent_run|session)\.[0-9a-f]{64}$/; /** * Last path segment of the file server's owner-checked delete operation, * `POST /sessions/:sid/objects/:fid/owner-delete`. File servers that predate @@ -88,6 +88,11 @@ export function ownerBindingValue(tenantId: string, subject: AgentRunSubject): s return `agent_run.${digest}`; } +/** Bind bytes to the server-derived session namespace, never a caller-supplied id. */ +export function sessionOwnerBindingValue(sessionKey: string): string { + return `session.${createHash('sha256').update(sessionKey).digest('hex')}`; +} + function ownerBindingKey(): Buffer | undefined { const token = (process.env[INTERNAL_SERVICE_TOKEN_ENV] ?? '').trim(); if (!token) return undefined; diff --git a/service/src/file-server.ts b/service/src/file-server.ts index 46f404be..6f5477c5 100644 --- a/service/src/file-server.ts +++ b/service/src/file-server.ts @@ -27,6 +27,7 @@ import { OWNER_BINDING_HEADER, ownerBindingFromHeader, sessionKeyForLog, + sessionOwnerBindingValue, } from './agent-run'; const { INSTANCE_ID } = env; @@ -263,14 +264,13 @@ async function uploadFile( if (readOnly) { metaData['X-Amz-Meta-Read-Only'] = 'true'; } - /* Durable agent-run owner binding (C4). Only a binding an internal caller - * signed for this exact object reaches here; it is stored with the bytes - * so it outlives the session cache and disappears with the object. */ - if (ownerBinding) { - metaData[OWNER_METADATA] = ownerBinding; - } - + /* The signed run binding takes precedence. User objects retain the + * server-owned session namespace with their bytes, including sandbox outputs. */ const sessionKey = await redisClient.get(`session:${session_id}`); + const durableOwner = ownerBinding ?? (sessionKey?.includes(':user:') ? sessionOwnerBindingValue(sessionKey) : undefined); + if (durableOwner) { + metaData[OWNER_METADATA] = durableOwner; + } const peeked = await peekStreamForEmpty(fileStream); let receivedBytes = 0; if (peeked.empty) { @@ -752,8 +752,8 @@ app.delete('/sessions/:session_id/objects/:fileId', async (req, res) => { }); /** - * Owner-checked deletion for agent_run objects whose session cache has - * expired. A separate operation (not the plain DELETE) so a file server that + * Owner-checked deletion for user and agent_run objects whose session cache + * has expired. A separate operation (not the plain DELETE) so a file server that * predates owner bindings answers 404 instead of deleting with the internal * credential. It never deletes without an expected owner, and never deletes an * object that has no stored binding or a different one. Outcomes: diff --git a/service/src/middleware/auth.ts b/service/src/middleware/auth.ts index c087d1fb..21326bc5 100644 --- a/service/src/middleware/auth.ts +++ b/service/src/middleware/auth.ts @@ -7,7 +7,7 @@ import { resolveSessionKey, parseUploadSessionKeyInput, SessionKeyResolutionErro import { LibreChatJwtAuthProvider, CodeApiJwtAuthError } from '../auth/librechat-jwt'; import { applyPrincipal, type AgentRunPrincipal, type CodeApiPrincipal } from '../auth/principal'; import { applyLocalPrincipal } from '../auth/local'; -import { agentRunLogFields, ownerBindingValue, sessionKeyForLog } from '../agent-run'; +import { agentRunLogFields, ownerBindingValue, sessionOwnerBindingValue, sessionKeyForLog } from '../agent-run'; import { internalServiceAuthEnabled } from '../internal-service-auth'; import { AuthProviderConfigError, getAuthProviderMode } from '../auth/provider'; import { @@ -311,14 +311,15 @@ export const sessionAuth = async (req: AuthenticatedRequest, res: Response, next } const cachedSessionKey = await connection.get(`session:${session_id}`); if (cachedSessionKey !== sessionKey) { - /* The session cache expires after SESSION_CACHE_TTL. A deletion-only - * agent_run token may then still delete, but only against the durable - * owner binding stored with the bytes: the file server removes the object - * only if that binding names this tenant, Agent and run. A present but - * different cache entry is a refusal. */ - if (cachedSessionKey === null && agentRunPrincipal?.agentRun.fileDelete && req.method === 'DELETE') { + /* An expired cache can be replaced only by the object's durable binding. + * A present but different cache entry still refuses the request. Shared + * resource identities never authorize this user-private fallback. */ + const userDelete = !agentRunPrincipal && sessionKeyInput.kind === 'user' && internalServiceAuthEnabled(); + if (cachedSessionKey === null && req.method === 'DELETE' && (agentRunPrincipal?.agentRun.fileDelete || userDelete)) { req.sessionKey = sessionKey; - req.ownerBindingExpectation = ownerBindingValue(agentRunPrincipal.tenantId, agentRunPrincipal.agentRun); + req.ownerBindingExpectation = agentRunPrincipal + ? ownerBindingValue(agentRunPrincipal.tenantId, agentRunPrincipal.agentRun) + : sessionOwnerBindingValue(sessionKey); logger.info('Session cache absent; deleting against the durable owner binding', authLogMeta(req)); next(); return; diff --git a/service/src/types/service.ts b/service/src/types/service.ts index 9d5ef9fc..fb6629da 100644 --- a/service/src/types/service.ts +++ b/service/src/types/service.ts @@ -320,9 +320,8 @@ export interface AuthenticatedRequest extends Request { planId?: string; executionIdentity?: ExecutionIdentity; codeApiAuthContext?: CodeApiAuthContext; - /** Set by sessionAuth when a deletion-only agent_run token authorizes a - * DELETE against the durable owner binding because the session cache entry - * has expired. The file server deletes only if the stored binding matches. */ + /** Set by sessionAuth for a user or deletion-only agent_run DELETE after + * session expiry. The file server deletes only if the durable binding matches. */ ownerBindingExpectation?: string; codeApiPrincipal?: CodeApiPrincipal; }