Skip to content

Require read permission for existing relationship references #971

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

Open
HarshMN2345 wants to merge 2 commits into
base: main
Choose a base branch
from
+79 −0
Open

Require read permission for existing relationship references #971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
432c238
Require read permission when linking existing relationship documents
HarshMN2345 Sep 14, 2026
f66f1a6
Assert persisted relationship state after successful writes
HarshMN2345 Sep 14, 2026
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
8 changes: 8 additions & 0 deletions src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6244,6 +6244,14 @@ private function relateDocumentsById(
$related = $this->skipRelationships(fn () => $this->getDocument($relatedCollection->getId(), $relationId));

if ($related->isEmpty() && $this->checkRelationshipsExist) {
$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.');
}
Comment on lines +6247 to +6252

@greptile-apps greptile-apps Bot Sep 14, 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 security 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.

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.

Fix in Claude Code Fix in Codex


// A nested write may reference its parent before it is inserted.
return;
}

Expand Down
71 changes: 71 additions & 0 deletions tests/e2e/Adapter/Scopes/RelationshipTests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1900,6 +1900,77 @@ public function testEnforceRelationshipPermissions(): void
$this->assertEquals(true, $bird3->isEmpty());
}

public function testCreateRelationshipReferenceRequiresRead(): void
{
$database = $this->getDatabase();
if (!$database->getAdapter()->getSupportForRelationships()) {
$this->expectNotToPerformAssertions();
return;
}

foreach ([
[Database::RELATION_ONE_TO_ONE, false],
[Database::RELATION_ONE_TO_ONE, true],
[Database::RELATION_ONE_TO_MANY, false],
[Database::RELATION_ONE_TO_MANY, true],
[Database::RELATION_MANY_TO_ONE, false],
[Database::RELATION_MANY_TO_ONE, true],
[Database::RELATION_MANY_TO_MANY, false],
[Database::RELATION_MANY_TO_MANY, true],
] as [$type, $twoWay]) {
$parents = 'referenceParents' . $type . (int) $twoWay;
$children = 'referenceChildren' . $type . (int) $twoWay;
$database->createCollection($parents, documentSecurity: true);
$database->createCollection($children, documentSecurity: true);
$database->createRelationship($parents, $children, $type, $twoWay, 'child', 'parent');
$database->createDocument($children, new Document([
'$id' => 'private',
'$permissions' => [Permission::update(Role::any())],
]));

$multiple = \in_array($type, [Database::RELATION_ONE_TO_MANY, Database::RELATION_MANY_TO_MANY], true);
$permissions = [Permission::read(Role::any()), Permission::update(Role::any())];
$parent = [
'$id' => 'reference',
'$permissions' => $permissions,
'child' => $multiple ? ['private'] : 'private',
];

try {
$database->createDocument($parents, new Document($parent));
$this->fail('An unreadable child must not be attached by ID.');
} catch (AuthorizationException $e) {
$this->assertSame('Missing read permission for the related document.', $e->getMessage());
}

$stored = $database->getAuthorization()->skip(
fn () => $database->skipRelationships(fn () => $database->getDocument($parents, 'reference'))
);
$this->assertTrue($stored->isEmpty());
Comment on lines +1946 to +1949

@greptile-apps greptile-apps Bot Sep 14, 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 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.

Suggested change
$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!

Fix in Claude Code Fix in Codex


$database->getAuthorization()->skip(fn () => $database->updateDocument($children, 'private', new Document([
'$permissions' => $permissions,
])));
$created = $database->createDocument($parents, new Document($parent));
$related = $database->getDocument($parents, $created->getId())->getAttribute('child');
$this->assertSame('private', ($multiple ? $related[0] : $related)->getId());

// Nested creation references a parent that has not been inserted yet.
$child = new Document(['$id' => 'nestedChild', '$permissions' => $permissions]);
$nested = $database->createDocument($parents, new Document([
'$id' => 'nestedParent',
'$permissions' => $permissions,
'child' => $multiple ? [$child] : $child,
]));
$related = $database->getDocument($parents, $nested->getId())->getAttribute('child');
$this->assertSame('nestedChild', ($multiple ? $related[0] : $related)->getId());
$this->assertFalse($database->getDocument($children, 'nestedChild')->isEmpty());

$database->deleteCollection($parents);
$database->deleteCollection($children);
}
}

public function testCreateRelationshipMissingCollection(): void
{
/** @var Database $database */
Expand Down
Loading