M6: Replace webhook SSRF substring blocklist with real IP/CIDR + DNS-resolution checks #203

New Issue

2026-07-28T14:04:50Z

KofTwentyTwo commented

2026-07-28 14:04:50 +00:00

(Migrated from github.com)

Context

Story for milestone M6. Read docs/STORY-WORKFLOW.md first — it defines the process, gates, and patterns; this issue defines only the specific work. crates/nuncio-filter/src/webhook.rs (WebhookDispatcher) dispatches the CALL WEBHOOK filter action as an outbound HTTP POST. Its "SSRF defense" is implemented in crates/nuncio-filter/src/validator.rs::NsqlValidator::pass6_action_security (validator.rs:271-287). Verified in the tree — the entire check is a case-insensitive substring match on the raw URL string:

if opts.block_private_webhooks {
    let url_lower = url.to_lowercase();
    if url_lower.contains("127.0.0.1")
        || url_lower.contains("localhost")
        || url_lower.contains("169.254.169.254")
        || url_lower.contains("0.0.0.0")
    {
        return Err(ValidationError::ActionSecurityViolation(...));
    }
}

This is trivially bypassable: any RFC1918 address (10.x.x.x, 172.16-31.x.x, 192.168.x.x), any other 169.254.0.0/16 link-local address (the substring only catches the one literal cloud-metadata IP, not the whole /16), decimal/octal/hex IP-literal encodings (e.g. http://2130706433/ = 127.0.0.1), http://127.1/, any IPv6 loopback/ULA/link-local address (http://[::1]/, http://[fc00::1]/, http://[fe80::1]/), and any DNS name that merely resolves to a private/loopback IP (DNS rebinding) all pass this check untouched. The ValidationOptions::block_private_webhooks doc comment (validator.rs:36) even claims "Pass 6 pre-flight DNS IP blacklisting for webhooks" — the DNS resolution it describes does not exist in the implementation (reconciles the reported webhook-SSRF substring-blocklist defect).

Goal

Replace the substring blocklist with real IP-literal parsing + CIDR-range checks covering RFC1918, loopback, link-local (which subsumes the cloud-metadata address), IPv6 loopback/ULA/link-local, and unspecified/multicast/broadcast addresses — plus a DNS-resolution step at dispatch time so a hostname that resolves to any of those ranges is also rejected, closing the gap the existing doc comment already claims to close but doesn't.

Files to touch / patterns to mirror

crates/nuncio-filter/src/validator.rs:271-287 (pass6_action_security, CallWebhook arm) — replace the substring check with real parsing:
1. Parse url with reqwest::Url::parse(url) (no new dependency needed — nuncio-filter already depends on reqwest, Cargo.toml:26, and reqwest::Url is a re-export of the url crate's Url type).
2. Extract the host via .host_str(). If it parses as a literal IP address (host.parse::<std::net::IpAddr>() succeeds), run it through the new blocked-range predicate (below) and reject if blocked.
3. If the host is a name (not a literal IP), this sync Pass-6 check cannot resolve DNS (it's not async) — leave hostname-based SSRF-via-DNS-rebind to the dispatch-time check described below; Pass 6 at rule-creation time only needs to catch literal-IP bypasses (which is what the current substring check was trying, and failing, to do).
crates/nuncio-filter/src/validator.rs (new private function, e.g. fn is_blocked_webhook_target(ip: std::net::IpAddr) -> bool) — implement using stable, non-unsafe std::net methods plus manual octet/segment masking where a needed range has no stable helper method on the pinned toolchain (verify against rust-toolchain.toml = 1.97.1 which stable Ipv4Addr/Ipv6Addr methods are available; do not assume — check the actual stdlib docs for that version before relying on e.g. is_unique_local()/is_unicast_link_local() for IPv6, which stabilized relatively recently). At minimum, block:
- IPv4: loopback (127.0.0.0/8), RFC1918 private (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), link-local (169.254.0.0/16 — this alone covers the cloud-metadata address 169.254.169.254 without hardcoding it), unspecified (0.0.0.0/8), broadcast, multicast.
- IPv6: loopback (::1), unique-local (fc00::/7), link-local (fe80::/10), unspecified (::), multicast.
- IPv4-mapped IPv6 addresses (::ffff:0:0/96) MUST be unwrapped to their embedded IPv4 address and re-checked against the IPv4 rules above — an attacker supplying http://[::ffff:127.0.0.1]/ must not bypass the IPv4 loopback check.
crates/nuncio-filter/src/webhook.rs:29-98 (WebhookDispatcher) — add a DNS-resolution SSRF check immediately before the actual HTTP POST (webhook.rs:84-95), using tokio::net::lookup_host (already available: tokio workspace dep has the "full" feature, Cargo.toml:37) on the URL's (host, port). Resolve, then reject the dispatch with WebhookError::SecurityViolation if any resolved address matches is_blocked_webhook_target (reuse the function from validator.rs, making it pub(crate) or moving it somewhere both modules can reach — validator.rs is a reasonable home since webhook.rs already imports from crate::validator, webhook.rs:4).
crates/nuncio-filter/src/webhook.rs:121-144 (#[cfg(test)] mod tests) — the existing test_blocked_private_ip_webhook test (webhook.rs:125-143) must still pass unchanged; add the new tests per Acceptance Criteria alongside it.

Implementation steps

Write is_blocked_webhook_target(ip: IpAddr) -> bool in validator.rs covering the ranges listed above, with the IPv4-mapped-IPv6 unwrap handled first.
Rewrite the CallWebhook arm of pass6_action_security to parse the URL, extract the host, and — if it's a literal IP — call is_blocked_webhook_target. Preserve the existing http:///https:// scheme check (validator.rs:272-276) unchanged.
Add the dispatch-time DNS-resolution check in WebhookDispatcher::dispatch_with_options, after the existing pass6_action_security call (webhook.rs:60-61) and before the HTTP POST (webhook.rs:84).
Document the residual DNS-rebind race explicitly in a code comment on the new dispatch-time check: resolving the hostname and checking the IP, then having reqwest independently re-resolve and connect moments later, leaves a narrow TOCTOU window where a malicious DNS server could return a safe IP to our check and a private IP to reqwest's connect. Note this as a known limitation rather than silently claiming full protection; closing it completely would require a custom resolver/connector pinning the exact checked IP for the actual request, which is out of scope for this story's size (S/M) — call this out in the PR description as a candidate follow-up if the PO wants it hardened further.
Add tests per Acceptance Criteria.

Acceptance criteria

is_blocked_webhook_target (or equivalently-named function) rejects, at minimum: 127.0.0.1, 10.1.2.3, 172.16.0.1, 172.31.255.255 (in-range) but allows 172.32.0.1 (just outside RFC1918's 172.16.0.0/12, a real bypass case for a naive prefix check — proves the CIDR math is correct, not approximate), 192.168.1.1, 169.254.169.254, 169.254.0.1 (any address in the /16, not just the one hardcoded metadata IP), 0.0.0.0, ::1, fc00::1, fe80::1, and ::ffff:127.0.0.1 (IPv4-mapped bypass attempt).
is_blocked_webhook_target allows ordinary public IPs (e.g. 93.184.216.34, 2606:2800:220:1:248:1893:25c8:1946 — example.com's addresses, or any other clearly-public literal) — a false-positive-blocking regression is also a bug.
pass6_action_security rejects CALL WEBHOOK actions whose URL host is a blocked literal IP (replacing the old substring test — update/extend test_pass6_action_security_validation, validator.rs:349-359, plus add cases for the RFC1918/IPv6/IPv4-mapped bypasses above that the old substring check missed).
WebhookDispatcher::dispatch/dispatch_with_options rejects a webhook whose hostname resolves (via a test-injectable or wiremock-free deterministic method — since real DNS resolution of a fabricated hostname can't be mocked with wiremock, prefer testing the resolved-IP-checking logic as a unit test against literal IP inputs, and/or use a hosts-file-independent technique such as directly unit-testing the extracted resolve+check helper function with synthetic IpAddr values rather than performing a live DNS lookup in the test) to a private/loopback address. Live network calls (including live DNS resolution in tests) are forbidden — structure the new dispatch-time check so its IP-blocking logic is unit-testable without performing a real lookup (e.g. factor "resolve host to IPs" and "reject if any IP is blocked" into separate, separately-testable pieces).
The existing test_blocked_private_ip_webhook test (webhook.rs:125-143) still passes unchanged.
cargo verify green (fmt + clippy -D warnings all-targets + test-all), 0 ignored. No live network calls, no live DNS resolution, in any test.

Definition of done & process

Follow docs/STORY-WORKFLOW.md: no fabricated data; comment convention (intent-only, no story/GH/phase refs in .rs/.proto); mock-only tests. Branch story/m6-webhook-ssrf off dev; PR to dev with the gate output pasted as evidence; the PO reviews before the issue is closed.

Dependencies / notes

Not a [PROTO] story. No new crate dependency required — use reqwest::Url (already a transitive re-export via the existing reqwest dependency) for URL/host parsing and std::net::{IpAddr, Ipv4Addr, Ipv6Addr} for the range checks; do not add ipnetwork/cidr/url as new direct dependencies unless a genuine need arises during implementation (if so, justify it in the PR). No file overlap with other M6 stories — safe to run fully in parallel. Full DNS-rebind pinning (custom resolver) is an explicitly out-of-scope stretch goal per the Implementation Steps note above; file a follow-up issue if the PO wants it. Size: M.

## Context Story for milestone M6. **Read [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md) first** — it defines the process, gates, and patterns; this issue defines only the specific work. `crates/nuncio-filter/src/webhook.rs` (`WebhookDispatcher`) dispatches the `CALL WEBHOOK` filter action as an outbound HTTP POST. Its "SSRF defense" is implemented in `crates/nuncio-filter/src/validator.rs::NsqlValidator::pass6_action_security` (`validator.rs:271-287`). Verified in the tree — the entire check is a case-insensitive **substring match on the raw URL string**: ```rust if opts.block_private_webhooks { let url_lower = url.to_lowercase(); if url_lower.contains("127.0.0.1") || url_lower.contains("localhost") || url_lower.contains("169.254.169.254") || url_lower.contains("0.0.0.0") { return Err(ValidationError::ActionSecurityViolation(...)); } } ``` This is trivially bypassable: any RFC1918 address (`10.x.x.x`, `172.16-31.x.x`, `192.168.x.x`), any other `169.254.0.0/16` link-local address (the substring only catches the one literal cloud-metadata IP, not the whole `/16`), decimal/octal/hex IP-literal encodings (e.g. `http://2130706433/` = `127.0.0.1`), `http://127.1/`, any IPv6 loopback/ULA/link-local address (`http://[::1]/`, `http://[fc00::1]/`, `http://[fe80::1]/`), and any DNS name that merely *resolves* to a private/loopback IP (DNS rebinding) all pass this check untouched. The `ValidationOptions::block_private_webhooks` doc comment (`validator.rs:36`) even claims "Pass 6 pre-flight **DNS** IP blacklisting for webhooks" — the DNS resolution it describes does not exist in the implementation (reconciles the reported webhook-SSRF substring-blocklist defect). ## Goal Replace the substring blocklist with real IP-literal parsing + CIDR-range checks covering RFC1918, loopback, link-local (which subsumes the cloud-metadata address), IPv6 loopback/ULA/link-local, and unspecified/multicast/broadcast addresses — plus a DNS-resolution step at dispatch time so a hostname that resolves to any of those ranges is also rejected, closing the gap the existing doc comment already claims to close but doesn't. ## Files to touch / patterns to mirror - `crates/nuncio-filter/src/validator.rs:271-287` (`pass6_action_security`, `CallWebhook` arm) — replace the substring check with real parsing: 1. Parse `url` with `reqwest::Url::parse(url)` (no new dependency needed — `nuncio-filter` already depends on `reqwest`, `Cargo.toml:26`, and `reqwest::Url` is a re-export of the `url` crate's `Url` type). 2. Extract the host via `.host_str()`. If it parses as a literal IP address (`host.parse::<std::net::IpAddr>()` succeeds), run it through the new blocked-range predicate (below) and reject if blocked. 3. If the host is a name (not a literal IP), this sync Pass-6 check cannot resolve DNS (it's not async) — leave hostname-based SSRF-via-DNS-rebind to the dispatch-time check described below; Pass 6 at rule-creation time only needs to catch literal-IP bypasses (which is what the current substring check was trying, and failing, to do). - `crates/nuncio-filter/src/validator.rs` (new private function, e.g. `fn is_blocked_webhook_target(ip: std::net::IpAddr) -> bool`) — implement using **stable, non-`unsafe`** `std::net` methods plus manual octet/segment masking where a needed range has no stable helper method on the pinned toolchain (verify against `rust-toolchain.toml` = 1.97.1 which stable `Ipv4Addr`/`Ipv6Addr` methods are available; do not assume — check the actual stdlib docs for that version before relying on e.g. `is_unique_local()`/`is_unicast_link_local()` for IPv6, which stabilized relatively recently). At minimum, block: - IPv4: loopback (`127.0.0.0/8`), RFC1918 private (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`), link-local (`169.254.0.0/16` — this alone covers the cloud-metadata address `169.254.169.254` without hardcoding it), unspecified (`0.0.0.0/8`), broadcast, multicast. - IPv6: loopback (`::1`), unique-local (`fc00::/7`), link-local (`fe80::/10`), unspecified (`::`), multicast. - IPv4-mapped IPv6 addresses (`::ffff:0:0/96`) MUST be unwrapped to their embedded IPv4 address and re-checked against the IPv4 rules above — an attacker supplying `http://[::ffff:127.0.0.1]/` must not bypass the IPv4 loopback check. - `crates/nuncio-filter/src/webhook.rs:29-98` (`WebhookDispatcher`) — add a DNS-resolution SSRF check immediately before the actual HTTP POST (`webhook.rs:84-95`), using `tokio::net::lookup_host` (already available: `tokio` workspace dep has the `"full"` feature, `Cargo.toml:37`) on the URL's `(host, port)`. Resolve, then reject the dispatch with `WebhookError::SecurityViolation` if **any** resolved address matches `is_blocked_webhook_target` (reuse the function from `validator.rs`, making it `pub(crate)` or moving it somewhere both modules can reach — `validator.rs` is a reasonable home since `webhook.rs` already imports from `crate::validator`, `webhook.rs:4`). - `crates/nuncio-filter/src/webhook.rs:121-144` (`#[cfg(test)] mod tests`) — the existing `test_blocked_private_ip_webhook` test (`webhook.rs:125-143`) must still pass unchanged; add the new tests per Acceptance Criteria alongside it. ## Implementation steps 1. Write `is_blocked_webhook_target(ip: IpAddr) -> bool` in `validator.rs` covering the ranges listed above, with the IPv4-mapped-IPv6 unwrap handled first. 2. Rewrite the `CallWebhook` arm of `pass6_action_security` to parse the URL, extract the host, and — if it's a literal IP — call `is_blocked_webhook_target`. Preserve the existing `http://`/`https://` scheme check (`validator.rs:272-276`) unchanged. 3. Add the dispatch-time DNS-resolution check in `WebhookDispatcher::dispatch_with_options`, after the existing `pass6_action_security` call (`webhook.rs:60-61`) and before the HTTP POST (`webhook.rs:84`). 4. Document the residual DNS-rebind race explicitly in a code comment on the new dispatch-time check: resolving the hostname and checking the IP, then having `reqwest` independently re-resolve and connect moments later, leaves a narrow TOCTOU window where a malicious DNS server could return a safe IP to our check and a private IP to `reqwest`'s connect. Note this as a known limitation rather than silently claiming full protection; closing it completely would require a custom resolver/connector pinning the exact checked IP for the actual request, which is out of scope for this story's size (S/M) — call this out in the PR description as a candidate follow-up if the PO wants it hardened further. 5. Add tests per Acceptance Criteria. ## Acceptance criteria - [ ] `is_blocked_webhook_target` (or equivalently-named function) rejects, at minimum: `127.0.0.1`, `10.1.2.3`, `172.16.0.1`, `172.31.255.255` (in-range) but allows `172.32.0.1` (just outside RFC1918's `172.16.0.0/12`, a real bypass case for a naive prefix check — proves the CIDR math is correct, not approximate), `192.168.1.1`, `169.254.169.254`, `169.254.0.1` (any address in the `/16`, not just the one hardcoded metadata IP), `0.0.0.0`, `::1`, `fc00::1`, `fe80::1`, and `::ffff:127.0.0.1` (IPv4-mapped bypass attempt). - [ ] `is_blocked_webhook_target` allows ordinary public IPs (e.g. `93.184.216.34`, `2606:2800:220:1:248:1893:25c8:1946` — example.com's addresses, or any other clearly-public literal) — a false-positive-blocking regression is also a bug. - [ ] `pass6_action_security` rejects `CALL WEBHOOK` actions whose URL host is a blocked literal IP (replacing the old substring test — update/extend `test_pass6_action_security_validation`, `validator.rs:349-359`, plus add cases for the RFC1918/IPv6/IPv4-mapped bypasses above that the old substring check missed). - [ ] `WebhookDispatcher::dispatch`/`dispatch_with_options` rejects a webhook whose hostname resolves (via a test-injectable or wiremock-free deterministic method — since real DNS resolution of a fabricated hostname can't be mocked with `wiremock`, prefer testing the resolved-IP-checking logic as a unit test against literal IP inputs, and/or use a hosts-file-independent technique such as directly unit-testing the extracted resolve+check helper function with synthetic `IpAddr` values rather than performing a live DNS lookup in the test) to a private/loopback address. Live network calls (including live DNS resolution in tests) are forbidden — structure the new dispatch-time check so its IP-blocking logic is unit-testable without performing a real lookup (e.g. factor "resolve host to IPs" and "reject if any IP is blocked" into separate, separately-testable pieces). - [ ] The existing `test_blocked_private_ip_webhook` test (`webhook.rs:125-143`) still passes unchanged. - [ ] `cargo verify` green (fmt + clippy `-D warnings` all-targets + test-all), 0 ignored. No live network calls, no live DNS resolution, in any test. ## Definition of done & process Follow [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md): no fabricated data; comment convention (intent-only, no story/GH/phase refs in `.rs`/`.proto`); mock-only tests. Branch `story/m6-webhook-ssrf` off `dev`; PR to `dev` with the gate output pasted as evidence; the PO reviews before the issue is closed. ## Dependencies / notes Not a [PROTO] story. No new crate dependency required — use `reqwest::Url` (already a transitive re-export via the existing `reqwest` dependency) for URL/host parsing and `std::net::{IpAddr, Ipv4Addr, Ipv6Addr}` for the range checks; do not add `ipnetwork`/`cidr`/`url` as new direct dependencies unless a genuine need arises during implementation (if so, justify it in the PR). No file overlap with other M6 stories — safe to run fully in parallel. Full DNS-rebind pinning (custom resolver) is an explicitly out-of-scope stretch goal per the Implementation Steps note above; file a follow-up issue if the PO wants it. Size: M.

KofTwentyTwo commented

2026-07-30 18:10:09 +00:00

(Migrated from github.com)

Delivered via PR #243 (squash-merged to dev). Replaced the substring SSRF blocklist with is_blocked_webhook_target (exact CIDR: RFC1918/loopback/link-local incl. metadata IP/unspecified/broadcast/multicast for IPv4; loopback/ULA fc00::/7/link-local fe80::/10/unspecified/multicast for IPv6; IPv4-mapped unwrapped first) in pass6 + a dispatch-time DNS-resolution check. Verified: 172.32.0.1 allowed (exact /12 boundary), ::ffff:127.0.0.1 blocked. Residual DNS-rebind TOCTOU documented as a scoped follow-up. NOTE: the WebhookDispatcher this hardens is currently unreachable from the daemon (outbox worker doesn't execute remote mutations yet — see audit follow-up); this makes it safe for when it is wired, and pass6 URL validation is reachable now at rule creation. Reconciles the webhook-SSRF substring defect.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#203