Conversation
Contributor
📝 WalkthroughWalkthroughRemoved legacy project key routes and logic from app/controllers/api/projects.php and introduced five new modular HTTP action handlers for project keys: Create, Get, Update, Delete, and XList under src/Appwrite/Platform/Modules/Project/Http/Project/Keys/. These handlers register equivalent endpoints (with new base path /v1/project/keys and aliases for /v1/projects/:projectId/keys*), include validation, authorization-aware database access, event queuing, cache purging, and response modeling. Project HTTP service was updated to register the new actions. Additionally, Key model gained an nullable expire property and accessor, one audit label in Variables Delete was adjusted, and three tests had a response-format header added. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR introduces a public API for managing project API keys, exposing five new endpoints (
Confidence Score: 4/5Safe to merge with caution — privilege escalation and secret-disclosure concerns from prior review rounds are still present in the code and should be resolved before the feature ships publicly. Two P1-level security issues flagged in previous review rounds remain unaddressed: an API key with
|
| Filename | Overview |
|---|---|
| src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Create.php | New endpoint to create API keys; allows AuthType::KEY callers to create keys with any scopes — privilege-escalation and secret-exposure concerns raised in prior review rounds remain unaddressed. |
| src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Get.php | New GET endpoint; returns the full secret field to all authenticated callers including API keys with only keys.read scope — secret-masking concern from prior review is still present. |
| src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Update.php | New PUT endpoint; scopes ?? [] silently clears all permissions when scopes is omitted, and the dead Duplicate catch block remains — both flagged in prior review rounds. |
| src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Delete.php | New DELETE endpoint; follows established patterns correctly with proper ownership check and cache purge. |
| src/Appwrite/Platform/Modules/Project/Http/Project/Keys/XList.php | New list endpoint; backward-compat default limit of 5000 matches the Platforms pattern; cursor pagination logic is correct. |
| src/Appwrite/Platform/Modules/Project/Services/Http.php | Registers all five new key actions correctly alongside existing platform and variable actions. |
| app/config/scopes/project.php | Adds keys.read, keys.write, platforms.read, and platforms.write scopes; descriptions are clear and consistent with existing entries. |
| src/Appwrite/Utopia/Request/Filters/V21.php | Existing fillKeyId handler in the V21 filter provides the required backward-compat for project.createKey; no new backward-compat cases needed since listKeys is a new endpoint. |
| tests/e2e/Services/Project/KeysBase.php | Comprehensive CRUD test coverage; no tests for privilege-escalation or secret-masking behaviour for API-key callers. |
| tests/e2e/Scopes/ProjectCustom.php | New keys.* and platforms.* scopes correctly added to the demo project key; duplicate sites.read/sites.write entries introduced in this PR. |
Reviews (4): Last reviewed commit: "Fix failing tests" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
RealtimeConsoleClientTest::testCreateDeployment |
1 | 2.11s | Logs |
UsageTest::testVectorsDBStats |
1 | 10.18s | Logs |
Commit 7371b68 - 2 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testVectorsDBStats |
1 | 10.45s | Logs |
WebhooksCustomServerTest::testUpdateCollection |
1 | 18.44s | Logs |
Commit c7a022b - 5 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testFunctionsStats |
1 | 10.27s | Logs |
UsageTest::testPrepareSitesStats |
1 | 7ms | Logs |
UsageTest::testEmbeddingsTextUsageDoesNotBreakProjectUsage |
1 | 6ms | Logs |
LegacyCustomClientTest::testCreateIndexes |
1 | 240.71s | Logs |
LegacyTransactionsCustomClientTest::testBulkOperations |
1 | 240.67s | Logs |
Commit a8c2491 - 25 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
DocumentsDBCustomClientTest::testCreateDatabase |
1 | 2.76s | Logs |
LegacyConsoleClientTest::testListDocumentsWithCache |
1 | 592ms | Logs |
LegacyCustomClientTest::testListAttributes |
1 | 243.08s | Logs |
LegacyCustomClientTest::testListIndexes |
1 | 40ms | Logs |
LegacyCustomClientTest::testCreateDocument |
1 | 26ms | Logs |
LegacyCustomClientTest::testUpsertDocument |
1 | 22ms | Logs |
LegacyCustomClientTest::testListDocuments |
1 | 19ms | Logs |
LegacyCustomClientTest::testListDocumentsWithCache |
1 | 23ms | Logs |
LegacyCustomClientTest::testListDocumentsCacheBustedByAttributeChange |
1 | 20ms | Logs |
LegacyCustomClientTest::testGetDocument |
1 | 30ms | Logs |
LegacyCustomClientTest::testGetDocumentWithQueries |
1 | 30ms | Logs |
LegacyCustomClientTest::testQueryBySequenceType |
1 | 26ms | Logs |
LegacyCustomClientTest::testListDocumentsAfterPagination |
1 | 32ms | Logs |
LegacyCustomClientTest::testListDocumentsBeforePagination |
1 | 28ms | Logs |
LegacyCustomClientTest::testListDocumentsLimitAndOffset |
1 | 24ms | Logs |
LegacyCustomClientTest::testDocumentsListQueries |
1 | 21ms | Logs |
LegacyCustomClientTest::testUpdateDocument |
1 | 18ms | Logs |
LegacyCustomClientTest::testDeleteDocument |
1 | 19ms | Logs |
LegacyCustomClientTest::testDefaultPermissions |
1 | 21ms | Logs |
LegacyCustomClientTest::testPersistentCreatedAt |
1 | 26ms | Logs |
LegacyCustomClientTest::testNotSearch |
1 | 45ms | Logs |
DatabasesStringTypesTest::testDeleteStringTypeAttributes |
1 | 30.39s | Logs |
UsageTest::testFunctionsStats |
1 | 10.24s | Logs |
UsageTest::testPrepareSitesStats |
1 | 7ms | Logs |
UsageTest::testEmbeddingsTextUsageDoesNotBreakProjectUsage |
1 | 5ms | Logs |
Commit f880b6e - 5 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
LegacyConsoleClientTest::testPatchAttribute |
1 | 128ms | Logs |
LegacyCustomServerTest::testNotStartsWith |
1 | 240.42s | Logs |
UsageTest::testFunctionsStats |
1 | 10.22s | Logs |
UsageTest::testPrepareSitesStats |
1 | 7ms | Logs |
UsageTest::testEmbeddingsTextUsageDoesNotBreakProjectUsage |
1 | 5ms | Logs |
Note: Flaky test results are tracked for the last 5 commits
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Create.php`:
- Around line 65-82: The param validator currently allows scopes to be null
(Nullable in the param definition) but action(string $keyId, string $name, array
$scopes, ?string $expire, ...) requires array, causing a TypeError for "scopes":
null; update the action signature to accept ?array $scopes (and update the
phpdoc `@param` to array<string>|null) and then normalize inside the action (e.g.,
$scopes = $scopes ?? []) before any use, ensuring callers that pass null get a
4xx validation response flow rather than a TypeError.
- Around line 91-105: When the caller is authenticated via an API key
(AuthType::KEY), enforce that the new key's scopes ($scopes) are a subset of the
caller key's scopes and that the requested expiration ($expire) is not later
than the caller key's expiration; implement this check before constructing the
new Document (where $key = new Document([...])) and either trim/reject scopes or
reject the request with an appropriate error, and similarly reject expirations
that exceed the caller's expire (or alternatively disallow AuthType::KEY
entirely on this endpoint). Also apply the same enforcement to the subsequent
key-creation code path referenced around the second creation block previously
noted.
In `@src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Get.php`:
- Around line 72-74: The endpoint currently returns full key objects including
their secret; update the logic in Get.php (the handler that eventually calls
$response->dynamic($key, Response::MODEL_KEY)) to detect if the caller's auth
type is AuthType::KEY and, if so, remove/scrub the secret from $key (e.g. unset
$key['secret'] or null out the secret property) before calling
$response->dynamic($key, Response::MODEL_KEY); alternatively you may reject
key-authenticated access by returning a 403 when auth type equals AuthType::KEY
— implement one of these two options and ensure the check happens immediately
before the $response->dynamic(...) call.
In `@src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Update.php`:
- Around line 63-77: The param definition for 'scopes' (the ->param('scopes',
null, new Nullable(new ArrayList(...))) call) allows null but the action method
signature public function action(..., array $scopes, ?string $expire, ...)
requires an array, causing a TypeError when null is passed; either remove the
Nullable wrapper so the validator disallows null (keep ->param('scopes', null,
new ArrayList(...))) or change the action signature to accept ?array $scopes and
add explicit handling inside action (e.g., treat null as [] or validate/throw)
to ensure consistent behavior between the validator and the action method.
- Around line 90-110: When the request is authenticated with an API key, ensure
the update cannot widen scopes or extend expiry beyond the caller key: before
calling $dbForPlatform->updateDocument('keys', ...) validate and clamp $scopes
and $expire against the caller key's allowed scopes/expiry (use the current auth
context via $authorization and the caller $key metadata) and reject if the
requested values exceed them; after updating, if the request AuthType is
AuthType::KEY, remove or redact the secret field from the returned $key (before
calling $response->dynamic($key, Response::MODEL_KEY)) so secrets are never
revealed to key-authenticated callers.
In `@src/Appwrite/Platform/Modules/Project/Http/Project/Keys/XList.php`:
- Around line 122-127: The list response currently returns full key documents
and leaks secrets to key-authenticated callers; before calling
response->dynamic(...) in XList (where $keys and $total are prepared and
MODEL_KEY_LIST is used), detect if the request auth type equals AuthType::KEY
and, if so, iterate $keys and remove or replace the 'secret' field (e.g., unset
or set to null/redacted) on each Document entry so key-authenticated callers
receive no raw secrets; ensure this redaction happens prior to constructing the
Document passed to response->dynamic.
🪄 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: ab8a547f-6b90-4ce6-ac84-042cd2a43442
📒 Files selected for processing (8)
app/controllers/api/projects.phpsrc/Appwrite/Platform/Modules/Project/Http/Project/Keys/Create.phpsrc/Appwrite/Platform/Modules/Project/Http/Project/Keys/Delete.phpsrc/Appwrite/Platform/Modules/Project/Http/Project/Keys/Get.phpsrc/Appwrite/Platform/Modules/Project/Http/Project/Keys/Update.phpsrc/Appwrite/Platform/Modules/Project/Http/Project/Keys/XList.phpsrc/Appwrite/Platform/Modules/Project/Http/Project/Variables/Delete.phpsrc/Appwrite/Platform/Modules/Project/Services/Http.php
💤 Files with no reviewable changes (1)
- app/controllers/api/projects.php
src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Create.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Get.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Update.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Project/Http/Project/Keys/XList.php
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
✨ Benchmark results
⚡ Benchmark Comparison
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Appwrite/Auth/Key.php (1)
183-192:⚠️ Potential issue | 🔴 CriticalWrap
DateTime::addSecondswithDateTime::formatTzto return an ISO 8601 string.Line 185 must format the datetime object to match the
?stringtype expected by the$expireparameter. The pattern throughout the codebase consistently usesDateTime::formatTz(DateTime::addSeconds(...))when storing expiration values. Change to:DateTime::formatTz(DateTime::addSeconds(new \DateTime(), 86400))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Auth/Key.php` around lines 183 - 192, The expiry value passed to the Key constructor uses DateTime::addSeconds(...) which returns a DateTime object but the $expire parameter expects a ?string; wrap the call with DateTime::formatTz so it returns an ISO8601 string. Locate the call using DateTime::addSeconds(new \DateTime(), 86400) in Key (around where the constructor is invoked) and replace it with DateTime::formatTz(DateTime::addSeconds(new \DateTime(), 86400)) so the stored expiration matches the codebase pattern and the ?string type.
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Update.php (1)
112-116: Consider defaultingscopesto empty array when null.Same concern as in
Create.php: if$scopesisnull, it may cause issues if other parts of the codebase expect an array when reading key scopes.$updates = new Document([ 'name' => $name, - 'scopes' => $scopes, + 'scopes' => $scopes ?? [], 'expire' => $expire, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Update.php` around lines 112 - 116, The document update is setting 'scopes' directly from $scopes which may be null; ensure $scopes defaults to an empty array when null before creating the Document (e.g., normalize $scopes = $scopes ?? []), so the 'scopes' field always contains an array in the Document instantiation in Update.php (the $scopes variable and the new Document([...]) call).src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Create.php (1)
110-122: Add null coalescing to defaultscopesto empty array when storing in Document.The
$scopesparameter is nullable (line 66, 83) and can benull. While line 97 already handles this with$scopes ?? [], line 117 stores the raw value directly. This creates inconsistency with how the rest of the codebase retrieves scopes—usinggetAttribute('scopes', [])with a default empty array—and with migration logic that normalizes empty scopes to arrays. Normalize at storage time:'scopes' => $scopes, +'scopes' => $scopes ?? [],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Create.php` around lines 110 - 122, The Document creation for $key stores 'scopes' using the nullable $scopes variable; change the 'scopes' entry in the new Document (the $key = new Document([...]) block) from 'scopes' => $scopes to 'scopes' => $scopes ?? [] so scopes are normalized to an empty array at storage time (refer to the $key Document creation and the $scopes parameter used earlier).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Appwrite/Auth/Key.php`:
- Around line 183-192: The expiry value passed to the Key constructor uses
DateTime::addSeconds(...) which returns a DateTime object but the $expire
parameter expects a ?string; wrap the call with DateTime::formatTz so it returns
an ISO8601 string. Locate the call using DateTime::addSeconds(new \DateTime(),
86400) in Key (around where the constructor is invoked) and replace it with
DateTime::formatTz(DateTime::addSeconds(new \DateTime(), 86400)) so the stored
expiration matches the codebase pattern and the ?string type.
---
Nitpick comments:
In `@src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Create.php`:
- Around line 110-122: The Document creation for $key stores 'scopes' using the
nullable $scopes variable; change the 'scopes' entry in the new Document (the
$key = new Document([...]) block) from 'scopes' => $scopes to 'scopes' =>
$scopes ?? [] so scopes are normalized to an empty array at storage time (refer
to the $key Document creation and the $scopes parameter used earlier).
In `@src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Update.php`:
- Around line 112-116: The document update is setting 'scopes' directly from
$scopes which may be null; ensure $scopes defaults to an empty array when null
before creating the Document (e.g., normalize $scopes = $scopes ?? []), so the
'scopes' field always contains an array in the Document instantiation in
Update.php (the $scopes variable and the new Document([...]) call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6b33ea9-028a-4299-8bde-1a0f63551205
📒 Files selected for processing (6)
src/Appwrite/Auth/Key.phpsrc/Appwrite/Platform/Modules/Project/Http/Project/Keys/Create.phpsrc/Appwrite/Platform/Modules/Project/Http/Project/Keys/Get.phpsrc/Appwrite/Platform/Modules/Project/Http/Project/Keys/Update.phpsrc/Appwrite/Platform/Modules/Project/Http/Project/Keys/XList.phptests/e2e/Services/Projects/ProjectsConsoleClientTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/Services/Projects/ProjectsConsoleClientTest.php
Meldiron
commented
Apr 7, 2026
src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Create.php
Outdated
Show resolved
Hide resolved
Meldiron
commented
Apr 7, 2026
Meldiron
commented
Apr 7, 2026
src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Get.php
Outdated
Show resolved
Hide resolved
Meldiron
commented
Apr 7, 2026
src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Update.php
Outdated
Show resolved
Hide resolved
Meldiron
commented
Apr 7, 2026
src/Appwrite/Platform/Modules/Project/Http/Project/Keys/Update.php
Outdated
Show resolved
Hide resolved
Meldiron
commented
Apr 7, 2026
src/Appwrite/Platform/Modules/Project/Http/Project/Keys/XList.php
Outdated
Show resolved
Hide resolved
eldadfux
approved these changes
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Public API for managing project API keys
Test Plan
Related PRs and Issues
x
Checklist