fix(webhooks): validate organisation and integration URLs against SSRF - #8220
fix(webhooks): validate organisation and integration URLs against SSRF#8220bardock-2393 wants to merge 2 commits into
Conversation
OrganisationWebhookSerializer, the GitLab/Sentry/generic webhook integrations, and every integration sharing IntegrationsModel.base_url (Dynatrace, Datadog, New Relic, Grafana, Amplitude, Heap, Mixpanel, Rudderstack) accepted arbitrary URLs with only format validation. Apply the existing NoSSRFURLField, used by environment webhooks, to these fields so they're resolved and checked against internal/private address ranges too. Also restrict NoSSRFURLField itself to http/https schemes, closing an ftp/ftps gap in the existing validator.
|
@bardock-2393 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change applies Estimated code review effort: 3 (Moderate) | ~25 minutes ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8220 +/- ##
==========================================
- Coverage 98.71% 98.57% -0.14%
==========================================
Files 1535 1536 +1
Lines 61329 61411 +82
==========================================
- Hits 60541 60536 -5
- Misses 788 875 +87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f70df97-6c85-42b5-a27c-5428f9459f4b
📒 Files selected for processing (14)
api/integrations/common/serializers.pyapi/integrations/gitlab/serializers.pyapi/integrations/sentry/serializers.pyapi/integrations/webhook/serializers.pyapi/organisations/serializers.pyapi/tests/unit/integrations/datadog/test_unit_datadog_views.pyapi/tests/unit/integrations/dynatrace/test_unit_dynatrace_views.pyapi/tests/unit/integrations/gitlab/test_configuration.pyapi/tests/unit/integrations/new_relic/test_unit_new_relic_views.pyapi/tests/unit/integrations/sentry/test_unit_sentry_views.pyapi/tests/unit/integrations/webhook/test_unit_webhook_views.pyapi/tests/unit/organisations/test_unit_organisations_serializers.pyapi/tests/unit/webhooks/test_webhooks_fields.pyapi/webhooks/fields.py
…r message Explicit NoSSRFURLField() declarations dropped the model's max_length=200 constraint. Also update the internal-address error message since the field is now shared by non-webhook integrations (Sentry, GitLab).
1 participant
Changes
Extends the existing SSRF-safe URL field (previously added for environment webhooks only) to
OrganisationWebhookSerializer, the GitLab/Sentry/generic webhook integration serializers, and every integration built on the sharedIntegrationsModel.base_url(Dynatrace, Datadog, New Relic, Grafana, Amplitude, Heap, Mixpanel, Rudderstack). These previously accepted any syntactically valid URL, including ones resolving to private/internal network ranges. Also tightens the shared validator to reject non-http(s) schemes (it previously allowedftp/ftpsvia Django's defaultURLValidator).Contributes to #8218 — coverage is limited to fields confirmed to be used for outbound server-side requests (verified via call-site grep against every app under
api/integrations/). Fields that only build display links (not fetched server-side) were left untouched.Note: this removes the ability for self-hosted deployments to point webhooks/integrations at
localhostor internal Docker-network hosts, even intentionally in dev/test setups — consistent with the existing environment-webhook precedent, now applied repo-wide. Happy to follow up with a settings-driven allowlist if that's needed for dev workflows.How did you test this code?
Added private-IP-rejection, DNS-rebind-rejection, and scheme-rejection tests across the touched serializers' existing test files. All 302 tests in the touched integration/webhook/organisation suites pass against real Postgres. Confirmed old-vs-new by reverting just the validator change: 2 tests fail (ftp/ftps accepted) on old code, pass on new code. Ran mypy (full project, clean) and the project's pre-commit hooks (all clean).
Review effort: 4/5