Chore: tests #11282
Chore: tests #11282
Conversation
📝 WalkthroughWalkthroughAdded app/config/cors.php with centralized CORS settings and loaded it during bootstrap via Config::load in app/init/configs.php. The HTTP/CORS resource in app/init/resources.php now reads the cors config and constructs the Cors instance from allowedMethods, allowedHeaders, and exposedHeaders (resource callback changed from an arrow fn to a function and now registers ['allowedHostnames']). tests/e2e/General/HTTPTest.php was updated to read CORS values from Config::getParam('cors') and assert response headers against those config-derived values. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/config/cors.php`:
- Line 9: The CORS config's allowedMethods array ('allowedMethods') is missing
HEAD, causing cross-origin HEAD requests to fail; update the 'allowedMethods'
array in app/config/cors.php (the 'allowedMethods' entry) to include 'HEAD' so
it becomes ['GET','POST','PUT','PATCH','DELETE','HEAD'] to match supported HEAD
requests referenced in tests/e2e/Client.php and the Functions execution module
whitelist.
🧹 Nitpick comments (1)
tests/e2e/General/HTTPTest.php (1)
32-41: Test asserts against the same source of truth it's verifying — consider the trade-off.The test now reads from
Config::getParam('cors')and asserts the response headers match. This ensures consistency with the config but won't catch if the config itself has incorrect/missing values (e.g., a required header accidentally removed). Previously, hardcoded values in the test served as an independent second source of truth.This is an acceptable trade-off for maintainability, just worth being aware of.
| * Centralised list of allowed methods, headers, and exposed headers for CORS responses. | ||
| */ | ||
| return [ | ||
| 'allowedMethods' => ['GET', 'POST', 'PUT', 'PATCH', 'DELETE'], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if HEAD method is used/handled in routes
rg -n --type=php -i 'HEAD|head.*method|method.*head' -C2 | head -60Repository: appwrite/appwrite
Length of output: 3927
🏁 Script executed:
# Search for CORS-related handling and middleware
rg -n --type=php 'cors|CORS|allowedMethods' -C2 | head -80Repository: appwrite/appwrite
Length of output: 4768
🏁 Script executed:
# Look at the Cors class implementation if it exists
fd -e php -i cors | head -20Repository: appwrite/appwrite
Length of output: 142
🏁 Script executed:
# Check the app/config/cors.php file to see the complete context
cat -n app/config/cors.phpRepository: appwrite/appwrite
Length of output: 1702
🏁 Script executed:
# Read the Cors.php implementation
cat -n src/Appwrite/Network/Cors.php | head -100Repository: appwrite/appwrite
Length of output: 3992
🏁 Script executed:
# Check if there's any framework-level HEAD method handling
rg -n 'HEAD|Method::HEAD' --type=php | head -40Repository: appwrite/appwrite
Length of output: 4841
🏁 Script executed:
# Look at the routing/framework setup to understand how methods are handled
fd -e php -type f | xargs grep -l "Route\|Request.*method" | head -10Repository: appwrite/appwrite
Length of output: 231
🏁 Script executed:
# Check if OPTIONS method is used/needed for CORS preflight
rg -n "OPTIONS|preflight" --type=php | head -30Repository: appwrite/appwrite
Length of output: 3937
🏁 Script executed:
# Look at the Swagger2 file to see how HEAD is being handled
cat -n src/Appwrite/SDK/Specification/Format/Swagger2.php | head -130Repository: appwrite/appwrite
Length of output: 5638
🏁 Script executed:
# Check if there are any routes explicitly handling HEAD method at route registration level
rg -n "Method.*HEAD|method.*HEAD|->get\(|->post\(|->method" app/controllers --type=php | head -30Repository: appwrite/appwrite
Length of output: 2536
🏁 Script executed:
# Look for any documentation or comments about HEAD request support
rg -n -i "head.*request|request.*head" --type=php | head -20Repository: appwrite/appwrite
Length of output: 3142
Add HEAD to the allowedMethods array.
The application explicitly supports HEAD requests (evidenced by test cases in tests/e2e/Client.php and tests/e2e/Services/Functions/FunctionsCustomServerTest.php, and the Functions execution module's whitelist), but the CORS configuration does not declare HEAD as allowed. This will cause cross-origin HEAD requests to fail with CORS preflight errors. Update line 9 to include HEAD:
'allowedMethods' => ['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'HEAD'],
🤖 Prompt for AI Agents
In `@app/config/cors.php` at line 9, The CORS config's allowedMethods array
('allowedMethods') is missing HEAD, causing cross-origin HEAD requests to fail;
update the 'allowedMethods' array in app/config/cors.php (the 'allowedMethods'
entry) to include 'HEAD' so it becomes
['GET','POST','PUT','PATCH','DELETE','HEAD'] to match supported HEAD requests
referenced in tests/e2e/Client.php and the Functions execution module whitelist.
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