Fix: Platform check for Sites with automatic rule #10043
Conversation
📝 WalkthroughWalkthroughThe changes remove the use of the Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 phpcs (3.7.2)app/controllers/general.php📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ 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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/init/resources.php (1)
953-997: Refactor goto statements for better code readability.The validation logic is sound, but the use of
gotostatements reduces code readability and maintainability. Consider refactoring to use early returns or a helper method.-App::setResource('httpReferrerSafe', function (Request $request, string $httpReferrer, array $clients, Database $dbForPlatform, Document $project, App $utopia): string { - $origin = \parse_url($request->getOrigin($httpReferrer), PHP_URL_HOST); - - // Safe if route is public - $route = $utopia->getRoute(); - if (!$route->getLabel('origin', false)) { - goto originToUrl; - } - - // Safe if added as web platform - $validator = new Hostname($clients); - if ($validator->isValid($origin)) { - goto originToUrl; - } - - // Safe if rule with same project ID exists - if (!empty($origin)) { - if (System::getEnv('_APP_RULES_FORMAT') === 'md5') { - $rule = Authorization::skip(fn () => $dbForPlatform->getDocument('rules', md5($origin ?? ''))); - } else { - $rule = Authorization::skip( - fn () => $dbForPlatform->find('rules', [ - Query::equal('domain', [$origin]), - Query::limit(1) - ]) - )[0] ?? new Document(); - } - - if (!$rule->isEmpty() && $rule->getAttribute('projectInternalId') === $project->getSequence()) { - goto originToUrl; - } - } - - // Unsafe; Localhost is always safe for ease of local development - $origin = 'localhost'; - - originToUrl: - - $protocol = \parse_url($request->getOrigin($httpReferrer), PHP_URL_SCHEME); - $port = \parse_url($request->getOrigin($httpReferrer), PHP_URL_PORT); - - $referrer = (!empty($protocol) ? $protocol : $request->getProtocol()) . '://' . $origin . (!empty($port) ? ':' . $port : ''); - - return $referrer; +App::setResource('httpReferrerSafe', function (Request $request, string $httpReferrer, array $clients, Database $dbForPlatform, Document $project, App $utopia): string { + $origin = \parse_url($request->getOrigin($httpReferrer), PHP_URL_HOST); + + $isSafe = false; + + // Safe if route is public + $route = $utopia->getRoute(); + if (!$route->getLabel('origin', false)) { + $isSafe = true; + } + + // Safe if added as web platform + if (!$isSafe) { + $validator = new Hostname($clients); + if ($validator->isValid($origin)) { + $isSafe = true; + } + } + + // Safe if rule with same project ID exists + if (!$isSafe && !empty($origin)) { + if (System::getEnv('_APP_RULES_FORMAT') === 'md5') { + $rule = Authorization::skip(fn () => $dbForPlatform->getDocument('rules', md5($origin ?? ''))); + } else { + $rule = Authorization::skip( + fn () => $dbForPlatform->find('rules', [ + Query::equal('domain', [$origin]), + Query::limit(1) + ]) + )[0] ?? new Document(); + } + + if (!$rule->isEmpty() && $rule->getAttribute('projectInternalId') === $project->getSequence()) { + $isSafe = true; + } + } + + // Unsafe; Localhost is always safe for ease of local development + if (!$isSafe) { + $origin = 'localhost'; + } + + $protocol = \parse_url($request->getOrigin($httpReferrer), PHP_URL_SCHEME); + $port = \parse_url($request->getOrigin($httpReferrer), PHP_URL_PORT); + + $referrer = (!empty($protocol) ? $protocol : $request->getProtocol()) . '://' . $origin . (!empty($port) ? ':' . $port : ''); + + return $referrer;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/general.php(6 hunks)app/init/resources.php(1 hunks)
⏰ 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 (7)
app/init/resources.php (1)
948-951: LGTM - Simple and effective implementation.The
httpReferrerresource correctly extracts the raw HTTP referrer from the request.app/controllers/general.php (6)
806-808: LGTM - Proper dependency injection of new resources.The function signature correctly injects the new
httpReferrerandhttpReferrerSaferesources, replacing the previous manual validation logic.
940-940: LGTM - Consistent use of centralized referrer handling.Using the
httpReferrerresource instead of directly calling$request->getReferer('')maintains consistency with the new centralized approach.
1014-1014: LGTM - Consistent CORS header handling.Setting the CORS
Access-Control-Allow-Originheader to the sanitizedhttpReferrerSafevalue ensures consistent and secure CORS behavior.
1035-1035: LGTM - Simplified and secure origin validation.The simplified check against
httpReferrerSafeleverages the centralized validation logic while maintaining security by checking if the safe referrer defaults to 'localhost' (indicating an untrusted origin).
1058-1059: LGTM - Consistent dependency injection in OPTIONS route.The OPTIONS route correctly injects the
httpReferrerSaferesource with updated function signature, maintaining consistency with the main API route.
1077-1077: LGTM - Consistent CORS header in OPTIONS route.The OPTIONS route now consistently uses
httpReferrerSafefor the CORS header, matching the behavior in the main API route.
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
What does this PR do?
Test Plan
Related PRs and Issues
x
Checklist