From 3703da188e7cef9130563eb256de5e77b75888f7 Mon Sep 17 00:00:00 2001 From: ArnabChatterjee20k Date: Thu, 14 May 2026 17:17:27 +0530 Subject: [PATCH] refactor: enhance presence management to prevent cross-user overwrites --- app/realtime.php | 2 +- src/Appwrite/Presences/State.php | 15 ---- tests/e2e/Services/Presences/PresenceBase.php | 71 +++++++++---------- 3 files changed, 36 insertions(+), 52 deletions(-) diff --git a/app/realtime.php b/app/realtime.php index 0e952e0de0..8066b99bc8 100644 --- a/app/realtime.php +++ b/app/realtime.php @@ -1268,7 +1268,7 @@ $server->onClose(function (int $connection) use ($realtime, $stats, $register, $ $publisherForUsage = $container->get('publisherForUsage'); try { - $deletionCount = $dbForProject->deleteDocuments('presenceLogs', [Query::equal('$id', $presenceIds)]); + $deletionCount = $dbForProject->getAuthorization()->skip(fn () => $dbForProject->deleteDocuments('presenceLogs', [Query::equal('$id', $presenceIds)])); $presenceState->triggerUsage($publisherForUsage, $project, -$deletionCount); } catch (Throwable $th) { Span::error($th); diff --git a/src/Appwrite/Presences/State.php b/src/Appwrite/Presences/State.php index f12bd65b75..19e7dc98b7 100644 --- a/src/Appwrite/Presences/State.php +++ b/src/Appwrite/Presences/State.php @@ -92,21 +92,6 @@ class State if ($dbForProject->getAdapter()->getSupportForUpsertOnUniqueIndex()) { $existingPresence = $dbForProject->findOne(self::COLLECTION_ID, [Query::equal('userInternalId', [$userInternalId])]); if ($existingPresence->isEmpty()) { - // Guard against cross-user overwrite: upsertDocument matches on primary key, - // so a caller-supplied $presenceId that collides with another user's record - $existingById = $dbForProject->getDocument( - self::COLLECTION_ID, - $presenceDocument->getId(), - ); - if ( - !$existingById->isEmpty() - && $existingById->getAttribute('userInternalId') !== $userInternalId - ) { - throw new Exception( - Exception::PRESENCE_ALREADY_EXISTS, - params: [$presenceDocument->getId()], - ); - } $presenceCreated = true; } else { $presenceDocument->setAttribute('$id', $existingPresence->getId()); diff --git a/tests/e2e/Services/Presences/PresenceBase.php b/tests/e2e/Services/Presences/PresenceBase.php index 40a15da474..1c94ade61b 100644 --- a/tests/e2e/Services/Presences/PresenceBase.php +++ b/tests/e2e/Services/Presences/PresenceBase.php @@ -1032,77 +1032,76 @@ trait PresenceBase */ public function testCrossUserUpsertDoesNotOverwriteForeignPresence(): void { - if ($this->getSide() !== 'server') { + if ($this->getSide() !== 'client') { $this->expectNotToPerformAssertions(); return; } $projectId = $this->getProject()['$id']; - $adminHeaders = \array_merge([ + $originalUser = $this->getUser(); + + $user1 = $this->getUser(true); + $user2 = $this->getUser(true); + + // Preserve the cached session for the rest of the test run. + self::$user[$projectId] = $originalUser; + + $headersUser1 = [ 'content-type' => 'application/json', 'x-appwrite-project' => $projectId, - ], $this->getHeaders()); - $presenceHeaders = [ + 'origin' => 'http://localhost', + 'cookie' => 'a_session_' . $projectId . '=' . $user1['session'], + ]; + $headersUser2 = [ 'content-type' => 'application/json', 'x-appwrite-project' => $projectId, - 'x-appwrite-key' => $this->getPresenceApiKey(), + 'origin' => 'http://localhost', + 'cookie' => 'a_session_' . $projectId . '=' . $user2['session'], ]; - - $suffix = \uniqid(); - - $userA = $this->client->call(Client::METHOD_POST, '/users', $adminHeaders, [ - 'userId' => ID::unique(), - 'email' => 'cross-upsert-a-' . $suffix . '@test.io', - 'password' => 'password-a-' . $suffix, - ]); - $this->assertEquals(201, $userA['headers']['status-code']); - $userAId = $userA['body']['$id']; - - $userB = $this->client->call(Client::METHOD_POST, '/users', $adminHeaders, [ - 'userId' => ID::unique(), - 'email' => 'cross-upsert-b-' . $suffix . '@test.io', - 'password' => 'password-b-' . $suffix, - ]); - $this->assertEquals(201, $userB['headers']['status-code']); - $userBId = $userB['body']['$id']; - $this->assertNotSame($userAId, $userBId); $sharedPresenceId = ID::unique(); $victim = $this->client->call( Client::METHOD_PUT, '/presences/' . $sharedPresenceId, - $presenceHeaders, + $headersUser1, [ - 'userId' => $userAId, 'status' => 'online', - 'metadata' => ['owner' => 'A'], + 'metadata' => ['owner' => 'user1'], ] ); $this->assertEquals(200, $victim['headers']['status-code']); $this->assertEquals($sharedPresenceId, $victim['body']['$id']); - $this->assertEquals($userAId, $victim['body']['userId']); + $this->assertEquals($user1['$id'], $victim['body']['userId']); $attack = $this->client->call( Client::METHOD_PUT, '/presences/' . $sharedPresenceId, - $presenceHeaders, + $headersUser2, [ - 'userId' => $userBId, 'status' => 'online', - 'metadata' => ['owner' => 'B'], + 'metadata' => ['owner' => 'user2'], ] ); - $this->assertEquals(409, $attack['headers']['status-code']); - $this->assertSame('presence_already_exists', $attack['body']['type'] ?? null); + $this->assertNotEquals( + 200, + $attack['headers']['status-code'], + 'Cross-user upsert must not succeed silently. Got body: ' . \json_encode($attack['body'] ?? []) + ); + // Verify User1's row is intact. Read via a presence-scoped API key to bypass + // any read-permission ambiguity and inspect the persisted state directly. $check = $this->client->call( Client::METHOD_GET, '/presences/' . $sharedPresenceId, - $presenceHeaders + [ + 'content-type' => 'application/json', + 'x-appwrite-project' => $projectId, + 'x-appwrite-key' => $this->getPresenceApiKey(), + ] ); $this->assertEquals(200, $check['headers']['status-code']); - $this->assertEquals($userAId, $check['body']['userId']); - $this->assertEquals(['owner' => 'A'], $check['body']['metadata']); + $this->assertEquals($user1['$id'], $check['body']['userId']); + $this->assertEquals(['owner' => 'user1'], $check['body']['metadata']); } }