From 68514344aad17dc8a58b231f86b917eb99df0c18 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 17 Jul 2026 00:22:21 +1200 Subject: [PATCH] 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 --- src/Database/Database.php | 55 +++++++--- tests/unit/ForeignDocumentGuardTest.php | 134 ++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 12 deletions(-) create mode 100644 tests/unit/ForeignDocumentGuardTest.php diff --git a/src/Database/Database.php b/src/Database/Database.php index cc87483a87..c281a6beb4 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -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); + } 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 @@ -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()}'"); + } + if ($this->isTtlExpired($collection, $document)) { return $this->createDocumentInstance($collection->getId(), []); } @@ -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()) { diff --git a/tests/unit/ForeignDocumentGuardTest.php b/tests/unit/ForeignDocumentGuardTest.php new file mode 100644 index 0000000000..22600b44f9 --- /dev/null +++ b/tests/unit/ForeignDocumentGuardTest.php @@ -0,0 +1,134 @@ +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')); + } +}