fix(database): guard getDocument() against by-id cache/adapter identity mismatch#920
fix(database): guard getDocument() against by-id cache/adapter identity mismatch#920abnegate wants to merge 1 commit into
Conversation
…ning 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>
📝 WalkthroughWalkthrough
ChangesForeign document identity guards
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds an identity guard to
Confidence Score: 3/5The guard correctly blocks the production incident scenario, but if The adapter-level foreign-row path is clean and safe. The cache-healing path has a gap: src/Database/Database.php — specifically the
|
| Filename | Overview |
|---|---|
| src/Database/Database.php | Adds isForeignDocument guard to getDocument() — cache-hit path purges and refetches on mismatch; adapter path throws. If purgeCachedDocument itself throws, the self-heal loop stalls and the poison persists. |
| tests/unit/ForeignDocumentGuardTest.php | New test file covering cache-poison purge+refetch, adapter-foreign throw, and normal cache-hit correctness. |
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/Database/Database.php:4891-4893
**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.
### Issue 2 of 3
src/Database/Database.php:4893
**`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.
### Issue 3 of 3
src/Database/Database.php:4941-4942
**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.
Reviews (1): Last reviewed commit: "fix(database): guard by-id reads against..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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); |
There was a problem hiding this comment.
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.
| // 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); |
There was a problem hiding this 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.
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.| if ($this->isForeignDocument($document, $id)) { | ||
| throw new DatabaseException("getDocument('{$collection->getId()}', '{$id}') returned document with mismatched \$id '{$document->getId()}'"); |
There was a problem hiding this comment.
$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.
|
Closing — after review this guard is defense-in-depth on the The wrong-document read originates upstream: a query for document A returned document B's row (a pooled-connection / result-set mixup, PDOProxy-class). That same mixup can corrupt The sensitive path (dedicated-DB by-id reads that can expose a foreign hostname/credentials) is already guarded in a targeted way in the consumer (appwrite cloud Store layer). Pursuing the connection-layer root cause instead of a hot-path assertion in core utopia. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/ForeignDocumentGuardTest.php (1)
124-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the partial-selection regression test.
The third test covers a normal cache hit, but no test exercises a returned document with
$idomitted. Add an adapter-backed projection test that strips$idand confirms the result is accepted.🤖 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 `@tests/unit/ForeignDocumentGuardTest.php` around lines 124 - 133, Add a regression test alongside testGetDocumentServesMatchingCachedDocument that uses the database adapter’s projection/partial-selection support to retrieve the same document without the $id field. Assert the returned document is accepted by the guard and verify its expected attributes, covering the omitted-identifier cache path.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/Database/Database.php`:
- Around line 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.
---
Nitpick comments:
In `@tests/unit/ForeignDocumentGuardTest.php`:
- Around line 124-133: Add a regression test alongside
testGetDocumentServesMatchingCachedDocument that uses the database adapter’s
projection/partial-selection support to retrieve the same document without the
$id field. Assert the returned document is accepted by the guard and verify its
expected attributes, covering the omitted-identifier cache path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b762502-c41f-4fca-8660-8004f16ada17
📒 Files selected for processing (2)
src/Database/Database.phptests/unit/ForeignDocumentGuardTest.php
| 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); |
There was a problem hiding this comment.
🩺 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.
| 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.
Found on Appwrite Cloud prod:
GET /v1/mysql/{id}served a different database's document (wrong$id, hostname, port) for ~90s while a sibling row was being written concurrently, then self-healed. The list endpoint stayed correct the whole time — proving the underlying row was never wrong, only the by-id cache entry.Root cause
Database::getDocument()is cache-aside and saves the adapter's returned body under a key derived from the requested id, with no assertion thatbody.$id === requestedId(read atDatabase.php~:4916, cached viasaveWithLease~:4969). If the adapter ever returns a foreign row (observed under a concurrent-write / pooled-connection result mixup), that body is cached under the wrong key and served sticky until the entry is invalidated.Fix
New
getDocument()identity guard viaisForeignDocument($document, $id)($actual !== '' && $actual !== $id):purgeCachedDocument+ refetch (self-heal).DatabaseException(fail-closed) — never cache or return it.select()that omits$idis not treated as foreign (no false positives on projections).Why fail-closed (throw) rather than degrade
Cache keys are namespaced per project, so a poisoned cache entry is project-bounded. But the connection-level mixup that can seed it is not inherently bounded in a shared multi-tenant shard-pool deployment — a result mixup could copy another tenant's row into the requester's keyspace. Serving another tenant's hostname/credentials must never happen, so a foreign adapter row that survives a refetch raises loudly instead of being returned.
Tests
tests/unit/ForeignDocumentGuardTest.php(3 tests; 2 fail without the guard) — covers the cached-foreign self-heal, the adapter-foreign throw, and the partial-select()no-false-positive case. Pint + PHPStan (L7) clean; neighbouring cache/document suites green.Consumer
Appwrite Cloud PR appwrite-labs/cloud#4827 adds a
Store-leveldatabaseId === requestedIdback-reference guard on top of this, and pins this branch via a dev VCS ref pending a tagged release.🤖 Generated with Claude Code
Summary by CodeRabbit