Conversation
|
Warning Rate limit exceeded@loks0n has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 33 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 selected for processing (2)
📝 WalkthroughWalkthroughThe pull request modifies CORS preflight request handling by updating the allowedHostnames resource to include the request hostname when the HTTP method is OPTIONS. A corresponding test method is added to HTTPTest to validate that preflight requests properly echo the origin header in the access-control-allow-origin response header. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
a61c1a4 to
646504e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/General/HTTPTest.php (1)
187-203: Strengthen preflight assertions (status code + core CORS headers)Right now this mainly checks echoing the origin. To make this test catch regressions better, assert the preflight status code and a couple of critical headers:
public function testPreflight() { - $endpoint = '/v1/projects'; // Can be any non-404 route - /** * Test for SUCCESS */ $response = $this->client->call(Client::METHOD_OPTIONS, $endpoint, [ 'origin' => 'http://random.com', 'access-control-request-headers' => 'X-Appwrite-Project', 'access-control-request-method' => 'GET' ]); + $this->assertEquals(204, $response['headers']['status-code']); $this->assertEquals('http://random.com', $response['headers']['access-control-allow-origin']); + $this->assertEquals('true', $response['headers']['access-control-allow-credentials']); + $this->assertStringContainsString('X-Appwrite-Project', $response['headers']['access-control-allow-headers']); }If the intended behavior is not to allow arbitrary origins on preflight, add a negative preflight case (similar to
testCors()’s failure assertions) to lock that down.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/init/resources.php(1 hunks)tests/e2e/General/HTTPTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/General/HTTPTest.php (1)
tests/e2e/Client.php (1)
Client(8-342)
⏰ 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). (8)
- GitHub Check: Benchmark
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Databases/Legacy)
🔇 Additional comments (1)
app/init/resources.php (1)
186-213: The preflight fix correctly allows legitimate requests without CORS bypass riskThe addition of
$request->getHostname()for OPTIONS requests is sound. TheCorsclass extracts the host from theOriginheader (line 73 ofCors.php), not theHostheader, so includingrequest.getHostname()in the allowlist only enables CORS when the Origin header's extracted host matches an allowed value. For legitimate preflight requests, the Host and Origin domains are identical (same-origin to the browser), so this change correctly permits the preflight to succeed.The concern about Host header spoofing does not apply here because the CORS decision is based on the Origin header host, not the Host header value. Empty hostnames are also handled gracefully (lines 74-76 of
Cors.php), returning without setting the Allow-Origin header if the extracted host is invalid.No changes needed.
✨ Benchmark results
⚡ Benchmark Comparison
|
646504e to
8845318
Compare
8845318 to
2cb86c5
Compare
No description provided.