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 5 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 | 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}
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 |
|
Unstract test resultsPer-group results
Critical paths
|
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.
| # Status only. The response body and headers are not the | ||
| # caller's to read, and request_headers carried back the | ||
| # Authorization value built from authorization_key. |
There was a problem hiding this comment.
[High] [Lens 4, 16] — This fix is half-applied: the error branch still returns the Authorization header
On CodeRabbit's thread at test_webhook_ssrf.py:108 you replied "Taken. The response is now success, status_code and url only", and CodeRabbit accepted it. That is true of the success branch only.
The except requests.exceptions.RequestException branch immediately below (:378-387) is untouched and still builds:
test_result = {
"success": False,
"error": str(e),
"url": validated_data["url"],
"request_headers": headers, # <-- still here
"request_payload": validated_data["payload"], # <-- still here
}And _build_headers (:396-418) is what populates headers:
if auth_type == AuthorizationType.BEARER.value and auth_key:
headers["Authorization"] = f"Bearer {auth_key}"
elif auth_type == AuthorizationType.API_KEY.value and auth_key:
headers["Authorization"] = auth_keySo any target that times out, fails DNS, or refuses the connection returns the credential — and the new guard at :344 does not prevent it, since a perfectly public host that simply does not answer reaches that branch. This is the most common failure path for a webhook test.
Impact is bounded — the disclosure goes back to the same internal caller that supplied authorization_key in the request body, and the endpoint is behind INTERNAL_SERVICE_API_KEY. Rated High because the comment directly above states this leak is closed, which is what will stop the next reader from checking, and because the new test can never catch it: test_response_body_and_headers_are_not_echoed only sets post.return_value.status_code = 200, so it goes green with the leak intact while asserting the property as general.
Suggested fix: reduce the error branch to {"success": False, "error": str(e), "url": ...}, and add a case where requests.post raises requests.exceptions.ConnectTimeout asserting both keys are absent. If the fields are deliberately kept for debugging, the comment needs to say so.
Lens 4 · 16
|
|
||
| URLField only checks the shape, so an internal address would be stored | ||
| and only refused later at the sink, silently and out of the user's | ||
| sight. Fail here instead; the sink guard stays as 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. | ||
|
|
||
| resolve=False keeps DNS off the request thread — getaddrinfo honours no | ||
| timeout, so a slow resolver here would stall the worker serving the | ||
| request. Literal internal addresses are still refused; a hostname | ||
| pointing inward is caught at the sink, which is the real control. |
There was a problem hiding this comment.
[High] [Lens 3, 10, 16] — This docstring claims to prevent the exact silent failure that resolve=False guarantees
To be clear about scope: I am not reopening the resolve=False trade-off. Greptile raised it, you answered it, and your answer is right — getaddrinfo honours no timeout, the sink is the real control, and keeping DNS off the request thread is the correct call. That is settled.
What is not settled is this docstring, and what happens after the sink refuses.
The contradiction. :45-47 says an internal address "would be stored and only refused later at the sink, silently and out of the user's sight. Fail here instead." But :61 passes resolve=False, which by the guard's own contract (ssrf.py:145-152) accepts every hostname — it only refuses literals. Webhook targets are overwhelmingly hostnames, so for the majority case this check achieves precisely the outcome the paragraph above it says it prevents. hooks.internal.corp, or any DNS name whose A record is 10.x, saves with a 201 and shows as a healthy notification in the UI.
What happens next is the part worth fixing. I traced the delivery path: send_webhook_request returns the refusal dict → WebhookProvider.send (workers/notification/providers/webhook_provider.py:146-163, format_failure_result) → send_webhook_notification (workers/notification/tasks.py:300-310) raises → except Exception (:338-361) → logger.error → return None. Nothing is persisted and nothing reaches the user. There is no error state on the notification record, the pipeline, or any UI surface — the notification simply never arrives, forever. The clubbed path already has the machinery for this (_mark_buffer_outcome → DEAD_LETTER at tasks.py:327); the direct path does not use it.
Suggested fix: two things, neither of which requires resolving on the request thread. (1) Correct the docstring so the next reader does not believe internal hostnames are refused at save time. (2) Persist a delivery outcome on the non-clubbed path the way the clubbed one does, so a refusal is queryable rather than log-only. Optionally, do a one-shot resolving check where latency is tolerable — WebhookTestAPIView already resolves — and surface it as a warning at configuration time.
Related: :63's error string "URL must resolve to a public address" is asserted on the path that performs no resolution. internal_views.py:346 uses the identical string where it is backed by a lookup — same message, two different guarantees.
Lens 3 · 10 · 16
| except (OSError, UnicodeError): | ||
| # UnicodeError: getaddrinfo IDNA-encodes internally and raises, not | ||
| # returns, on an over-long or empty label. Unresolvable either way. | ||
| return set() |
There was a problem hiding this comment.
[High] [Lens 3, 8, 10] — A transient DNS failure is reported as a permanent security refusal, and the default config drops the notification on the first blip
_resolve swallows OSError and returns an empty set. OSError covers socket.gaierror — resolver down, timeout, EAI_AGAIN, NXDOMAIN — so a resolver outage in the worker's netns makes is_safe_webhook_url return False for every customer's perfectly public webhook. The sink then logs "Refusing webhook to a non-public or ambiguous URL" and returns error: "Webhook URL is not an allowed public destination".
A transient resolver failure and a genuinely internal target are indistinguishable anywhere in the logs — identical logger.error line at notification_utils.py:142, identical error string. On the postprocessor path (postprocessor.py:65) both produce the same logger.warning with no URL and no reason.
The retry budget does not save it. Notification.max_retries defaults to 0 (backend/notification_v2/models.py:39-41), so tasks.py:340-341 evaluates self.request.retries (0) < max_retries (0) as false — no retry at all. One DNS blip, one permanently lost notification, on default configuration. (The PR description's "the task retries up to its configured max_retries, which is capped at 4" is accurate only for a user who explicitly raised it above the default.)
Suggested fix: separate "could not resolve" from "resolved to a non-public address" — return a reason from _resolve/is_safe_webhook_url, or raise a distinct WebhookResolutionError — and let the DNS-failure case take the retryable path while the refused-as-internal case dead-letters immediately. That pairs with M2 on the retry loop: unresolvable should retry, refused should not.
Lens 3 · 8 · 10
| # Guard at the sink so it cannot be skipped by a caller. This path has | ||
| # always required TLS, so keep it to https. | ||
| if not is_safe_webhook_url(webhook_url, allowed_schemes=("https",)): | ||
| logger.warning("Postprocessing webhook URL is not allowed; skipping.") | ||
| return None |
There was a problem hiding this comment.
[High] [Lens 3, 12] — A refused URL silently degrades to "original data returned" on a run reported successful
Moving the guard into the sink is right. The return None is the problem: it is indistinguishable at :116 from the five pre-existing failure returns (:73 non-200, :78 bad JSON, :80 timeout, :82 request error, :84 unexpected), all of which postprocess_data maps to return parsed_data, highlight_data.
So a user whose webhook URL is refused sees a fully successful extraction whose configured postprocessing never ran. The structured output they get back is not the output their configuration says it should be, and nothing in the run record says so. That is a silent correctness failure on the primary flow, not merely a skipped optional step.
This is not house style, either — the immediate neighbour workers/executor/executors/lookup_enrichment.py:150-159 emits shim.stream_log(..., level=LogLevel.WARN) for a less consequential skip ("supports JSON outputs only"). The run-log channel exists and is used one frame up; this path does not use it.
Suggested fix: surface the refusal on the run's log stream the way lookup_enrichment.py:155-159 does, and distinguish "postprocessing was configured but did not run" from "postprocessing ran and made no changes" in the result metadata. Silently substituting unprocessed data for processed data on a success-reported run is the worst of the available options.
Lens 3 · 12
|
|
||
| host = _normalize_host(parsed.hostname) | ||
| if host != _normalize_host(transport_host): | ||
| logger.warning("Refusing webhook URL: validator and transport disagree on host") |
There was a problem hiding this comment.
[High] [Lens 10] — Eight distinct rejection reasons collapse into a bare False; one logs, and without the host
is_safe_webhook_url returns False at :115 (empty url), :123 (scheme), :128 (credentials), :135 (LocationParseError), :143 (empty host), :152 (literal internal on the no-resolve path), :158 (unresolvable) and :160 (non-public address). Exactly one of those logs — this line — and it carries neither the URL nor the host.
Failure mode: support receives "my webhook stopped firing after the upgrade". The available evidence is "Refusing webhook to a non-public or ambiguous URL" (no URL, no reason) on the notification path and "Postprocessing webhook URL is not allowed; skipping." (no URL, no reason) at postprocessor.py:65 and answer_prompt.py:242. There is no way to tell whether the cause was http vs https, a @ in the netloc, a CGNAT address, a parser disagreement, or the resolver being down — five entirely different remediations.
This matters more than it normally would because the PR widens the rule set relative to the old check: is_global newly refuses 100.64.0.0/10, 198.18.0.0/15 and 192.0.0.0/24 where the previous five-flag test allowed them, and "@" in parsed.netloc (:127) is newly refused. Customers on those ranges break on upgrade with no diagnosable log line.
Suggested fix: return a reason alongside the boolean — tuple[bool, str] or a small enum — and have each sink log it. backend/notification_v2/helper.py:61-66 already has webhook_url_hash for logging a target without leaking query-string tokens. At minimum, log the parsed host at the refusal sites.
Lens 10
| urllib3 keeps the brackets on an IPv6 literal and punycodes a unicode host; | ||
| ``urlparse`` does neither. Comparing the raw strings would refuse both of | ||
| those legitimate URLs. | ||
| """ | ||
| if not host: | ||
| return "" | ||
| host = host.strip().strip("[]").rstrip(".").lower() | ||
| try: | ||
| return host.encode("idna").decode("ascii") |
There was a problem hiding this comment.
[Medium] [Lens 3] — _normalize_host uses IDNA-2003 while urllib3 uses UTS-46, so legitimate IDN hosts are refused
The docstring's justification for normalizing is correct, and the two claims it makes about urllib3 are accurate (verified: [::1] vs ::1, xn--e1afmkfd.xn--p1ai vs пример.рф). The problem is which encoder is used.
str.encode("idna") is stdlib IDNA-2003 with nameprep — ß→ss, ς→σ, NFKC folding. urllib3 2.7.0's _idna_encode uses the idna package with strict=True, std3_rules=True, i.e. UTS-46 non-transitional. Wherever the two mappings differ, the parser-agreement check at :138 fires and refuses the URL.
Measured with DNS stubbed to a public address:
| host | validator sees | transport sees | result |
|---|---|---|---|
faß.de |
fass.de |
xn--fa-hia.de |
refused |
σόλος.gr |
xn--wxaikc6b.gr |
xn--wxaijb9b.gr |
refused |
пример.рф |
xn--e1afmkfd.xn--p1ai |
same | allowed |
The suite only covers пример.рф, which is a case where both mappings coincide — so the gap is invisible to CI.
This fails closed, so it is a false reject, not a bypass. But it also undercuts the module docstring's claim at :14-15 that comparing the two parsers "is an invariant rather than a list of characters to reject, so it holds as either parser changes" — as written, _normalize_host is coupled to stdlib's IDNA version and must track urllib3's encoder.
Suggested fix: normalize with the library the transport uses — idna.encode(host, uts46=True), already a hard requests dependency — falling back to the raw host on idna.IDNAError. That makes the comparison like-for-like and restores the invariant the docstring claims.
Lens 3 · 16
| if not resolve: | ||
| # No DNS on this path. A literal address is still checked, since that | ||
| # needs no lookup and is how most internal targets are written. | ||
| try: | ||
| ipaddress.ip_address(host) | ||
| except ValueError: | ||
| return True | ||
| return _is_public(host) |
There was a problem hiding this comment.
[Medium] [Lens 16] — "a literal internal IP is still refused" is not true for non-dotted-quad encodings
The resolve arg docstring at :356-358 and the serializer's at serializers.py:55-56 both promise that literal internal addresses are still refused on this path. That holds only for what ipaddress.ip_address() can parse — and it rejects decimal, octal, hex and short-form IPv4.
Measured:
is_safe_webhook_url("http://2130706433/hook", resolve=False) # True
is_safe_webhook_url("http://0177.0.0.1/hook", resolve=False) # True
is_safe_webhook_url("http://127.1/hook", resolve=False) # True
is_safe_webhook_url("http://localhost/hook", resolve=False) # TrueAll four are 127.0.0.1. They fall through the except ValueError: return True at :150-151 and persist through the notification API.
Not a live SSRF — I confirmed the sink catches every one of them: both parsers agree on these hosts, and glibc getaddrinfo resolves all four to 127.0.0.1, so _is_public refuses. The finding is that the stated contract is wrong, and TestWithoutResolution::test_literal_and_syntactic_cases_still_refused uses only dotted-quad and bracketed-IPv6 forms, so nothing pins it either way.
Suggested fix: either narrow both docstrings to "dotted-quad and bracketed IPv6 literals", or canonicalize all-numeric hosts (ipaddress.IPv4Address(int(host, 0))) and reject localhost by name on this path. The first is fine — the sink is the real control — but the sentence should match what runs.
Lens 16
| if "url" not in data: | ||
| return | ||
| url = data["url"] | ||
| if url and not is_safe_webhook_url(url, resolve=False): |
There was a problem hiding this comment.
[Medium] [Lens 3] — A create that omits url persists a webhook notification with no destination
The "url" not in data early return is correct for the PATCH case it was written for. It also lets a create through unchecked.
Notification.url is models.URLField(null=True) (backend/notification_v2/models.py:27). DRF's get_field_kwargs maps a null=True model field to required=False, allow_null=True, and Meta.fields = "__all__" with validators = [] adds nothing back. So a POST that omits url hits the early return at :59, and one that sends "url": null is short-circuited by if url and ... at :61. Either way a webhook notification is persisted with no destination and returns 201.
At dispatch, is_safe_webhook_url(None) returns False at ssrf.py:115, and the user's log says the URL "is not an allowed public destination" — for a URL that was never set. I found no upstream check in backend/notification_v2/views.py that would block it first.
Suggested fix: require url when notification_type == WEBHOOK, in the same validate() that already cross-validates api/pipeline and authorization. Keep the "url" not in data early return for the documented PATCH case, but gate it on self.partial (or on the instance already having a URL) so a create cannot use it.
Lens 3
| logger.warning("Postprocessing webhook enabled but URL missing; skipping.") | ||
| return parsed_data, None | ||
| if not _is_safe_public_url(webhook_url): | ||
| if not is_safe_webhook_url(webhook_url, allowed_schemes=("https",)): |
There was a problem hiding this comment.
[Medium] [Lens 9, 15] — This check is now fully redundant, and doubles DNS resolution on a per-prompt path
_run_webhook_postprocess calls the guard here, then unconditionally calls postprocess_data → _make_webhook_request, which calls the identical guard with the identical allowed_schemes=("https",) at postprocessor.py:64. Two blocking getaddrinfo calls per webhook.
I checked for a divergence and there is none: same schemes, same resolve=True, and both callers return the original data on refusal, so the outcomes are identical. The only difference is TOCTOU between the two lookups, which does not change the observable result.
The cost is that this path runs per prompt per document (reachable from lookup_enrichment.py:167), and the guard's own docstring at ssrf.py:98-102 warns that getaddrinfo honours no timeout — so this doubles exposure to exactly the stall the module warns about, on the executor hot path.
Moving the guard into the sink was the stated point of this change (workers/tests/test_webhook_ssrf_sink.py:3-5 — "It now sits in the sink"). Deleting :241-243 completes that; as it stands the check was duplicated rather than moved, and the test file's comment describes an end state the code has not reached.
Lens 9 · 15 · 16
| Three things are checked, in order: | ||
|
|
||
| 1. **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. | ||
| 2. **Scheme and userinfo.** Anything outside the caller's allowlist is refused, | ||
| as is a URL carrying credentials. | ||
| 3. **Resolved address.** 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. |
There was a problem hiding this comment.
[Medium] [Lens 16] — The docstring lists the checks in an order the code does not follow, and states step 3 unconditionally
The docstring says the checks run "in order: 1. Parser agreement. 2. Scheme and userinfo. 3. Resolved address."
Actual order in is_safe_webhook_url: scheme at :122, userinfo at :127, parser agreement at :130-140, resolution at :145+. Items 1 and 2 are swapped.
This is not merely cosmetic here, because of the logging situation: the parser-agreement check at :139 emits the module's only diagnostic log line. A reader trusting the stated order will assume a refused URL got past the scheme and credential checks and will mis-diagnose which check fired — and since no other reason is logged at all (see the :136-139 finding), that stated order is the only map they have.
Item 3 is also stated unconditionally, but the entire resolution step is skipped when resolve=False (:145-152) — which is how NotificationSerializer calls it.
Suggested fix: renumber to match the code, and qualify item 3 with "(when resolve is set)". Alternatively move the parser-agreement check first so the log line actually leads — but the docstring is the cheaper fix.
Lens 16
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.