Add URL validation for notification webhook fields#9224
Conversation
Add SafeWebhookUrl validation rule to notification webhook URL fields (Slack, Discord, custom webhook) to enforce safe URL patterns including scheme validation and hostname checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis change replaces the built-in URL validation rule with a custom ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Livewire/Notifications/Webhook.php`:
- Around line 24-25: The Livewire property validation is insufficient because
WebhookNotificationSettings::webhook_url can be set outside Livewire; update
App\Jobs\SendWebhookJob::handle() to validate the stored webhook URL with the
same SafeWebhookUrl rule (or a shared validator/helper) immediately before
calling Http::post(), and abort/log/throw (skip sending) if validation fails;
reference WebhookNotificationSettings::webhook_url, SafeWebhookUrl and
Http::post() so you add the check in SendWebhookJob::handle() and ensure only
validated URLs are posted.
In `@app/Rules/SafeWebhookUrl.php`:
- Around line 43-93: The SafeWebhookUrl rule currently only checks the host
string and literal IPv4 in isLinkLocal/isLoopback, allowing DNS or IPv6 bypasses
(e.g., ::ffff:127.0.0.1, fe80::1, hostnames resolving to 127.0.0.0/8 or
169.254.0.0/16). Fix by resolving the hostname to A and AAAA records
(dns_get_record or gethostbynamel + DNS_AAAA) in the validation path that uses
$host (SafeWebhookUrl::validate / the block around $blockedHosts), then for each
resolved address: normalize with inet_pton and detect IPv4-mapped IPv6 addresses
(treat ::ffff:x.y.z.w as IPv4), use existing isLoopback(string $ip) and extend
it to accept normalized IPv6-mapped IPv4, and update isLinkLocal(string $ip) to
handle IPv6 link-local ranges (fe80::/10) and to check IPv4 addresses by numeric
mask (169.254.0.0/16) using ip2long bitmasking; reject the URL if any resolved
address matches loopback or link-local rules and log via Log::warning as done
now.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 153b697d-5b26-4af5-aea2-d9a6ff1b3768
📒 Files selected for processing (5)
app/Livewire/Notifications/Discord.phpapp/Livewire/Notifications/Slack.phpapp/Livewire/Notifications/Webhook.phpapp/Rules/SafeWebhookUrl.phptests/Unit/SafeWebhookUrlTest.php
Prevent server-side request forgery (SSRF) attacks by validating webhook URLs before sending requests. Blocks loopback addresses, cloud metadata endpoints, and localhost URLs. - Add SafeWebhookUrl rule validation in SendWebhookJob.handle() - Log warning when unsafe URLs are rejected - Add comprehensive unit tests covering valid and invalid URL scenarios
Summary
SafeWebhookUrlvalidation rule for notification webhook URL fieldsTest plan
php artisan test --compact --filter=SafeWebhookUrl🤖 Generated with Claude Code