Conversation
- Created WebhookUrlValidator service to block private, reserved, and loopback IPs - Applied validation to WebhookEndpoint creation and updates - Added runtime URL validation in DeliverWebhookJob before delivery - Added comprehensive feature tests for various SSRF scenarios
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @Snider, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security of the webhook system by implementing multi-layered Server-Side Request Forgery (SSRF) protection. It introduces a dedicated validation service and integrates its checks at multiple points: during webhook endpoint creation, updates, and critically, just before any external HTTP request is made. This ensures that webhooks cannot be maliciously used to scan internal networks or access sensitive cloud metadata, thereby bolstering the application's overall security posture. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
QA Pipeline Results
Artifacts
Generated by core php qa pipeline |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces robust SSRF protection for webhooks, which is an excellent security enhancement. The implementation is well-structured with multiple layers of validation: at creation/update time in the WebhookEndpoint model, and as a final check at runtime in the DeliverWebhookJob. The new WebhookUrlValidator service is comprehensive, and the added tests cover a wide range of scenarios.
I've made a couple of suggestions in the WebhookUrlValidator service to simplify the code by removing redundant checks and to improve the robustness of DNS resolution. Overall, this is a solid implementation.
| protected function resolveHost(string $host): array | ||
| { | ||
| $ips = []; | ||
|
|
||
| // IPv4 resolution | ||
| $ipv4s = gethostbynamel($host); | ||
| if ($ipv4s !== false) { | ||
| $ips = array_merge($ips, $ipv4s); | ||
| } | ||
|
|
||
| // IPv6 resolution (if dns_get_record is available) | ||
| if (function_exists('dns_get_record')) { | ||
| try { | ||
| $ipv6s = @dns_get_record($host, DNS_AAAA); | ||
| if ($ipv6s) { | ||
| foreach ($ipv6s as $record) { | ||
| if (isset($record['ipv6'])) { | ||
| $ips[] = $record['ipv6']; | ||
| } | ||
| } | ||
| } | ||
| } catch (\Throwable $e) { | ||
| // Ignore DNS errors, rely on IPv4 or IP-based check | ||
| } | ||
| } | ||
|
|
||
| return array_unique($ips); | ||
| } |
There was a problem hiding this comment.
The resolveHost method uses gethostbynamel for IPv4 resolution, which is an older function with some known issues (e.g., it's blocking, and its behavior can be inconsistent in some environments). For improved robustness and consistency, consider refactoring this method to use dns_get_record for both IPv4 (A records) and IPv6 (AAAA records) lookups. This would unify the DNS resolution logic and leverage a more modern PHP function.
| if ($isPublic === false) { | ||
| return true; | ||
| } | ||
|
|
||
| // Additional manual checks for safety | ||
| $lowIp = strtolower($ip); | ||
|
|
||
| // IPv6 loopback | ||
| if ($lowIp === '::1' || $lowIp === '0000:0000:0000:0000:0000:0000:0000:0001') { | ||
| return true; | ||
| } | ||
|
|
||
| // Check for 169.254.x.x (Link-local / AWS Metadata) - double check | ||
| if (str_starts_with($ip, '169.254.')) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check for IPv6 link-local (fe80::/10) | ||
| if (str_starts_with($lowIp, 'fe80:')) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
The manual checks for private and reserved IP addresses are redundant. The filter_var call on line 124 with the flags FILTER_FLAG_NO_PRIV_RANGE and FILTER_FLAG_NO_RES_RANGE already covers all these cases, including IPv4/IPv6 loopback, link-local, and private ranges. The logic can be simplified to just check the result of filter_var.
return $isPublic === false;
}- Created WebhookUrlValidator to block restricted IP ranges (localhost, private, cloud metadata) - Applied URL validation to WebhookEndpoint creation, updates, and delivery - Fixed CI failure by adding missing host-uk/core repository to composer.json - Set minimum-stability to dev to allow resolution of internal dependencies - Fixed invalid checkout version in ci.yml workflow
- Created WebhookUrlValidator to block private/reserved IP ranges - Applied multi-layered URL validation to WebhookEndpoint and DeliverWebhookJob - Fixed CI failure by correctly configuring host-uk/core dependency - Added missing host-uk/core-php VCS repository with no-api: true to composer.json - Fixed non-existent actions/checkout version in ci.yml
- Created WebhookUrlValidator to block restricted IP ranges - Applied multi-layered URL validation to WebhookEndpoint and DeliverWebhookJob - Fixed CI failures by adding missing dev dependencies to composer.json - Configured host-uk/core repository with no-api: true to resolve VCS dependency - Downgraded actions/checkout to v4 in ci.yml workflow
|
Warning Rate limit exceeded
⌛ 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. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
1 participant
This change implements Server-Side Request Forgery (SSRF) protection for the webhook system.
Key changes:
WebhookUrlValidatorService: A dedicated service that validates URLs by resolving their hostnames to IPs (supporting both IPv4 and IPv6) and checking them against restricted ranges usingfilter_varwithFILTER_FLAG_NO_PRIV_RANGEandFILTER_FLAG_NO_RES_RANGE. It also includes manual blocks for loopback addresses, common localhost names, and cloud metadata endpoints (e.g., 169.254.169.254).WebhookEndpointmodel to validate URLs in thecreateForWorkspacemethod and added asetUrlAttributemutator to ensure all updates to the URL are validated.DeliverWebhookJobjust before the HTTP request is made. If the URL is found to be restricted at runtime, the delivery is cancelled, marked as failed with a security log entry, and retries are disabled.WebhookUrlValidationTestthat covers blocked hostnames, private IPs, link-local addresses, and valid public URLs.This provides multiple layers of defense against attackers trying to use webhooks to scan internal networks or access sensitive cloud metadata.
Fixes #4
PR created automatically by Jules for task 18092576128623276148 started by @Snider