Conversation
This prevents requests with `Range: 0-<file length>` and limits it to APP_STORAGE_READ_BUFFER
fix(storage): do not allow full range
fix: task coroutine hooks
added encrypt property in the attribute string response model
added checking for encrypt and plan allowing encryption of string att…
Feat sync encrypt updates
feat: add builds worker group
Revert "Feat sync encrypt updates"
# Conflicts: # composer.lock # src/Appwrite/Platform/Workers/Builds.php
WalkthroughThe changes update coroutine hook settings to enable all hooks, standardize storage buffer size constants, and improve HTTP header handling order in storage endpoints. Additionally, several scheduling task methods and closures are refactored to remove unnecessary Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StorageController
participant Response
User->>StorageController: Request file (download/view/push)
StorageController->>StorageController: Handle range requests, set buffer size using APP_STORAGE_READ_BUFFER
StorageController->>StorageController: Set HTTP headers after range validation
StorageController->>Response: Send file content with correct headers
sequenceDiagram
participant Scheduler
participant ScheduleBase
participant ScheduleFunctions
participant ScheduleMessages
Scheduler->>ScheduleBase: action()
ScheduleBase->>ScheduleBase: collectSchedules(dbForPlatform, getProjectDB, lastSyncUpdate)
ScheduleBase->>ScheduleFunctions: enqueueResources(schedules, dbForPlatform)
ScheduleBase->>ScheduleMessages: enqueueResources(schedules, dbForPlatform)
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/cli.php(1 hunks)app/controllers/api/storage.php(7 hunks)src/Appwrite/Platform/Tasks/ScheduleBase.php(3 hunks)src/Appwrite/Platform/Tasks/ScheduleFunctions.php(1 hunks)src/Appwrite/Platform/Tasks/ScheduleMessages.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Appwrite/Platform/Tasks/ScheduleBase.php (2)
app/realtime.php (1)
getProjectDB(76-122)src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-92)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (12)
app/cli.php (1)
287-287: LGTM: TCP hooks enabled for coroutines.The change enables TCP hooks that were previously excluded from coroutine execution. This aligns with the broader refactoring in the scheduling system to improve coroutine and resource pool handling.
src/Appwrite/Platform/Tasks/ScheduleFunctions.php (1)
79-79: LGTM: Removed unused $pools parameter.The
$poolsvariable was not used within the closure, so removing it from theuseclause is a good cleanup that reduces unnecessary variable capture.src/Appwrite/Platform/Tasks/ScheduleMessages.php (1)
43-43: LGTM: Removed unused $pools parameter.The
$poolsvariable was not used within the closure, so removing it from theuseclause is appropriate cleanup that eliminates unnecessary variable capture.src/Appwrite/Platform/Tasks/ScheduleBase.php (3)
82-82: LGTM: Updated method calls to match new signature.The calls to
collectScheduleshave been properly updated to remove the$poolsparameter, maintaining consistency with the updated method signature.Also applies to: 88-88, 91-91
106-106: LGTM: Removed unused parameter from method signature.The
Group $poolsparameter was not used within thecollectSchedulesmethod implementation, so removing it simplifies the method signature and reduces unnecessary dependencies.
118-118: LGTM: Removed unused variable from closure.The
$poolsvariable was not used within the$getScheduleclosure, so removing it from theuseclause is appropriate cleanup.app/controllers/api/storage.php (6)
1189-1189: Good standardization of buffer size constant.Replacing the hardcoded 2MB limit with
APP_STORAGE_READ_BUFFERimproves code consistency and maintainability. This ensures all buffer size limits use the same constant throughout the codebase.
1356-1357: Consistent improvement across endpoints.Good to see the same buffer size standardization applied consistently across the
getFileViewendpoint, matching the changes ingetFileDownload.
1371-1378: Consistent header reordering pattern.The header setting has been moved after range processing in
getFileView, consistent with the changes ingetFileDownload. The same verification regarding header ordering safety applies here.
1509-1510: Complete consistency across file serving endpoints.Excellent consistency - all three file serving endpoints (
getFileDownload,getFileView,getFilePush) now use the same buffer size constant for range handling.
1524-1530: Consistent header reordering completed.The header setting pattern is now consistent across all file serving endpoints. All three endpoints (
getFileDownload,getFileView,getFilePush) now set response headers after range processing.
1007-1007:Details
✅ Verification successful
Verify that
APP_STORAGE_READ_BUFFERconstant is defined and has appropriate value.The hardcoded preview limit fallback has been replaced with
APP_STORAGE_READ_BUFFER. Ensure this constant is properly defined and has a suitable value for preview size limits.
🏁 Script executed:
#!/bin/bash # Description: Verify APP_STORAGE_READ_BUFFER constant definition and usage # Expected: The constant should be defined and used consistently across the codebase # Search for the constant definition echo "=== Searching for APP_STORAGE_READ_BUFFER definition ===" rg -A 2 -B 2 "define.*APP_STORAGE_READ_BUFFER|APP_STORAGE_READ_BUFFER.*=" # Search for all usages to ensure consistency echo -e "\n=== All usages of APP_STORAGE_READ_BUFFER ===" rg -A 1 -B 1 "APP_STORAGE_READ_BUFFER" # Check if there are any remaining hardcoded values that should use this constant echo -e "\n=== Checking for potential hardcoded values that should use the constant ===" rg -A 2 -B 2 "\b(20000000|2000000)\b"Length of output: 12521
APP_STORAGE_READ_BUFFER is properly defined and matches the previous 20 MB fallback
The constant
APP_STORAGE_READ_BUFFERis declared inapp/init/constants.phpas:const APP_STORAGE_READ_BUFFER = 20 * (1000 * 1000); // 20MBIts value is identical to the former hardcoded 20 MB, and it’s used consistently throughout the storage API. No further changes are required for the preview‐limit check.
| $response | ||
| ->setContentType($file->getAttribute('mimeType')) | ||
| ->addHeader('Cache-Control', 'private, max-age=3888000') // 45 days | ||
| ->addHeader('X-Peak', \memory_get_peak_usage()) | ||
| ->addHeader('Content-Disposition', 'attachment; filename="' . $file->getAttribute('name', '') . '"') | ||
| ; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that moving header setting after range processing doesn't cause issues.
The HTTP response headers are now set after range handling logic instead of before. While the range processing appears to only set headers without sending the response, this reordering could potentially affect response construction or header precedence.
🏁 Script executed:
#!/bin/bash
# Description: Check for any response sending or header conflicts in range processing
# Expected: Range processing should only set headers, not send response
echo "=== Checking range processing logic for response operations ==="
# Look for response send/output operations in range handling
rg -A 5 -B 5 "(response.*send|response.*output|response.*chunk)" app/controllers/api/storage.php
echo -e "\n=== Checking header setting patterns in other endpoints ==="
# Check if other endpoints follow similar patterns
rg -A 10 -B 5 "setContentType.*addHeader.*Cache-Control" app/controllers/api/storage.phpLength of output: 4574
Fix header placement in download endpoint
The response headers (Content-Type, Cache-Control, X-Peak, Content-Disposition) are currently added after calls to $response->send() and $response->chunk(), so any partial (range) or chunked responses will be sent without those headers. Move the header setup to before the range-handling logic to ensure all responses include them.
• File: app/controllers/api/storage.php
Endpoint: GET /v1/storage/buckets/:bucketId/files/:fileId/download (around lines 1180–1209)
Suggested diff:
--- a/app/controllers/api/storage.php
+++ b/app/controllers/api/storage.php
@@ -1180,6 +1180,12 @@ App::get('/v1/storage/buckets/:bucketId/files/:fileId/download')
+ // Set response headers before any send()/chunk()
+ $response
+ ->setContentType($file->getAttribute('mimeType'))
+ ->addHeader('Cache-Control', 'private, max-age=3888000') // 45 days
+ ->addHeader('X-Peak', \memory_get_peak_usage())
+ ->addHeader('Content-Disposition', 'attachment; filename="' . $file->getAttribute('name', '') . '"');
+
// Range and body handling
if (!empty($rangeHeader)) {
$response->send(substr($source, $start, $end - $start + 1));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/controllers/api/storage.php around lines 1204 to 1209, the response
headers such as Content-Type, Cache-Control, X-Peak, and Content-Disposition are
being set after calls to $response->send() and $response->chunk(), causing
partial or chunked responses to be sent without these headers. To fix this, move
the header-setting code to before any range processing or response sending logic
in the download endpoint so that all responses include the necessary headers
consistently.
✨ Benchmark results
⚡ Benchmark Comparison
|
4 participants
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist
Summary by CodeRabbit
Bug Fixes
Refactor