UN-3815 [FIX] Validate webhook URLs in one place, at both sinks - #2214
Open
athul-rs wants to merge 5 commits into
Open
UN-3815 [FIX] Validate webhook URLs in one place, at both sinks#2214athul-rs wants to merge 5 commits into
athul-rs wants to merge 5 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Summary by CodeRabbit
WalkthroughWebhook URL validation is centralized in a shared SSRF guard and applied to backend serializers, the webhook test endpoint, core notification delivery, and worker webhook sinks. Redirects are disabled, sensitive test response fields are removed, and regression tests cover internal targets, DNS behavior, parsing, and delivery. ChangesWebhook SSRF Protection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WebhookSurface
participant is_safe_webhook_url
participant DNS
participant HTTPClient
Client->>WebhookSurface: Provide webhook URL
WebhookSurface->>is_safe_webhook_url: Validate scheme, host, and credentials
is_safe_webhook_url->>DNS: Resolve normalized hostname when enabled
DNS-->>is_safe_webhook_url: Return resolved addresses
is_safe_webhook_url-->>WebhookSurface: Accept or reject target
WebhookSurface->>HTTPClient: Send POST with redirects disabled
HTTPClient-->>WebhookSurface: Return delivery result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
|
| Filename | Overview |
|---|---|
| unstract/core/src/unstract/core/network/ssrf.py | Adds the shared parser-agreement, scheme, credentials, DNS, and globally routable address checks; the prior non-global-address issue is fixed. |
| unstract/core/src/unstract/core/notification_utils.py | Guards notification delivery at the network sink and disables redirect following. |
| workers/executor/executors/postprocessor.py | Moves TLS-only webhook validation into the postprocessing request sink. |
| backend/notification_v2/serializers.py | Validates supplied URLs without re-resolving an unchanged stored URL during partial updates. |
| backend/notification_v2/internal_views.py | Guards webhook tests, disables redirects, correctly limits success to 2xx, and removes echoed request and response data. |
Sequence Diagram
sequenceDiagram
participant Tenant
participant API as Notification API
participant Worker as Webhook Sink
participant Guard as SSRF Guard
participant Target as Public Webhook
Tenant->>API: Create or update webhook URL
API->>Guard: Validate syntax and literal address
Guard-->>API: Accept or reject
Worker->>Guard: Resolve and validate every address
alt URL is globally routable
Guard-->>Worker: Safe
Worker->>Target: POST without redirects
Target-->>Worker: Response
else URL is unsafe or ambiguous
Guard-->>Worker: Reject
Worker-->>Worker: Skip outbound request
end
Reviews (6): Last reviewed commit: "Merge branch 'main' into UN-3794-webhook..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 3
🤖 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 `@backend/notification_v2/serializers.py`:
- Around line 42-54: Update _validate_url to validate only when “url” is present
in the incoming data, avoiding re-validation of the instance’s existing URL
during unrelated PATCH requests. Preserve the public-address check and
ValidationError for newly supplied URLs, while allowing other fields on legacy
records to update.
In `@unstract/core/src/unstract/core/network/ssrf.py`:
- Around line 58-73: The synchronous getaddrinfo call in _resolve can block
request-handling threads for the resolver’s full timeout. Bound DNS resolution
with an explicit timeout using a suitable worker-thread executor or
timeout-capable DNS resolver, return an empty set when the deadline is exceeded,
and preserve the existing direct-IP and resolution-failure behavior.
- Around line 76-88: Update _is_public to return ip.is_global after parsing the
address, replacing the manually assembled
private/loopback/link-local/reserved/multicast/unspecified predicate so RFC 6598
shared-address-space addresses and all other non-globally-reachable ranges are
rejected.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 6f65e249-a62d-4973-98f3-0ef16c4d426a
📒 Files selected for processing (11)
backend/notification_v2/internal_views.pybackend/notification_v2/serializers.pybackend/notification_v2/tests/__init__.pybackend/notification_v2/tests/test_webhook_ssrf.pyunstract/core/src/unstract/core/network/__init__.pyunstract/core/src/unstract/core/network/ssrf.pyunstract/core/src/unstract/core/notification_utils.pyunstract/core/tests/test_ssrf_guard.pyworkers/executor/executors/answer_prompt.pyworkers/executor/executors/postprocessor.pyworkers/tests/test_webhook_ssrf_sink.py
athul-rs
changed the title
athul-rs
marked this pull request as draft
July 27, 2026 19:16
Two paths send a request to a URL a tenant supplied: prompt postprocessing and pipeline notifications. They disagreed on what they checked. Postprocessing read the URL with urlparse while the transport under requests resolves it with urllib3. The two parsers do not always agree on the host, so the host that was checked is not necessarily the host the socket connects to. Notification delivery did not check the URL at all, and left allow_redirects at the requests default, so a redirect decided where the request landed. Add unstract.core.network.ssrf.is_safe_webhook_url and call it from both sinks rather than from their callers, so a new caller does not have to remember it. It refuses when the two parsers disagree on the host — an invariant, not a list of characters to reject — when the URL carries credentials, and when any resolved address is not publicly routable. Hosts are normalized before comparison so IPv6 literals and unicode IDN hosts are not rejected. Redirects are off on both paths. Also applies the guard to the internal webhook-test endpoint, which had none, and reduces its response to the status code — the body and headers of whatever it reached are not the caller's to read. NotificationSerializer now rejects a non-public URL at creation instead of storing it and failing at delivery. Note the ceiling: resolve-then-connect cannot cover a name re-resolved between the check and the socket. That needs an egress policy on the worker pods.
Three corrections from review: - _is_public enumerated six negative flags, which misses ranges that belong to none of them. RFC 6598 shared address space (100.64.0.0/10) passed as public on Python 3.12, as do RFC 2544 benchmarking and IETF protocol assignment ranges. Use ipaddress.is_global instead: an allowlist maintained against the IANA registries, so it stays correct as ranges are added, and shorter. - NotificationSerializer re-resolved the stored URL on any PATCH, so a brief DNS failure or a legacy record made an unrelated field edit fail on a field the caller never sent. Only validate a URL that was supplied; the sink guard remains the real control. - The internal webhook-test endpoint reported success on any status below 400, but redirects are not followed, so a 301/302 means the payload never reached the destination. Report success on 2xx only. Each has a test that fails without the corresponding fix.
athul-rs
force-pushed
the
UN-3794-webhook-egress
branch
from
July 29, 2026 09:42
f481b06 to
71c9d3b
Compare
athul-rs
marked this pull request as ready for review
July 29, 2026 09:42
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@backend/notification_v2/tests/test_webhook_ssrf.py`:
- Around line 42-44: Remove live DNS dependencies in all three tests: in
backend/notification_v2/tests/test_webhook_ssrf.py lines 42-44, mock the shared
SSRF resolver to return a stable public address; in lines 81-86 and 94-100, stub
the endpoint validator as safe so response serialization and redirect handling
remain isolated from network resolution.
- Around line 81-92: Update the webhook endpoint exercised by _post and its test
test_response_body_and_headers_are_not_echoed to return only the upstream status
code, removing request_headers, request_payload, and url from the response.
Replace the individual field exclusions with an exact response-data shape
assertion containing only status_code, while preserving the existing status and
redirect assertions.
In `@unstract/core/src/unstract/core/network/__init__.py`:
- Line 6: Update the __all__ declaration to order its exported symbols as
HTTPMethod, HttpClient, get_retry_session, and is_safe_webhook_url, satisfying
Ruff’s RUF022 ordering requirement.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 98c5cd4b-617e-466e-87c1-8b418219db5f
📒 Files selected for processing (11)
backend/notification_v2/internal_views.pybackend/notification_v2/serializers.pybackend/notification_v2/tests/__init__.pybackend/notification_v2/tests/test_webhook_ssrf.pyunstract/core/src/unstract/core/network/__init__.pyunstract/core/src/unstract/core/network/ssrf.pyunstract/core/src/unstract/core/notification_utils.pyunstract/core/tests/test_ssrf_guard.pyworkers/executor/executors/answer_prompt.pyworkers/executor/executors/postprocessor.pyworkers/tests/test_webhook_ssrf_sink.py
🚧 Files skipped from review as they are similar to previous changes (5)
- workers/executor/executors/postprocessor.py
- backend/notification_v2/serializers.py
- backend/notification_v2/internal_views.py
- unstract/core/src/unstract/core/network/ssrf.py
- workers/executor/executors/answer_prompt.py
The guard now runs inside _make_webhook_request, before the mocked requests.post. These tests use hook.example.com, which does not resolve, so two success-path assertions failed and several failure-path ones started passing for the wrong reason. Patch the guard for this class only — it tests postprocessing behaviour, not URL safety, which has its own coverage in test_webhook_ssrf_sink and unstract/core's test_ssrf_guard.
Review findings on the egress guard: - is_safe_webhook_url resolved DNS inline, and NotificationSerializer calls it while handling a request. socket.getaddrinfo honours no timeout, so a slow or hostile resolver would stall the worker serving that request. Add resolve=False for request-path callers: the syntactic checks and literal-IP check still run, and a hostname that points inward is caught at the sink, which is the real control. - The internal webhook-test endpoint returned request_headers, which carries the Authorization value built from authorization_key. Response is now status, success and url only. - Sort __all__ (RUF022). - Stub DNS in the backend webhook tests; they resolved example.com for real and would fail in an isolated runner.
Contributor
|
@greptileai please review this |
Contributor
Author
|
@greptileai re-review this PR |
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
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
2 participants
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.
What
unstract.core.network.ssrf.is_safe_webhook_url, one validator for tenant-supplied webhook URLs.postprocessor._make_webhook_requestandnotification_utils.send_webhook_request— rather than from their callers.NotificationSerializer.Why
Two paths send a request to a URL a tenant supplied, and they disagreed on what they checked.
_is_safe_public_urlread the URL withurlparse, while the transport underrequestsresolves it withurllib3. The two do not always agree on the host, so the host that was validated is not necessarily the host the socket connects to. Verified still divergent on the pinnedurllib3 2.7.0/requests 2.33.0._is_safe_public_urlran one frame up inanswer_prompt, so any new caller of_make_webhook_requestreached the network unchecked.send_webhook_requestwent straight torequests.postwith no scheme or host validation, and leftallow_redirectsat therequestsdefault ofTrue— so a redirect, not the configured URL, decided where the request landed (and 302/303 rewrites POST to GET).Notification.urlis aURLField, which validates shape only.How
urllib3keeps brackets on IPv6 literals and punycodes unicode hosts whileurlparsedoes neither. Without this,https://[2606:4700::1111]/andhttps://пример.рф/would be rejected as parser disagreements.allowed_schemesdefaults to("http", "https"); the postprocessing path passes("https",)to keep the TLS-only behaviour it already had.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Yes, in three ways, all intentional and all covered:
max_retries, which is capped at 4, then gives up), and editing such a notification returns a 400 on theurlfield until it is pointed at a public address. Deliberate — that is the behaviour being fixed — but it is a visible change for anyone who had one configured.response_body/response_headers. Any internal caller reading those fields needs updating;status_codeandsuccessare unchanged.Legitimate public webhooks are unaffected —
test_public_url_is_still_deliveredand the public-target cases pin that, including trailing-dot, uppercase, punycode and unicode-IDN hosts.Known ceiling, stated rather than implied: resolve-then-connect cannot cover a name re-resolved to an internal address between the check and the socket. The control for that is an egress policy on the worker pods, not application code.
One operational note: the validator resolves DNS inline, including inside
NotificationSerializer.validate.getaddrinfotakes no timeout, so a slow resolver stalls that request thread for the system resolver's timeout.Database Migrations
None.
Env Config
None.
Relevant Docs
None.
Related Issues or PRs
UN-3815
Dependencies Versions
Unchanged.
urllib3is already a transitive dependency ofrequests;unstract-corepinsrequests==2.33.0.Notes on Testing
unstract/core/tests/test_ssrf_guard.py— parser-disagreement cases in both directions, internal targets, disallowed schemes and credentials, public targets that must still pass (IPv6, IDN, trailing dot, uppercase), multi-answer DNS where one address is internal, and hosts that makegetaddrinforaise rather than fail to resolve. Plus the notification sink: blocked URLs never reach the network, redirects are off, public URLs still deliver.workers/tests/test_webhook_ssrf_sink.py— calls_make_webhook_requestdirectly with blocked URLs and assertsrequests.postis never reached, which is the point of moving the guard into the sink.backend/notification_v2/tests/test_webhook_ssrf.py— serializer rejects non-public URLs; the internal endpoint refuses before issuing a request and no longer echoes the body or headers.mainbefore the fix. Full backend suite: identical failure set tomain(36, all pre-existing), zero new.Screenshots
Checklist
I have read and understood the Contribution Guidelines.