UN-3815 [FIX] Validate webhook URLs in one place, at both sinks - #2214
UN-3815 [FIX] Validate webhook URLs in one place, at both sinks#2214athul-rs wants to merge 13 commits into
Conversation
|
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 |
|
| Filename | Overview |
|---|---|
| unstract/core/src/unstract/core/network/ssrf.py | Centralizes webhook URL validation and now rejects every resolved address that is not globally routable. |
| backend/notification_v2/serializers.py | Enforces required webhook destinations while avoiding revalidation of unchanged valid URLs. |
| backend/notification_v2/internal_views.py | Guards test webhook targets, disables redirects, limits success to 2xx, and removes sensitive response details. |
| unstract/core/src/unstract/core/notification_utils.py | Applies the shared SSRF validation at the notification delivery sink and disables redirects. |
| workers/executor/executors/postprocessor.py | Applies TLS-only shared URL validation directly at the postprocessing webhook sink. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Input[Tenant webhook URL] --> Parse[Require parser agreement]
Parse --> Literal{Address literal?}
Literal -->|Yes| Global[Require globally routable address]
Literal -->|No, serializer| Save[Save configuration]
Literal -->|No, delivery sink| DNS[Resolve every address]
DNS --> Global
Global -->|Safe| Send[Send without redirects]
Global -->|Unsafe| Refuse[Refuse request]
Reviews (11): Last reviewed commit: "UN-3815 [FIX] Gate the required-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}
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
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.
f481b06 to
71c9d3b
Compare
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.
|
@greptileai please review this |
|
@greptileai re-review this PR |
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Standardized review — PR #2214
Verdict: REQUEST CHANGES
Summary — Critical: 0 · High: 6 · Medium: 6 · Low: 7 · Lenses run: 16/16
Reviewed under the unstract:standard-review 16-lens rubric. Findings are deduplicated against the existing CodeRabbit and Greptile threads. Not re-raised:
- CodeRabbit's Critical on
ssrf.py:73(unbounded DNS on request threads) — you took it,resolve=Falselanded. Resolved. - Greptile's
serializers.py:61"internal hostnames pass creation validation" and your "deliberate trade" reply, which Greptile accepted. I am not reopening the trade-off — the sink is the real control and that reasoning holds. The finding I do file on that line is about the docstring contradicting itself and about there being no failure surface at all on the delivery path, neither of which the thread covered. - Greptile's
internal_views.py:364"unfollowed redirects report success" — fixed by the2xx onlychange. - CodeRabbit's
serializers.py:65PATCH re-validation — fixed by the"url" not in dataearly return. - CodeRabbit's
test_webhook_ssrf.py:62live-DNS-in-unit-tests — fixed by the stub. - CodeRabbit's
network/__init__.py:6RUF022__all__sort — done.
One correction offered rather than filed, since it is not my thread to close: Greptile's and CodeRabbit's ssrf.py:88 "non-global addresses pass validation" (e.g. 100.64.0.1) looks like a false positive. Measured on the pinned CPython 3.12.9: IPv4Address("100.64.0.1").is_global is False, as are 198.18.0.1, 192.0.0.1, fc00::1, fe80::1, ::, 240.0.0.1, ::ffff:127.0.0.1. _is_public refuses all of them today.
Lens checklist
| # | Lens | Result |
|---|---|---|
| 1 | Spec & intent | Clean |
| 2 | Architectural fit | See M1 |
| 3 | Correctness & edge cases | See H2, H3, H4, M3, M4, M5 |
| 4 | Security | See H1. Core guard verified sound — see below |
| 5 | Data integrity & migrations | N/A — no schema change |
| 6 | Concurrency | N/A |
| 7 | API & contract compatibility | Clean — no consumer of the removed response_body/response_headers/request_headers/request_payload exists in OSS or unstract-cloud; the endpoint has no caller at all outside its own test |
| 8 | Reliability & resilience | See H3, M2 |
| 9 | Performance & cost | See M6 |
| 10 | Observability | See H5 |
| 11 | Operational safety | See open question 2 |
| 12 | LLM/agent | See H4 — postprocessing sits on the prompt path |
| 13 | Testing | See H6 |
| 14 | Dependencies & build | Clean — urllib3 is undeclared in unstract/core/pyproject.toml, but network/retry.py:3 already imported it directly, so this PR does not change that |
| 15 | Code quality | Clean |
| 16 | Doc & comment accuracy | See H1, H2, M3, M4, and Lows |
Unanchored findings (outside the diff hunks)
[Medium] [Lens 2, 4] — A third webhook sink keeps its own weaker validator, against this PR's stated premise. workers/shared/patterns/notification/webhook.py:29-68, exported via shared/patterns/__init__.py:18. Its check is parsed.hostname.startswith(("10.", "172.", "192.168.")) plus a hardcoded ["localhost", "127.0.0.1", "0.0.0.0"] list — it misses 169.254.169.254, all of 100.64/10, [::1], decimal/octal encodings and every hostname that resolves inward, while over-blocking legitimate hosts across all of 172.0.0.0/8. It then posts with follow_redirects=True at :125, and its except Exception at :65-67 collapses any parse error into a generic string. I found no live caller in OSS or unstract-cloud, so this is latent rather than exploitable — but ssrf.py:5-6 says the point of this module is "so a new sink does not have to carry its own copy of the rules", and a future caller wiring into WorkerWebhookService inherits none of it. Delete it if dead, or route it through is_safe_webhook_url with follow_redirects=False.
Low (7)
unstract/core/src/unstract/core/network/ssrf.py:79-80—_is_public's docstring callsis_global"an allowlist maintained against the IANA special-purpose registries". CPython 3.12.9 implements it asself not in self._constants._public_network and not self.is_private— a denylist negating a fixed 14-entry list, updated only when a new CPython lands in the image. The second half of the docstring is correct and worth keeping:100.64.0.1really does measureis_private=False, is_global=False, is_reserved=False, so the "enumerating negative flags misses it" argument holds.unstract/core/tests/test_ssrf_guard.py:67-71— the comment "Ranges that belong to no singleis_private-style flag" is right for100.64.0.1only;198.18.0.1and192.0.0.1both measureis_private=True. Someone trimming the list "because is_private covers these" would remove the one case that justifies usingis_global.unstract/core/tests/test_ssrf_guard.py:8-9— "the resolver is exercised separately through the public-address cases" is backwards; thestub_dnsfixture isautouse=True, so those cases run against the stub. The real-resolver case istest_unresolvable_hosts_return_false_rather_than_raising, whose own docstring is accurate (verified:getaddrinfois invoked, raises during IDNA encoding, no DNS query leaves the box).unstract/core/tests/test_ssrf_guard.py:28— the fixture keyrebind.testimplies TOCTOU rebinding coverage; the test at:169covers a multi-answer RRset. Rebinding is correctly stated as a ceiling in the module docstring and not tested — the name invites the opposite conclusion.multi-answer.testwould read straight.- Both DNS stubs (
test_ssrf_guard.py:33-40,backend/notification_v2/tests/test_webhook_ssrf.py:33-42) patchsocket.getaddrinfoprocess-wide, sincessrf.pydoesimport socket. Not an isolation defect today — monkeypatch restores at teardown, both suites are green serially and under-n 4, and nothing else in either module resolves. The tell is thattest_unresolvable_hosts_...has to re-patch the real resolver back in. Patchingssrf._resolveinstead would keep the blast radius local. backend/notification_v2/tests/test_webhook_ssrf.py:60, :64—assert NotificationSerializer().validate(data) == datacompares the same object to itself; only the absence of a raised exception is being tested.- PR-narrative comments that go stale on merge:
test_webhook_ssrf.py:6and:80("used to return the response body", "had no URL check"),workers/tests/test_webhook_ssrf_sink.py:3("The URL check used to run one frame up"). RepoCLAUDE.mdasks for comments that read correctly without the change's context — stating the invariant works better.
Verified sound, for the record
The core guard holds up, and I want that on the record alongside the findings.
requests.models.PreparedRequest.prepare_url was traced on the pinned pair (requests==2.33.0, urllib3==2.7.0): urllib3 2.7 already returns an ASCII punycoded host, so unicode_is_ascii(host) is true and requests' own idna.encode path is skipped — meaning the host requests connects to really is parse_url(url).host, and the parser-agreement comparison is genuinely the right invariant. A ~4000-URL fuzz plus a 24-case hand-built corpus (backslash-userinfo in both directions, @@, %2f@, #@, ?@, ideographic and fullwidth full stops, %00, whitespace variants): every case that made requests dial evil.example was refused, and no exception escaped is_safe_webhook_url in either resolve mode.
strip("[]") on unmatched brackets could not be turned into a bypass — urllib3 raises LocationParseError on every unbalanced-bracket URL first. IPv6 literals survive normalization correctly ([::1]→::1, [::]→::, [::ffff:127.0.0.1] unchanged), and all parse in ipaddress. "".encode("idna") returns b'' without raising. Decimal, octal and hex IPv4 (2130706433, 0177.0.0.1, 0x7f.0.0.1, 127.1) are blocked at the sink: both parsers agree, ipaddress rejects them, and glibc getaddrinfo resolves all four to 127.0.0.1. NotificationViewSet is the only writer of Notification.url — no bulk_create, no admin registration, and WebhookInternalViewSet is read-only. WebhookTestSerializer.url is URLField(required=True), and DRF accepts all three INTERNAL_URLS test inputs including the backslash one, so those tests genuinely exercise the guard rather than passing on a field-level 400. No tests were deleted or weakened — the removed _is_safe_public_url had no coverage before this PR. All three new test files land in existing CI rig groups.
One curiosity, noted but not filed: 64:ff9b::7f00:1 (NAT64 well-known prefix mapping to 127.0.0.1) measures is_global == True. Only reachable with a NAT64 gateway on the pod network.
Open questions
- Is retrying a deterministic SSRF refusal intended? See M2.
- Unstract ships on-prem, where a customer's webhook target on
10.xis legitimate. There is no allowlist or opt-out —ENABLE_WEBHOOK_DELIVERYis all-or-nothing. Is breaking those deployments intended, or does this want aWEBHOOK_ALLOWED_PRIVATE_HOSTSescape hatch before it ships?
Reviewed with unstract:standard-review v0.18.1 (16-lens rubric, 4 specialist agents). Comments are advisory; event: COMMENT, no merge gate.
Reason the refusals instead of collapsing them into a bare False, and fix the cases where the guard's stated contract did not match what it ran. - The guard now returns a reason. is_safe_webhook_url keeps its boolean shape and logs the reason with the host; webhook_url_refusal exposes it so a sink can separate a resolver outage, which may clear, from a refusal that never will. A refused notification is marked non-retryable and dead-letters at once rather than re-resolving a tenant-supplied hostname on every attempt. - Normalize hosts with urllib3's own encoder. The stdlib "idna" codec is IDNA-2003, so it read fass.de where the transport dialled xn--fa-hia.de and the parser-agreement check refused every such host. - Parse legacy IPv4 literals. 2130706433, 0177.0.0.1 and 127.1 are all 127.0.0.1, but ipaddress.ip_address parses none of them, so the no-resolve path took them for hostnames and let them through. localhost is refused by name on that path too. - Stop echoing request_headers and request_payload from the webhook-test error branch: it carried back the Authorization value built from authorization_key, and a host that simply times out reaches it. - Require a URL on webhook creation. url is null=True, so DRF made it optional and a webhook could persist with no destination at all. - Drop the duplicate guard in _run_webhook_postprocess. The sink applies the identical check, so it only bought a second blocking getaddrinfo per prompt per document. - Correct the docstrings that overstated the guarantees: the check order, the resolve=False split, and the serializer's claim that internal hostnames are refused at save time. Tests: allow-path coverage on both worker sinks, so a guard that refuses everything no longer keeps the suite green; the ftp-scheme mutant that previously passed now fails three cases. Adds legacy-literal, IDN normalization, refusal-reason and retryability cases, plus a transport-failure case pinning that the credential is not echoed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…3794-webhook-egress
![uv-lock-automation[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
|
…ust create `self.partial` alone was the wrong gate. A PATCH that switches an existing URL-less notification to WEBHOOK creates a destination-less webhook just as a create does, and the partial check suppressed the validation. Now the type change is checked alongside it. Raised by Greptile on the previous push. Latent rather than live today, since NotificationType has only WEBHOOK, but the enum is written to be extended and the hole is in the fix that exists to close exactly this case. Mutation-tested: reverting to the partial-only gate fails the new case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ckfile Declaring `idna` on unstract-core invalidated the lockfile of every package that depends on it, not just the four updated with the original change. The e2e image build runs `uv sync --locked`, so `tool-sidecar` failed to build. Adds the entry to the seven remaining locks. `unstract/workflow-execution` also picked up an `unstract-sdk1` entry that was already missing before this branch — that lock was stale independently and `uv lock --check` now passes there for the first time. All 13 lockfiles verified with `uv lock --check`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…not on partial A PATCH that omitted `url` on an existing WEBHOOK row whose stored URL was already null passed validation and left the row undeliverable: neither `not self.partial` nor the type-transition check fired. Gating on `getattr(self.instance, "url", None)` alone covers every route into that state — create, switch to WEBHOOK, and an unrelated edit of a legacy row — and is simpler than the two conditions it replaces. A row that already has a URL is untouched, so the documented PATCH case still works. Raised by Greptile; its suggested predicate was better than the one it replaced. Mutation-tested: restoring the partial gate fails two cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Unstract test resultsPer-group results
Critical paths
|
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Reviewed at a8c940d. Nothing here blocks merge - the shape is right.
What is genuinely good, so it does not get lost in the nits:
- Guard moved inside each sink, so a new caller cannot reach the network unchecked.
- Parser agreement (
urlparsevsurllib3.util.parse_url) as an invariant rather than a denylist of confusing characters - it holds as either parser changes. is_globalinstead of enumeratingis_private/is_reserved/ etc. The old_is_safe_public_urlmissed 100.64.0.0/10 (RFC 6598 CGNAT), which belongs to none of those flags.- The
inet_atonfallback in_as_ipcloses a real bypass: under plainipaddress.ip_address,http://2130706433/,0177.0.0.1and127.1all read as hostnames and got resolved. All three are127.0.0.1to the resolver.
Checked the fan-out: api_webhook, slack_webhook and webhook_provider all funnel through send_webhook_request, and format_failure_result(details=result) at workers/notification/providers/webhook_provider.py:162 does carry retryable through to the non-retryable shortcut at workers/notification/tasks.py:316. That three-hop contract works today, though it is implicit.
One follow-up outside this diff: workers/executor/executors/variable_replacement.py:170 - fetch_dynamic_variable_value posts to a tenant-supplied URL with no scheme check, no host check and redirects on. Same bug class as the two sinks fixed here, and the next _is_safe_public_url-shaped hole. Worth a ticket now that the shared helper exists.
Inline comments cover: the _validate_url simplification, the undeclared urllib3 dependency, the *.localhost subtree, DNS on the request thread, the inline import requests, and comment density.
| def _validate_url(self, data): | ||
| """Reject webhook targets written as an internal address literal. | ||
|
|
||
| This is a convenience check, not the control. URLField only checks the | ||
| shape, so ``http://169.254.169.254/`` would otherwise save cleanly and | ||
| fail much later at the sink, out of the user's sight. Catching the | ||
| literal forms here turns the common mistake into a 400 at save time. | ||
|
|
||
| What it deliberately does not catch: ``resolve=False`` skips DNS, so a | ||
| *hostname* that points at an internal address — the majority of URLs — | ||
| is accepted here and refused at the sink. That is the intended split. | ||
| getaddrinfo honours no timeout, so resolving on the request thread | ||
| would let a slow or hostile resolver stall the worker serving it. The | ||
| sink resolves, and the sink is the real control. | ||
|
|
||
| Only checks a URL the caller actually sent. Re-resolving the stored one | ||
| would make an unrelated PATCH fail whenever DNS is briefly unavailable | ||
| or a legacy record predates this check. | ||
| """ | ||
| notification_type = data.get( | ||
| "notification_type", getattr(self.instance, "notification_type", None) | ||
| ) | ||
| is_webhook = notification_type == NotificationType.WEBHOOK.value | ||
|
|
||
| if "url" not in data: | ||
| # A PATCH that does not touch the URL leaves the stored one alone. | ||
| # A create has nothing to leave alone: url is null=True on the | ||
| # model, so DRF makes it optional and a webhook would otherwise | ||
| # persist with no destination at all. | ||
| # | ||
| # Gate on the stored URL, not on `partial`: a webhook with no | ||
| # destination is invalid however it got that way — a create, a | ||
| # switch to WEBHOOK, or a legacy row being edited for something | ||
| # else. A row that already has a URL is untouched, which is what | ||
| # keeps the documented PATCH case working. | ||
| if is_webhook and not getattr(self.instance, "url", None): | ||
| raise serializers.ValidationError( | ||
| {"url": "A webhook notification requires a URL."} | ||
| ) | ||
| return | ||
|
|
||
| url = data["url"] | ||
| if not url: | ||
| if is_webhook: | ||
| raise serializers.ValidationError( | ||
| {"url": "A webhook notification requires a URL."} | ||
| ) | ||
| return | ||
|
|
||
| if not is_safe_webhook_url(url, resolve=False): | ||
| raise serializers.ValidationError( | ||
| {"url": "URL must not be an internal or ambiguous address."} | ||
| ) |
There was a problem hiding this comment.
_validate_url collapses to ~9 lines of logic with identical behaviour, and it matches how _validate_api_or_pipeline / _validate_authorization just below already merge instance state.
Why the merge is safe:
data.get("url", instance.url)only falls back when the key is absent, so an explicit{"url": null}still yieldsNoneand still errors - the one case the two-branch form was guarding.""cannot reachvalidate(): the model field has noblank=True, so DRF setsallow_blank=Falseand the field errors first.- Keeping
"url" in dataon the safety check preserves the documented "don't re-check the stored URL" behaviour. Drop it and a legacy row holdinghttp://10.0.0.1/becomes un-PATCHable - including{"is_active": false}to switch it off.
Two related points on this method:
is_webhookis always true today.NotificationTypehas exactly one member and it is the model default, so both branches always raise. Fine to keep as a guard for a futureEMAIL, but it is currently unexercised.- The required-URL rule is a behaviour change beyond SSRF - a webhook could previously be created with no URL and now gets a 400. Worth adding to the "Can this PR break any existing features" section, which lists three changes and not this one.
| def _validate_url(self, data): | |
| """Reject webhook targets written as an internal address literal. | |
| This is a convenience check, not the control. URLField only checks the | |
| shape, so ``http://169.254.169.254/`` would otherwise save cleanly and | |
| fail much later at the sink, out of the user's sight. Catching the | |
| literal forms here turns the common mistake into a 400 at save time. | |
| What it deliberately does not catch: ``resolve=False`` skips DNS, so a | |
| *hostname* that points at an internal address — the majority of URLs — | |
| is accepted here and refused at the sink. That is the intended split. | |
| getaddrinfo honours no timeout, so resolving on the request thread | |
| would let a slow or hostile resolver stall the worker serving it. The | |
| sink resolves, and the sink is the real control. | |
| Only checks a URL the caller actually sent. Re-resolving the stored one | |
| would make an unrelated PATCH fail whenever DNS is briefly unavailable | |
| or a legacy record predates this check. | |
| """ | |
| notification_type = data.get( | |
| "notification_type", getattr(self.instance, "notification_type", None) | |
| ) | |
| is_webhook = notification_type == NotificationType.WEBHOOK.value | |
| if "url" not in data: | |
| # A PATCH that does not touch the URL leaves the stored one alone. | |
| # A create has nothing to leave alone: url is null=True on the | |
| # model, so DRF makes it optional and a webhook would otherwise | |
| # persist with no destination at all. | |
| # | |
| # Gate on the stored URL, not on `partial`: a webhook with no | |
| # destination is invalid however it got that way — a create, a | |
| # switch to WEBHOOK, or a legacy row being edited for something | |
| # else. A row that already has a URL is untouched, which is what | |
| # keeps the documented PATCH case working. | |
| if is_webhook and not getattr(self.instance, "url", None): | |
| raise serializers.ValidationError( | |
| {"url": "A webhook notification requires a URL."} | |
| ) | |
| return | |
| url = data["url"] | |
| if not url: | |
| if is_webhook: | |
| raise serializers.ValidationError( | |
| {"url": "A webhook notification requires a URL."} | |
| ) | |
| return | |
| if not is_safe_webhook_url(url, resolve=False): | |
| raise serializers.ValidationError( | |
| {"url": "URL must not be an internal or ambiguous address."} | |
| ) | |
| def _validate_url(self, data): | |
| """Reject internal address literals at save time; the sink is the real control. | |
| resolve=False keeps DNS off the request thread - getaddrinfo takes no | |
| timeout. A hostname pointing inward is accepted here, refused at the sink. | |
| """ | |
| notification_type = data.get( | |
| "notification_type", getattr(self.instance, "notification_type", None) | |
| ) | |
| url = data.get("url", getattr(self.instance, "url", None)) | |
| if not url: | |
| if notification_type == NotificationType.WEBHOOK.value: | |
| raise serializers.ValidationError( | |
| {"url": "A webhook notification requires a URL."} | |
| ) | |
| return | |
| # Only a URL the caller actually sent - re-checking the stored one would | |
| # 400 an unrelated PATCH on a legacy row. | |
| if "url" in data and not is_safe_webhook_url(url, resolve=False): | |
| raise serializers.ValidationError( | |
| {"url": "URL must not be an internal or ambiguous address."} | |
| ) |
| # Already present transitively via requests; declared because the egress | ||
| # guard imports it directly to match urllib3's host encoding. | ||
| "idna>=3.0", |
There was a problem hiding this comment.
ssrf.py imports urllib3.exceptions.LocationParseError and urllib3.util.parse_url directly, but only idna is declared. Same reasoning as the comment right here, so worth being consistent - otherwise a future requests that repins or drops urllib3 breaks the guard's import with nothing in this file to catch it.
| # Already present transitively via requests; declared because the egress | |
| # guard imports it directly to match urllib3's host encoding. | |
| "idna>=3.0", | |
| # Present transitively via requests; declared because the egress guard | |
| # imports them directly. | |
| "idna>=3.0", | |
| "urllib3>=2.0", |
| # hostname is accepted here and caught at the sink. | ||
| if literal is not None and not _is_public(str(literal)): | ||
| return REFUSED_INTERNAL_LITERAL | ||
| if host in _LOOPBACK_NAMES: |
There was a problem hiding this comment.
RFC 6761 reserves the whole localhost subtree, not just the apex, so http://api.localhost/hook passes this path today. The sink still catches it on resolution, so this is convenience-layer only - but _LOOPBACK_NAMES' comment claims RFC 6761 coverage, and this is the cheap half of it.
| if host in _LOOPBACK_NAMES: | |
| if host in _LOOPBACK_NAMES or host.endswith(".localhost"): |
| """Shared egress guard for user-supplied webhook URLs. | ||
|
|
||
| Both webhook sinks — prompt postprocessing and pipeline notifications — take a | ||
| URL from a tenant and hand it to ``requests``. This module is the single place | ||
| that decides whether such a URL may be dialled, so a new sink does not have to | ||
| carry its own copy of the rules. | ||
|
|
||
| Three things are checked, in the order the code runs them: | ||
|
|
||
| 1. **Scheme and userinfo.** Anything outside the caller's allowlist is refused, | ||
| as is a URL carrying credentials. | ||
| 2. **Parser agreement.** This module reads the URL with ``urllib.parse`` while | ||
| the transport underneath ``requests`` resolves it with ``urllib3``. The two | ||
| do not always agree on the host, and where they disagree the URL is refused, | ||
| because the host approved here is not the host the socket connects to. | ||
| Comparing the two parsers is an invariant rather than a list of characters to | ||
| reject, so it holds as either parser changes — provided both sides normalize | ||
| the same way, which is why ``_normalize_host`` mirrors urllib3's encoder | ||
| rather than reaching for the stdlib ``idna`` codec. | ||
| 3. **Resolved address**, when ``resolve`` is set. Every address the host | ||
| resolves to must be publicly routable. Loopback, private, link-local (which | ||
| covers the cloud metadata endpoints), reserved and multicast ranges are all | ||
| refused. | ||
|
|
||
| Every refusal carries a reason. ``is_safe_webhook_url`` answers the yes/no | ||
| question and logs the reason; ``webhook_url_refusal`` returns it, so a sink can | ||
| tell a transient resolver failure (retryable) from a URL that will never be | ||
| allowed (not retryable). | ||
|
|
||
| Note the ceiling: resolve-then-connect cannot cover a name that is re-resolved | ||
| to an internal address between this check and the socket. The control for that | ||
| is an egress policy on the worker pods, not application code. | ||
| """ |
There was a problem hiding this comment.
General ask for the PR, anchored here because this file is where it shows most: keep code comments concise and generic - a short WHY that still reads correctly a year from now, to someone without the context of this review.
This module is 113 prose lines against 100 lines of code (24 comment + 89 docstring), in a 257-line file.
The per-function comments are the good kind and should stay - each one stops a reader from "simplifying" the line back into a bug:
_normalize_hoston why the stdlibidnacodec is not interchangeable (IDNA-2003 nameprep mapsfass.dewhere the transport producesxn--fa-hia.de)._as_iponinet_atonaccepting the legacy encodings the resolver accepts._is_publiconis_globalbeing a maintained allowlist, so 100.64.0.0/10 is not missed.
This module docstring is the other kind: a numbered walkthrough of what those three functions already document, plus a restatement of the PR description. It drifts the first time a check moves or is reordered, and then it actively misleads. Same pattern at _validate_url in serializers.py (18-line docstring over 25 lines of code) and the block comment at serializers.py:67-76, which explains a decision rather than the code.
Suggest trimming each down to the one or two facts that are not already visible from the code.
| # Same guard as the delivery sinks. This endpoint is behind | ||
| # INTERNAL_SERVICE_API_KEY and not tenant-reachable, but it takes | ||
| # an arbitrary URL and so gets the same treatment. | ||
| if not is_safe_webhook_url(validated_data["url"]): |
There was a problem hiding this comment.
Note only, no change requested: this is the one call site running resolve=True on a Django request thread, which is exactly the stall _validate_url avoids with resolve=False (getaddrinfo honours no timeout, so a slow or hostile resolver holds the worker).
Unavoidable here - the endpoint actually dials the URL, so it has to resolve - and it sits behind INTERNAL_SERVICE_API_KEY. Just flagging that the concern documented in the serializer applies here unmitigated, in case a timeout-bounded resolver is worth it later.
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
||
| import requests |
There was a problem hiding this comment.
Pre-existing, but this hunk already touches the lines around it: move import requests to the top of the module. Project convention is imports at the top unless required for circular-dependency resolution, which is not the case here.
3 participants
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.