fix: harden webhook/link/OAuth-avatar SSRF (advisory clusters A/B/C/E)#9163
Merged
Conversation
Resolves three overlapping SSRF advisory clusters around webhook delivery
and work-item link unfurling:
- Cluster A (private-IP validation + PATCH bypass): the webhook PATCH
handler passed context={request: request} (the request object as the
dict key) so the loopback/disallowed-domain guard silently no-op'd —
now context={"request": request}. Hardened IP classification
(is_blocked_ip) to also block multicast, unspecified, CGNAT
(100.64.0.0/10), and IPv4 embedded in IPv6 transition addresses
(IPv4-mapped, NAT64, 6to4, Teredo), robust across Python versions.
- Cluster B (DNS-rebinding TOCTOU): validators resolved DNS, then
requests resolved it again at connect time. New pinned-IP client
(plane/utils/url_security.py) resolves+validates once and connects to
the validated IP literal so urllib3 performs no second lookup, while
preserving Host header, TLS SNI and certificate verification against
the real hostname.
- Cluster C (redirect SSRF): webhook delivery never follows redirects;
the link crawler follows them manually, re-resolving + re-validating +
re-pinning every hop.
Also: pin requests==2.33.0 in base.txt (imported directly; the pinning
adapter needs the >=2.32 get_connection_with_tls_context hook), and log
webhook URL-validation rejections to WebhookLog instead of swallowing
them.
Tests: new test_url_security.py (pinning, rebinding, redirect
re-validation, IP edge cases, TLS SNI) + updated link-task tests.
Full unit suite: 178 passed.
sriramveeraghanta
requested review from
dheeru0198 and
pablohashescobar
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughCentralizes SSRF defenses: adds explicit IP deny-list and resolver, implements DNS-pinned HTTP adapter and per-hop re-validation, integrates pinned fetches into webhook sending, link unfurling, and avatar downloads, and adds extensive unit tests covering pinning, redirects, and edge cases. ChangesSSRF Defense Infrastructure & Integration
Sequence Diagram (high-level request flow) sequenceDiagram
participant Client
participant PinnedFetch
participant Resolver
participant Adapter
participant RemoteServer
Client->>PinnedFetch: pinned_fetch(method, url, headers...)
PinnedFetch->>Resolver: resolve_and_validate(hostname)
Resolver-->>PinnedFetch: validated IPs
PinnedFetch->>Adapter: request to IP literal (Host=SNI=hostname)
Adapter->>RemoteServer: TCP/TLS to IP (SNI=hostname)
RemoteServer-->>Adapter: HTTP response (200/3xx)
Adapter-->>PinnedFetch: Response
alt 3xx
PinnedFetch->>Resolver: resolve_and_validate(new-host)
PinnedFetch->>Adapter: new pinned request to validated IP
end
PinnedFetch-->>Client: final response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens outbound HTTP usage in the API (webhook delivery and work-item link unfurling) against multiple SSRF vectors by eliminating DNS-rebinding TOCTOU, broadening blocked-IP coverage (including IPv6 transition formats), and ensuring validation is correctly applied on webhook updates.
Changes:
- Introduces a shared “pinned IP”
requestsclient (pinned_fetch*) that resolves + validates once, then connects directly to the validated IP literal while preserving Host/SNI/cert verification semantics. - Hardens IP blocking/validation (
is_blocked_ip,resolve_and_validate) to cover additional unsafe ranges and IPv4 embedded in IPv6 transition formats; updatesvalidate_url()to use the shared resolver/validator. - Moves
requests==2.33.0to production requirements and updates unit tests to cover pinning, redirect re-validation, and edge-case IP ranges.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/requirements/base.txt | Pins requests==2.33.0 in production to support the adapter hook used for IP pinning. |
| apps/api/requirements/test.txt | Removes requests from test-only deps now that it’s required by production code. |
| apps/api/plane/utils/url_security.py | Adds SSRF-safe pinned-IP HTTP client and manual redirect following with per-hop revalidation/repinning. |
| apps/api/plane/utils/ip_address.py | Adds hardened blocked-IP detection and shared resolve_and_validate() used by both validators and pinning client. |
| apps/api/plane/tests/unit/bg_tasks/test_work_item_link_task.py | Updates/extends unit coverage for hardened IP blocking and pinned redirect-safe fetching behavior. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/utils/url_security.py (1)
211-234: ⚡ Quick winCross-host credential stripping is unnecessary for current callers; optional hardening if future callers pass auth headers.
Current repo usage only supplies a
User-Agentheader topinned_fetch_following_redirects(viasafe_getinwork_item_link_task.py), and there are no call sites/tests passingAuthorization/Cookie/Proxy-Authorization. The proposed header-dropping logic is therefore not needed for present behavior, though it could be kept as defense-in-depth for future credential-bearing usages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/utils/url_security.py` around lines 211 - 234, The redirect-following loop in pinned_fetch_following_redirects currently includes cross-host credential-stripping logic that is unnecessary for current callers (safe_get via work_item_link_task.py) which only pass a User-Agent; remove the header-dropping behavior or make it conditional/opt-in so we don't strip Authorization/Cookie/Proxy-Authorization for present uses. Locate pinned_fetch_following_redirects and its callsites (safe_get) and either remove the cross-host header modification or add an explicit flag/parameter (e.g., strip_credentials=True) and default it to False, updating callers accordingly to preserve current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/utils/ip_address.py`:
- Around line 136-139: The except block around socket.getaddrinfo in
resolve_and_validate currently only catches socket.gaierror, so UnicodeError
from IDNA/Unicode hostname processing escapes and is not converted to a
ValueError; update the except to also catch UnicodeError (e.g., except
(socket.gaierror, UnicodeError):) so any IDNA/Unicode failures when calling
socket.getaddrinfo(hostname, None) are converted into the same
ValueError("Hostname could not be resolved") and will be handled by
webhook_send_task as a URL rejection.
---
Nitpick comments:
In `@apps/api/plane/utils/url_security.py`:
- Around line 211-234: The redirect-following loop in
pinned_fetch_following_redirects currently includes cross-host
credential-stripping logic that is unnecessary for current callers (safe_get via
work_item_link_task.py) which only pass a User-Agent; remove the header-dropping
behavior or make it conditional/opt-in so we don't strip
Authorization/Cookie/Proxy-Authorization for present uses. Locate
pinned_fetch_following_redirects and its callsites (safe_get) and either remove
the cross-host header modification or add an explicit flag/parameter (e.g.,
strip_credentials=True) and default it to False, updating callers accordingly to
preserve current behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 433b7fce-b817-4461-91e5-c67cd8b8b9ad
📒 Files selected for processing (9)
apps/api/plane/app/views/webhook/base.pyapps/api/plane/bgtasks/webhook_task.pyapps/api/plane/bgtasks/work_item_link_task.pyapps/api/plane/tests/unit/bg_tasks/test_url_security.pyapps/api/plane/tests/unit/bg_tasks/test_work_item_link_task.pyapps/api/plane/utils/ip_address.pyapps/api/plane/utils/url_security.pyapps/api/requirements/base.txtapps/api/requirements/test.txt
…tests Verified every SSRF-class advisory against the current code. The webhook / link / favicon reports — including the published CVE-2026-30242 and CVE-2026-39843 and the newer "still bypassable" reports (DNS rebinding GHSA-3856/-fgcv/-9292/-whh3/-4mjx/-6p39/-fv24/-8wvv, IP-classification gaps GHSA-75fg, redirect GHSA-6v37/-jw6g/-mq87) — are resolved by the pinned-IP client + hardened classifier in this branch. The one SSRF family still unresolved was the OAuth avatar path: download_and_upload_avatar() fetched the provider-supplied avatar_url with a raw requests.get (no IP validation, default redirect following), so an attacker-controlled avatar could reach internal addresses and be exfiltrated via the static-asset endpoint (GHSA-cv9p-325g-wmv5, and the avatar hop of the Gitea SSRF GHSA-hx79-5pj5-qh42). It now uses pinned_fetch_following_redirects, which validates + pins every hop and blocks internal targets. Adds test_ssrf_advisories.py: a per-advisory regression map covering webhook IP validation, the PATCH context-key guard, webhook DNS rebinding, webhook redirect, favicon redirect + rebinding, and OAuth avatar SSRF. docker compose test: 199 unit tests pass.
sriramveeraghanta
changed the title
Member
Author
SSRF advisory verification (all similar cases checked)I pulled every SSRF-class advisory from the repo's security advisories and verified each against the code on this branch. Summary: the webhook/link/favicon reports — including the ones users are re-filing as "still bypassable" — are resolved here, and the one genuinely-unresolved family (OAuth avatar SSRF) is now fixed too.
Note on GHSA-hx79 first-hop: Tests
🤖 Generated with Claude Code |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/tests/unit/bg_tasks/test_ssrf_advisories.py (1)
49-51: ⚡ Quick winUse socket.AF_INET/AF_INET6 in the _addr addrinfo stub (optional fidelity).
The validators currently only read the IP string fromaddr[4][0](not thefamilyfield), so hard-coding6/2doesn’t affect correctness today—though usingsocket.AF_INET6/socket.AF_INETwould make the mock a more faithful addrinfo tuple.Suggested fix
+import socket + def _addr(ip): - family = 6 if ":" in ip else 2 + family = socket.AF_INET6 if ":" in ip else socket.AF_INET return (family, None, None, None, (ip, 0))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/unit/bg_tasks/test_ssrf_advisories.py` around lines 49 - 51, The addrinfo stub function _addr currently hardcodes the address family as 6/2; update it to use socket.AF_INET6 and socket.AF_INET respectively so the tuple returned by _addr matches real addrinfo semantics. Modify the _addr helper used in tests (function name _addr) to import/ reference socket and set family = socket.AF_INET6 if ":" in ip else socket.AF_INET, leaving the rest of the tuple structure unchanged to keep compatibility with validators that read addr[4][0].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/authentication/adapter/base.py`:
- Around line 156-158: The avatar download guard is ineffective because
pinned_fetch_following_redirects/_request_to_ip call requests without
stream=True and close the Session eagerly, and the upload still uses
response.content; update pinned_fetch_following_redirects and its helper
_request_to_ip to request with stream=True and ensure the
requests.Session/response stay open for the caller to iterate, moving session
close until after the caller finishes reading, and when constructing the upload
use the bounded in-memory buffer (the variable named content built by the
size-limited iter_content loop) instead of BytesIO(response.content) so the
Content-Length/iter_content max-size checks actually prevent large buffering.
---
Nitpick comments:
In `@apps/api/plane/tests/unit/bg_tasks/test_ssrf_advisories.py`:
- Around line 49-51: The addrinfo stub function _addr currently hardcodes the
address family as 6/2; update it to use socket.AF_INET6 and socket.AF_INET
respectively so the tuple returned by _addr matches real addrinfo semantics.
Modify the _addr helper used in tests (function name _addr) to import/ reference
socket and set family = socket.AF_INET6 if ":" in ip else socket.AF_INET,
leaving the rest of the tuple structure unchanged to keep compatibility with
validators that read addr[4][0].
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72d2f58a-6900-401b-a500-6689c682c82d
📒 Files selected for processing (2)
apps/api/plane/authentication/adapter/base.pyapps/api/plane/tests/unit/bg_tasks/test_ssrf_advisories.py
- url_security: preserve URL-embedded credentials (user:pass@host) as Basic Auth instead of silently dropping them when rewriting to the IP literal (Copilot); bracket IPv6-literal hostnames in the Host header (Copilot); add stream=True support that keeps the session open until the response is closed, and release intermediate redirect hops. - ip_address / work_item_link_task: treat UnicodeError (IDNA failures) from getaddrinfo as a resolution failure, not an uncaught exception (CodeRabbit). - authentication/adapter/base: stream the avatar download so the size cap actually bounds memory, upload the size-bounded buffer (not response.content), and always close the response (CodeRabbit, major). - tests: cover auth preservation, IPv6 Host bracketing, IDNA handling, and streamed session lifetime; drop an unused import. docker compose test: 204 unit tests pass.
pushya22
approved these changes
May 30, 2026
Prashant-Surya
approved these changes
May 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Resolves three overlapping SSRF advisory clusters affecting webhook delivery and work-item link unfurling. The root fix is a single shared pinned-IP HTTP client so the address we validate is exactly the address we connect to.
Cluster A — private-IP validation + PATCH bypass
WebhookEndpoint.patchpassedcontext={request: request}— therequestobject as the dict key — soself.context.get("request")was alwaysNoneand the loopback/disallowed-domain guard silently no-op'd on update. Fixed tocontext={"request": request}(GHSA-6485-m23r-fx8q).is_blocked_ip) to also block multicast, unspecified, CGNAT100.64.0.0/10(notis_privateon Python 3.12), and IPv4 addresses embedded in IPv6 transition formats (IPv4-mapped::ffff:, NAT64, 6to4, Teredo). Fails closed and is robust across Python versions rather than relying on 3.12-specificipaddresssemantics.Cluster B — DNS-rebinding TOCTOU (the previously-unpatched core)
requestsresolved it again at connect time, letting an attacker swap in an internal IP between the two lookups.plane/utils/url_security.pyresolves + validates once, then connects to the validated IP literal so urllib3 performs no second DNS lookup (TOCTOU closed by construction), while preserving the original hostname for theHostheader, TLS SNI, and certificate verification (PinnedIPAdapterinjectsserver_hostname;assert_hostnameleft atNoneso cert checking stays on).Cluster C — SSRF via HTTP redirect following
allow_redirects=False); the link crawler follows them manually, re-resolving + re-validating + re-pinning every hop (a relativeLocationis resolved against the logical hostname URL, not the IP-literal URL).Implementation notes:
requests==2.33.0inbase.txt(it is imported directly by production code, and the pinning adapter needs the>=2.32get_connection_with_tls_contexthook).trust_env=False+ proxies disabled on the pinned session so an ambientHTTP(S)_PROXYcan't tunnel around the pin.WEBHOOK_ALLOWED_HOSTSentries skip the block check but are still pinned, so a trusted hostname can't be rebound to a different internal target.WebhookLog(visible to admins) instead of being silently swallowed.This work was hardened by an adversarial review pass (independent attacker/verifier agents) which caught and fixed: the
allowed_hostspath missing pinning, a NAT64 prefix-mask inconsistency, and silentValueErrorhandling.Type of Change
Screenshots and Media (if applicable)
N/A — backend SSRF hardening, no UI changes.
Test Scenarios
Host/TLS/cert verified against the real hostname (connection pinned to the resolved IP).127.0.0.1,169.254.169.254,100.64.0.1,::ffff:169.254.169.254) is rejected and the rejection is recorded inWebhookLog.docker compose -f docker-compose-test.yml run --rm api-tests pytest -m unit→ 178 passed. Newtest_url_security.pycovers pinning, rebinding, redirect re-validation, IP edge cases, and TLS SNI injection.References
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Security Improvements
Chores
Tests