Skip to content

fix(ssrf,channels,mcp): close 3 followup safety gaps#4110

Merged
houko merged 8 commits into
mainfrom
fix/ssrf-signature-followup
Apr 30, 2026
Merged

fix(ssrf,channels,mcp): close 3 followup safety gaps#4110
houko merged 8 commits into
mainfrom
fix/ssrf-signature-followup

Conversation

@houko

@houko houko commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Three independent input-safety gaps batched into one PR, following the
shape of PR-K #4099 / #4100 / #4104.

MCP OAuth — token/registration endpoints (#3623)

parse_authorization_server_metadata already ran a literal
is_ssrf_blocked_host check against discovered endpoints, but:

  • is_ssrf_blocked_url did not reject userinfo URLs — so
    http://[email protected]/ would pass the host check
    while reqwest connected to the IMDS literal (same shape as WASM host_net_fetch URL parser ignores userinfo — SSRF bypass to IMDS / loopback #3527).
  • KernelOAuthProvider::register_client POSTed to the registration
    endpoint with no SSRF re-check, even though the value could come
    from a cached vault entry written before policy tightened.
  • The API-layer auth-code exchange in routes/mcp_auth.rs POSTed
    to the stored token_endpoint from the vault with no re-check —
    the kernel's try_refresh already had the matching guard.

Fix: tighten is_ssrf_blocked_url to reject non-http/https schemes
and any URL with userinfo, then re-validate at every outbound site
(parser, register_client, auth-code exchange).

Channels — attachment URLs (#3442)

bridge::download_file_to_blocks and download_image_to_blocks
validated only URL scheme. A forged inbound message could smuggle
http://169.254.169.254/latest/meta-data/... or
http://127.0.0.1:4545/api/agents and have the response base64'd
into the agent's LLM context.

Fix: route both paths through the existing
http_client::validate_url_for_fetch SSRF guard already used by the
generic webhook adapter — rejects loopback / private / link-local /
unique-local / multicast / IPv4-mapped IPv6 / NAT64 / internal
hostnames.

DingTalk — replay window + silent-empty-secret (#3441)

The non-numeric-timestamp bypass itself was already closed (parser
early returns 400). Two remaining issues:

  • Replay window was 1 hour — DingTalk signs with current millis,
    so anything wider than a few minutes is pure replay surface.
    Tightened to ±5 min.
  • read_token(secret_env).unwrap_or_default() accepted an empty
    signing secret silently — verification still rejects (HMAC of an
    empty key vs. a real signature never matches), but the operator
    got no signal. Now refuses to register the adapter with a loud
    error! log when the secret env is unset or empty, matching the
    Teams default-deny shape from fix(channels): close 3 inbound-safety holes (LINE/Teams/email) #4100.

Closes #3623
Closes #3442
Closes #3441

Test plan

  • New unit tests:
    • is_ssrf_blocked_url_rejects_userinfo —
      http://[email protected]/,
      http://user:[email protected]/x, http://[email protected]/
    • is_ssrf_blocked_url_rejects_non_http_schemes — file/ftp/gopher
    • parse_authorization_server_metadata_rejects_userinfo_endpoint
    • parse_authorization_server_metadata_rejects_imds_registration_endpoint
    • test_validate_url_scheme_blocks_ssrf_targets — 8 SSRF targets
      on the channel attachment path
  • Existing tests for validate_url_scheme, is_ssrf_blocked_host,
    well_known_url, validate_metadata_endpoints stay green
  • Reviewer: confirm DingTalk operators with secret_env unset
    see the new error! log at boot rather than silent drop
  • CI: cargo build / cargo test / cargo clippy

@github-actions github-actions Bot added area/channels Messaging channel adapters area/kernel Core kernel (scheduling, RBAC, workflows) size/M 50-249 lines changed labels Apr 29, 2026
@houko
fix(ssrf,channels,mcp): close 3 followup safety gaps
fa1c95e 
Three independent inbound-side / outbound-side input-safety bypasses
batched into one PR per request, following PR-K #4099 / #4100 / #4104.

### MCP OAuth — token/registration endpoints not SSRF-validated (#3623)

`parse_authorization_server_metadata` already ran a literal
`is_ssrf_blocked_host` check against discovered endpoints, but:

* `is_ssrf_blocked_url` did not reject userinfo-bearing URLs, so
  `http://[email protected]/` would pass the host check
  while reqwest connected to the IMDS literal — same shape as #3527.
* `KernelOAuthProvider::register_client` POSTed to the
  registration endpoint with no SSRF re-check; the parser's check
  could be bypassed if the value reached `register_client` from a
  cached vault entry written before policy tightened.
* The API-layer auth-code exchange in `routes/mcp_auth.rs` POSTed
  to the stored `token_endpoint` from the vault with no re-check —
  the kernel `try_refresh` already had the matching guard.

Fix: tighten `is_ssrf_blocked_url` to also reject non-http/https
schemes and any URL with userinfo, then re-validate at every actual
outbound site (parser, register_client, auth-code exchange) so the
guard is uniform whether the value came from discovery or vault.

### Channels — attachment URLs fetched server-side without SSRF (#3442)

`bridge::download_file_to_blocks` and `download_image_to_blocks`
both validated only the URL scheme.  A forged inbound message could
smuggle `http://169.254.169.254/latest/meta-data/...` or
`http://127.0.0.1:4545/api/agents` and have the response base64'd
into the agent's LLM context.

Fix: route both paths through the existing
`http_client::validate_url_for_fetch` SSRF guard (same one the
generic webhook adapter uses), which rejects loopback / private /
link-local / unique-local / multicast / IPv4-mapped IPv6 / NAT64 /
internal hostnames.

### DingTalk — replay window too wide + silent-empty-secret (#3441)

The non-numeric-timestamp bypass was already closed (parser early
returns 400). Two remaining issues:

* Replay window was 1 hour — DingTalk signs with current millis,
  so anything wider than a few minutes is pure replay surface.
  Tightened to ±5 min.
* `read_token(secret_env).unwrap_or_default()` accepted an empty
  signing secret silently — verification would still reject (HMAC
  with empty key vs. a real signature never matches), but the
  operator got no signal.  Now refuses to register the adapter
  with a loud `error!` log when the secret env is unset or empty,
  matching the Teams default-deny shape from #4100.

Closes #3623
Closes #3442
Closes #3441
@houko
houko force-pushed the fix/ssrf-signature-followup branch from 3604771 to fa1c95e Compare April 29, 2026 14:37
houko and others added 7 commits April 30, 2026 00:54
@houko
fix(quality): cargo fmt #4110 mcp_oauth_provider.rs + mcp_oauth.rs
e2db527
@github-actions
Merge branch 'main' into fix/ssrf-signature-followup
744653e
@houko
Merge branch 'main' into fix/ssrf-signature-followup
e613e47
@houko
Merge branch 'main' into fix/ssrf-signature-followup
12f1741
@houko
Merge branch 'main' into fix/ssrf-signature-followup
69cd7d3
@houko
docs(mcp-oauth): correct userinfo-guard rationale
1216bc0 
The original doc/test comments claimed `host_str()` returns "allowed.com"
for `http://[email protected]/`, implying the host check was
bypassed.  Verified with the `url` crate (RFC 3986 compliant): host_str()
returns `169.254.169.254` — the IMDS literal — so that URL is already
caught by `is_ssrf_blocked_host`.

The userinfo guard is still worth keeping, but for a different reason:
phishing-shape inputs like `http://[email protected]/` where the
post-`@` host is legitimate and only the userinfo guard rejects.  That
case is also the unique regression point for the new test — flagged
inline so a future refactor that drops the guard will see exactly which
assertion fails.
@houko
fix(ssrf,dingtalk): address review comments from #4110
3774a2e 
- mcp_oauth_provider: replace bare reqwest::Client::new() with
  librefang_extensions::http_client::new_client() in both try_refresh
  and register_client so redirect limits and timeouts apply on the
  post-SSRF-check outbound requests.
- dingtalk: change REPLAY_WINDOW_MS to u64 and use saturating_sub
  before unsigned_abs to prevent i64 overflow on a forged extreme
  timestamp; cast eliminated since both sides are now u64.
- channel_bridge: remove duplicate secret_env from the error! format
  string — the value is already captured as a structured field via
  env = %dt_config.secret_env.
@houko
houko merged commit 4ed4d22 into main Apr 30, 2026
12 of 13 checks passed
@houko
houko deleted the fix/ssrf-signature-followup branch April 30, 2026 22:19
@houko houko mentioned this pull request Apr 30, 2026
Merged
2 tasks
houko added a commit that referenced this pull request Apr 30, 2026
@houko
docs(mcp-oauth): align parser test + SECURITY note with userinfo fix (#…
aba5ffc 
…4156)

* docs(mcp-oauth): align parser test + SECURITY note with userinfo fix

Followup to PR #4110.  That PR's first round of comment cleanup landed,
but two stragglers in the same file kept the older (and incorrect)
explanation of the userinfo guard:

* `parse_authorization_server_metadata`'s SECURITY note still pointed
  at `is_ssrf_blocked_host`, even though the parser now goes through
  `is_ssrf_blocked_url` (which adds scheme + userinfo on top).
* `parse_authorization_server_metadata_rejects_userinfo_endpoint` used
  `https://[email protected]/authorize`, which is also caught by
  the host check (loopback after the @) — so removing the userinfo
  guard would not actually fail the test.  Switched to
  `https://[email protected]/authorize` so the assertion fires
  ONLY on the userinfo path, matching the regression-isolation shape
  of `is_ssrf_blocked_url_rejects_userinfo`.

No behaviour change.  Comments + one test URL only.

* docs(mcp-oauth): drop "resolves" from SECURITY note (no DNS check)

`is_ssrf_blocked_host` matches the literal host string only — IP
literals are parsed and range-checked, hostnames are matched against
a fixed list (`localhost`, `metadata.google.internal`).  No DNS
resolution happens.  The earlier wording "any host that resolves
into a loopback…" implied the opposite and contradicted the
intentional "DNS resolution is out of scope" stance documented
elsewhere in the file.

Re-worded to make the literal-only behaviour explicit and to call
out IPv4-mapped IPv6 / NAT64 / IMDS — those are part of the actual
guard but were not mentioned.  Added an explicit "DNS resolution
out of scope; mitigate rebinding at the network layer" line so the
contract is unambiguous.

* docs(mcp-oauth): drop "resolves" from SSRF helpers + complete the range list

Two more places in this file used "resolves to..." wording for the
SSRF helpers, even though neither helper does any DNS resolution:

* `is_ssrf_blocked_host`: said "resolves to a network range" — the
  function only matches the literal host string against IP ranges
  and a fixed hostname list.  Reworded, and added the missing
  IPv4-mapped IPv6 + NAT64 entries to the blocked-values list (they
  exist in the impl via `ipv6_embedded_ipv4`, but were not surfaced
  in the doc summary).
* `well_known_url`: said "the origin resolves to" — same fix.

Both now spell out "no DNS resolution; mitigate rebinding at the
network layer" so the contract is self-consistent across helpers.
No behaviour change.
@houko houko mentioned this pull request May 1, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@houko houko

Labels

area/channels Messaging channel adapters area/kernel Core kernel (scheduling, RBAC, workflows) size/M 50-249 lines changed

Projects

None yet

Milestone

No milestone

1 participant

@houko