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 11 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 | Introduces the shared webhook URL guard and now uses global-routability classification, resolving the previously reported special-use address gap. |
| backend/notification_v2/internal_views.py | Guards test requests, disables redirects, limits response disclosure, and correctly marks only 2xx responses as successful. |
| backend/notification_v2/serializers.py | Adds save-time URL validation while preserving unrelated PATCH behavior and rejecting URL-less webhook creation or transitions. |
| unstract/core/src/unstract/core/notification_utils.py | Applies SSRF validation inside the notification sink, disables redirects, and distinguishes retryable DNS failures from permanent refusals. |
| workers/executor/executors/postprocessor.py | Moves HTTPS webhook validation into the postprocessing network sink so direct callers cannot bypass it. |
| workers/notification/tasks.py | Dead-letters permanent URL refusals without consuming futile delivery retries. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Tenant[Tenant webhook configuration] --> Serializer[NotificationSerializer]
Serializer -->|Literal and syntax checks| Store[(Notification configuration)]
Store --> Worker[Notification worker]
Worker --> Guard[Shared SSRF guard]
Prompt[Prompt postprocessor] --> Guard
Test[Internal webhook-test endpoint] --> Guard
Guard -->|Parser agreement, scheme, credentials, DNS, global IP| Public[Public webhook destination]
Guard -->|Refused| Failure[Reject or dead-letter]
Reviews (9): Last reviewed commit: "UN-3815 [FIX] Propagate the unstract-cor..." | 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.
|
|
||
| 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
There was a problem hiding this comment.
Taking (1) — the docstring did claim the outcome that resolve=False guarantees it cannot deliver. Rewritten to lead with what the check actually is: a convenience check that turns the common mistake (an internal address literal) into a 400 at save time, explicitly not the control. It now states that a hostname pointing inward is accepted here by design and refused at the sink, and why DNS stays off the request thread.
The error string is fixed in the same pass: this path now says "URL must not be an internal or ambiguous address", leaving "URL must resolve to a public address" to internal_views.py:346, where a lookup actually backs it. Same wording for two different guarantees was the fair part of that observation.
Not taking (2) in this PR. Persisting a delivery outcome on the non-clubbed path is a real gap and the trace is right, but it is a pre-existing one — that path had no error state before this change and this PR does not make it worse. Building it here means adding notification state modelling to an SSRF fix, and it wants its own review. Worth a follow-up ticket.
What did land, which covers part of the concern: a refusal is now distinguishable from a transient failure at the sink and is marked non-retryable, so it dead-letters immediately via _mark_buffer_outcome on the clubbed path instead of burning the retry budget first.
| # 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
There was a problem hiding this comment.
The failure mode is real and described accurately — a refused URL returns unprocessed data on a run reported successful, and that is worse than a skipped optional step.
Not fixing it in this PR, deliberately, and this one is worth arguing rather than quietly deferring. The return None at :66 is the sixth member of a family: :73 non-200, :78 bad JSON, :80 timeout, :82 request error and :84 unexpected all reach the same return parsed_data, highlight_data at :116, and all five predate this change. As the finding itself notes, the new one is indistinguishable from them. Fixing only the SSRF member would make the guard's refusal louder than a postprocessing server returning 500 — inconsistent in a way that is arguably more confusing than the current uniform silence.
The right fix is one change that gives all six a reported outcome, distinguishing "postprocessing was configured but did not run" from "ran and made no changes", surfaced on the run log the way lookup_enrichment.py:155-159 does. That needs a signal threaded from the sink up to the frame that holds the shim, which is a behavioural change to the executor's result contract and wants reviewing on its own. Follow-up ticket rather than this PR.
What did improve here: the guard logs its refusal reason and the host at the point of refusal, so "my webhook stopped firing after the upgrade" is now diagnosable even though the run still reports success. Leaving this thread open for the follow-up decision.
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>
| notification_type = data.get( | ||
| "notification_type", getattr(self.instance, "notification_type", None) | ||
| ) |
There was a problem hiding this comment.
When a creation request omits both notification_type and url, this fallback treats the type as None, so URL validation passes before the model defaults the notification to WEBHOOK, causing an undeliverable notification with url=None to be persisted.
| notification_type = data.get( | |
| "notification_type", getattr(self.instance, "notification_type", None) | |
| ) | |
| notification_type = data.get( | |
| "notification_type", | |
| getattr( | |
| self.instance, | |
| "notification_type", | |
| NotificationType.WEBHOOK.value, | |
| ), | |
| ) |
Knowledge Base Used: Usage Tracking, Dashboard Metrics, and Notifications
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/notification_v2/serializers.py
Line: 61-63
Comment:
**Default webhook permits missing URL**
When a creation request omits both `notification_type` and `url`, this fallback treats the type as `None`, so URL validation passes before the model defaults the notification to `WEBHOOK`, causing an undeliverable notification with `url=None` to be persisted.
```suggestion
notification_type = data.get(
"notification_type",
getattr(
self.instance,
"notification_type",
NotificationType.WEBHOOK.value,
),
)
```
**Knowledge Base Used:** [Usage Tracking, Dashboard Metrics, and Notifications](https://app.greptile.com/zipstack/-/custom-context/knowledge-base/zipstack/unstract/-/docs/observability-usage.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
Checked this one against the running code rather than the diff, and it is not reachable.
notification_type is an explicitly declared serializer field:
notification_type = serializers.ChoiceField(choices=NotificationType.choices())ChoiceField defaults to required=True, and an explicit declaration overrides whatever
ModelSerializer would have inferred from the model's default=. So a create that omits it never
reaches validate() — DRF field validation rejects it first. Measured:
notification_type required = True
is_valid = False
errors = {'notification_type': [ErrorDetail(string='This field is required.', code='required')]}
The premise about the model is correct — Notification.notification_type does default to
WEBHOOK — but the model default is never consulted, because the request cannot get past field
validation without supplying the value.
That makes the getattr(self.instance, "notification_type", None) fallback reachable only on
PATCH, where an instance exists and supplies the real type, which is the case it was written for.
Defaulting it to WEBHOOK would encode a state the serializer cannot be in.
Worth recording the coupling, though: this holds because the field is required. If
notification_type is ever given required=False to mirror the model default, the required-URL
check has to be revisited at the same time, and the suggested fallback becomes correct.
…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>
|
Unstract test resultsPer-group results
Critical paths
|
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.