feat: per bucket image transformations flag #10722
Conversation
|
Warning Rate limit exceeded@ChiragAgg5k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (86)
📒 Files selected for processing (4)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new boolean bucket-level attribute Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a per-bucket flag to enable or disable image transformations. When transformations are disabled for a bucket, requests for image previews return a 403 error, except for console and API key requests which bypass this restriction.
- Adds a
transformationsboolean attribute to bucket configuration (defaults to true) - Enforces transformation restrictions in the file preview/transformation endpoints
- Includes migration logic to add the new attribute to existing buckets
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/Services/Storage/StorageCustomClientTest.php | Adds test verifying transformations can be disabled and preview requests return 403 |
| tests/e2e/Services/Storage/StorageConsoleClientTest.php | Adds test verifying console requests bypass transformation restrictions |
| src/Appwrite/Utopia/Response/Model/Bucket.php | Adds transformations field to bucket response model |
| src/Appwrite/Utopia/Database/Validator/Queries/Buckets.php | Adds transformations to queryable bucket attributes |
| src/Appwrite/Migration/Version/V23.php | Adds migration to create transformations attribute on existing buckets |
| src/Appwrite/Extend/Exception.php | Defines new exception constant for disabled transformations |
| app/controllers/shared/api.php | Enforces transformation restriction check in shared controller |
| app/controllers/api/storage.php | Adds transformation parameter to bucket create/update and enforces restriction in preview endpoint |
| app/config/errors.php | Adds error configuration for transformation disabled exception |
| app/config/collections/common.php | Adds transformations boolean attribute definition to buckets collection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/e2e/Services/Storage/StorageCustomClientTest.php (1)
1439-1441: Prefer asserting error type over full message text.Matching the full error message is brittle. Assert the error type (storage_bucket_transformations_disabled) and 403 instead.
Example:
- $this->assertEquals(403, $preview['headers']['status-code']); - $this->assertStringContainsString('Image transformations are disabled for the requested bucket.', $preview['body']['message']); + $this->assertEquals(403, $preview['headers']['status-code']); + $this->assertEquals('storage_bucket_transformations_disabled', $preview['body']['type']);tests/e2e/Services/Storage/StorageConsoleClientTest.php (1)
149-155: Comment tweak (optional).The 200 after disabling is because privileged/console requests bypass the transformations gate, not because “image transformations are not counted.” Consider rewording for clarity.
app/controllers/api/storage.php (2)
302-303: Update flow: maketransformationsnullable to preserve existing when omitted.You coalesce
$transformationsto the current bucket value, but the parameter is typedbooland defaults totrue, so omission won’t be distinguishable and will reset to true.Apply:
- ->param('transformations', true, new Boolean(true), 'Are image transformations enabled?', true) + ->param('transformations', null, new Boolean(true), 'Are image transformations enabled?', true) ... - ->action(function (string $bucketId, string $name, ?array $permissions, bool $fileSecurity, bool $enabled, ?int $maximumFileSize, array $allowedFileExtensions, ?string $compression, ?bool $encryption, bool $antivirus, bool $transformations, Response $response, Database $dbForProject, Event $queueForEvents) { + ->action(function (string $bucketId, string $name, ?array $permissions, bool $fileSecurity, bool $enabled, ?int $maximumFileSize, array $allowedFileExtensions, ?string $compression, ?bool $encryption, bool $antivirus, ?bool $transformations, Response $response, Database $dbForProject, Event $queueForEvents) { ... - $transformations ??= $bucket->getAttribute('transformations', true); + $transformations ??= $bucket->getAttribute('transformations', true);If other update params use “replace” semantics intentionally, keep consistency and ignore; otherwise consider the nullable pattern above.
Also applies to: 306-321, 334-335
982-987: Name consistency:$isAPIKeyvs$isAppUser.Other handlers use
$isAPIKey = Auth::isAppUser(...). Use one name across the file for readability.- $isAppUser = Auth::isAppUser(Authorization::getRoles()); + $isAPIKey = Auth::isAppUser(Authorization::getRoles()); ... - if ($bucket->isEmpty() || (!$bucket->getAttribute('enabled') && !$isAppUser && !$isPrivilegedUser)) { + if ($bucket->isEmpty() || (!$bucket->getAttribute('enabled') && !$isAPIKey && !$isPrivilegedUser)) { ... - if (!$bucket->getAttribute('transformations', true) && !$isAppUser && !$isPrivilegedUser) { + if (!$bucket->getAttribute('transformations', true) && !$isAPIKey && !$isPrivilegedUser) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/config/collections/common.php(1 hunks)app/config/errors.php(1 hunks)app/controllers/api/storage.php(6 hunks)app/controllers/shared/api.php(1 hunks)src/Appwrite/Extend/Exception.php(1 hunks)src/Appwrite/Migration/Version/V23.php(1 hunks)src/Appwrite/Utopia/Database/Validator/Queries/Buckets.php(1 hunks)src/Appwrite/Utopia/Response/Model/Bucket.php(1 hunks)tests/e2e/Services/Storage/StorageConsoleClientTest.php(2 hunks)tests/e2e/Services/Storage/StorageCustomClientTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/Appwrite/Migration/Version/V23.php (1)
src/Appwrite/Migration/Migration.php (1)
createAttributeFromCollection(323-371)
app/controllers/shared/api.php (2)
src/Appwrite/Auth/Auth.php (2)
isAppUser(441-448)isPrivilegedUser(421-432)src/Appwrite/Extend/Exception.php (1)
Exception(7-465)
app/config/errors.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-465)
src/Appwrite/Utopia/Response/Model/Bucket.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
tests/e2e/Services/Storage/StorageCustomClientTest.php (1)
tests/e2e/Client.php (1)
Client(8-342)
tests/e2e/Services/Storage/StorageConsoleClientTest.php (2)
tests/e2e/Client.php (1)
Client(8-342)tests/e2e/Scopes/Scope.php (1)
getHeaders(145-145)
app/controllers/api/storage.php (3)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-356)src/Appwrite/Auth/Auth.php (4)
isAppUser(441-448)Auth(18-515)getRoles(456-502)isPrivilegedUser(421-432)src/Appwrite/Extend/Exception.php (1)
Exception(7-465)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Linter
- GitHub Check: Check if utopia-php/database changed
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (9)
app/controllers/shared/api.php (1)
597-599: LGTM! Authorization check is correctly implemented.The transformations check properly:
- Uses a safe default of
truewhen the attribute doesn't exist- Validates user privileges (App users and privileged users can bypass)
- Follows the same pattern as the
enabledattribute check abovesrc/Appwrite/Extend/Exception.php (1)
153-153: LGTM! Exception constant follows naming conventions.The new constant is correctly placed in the Storage section and follows the established naming pattern.
src/Appwrite/Utopia/Database/Validator/Queries/Buckets.php (1)
13-14: LGTM! Validator correctly extended.The
transformationsattribute is properly added to the allowed attributes list for bucket queries.app/config/errors.php (1)
525-529: LGTM! Error mapping is properly configured.The error entry provides a clear description and uses an appropriate HTTP 403 status code for this authorization scenario.
src/Appwrite/Utopia/Response/Model/Bucket.php (1)
89-94: LGTM! Response model correctly extended.The
transformationsrule is properly configured with appropriate type, default value, and description. The default oftrueis consistent with the collection schema.app/config/collections/common.php (1)
1530-1540: LGTM! Collection schema properly extended.The
transformationsattribute is correctly defined with:
- Appropriate default value of
truefor backward compatibilityrequired: falseto avoid breaking existing buckets- Consistent structure with other boolean attributes
app/controllers/api/storage.php (3)
88-93: Create flow: new bucket.transformations wired correctly.Param, action signature, and persistence look good. No functional concerns.
Also applies to: 145-147
989-991: Preview gate aligns with requirements.403 for non-app/non-privileged when bucket.transformations=false is correctly enforced before file IO.
302-303: All public schema/model/migration requirements fortransformationsare properly implemented.
- Error constant
STORAGE_BUCKET_TRANSFORMATIONS_DISABLEDis defined and mapped in error config- Response::MODEL_BUCKET exposes
transformationsas a Boolean field with default true- Validator includes
transformationsin the Buckets query attributes- Migration V23 creates the
transformationsattribute viacreateAttributeFromCollection- Endpoint parameter correctly uses Boolean validator with consistent defaults
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/api/storage.php (1)
984-984: Consider consistent variable naming across endpoints.The variable is renamed from
$isAPIKeyto$isAppUser, which is more semantically accurate. However, other file endpoints (lines 439, 801, 902, 1180, 1341, 1511, 1668, 1782) still use$isAPIKeyfor the same purpose, creating inconsistency.Consider either:
- Reverting to
$isAPIKeyhere for consistency, or- Renaming
$isAPIKeyto$isAppUseracross all endpoints in a separate refactoring effortApply this diff to maintain consistency with existing code:
- $isAppUser = Auth::isAppUser(Authorization::getRoles()); + $isAPIKey = Auth::isAppUser(Authorization::getRoles()); $isPrivilegedUser = Auth::isPrivilegedUser(Authorization::getRoles()); - if ($bucket->isEmpty() || (!$bucket->getAttribute('enabled') && !$isAppUser && !$isPrivilegedUser)) { + if ($bucket->isEmpty() || (!$bucket->getAttribute('enabled') && !$isAPIKey && !$isPrivilegedUser)) { throw new Exception(Exception::STORAGE_BUCKET_NOT_FOUND); } - if (!$bucket->getAttribute('transformations', true) && !$isAppUser && !$isPrivilegedUser) { + if (!$bucket->getAttribute('transformations', true) && !$isAPIKey && !$isPrivilegedUser) { throw new Exception(Exception::STORAGE_BUCKET_TRANSFORMATIONS_DISABLED); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/storage.php(6 hunks)app/controllers/shared/api.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/shared/api.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-17T08:07:54.565Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9661
File: app/controllers/api/migrations.php:332-337
Timestamp: 2025-04-17T08:07:54.565Z
Learning: In the Appwrite codebase, error handling intentionally uses the same error codes for different failure conditions (like STORAGE_BUCKET_NOT_FOUND for both non-existent buckets and authorization failures) as a security measure to prevent information disclosure about what specifically went wrong.
Applied to files:
app/controllers/api/storage.php
🧬 Code graph analysis (1)
app/controllers/api/storage.php (2)
src/Appwrite/Auth/Auth.php (2)
isAppUser(441-448)Auth(18-515)src/Appwrite/Extend/Exception.php (1)
Exception(7-465)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (3)
app/controllers/api/storage.php (3)
88-88: LGTM! Bucket creation properly extended with transformations flag.The new
transformationsparameter follows the same pattern as other bucket configuration flags (antivirus, encryption, compression). Default value oftrueensures backward compatibility.Also applies to: 92-92, 145-145
303-303: LGTM! Bucket update correctly implements transformations flag.The update logic properly handles the null coalescing pattern to preserve existing values when not provided, consistent with other bucket attributes.
Also applies to: 307-307, 321-321, 335-336
991-993: LGTM! Transformation enforcement logic is correct.The authorization check properly denies transformations when disabled for non-app/non-privileged users while allowing app users and privileged users to bypass the restriction. The placement in the preview endpoint is correct since download and view operations don't perform transformations.
There was a problem hiding this comment.
Copilot reviewed 96 out of 96 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
commit: |
No description provided.