Require read permission for existing relationship references - #971
HarshMN2345 wants to merge 2 commits into
Conversation
|
Warning Review limit reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $exists = $this->authorization->skip( | ||
| fn () => $this->skipRelationships(fn () => $this->getDocument($relatedCollection->getId(), $relationId)) | ||
| ); | ||
| if (!$exists->isEmpty()) { | ||
| throw new AuthorizationException('Missing read permission for the related document.'); | ||
| } |
There was a problem hiding this comment.
A caller allowed to create or update a parent can supply arbitrary relationship IDs. An existing but unreadable ID now produces a distinct authorization error, while a nonexistent ID follows the early return and lets the write continue. This defeats getDocument()'s existing behavior of treating missing and unreadable records alike, allowing callers to enumerate hidden document IDs. The missing-reference exception should be limited to a trusted nested-write state, or both externally supplied cases must remain indistinguishable.
How this was verified: Relationship IDs from parent writes reach this lookup, and the permission-skipped result produces an exception only when the unreadable target exists.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Database/Database.php
Line: 6247-6252
Comment:
**Relationship IDs Leak Existence**
A caller allowed to create or update a parent can supply arbitrary relationship IDs. An existing but unreadable ID now produces a distinct authorization error, while a nonexistent ID follows the early return and lets the write continue. This defeats `getDocument()`'s existing behavior of treating missing and unreadable records alike, allowing callers to enumerate hidden document IDs. The missing-reference exception should be limited to a trusted nested-write state, or both externally supplied cases must remain indistinguishable.
**How this was verified:** Relationship IDs from parent writes reach this lookup, and the permission-skipped result produces an exception only when the unreadable target exists.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| $stored = $database->getAuthorization()->skip( | ||
| fn () => $database->skipRelationships(fn () => $database->getDocument($parents, 'reference')) | ||
| ); | ||
| $this->assertTrue($stored->isEmpty()); |
There was a problem hiding this comment.
This test reproduces the production implementation's exact authorization->skip(skipRelationships(getDocument(...))) helper composition merely to check whether the parent persisted. That violates the repository directive to test observable behavior rather than mirror implementation details. Because the parent already grants read(Role::any()), a normal getDocument($parents, 'reference') can verify the same rollback behavior without coupling the regression to authorization and relationship-resolution internals. This repository requirement must be satisfied before merging.
| $stored = $database->getAuthorization()->skip( | |
| fn () => $database->skipRelationships(fn () => $database->getDocument($parents, 'reference')) | |
| ); | |
| $this->assertTrue($stored->isEmpty()); | |
| $stored = $database->getDocument($parents, 'reference'); | |
| $this->assertTrue($stored->isEmpty()); |
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/Adapter/Scopes/RelationshipTests.php
Line: 1946-1949
Comment:
**Test Copies Internal Bypasses**
This test reproduces the production implementation's exact `authorization->skip(skipRelationships(getDocument(...)))` helper composition merely to check whether the parent persisted. That violates the repository directive to test observable behavior rather than mirror implementation details. Because the parent already grants `read(Role::any())`, a normal `getDocument($parents, 'reference')` can verify the same rollback behavior without coupling the regression to authorization and relationship-resolution internals. This repository requirement must be satisfied before merging.
```suggestion
$stored = $database->getDocument($parents, 'reference');
$this->assertTrue($stored->isEmpty());
```
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Creating a document with a relationship ID can persist a reference to an existing document the caller cannot read. The permission-filtered lookup returns an empty document, and the early return leaves the supplied foreign key in the new parent.
Distinguish an unreadable existing target from a missing target and reject the former with an authorization error. Preserve missing references needed when nested writes link back to a parent that has not been inserted yet.
The shared adapter regression covers all four relationship types in one-way and two-way configurations, verifies that rejected writes leave no parent, then checks a readable reference and nested creation. It fails against the current implementation and passes locally with SQLite, shared-table SQLite, and Mirror-backed SQLite (three tests, 120 assertions). Changed-file syntax and Pint checks pass; PHPStan level 7 reports no new diagnostics compared with the unchanged base. GitHub CI passes on the current head: all 16 adapter suites, unit tests, Pint, and CodeQL. The PostgreSQL and Mirror job logs confirm the regression executed.
Addresses appwrite/appwrite#7263.
Appwrite must adopt a released package version to receive this change.