Skip to content

fix(database): guard getDocument() against by-id cache/adapter identity mismatch #920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
abnegate wants to merge 1 commit into from
Closed

fix(database): guard getDocument() against by-id cache/adapter identity mismatch #920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4883,23 +4883,33 @@ public function getDocument(string $collection, string $id, array $queries = [],
// compare equal under strict equality (e.g. in updateDocument).
$document = $this->casting($collection, $document);

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

@greptile-apps greptile-apps Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Unhandled exception from purgeCachedDocument prevents refetch

If purgeCachedDocument throws (its $this->trigger(EVENT_DOCUMENT_PURGE, ...) call can throw, and the method carries no try-catch), the exception propagates to the caller before the code reaches the adapter refetch. The poisoned cache entry remains intact, and every subsequent call to getDocument for this id will repeat the cycle: detect poison → attempt purge → throw → never refetch. Wrapping the purge + fallthrough in a try-catch that logs the purge failure and proceeds to the adapter read would preserve the self-heal intent even when the purge layer is temporarily unavailable.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/Database/Database.php
Line: 4891-4893

Comment:
**Unhandled exception from `purgeCachedDocument` prevents refetch**

If `purgeCachedDocument` throws (its `$this->trigger(EVENT_DOCUMENT_PURGE, ...)` call can throw, and the method carries no try-catch), the exception propagates to the caller before the code reaches the adapter refetch. The poisoned cache entry remains intact, and every subsequent call to `getDocument` for this id will repeat the cycle: detect poison → attempt purge → throw → never refetch. Wrapping the purge + fallthrough in a try-catch that logs the purge failure and proceeds to the adapter read would preserve the self-heal intent even when the purge layer is temporarily unavailable.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

@greptile-apps greptile-apps Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 purgeCachedDocument fires EVENT_DOCUMENT_PURGE from a read path

purgeCachedDocument is the event-emitting variant; it fires EVENT_DOCUMENT_PURGE with the victim id. A read operation that self-heals a corrupt cache entry will emit an event indistinguishable from an explicit application-level purge, so any listener that sends webhooks, writes audit entries, or propagates invalidations to external systems will react to it. The protected purgeCachedDocumentInternal($collection->getId(), $id) purges the local cache layers without firing the event, which is safer for a transparent self-heal. If distributed-cache propagation is needed, a targeted silent invalidation path is preferable to reusing the full public purge event.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/Database/Database.php
Line: 4893

Comment:
**`purgeCachedDocument` fires `EVENT_DOCUMENT_PURGE` from a read path**

`purgeCachedDocument` is the event-emitting variant; it fires `EVENT_DOCUMENT_PURGE` with the victim id. A read operation that self-heals a corrupt cache entry will emit an event indistinguishable from an explicit application-level purge, so any listener that sends webhooks, writes audit entries, or propagates invalidations to external systems will react to it. The protected `purgeCachedDocumentInternal($collection->getId(), $id)` purges the local cache layers without firing the event, which is safer for a transparent self-heal. If distributed-cache propagation is needed, a targeted silent invalidation path is preferable to reusing the full public purge event.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

Comment on lines +4891 to +4893

@coderabbitai coderabbitai Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Do not let cache purge failures block the source fallback.

purgeCachedDocument() can throw, aborting the read instead of refetching the correct document. Log the failure and continue; the foreign cached value must still never be served.

Proposed fix 
 if ($this->isForeignDocument($document, $id)) {
     Console::error("Cache for '{$collection->getId()}:{$id}' held foreign document '{$document->getId()}'; purging and refetching");
-    $this->purgeCachedDocument($collection->getId(), $id);
+    try {
+        $this->purgeCachedDocument($collection->getId(), $id);
+    } catch (\Throwable $e) {
+        Console::warning('Failed to purge foreign cached document: ' . $e->getMessage());
+    }
 } else {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if ($this->isForeignDocument($document, $id)) {
Console::error("Cache for '{$collection->getId()}:{$id}' held foreign document '{$document->getId()}'; purging and refetching");
$this->purgeCachedDocument($collection->getId(), $id);
if ($this->isForeignDocument($document, $id)) {
Console::error("Cache for '{$collection->getId()}:{$id}' held foreign document '{$document->getId()}'; purging and refetching");
try {
$this->purgeCachedDocument($collection->getId(), $id);
} catch (\Throwable $e) {
Console::warning('Failed to purge foreign cached document: ' . $e->getMessage());
}
} else {
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Database/Database.php` around lines 4891 - 4893, Wrap the
purgeCachedDocument call in the foreign-document branch of the read flow with
failure handling so purge exceptions are logged and do not prevent refetching
from the source. Preserve the existing behavior of rejecting the foreign cached
value and continuing to the fallback fetch, regardless of purge success.

} else {
if ($collection->getId() !== self::METADATA) {

if (!$this->authorization->isValid(new Input(self::PERMISSION_READ, [
...$collection->getRead(),
...($documentSecurity ? $document->getRead() : [])
]))) {
return $this->createDocumentInstance($collection->getId(), []);
if (!$this->authorization->isValid(new Input(self::PERMISSION_READ, [
...$collection->getRead(),
...($documentSecurity ? $document->getRead() : [])
]))) {
return $this->createDocumentInstance($collection->getId(), []);
}
}
}

$this->trigger(self::EVENT_DOCUMENT_READ, $document);
$this->trigger(self::EVENT_DOCUMENT_READ, $document);

if ($this->isTtlExpired($collection, $document)) {
return $this->createDocumentInstance($collection->getId(), []);
}
if ($this->isTtlExpired($collection, $document)) {
return $this->createDocumentInstance($collection->getId(), []);
}

return $document;
return $document;
}
}

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

// A row whose $id differs from the requested id is a source-level identity
// violation (e.g. a result-set delivered on the wrong pooled connection),
// not a cache artifact. Never cache or return it: caching would poison this
// key, and returning it would serve one document's body under another's id.
if ($this->isForeignDocument($document, $id)) {
throw new DatabaseException("getDocument('{$collection->getId()}', '{$id}') returned document with mismatched \$id '{$document->getId()}'");
Comment on lines +4941 to +4942

@greptile-apps greptile-apps Bot Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 security Foreign document $id is included in exception message

The DatabaseException message includes $document->getId() — the foreign row's identifier. If this exception bubbles through Appwrite's error handling and the raw message reaches an API response or is written to a log that another tenant can read, the foreign $id is exposed cross-tenant. Replacing $document->getId() with a placeholder (e.g. [redacted]) in the exception message, and reserving the full detail for a server-side Console::error call (where you already log the collection id and both ids on the cache path), would eliminate the exposure while preserving debuggability.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/Database/Database.php
Line: 4941-4942

Comment:
**Foreign document `$id` is included in exception message**

The `DatabaseException` message includes `$document->getId()` — the foreign row's identifier. If this exception bubbles through Appwrite's error handling and the raw message reaches an API response or is written to a log that another tenant can read, the foreign `$id` is exposed cross-tenant. Replacing `$document->getId()` with a placeholder (e.g. `[redacted]`) in the exception message, and reserving the full detail for a server-side `Console::error` call (where you already log the collection id and both ids on the cache path), would eliminate the exposure while preserving debuggability.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

}

if ($this->isTtlExpired($collection, $document)) {
return $this->createDocumentInstance($collection->getId(), []);
}
Expand Down Expand Up @@ -4979,6 +4997,19 @@ public function getDocument(string $collection, string $id, array $queries = [],
return $document;
}

/**
* Whether a document read for $id came back carrying a different, non-empty
* $id — i.e. another row's body served under this key. 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.
*/
private function isForeignDocument(Document $document, string $id): bool
{
$actual = $document->getId();

return $actual !== '' && $actual !== $id;
}

private function isTtlExpired(Document $collection, Document $document): bool
{
if (!$this->adapter->getSupportForTTLIndexes()) {
Expand Down
134 changes: 134 additions & 0 deletions tests/unit/ForeignDocumentGuardTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
<?php

namespace Tests\Unit;

use PHPUnit\Framework\TestCase;
use Utopia\Cache\Adapter\Memory as CacheMemory;
use Utopia\Cache\Cache;
use Utopia\Database\Adapter\Memory as DatabaseMemory;
use Utopia\Database\Database;
use Utopia\Database\Document;
use Utopia\Database\Exception as DatabaseException;
use Utopia\Database\Helpers\Permission;
use Utopia\Database\Helpers\Role;

/**
* Regression guard for the by-id read identity invariant: getDocument($id) must
* never return, or keep serving, a document whose $id differs from $id.
*
* Reproduces a production incident where a by-id read cached a sibling row's
* body under the requested key, so every subsequent read served the wrong
* document for ~90s until the cache entry expired (DAT-1904).
*/
class ForeignDocumentGuardTest extends TestCase
{
private DatabaseMemory $adapter;

private CacheMemory $cacheAdapter;

private Database $database;

protected function setUp(): void
{
$this->adapter = new DatabaseMemory();
$this->cacheAdapter = new CacheMemory();
$this->database = new Database($this->adapter, new Cache($this->cacheAdapter));
$this->database
->setDatabase('utopiaTests')
->setNamespace('foreign_doc_' . \uniqid());

$this->database->create();
$this->database->createCollection('projects');
$this->database->createAttribute('projects', 'name', Database::VAR_STRING, 255, false);

foreach (['victim', 'sibling'] as $id) {
$this->database->createDocument('projects', new Document([
'$id' => $id,
'$permissions' => [
Permission::read(Role::any()),
Permission::update(Role::any()),
],
'name' => $id,
]));
}
}

/**
* Overwrite the requested key's cached body with the sibling row's body,
* exactly as a mismatched read would have persisted it under this key.
*/
private function poisonCache(string $requestedId, string $foreignId): void
{
$this->database->getDocument('projects', $requestedId);

[, $documentKey] = $this->database->getCacheKeys('projects', $requestedId);
$foreign = $this->adapter->getDocument($this->database->getCollection('projects'), $foreignId);

$this->assertArrayHasKey($documentKey, $this->cacheAdapter->store, 'read should have populated the cache');
$this->cacheAdapter->store[$documentKey]['data'] = $foreign->getArrayCopy();
}

public function testGetDocumentPurgesPoisonedCacheEntryAndRefetches(): void
{
$this->poisonCache('victim', 'sibling');

$document = $this->database->getDocument('projects', 'victim');

// The poisoned entry must never be served: the correct row is refetched.
$this->assertSame('victim', $document->getId());
$this->assertSame('victim', $document->getAttribute('name'));

// ...and the poison is gone, so a later read stays correct without a write.
[, $documentKey] = $this->database->getCacheKeys('projects', 'victim');
$this->assertSame('victim', $this->cacheAdapter->store[$documentKey]['data']['$id']);
$this->assertSame('victim', $this->database->getDocument('projects', 'victim')->getId());
}

public function testGetDocumentThrowsWhenSourceReturnsForeignRow(): void
{
// A source read whose row carries the wrong $id (e.g. a result-set
// delivered on the wrong pooled connection) is an identity violation:
// it must fail loudly, never be cached, and never be served.
$adapter = new class () extends DatabaseMemory {
public function getDocument(Document $collection, string $id, array $queries = [], bool $forUpdate = false): Document
{
$document = parent::getDocument($collection, $id, $queries, $forUpdate);

if ($collection->getId() === 'projects' && $id === 'victim' && !$document->isEmpty()) {
$document->setAttribute('$id', 'sibling');
}

return $document;
}
};

$database = new Database($adapter, new Cache(new CacheMemory()));
$database
->setDatabase('utopiaTests')
->setNamespace('foreign_src_' . \uniqid());
$database->create();
$database->createCollection('projects');
$database->createAttribute('projects', 'name', Database::VAR_STRING, 255, false);
$database->createDocument('projects', new Document([
'$id' => 'victim',
'$permissions' => [Permission::read(Role::any())],
'name' => 'victim',
]));

$this->expectException(DatabaseException::class);
$this->expectExceptionMessage("mismatched \$id 'sibling'");

$database->getDocument('projects', 'victim');
}

public function testGetDocumentServesMatchingCachedDocument(): void
{
// The guard must not disturb the normal cache hit path.
$first = $this->database->getDocument('projects', 'victim');
$second = $this->database->getDocument('projects', 'victim');

$this->assertSame('victim', $first->getId());
$this->assertSame('victim', $second->getId());
$this->assertSame('victim', $second->getAttribute('name'));
}
}
Loading