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')); + } +}