Skip to content

Commit 6851434

Browse files
abnegateclaude
abnegate
and
claude
committed
fix(database): guard by-id reads against foreign-document cache poisoning
getDocument($id) must never return, or keep serving, a document whose $id differs from the requested id. A production incident (DAT-1904) served a sibling row's body under the requested key after a mismatched read was persisted cache-aside, so every subsequent by-id read returned the wrong document until the entry expired (~90s), while list reads stayed correct. Two-tier guard in getDocument(): - a cached body whose $id differs from the requested id is a poisoned entry: purge it and fall through to the source read instead of serving it, so the cache self-heals on the next read rather than after TTL expiry. - a source (adapter) row whose $id differs from the requested id is an identity violation (e.g. a result-set delivered on the wrong pooled connection): throw instead of caching or returning it, converting silent cross-serving into a loud, observable failure. A partial selection that omits $id yields an empty id and is not treated as foreign, so the check never false-positives on a legitimate projection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent d5ebecc commit 6851434

2 files changed

Lines changed: 177 additions & 12 deletions

File tree

‎src/Database/Database.php‎

Lines changed: 43 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4883,23 +4883,33 @@ public function getDocument(string $collection, string $id, array $queries = [],
48834883
// compare equal under strict equality (e.g. in updateDocument).
48844884
$document = $this->casting($collection, $document);
48854885

4886-
if ($collection->getId() !== self::METADATA) {
4886+
// Defend the by-id read invariant: a cache entry whose body carries a
4887+
// different $id than requested is a poisoned key (a mismatched read
4888+
// was persisted under this key). Serving it returns the wrong document
4889+
// for every reader until the entry expires. Purge it and fall through
4890+
// to the source read instead of trusting the poisoned copy.
4891+
if ($this->isForeignDocument($document, $id)) {
4892+
Console::error("Cache for '{$collection->getId()}:{$id}' held foreign document '{$document->getId()}'; purging and refetching");
4893+
$this->purgeCachedDocument($collection->getId(), $id);
4894+
} else {
4895+
if ($collection->getId() !== self::METADATA) {
48874896

4888-
if (!$this->authorization->isValid(new Input(self::PERMISSION_READ, [
4889-
...$collection->getRead(),
4890-
...($documentSecurity ? $document->getRead() : [])
4891-
]))) {
4892-
return $this->createDocumentInstance($collection->getId(), []);
4897+
if (!$this->authorization->isValid(new Input(self::PERMISSION_READ, [
4898+
...$collection->getRead(),
4899+
...($documentSecurity ? $document->getRead() : [])
4900+
]))) {
4901+
return $this->createDocumentInstance($collection->getId(), []);
4902+
}
48934903
}
4894-
}
48954904

4896-
$this->trigger(self::EVENT_DOCUMENT_READ, $document);
4905+
$this->trigger(self::EVENT_DOCUMENT_READ, $document);
48974906

4898-
if ($this->isTtlExpired($collection, $document)) {
4899-
return $this->createDocumentInstance($collection->getId(), []);
4900-
}
4907+
if ($this->isTtlExpired($collection, $document)) {
4908+
return $this->createDocumentInstance($collection->getId(), []);
4909+
}
49014910

4902-
return $document;
4911+
return $document;
4912+
}
49034913
}
49044914

49054915
// Capture the generation before reading: if a concurrent purge advances
@@ -4924,6 +4934,14 @@ public function getDocument(string $collection, string $id, array $queries = [],
49244934
return $this->createDocumentInstance($collection->getId(), []);
49254935
}
49264936

4937+
// A row whose $id differs from the requested id is a source-level identity
4938+
// violation (e.g. a result-set delivered on the wrong pooled connection),
4939+
// not a cache artifact. Never cache or return it: caching would poison this
4940+
// key, and returning it would serve one document's body under another's id.
4941+
if ($this->isForeignDocument($document, $id)) {
4942+
throw new DatabaseException("getDocument('{$collection->getId()}', '{$id}') returned document with mismatched \$id '{$document->getId()}'");
4943+
}
4944+
49274945
if ($this->isTtlExpired($collection, $document)) {
49284946
return $this->createDocumentInstance($collection->getId(), []);
49294947
}
@@ -4979,6 +4997,19 @@ public function getDocument(string $collection, string $id, array $queries = [],
49794997
return $document;
49804998
}
49814999

5000+
/**
5001+
* Whether a document read for $id came back carrying a different, non-empty
5002+
* $id — i.e. another row's body served under this key. A partial selection
5003+
* that omits $id yields an empty id and is not treated as foreign, so the
5004+
* check never false-positives on a legitimate projection.
5005+
*/
5006+
private function isForeignDocument(Document $document, string $id): bool
5007+
{
5008+
$actual = $document->getId();
5009+
5010+
return $actual !== '' && $actual !== $id;
5011+
}
5012+
49825013
private function isTtlExpired(Document $collection, Document $document): bool
49835014
{
49845015
if (!$this->adapter->getSupportForTTLIndexes()) {

‎tests/unit/ForeignDocumentGuardTest.php‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
<?php
2+
3+
namespace Tests\Unit;
4+
5+
use PHPUnit\Framework\TestCase;
6+
use Utopia\Cache\Adapter\Memory as CacheMemory;
7+
use Utopia\Cache\Cache;
8+
use Utopia\Database\Adapter\Memory as DatabaseMemory;
9+
use Utopia\Database\Database;
10+
use Utopia\Database\Document;
11+
use Utopia\Database\Exception as DatabaseException;
12+
use Utopia\Database\Helpers\Permission;
13+
use Utopia\Database\Helpers\Role;
14+
15+
/**
16+
* Regression guard for the by-id read identity invariant: getDocument($id) must
17+
* never return, or keep serving, a document whose $id differs from $id.
18+
*
19+
* Reproduces a production incident where a by-id read cached a sibling row's
20+
* body under the requested key, so every subsequent read served the wrong
21+
* document for ~90s until the cache entry expired (DAT-1904).
22+
*/
23+
class ForeignDocumentGuardTest extends TestCase
24+
{
25+
private DatabaseMemory $adapter;
26+
27+
private CacheMemory $cacheAdapter;
28+
29+
private Database $database;
30+
31+
protected function setUp(): void
32+
{
33+
$this->adapter = new DatabaseMemory();
34+
$this->cacheAdapter = new CacheMemory();
35+
$this->database = new Database($this->adapter, new Cache($this->cacheAdapter));
36+
$this->database
37+
->setDatabase('utopiaTests')
38+
->setNamespace('foreign_doc_' . \uniqid());
39+
40+
$this->database->create();
41+
$this->database->createCollection('projects');
42+
$this->database->createAttribute('projects', 'name', Database::VAR_STRING, 255, false);
43+
44+
foreach (['victim', 'sibling'] as $id) {
45+
$this->database->createDocument('projects', new Document([
46+
'$id' => $id,
47+
'$permissions' => [
48+
Permission::read(Role::any()),
49+
Permission::update(Role::any()),
50+
],
51+
'name' => $id,
52+
]));
53+
}
54+
}
55+
56+
/**
57+
* Overwrite the requested key's cached body with the sibling row's body,
58+
* exactly as a mismatched read would have persisted it under this key.
59+
*/
60+
private function poisonCache(string $requestedId, string $foreignId): void
61+
{
62+
$this->database->getDocument('projects', $requestedId);
63+
64+
[, $documentKey] = $this->database->getCacheKeys('projects', $requestedId);
65+
$foreign = $this->adapter->getDocument($this->database->getCollection('projects'), $foreignId);
66+
67+
$this->assertArrayHasKey($documentKey, $this->cacheAdapter->store, 'read should have populated the cache');
68+
$this->cacheAdapter->store[$documentKey]['data'] = $foreign->getArrayCopy();
69+
}
70+
71+
public function testGetDocumentPurgesPoisonedCacheEntryAndRefetches(): void
72+
{
73+
$this->poisonCache('victim', 'sibling');
74+
75+
$document = $this->database->getDocument('projects', 'victim');
76+
77+
// The poisoned entry must never be served: the correct row is refetched.
78+
$this->assertSame('victim', $document->getId());
79+
$this->assertSame('victim', $document->getAttribute('name'));
80+
81+
// ...and the poison is gone, so a later read stays correct without a write.
82+
[, $documentKey] = $this->database->getCacheKeys('projects', 'victim');
83+
$this->assertSame('victim', $this->cacheAdapter->store[$documentKey]['data']['$id']);
84+
$this->assertSame('victim', $this->database->getDocument('projects', 'victim')->getId());
85+
}
86+
87+
public function testGetDocumentThrowsWhenSourceReturnsForeignRow(): void
88+
{
89+
// A source read whose row carries the wrong $id (e.g. a result-set
90+
// delivered on the wrong pooled connection) is an identity violation:
91+
// it must fail loudly, never be cached, and never be served.
92+
$adapter = new class () extends DatabaseMemory {
93+
public function getDocument(Document $collection, string $id, array $queries = [], bool $forUpdate = false): Document
94+
{
95+
$document = parent::getDocument($collection, $id, $queries, $forUpdate);
96+
97+
if ($collection->getId() === 'projects' && $id === 'victim' && !$document->isEmpty()) {
98+
$document->setAttribute('$id', 'sibling');
99+
}
100+
101+
return $document;
102+
}
103+
};
104+
105+
$database = new Database($adapter, new Cache(new CacheMemory()));
106+
$database
107+
->setDatabase('utopiaTests')
108+
->setNamespace('foreign_src_' . \uniqid());
109+
$database->create();
110+
$database->createCollection('projects');
111+
$database->createAttribute('projects', 'name', Database::VAR_STRING, 255, false);
112+
$database->createDocument('projects', new Document([
113+
'$id' => 'victim',
114+
'$permissions' => [Permission::read(Role::any())],
115+
'name' => 'victim',
116+
]));
117+
118+
$this->expectException(DatabaseException::class);
119+
$this->expectExceptionMessage("mismatched \$id 'sibling'");
120+
121+
$database->getDocument('projects', 'victim');
122+
}
123+
124+
public function testGetDocumentServesMatchingCachedDocument(): void
125+
{
126+
// The guard must not disturb the normal cache hit path.
127+
$first = $this->database->getDocument('projects', 'victim');
128+
$second = $this->database->getDocument('projects', 'victim');
129+
130+
$this->assertSame('victim', $first->getId());
131+
$this->assertSame('victim', $second->getId());
132+
$this->assertSame('victim', $second->getAttribute('name'));
133+
}
134+
}

0 commit comments

Comments
 (0)