fix(channels,ssrf): DNS-rebind, chunk loss, journal stall, lag drops#4104
Merged
Conversation
…rops - WebhookStore: add validate_webhook_url_resolved that DNS-resolves the host and rejects any A/AAAA pointing at private/loopback/link-local / metadata, called at fire-time so DNS rebind (#3701) cannot reach IMDS or RFC 1918 even after registration. IPv6 ULA + multicast + link-local are now also caught at the literal level. Closes #3701. - Telegram api_send_message: chunk N failure now propagates as Err with chunk index in the message instead of silently sending only chunk 1 and dropping the rest; the streaming continuation loop also surfaces the failure to its caller so retry / alerting can react. Closes #3664. - message_journal compact(): wrap sync File::create + flush + sync_all + rename in spawn_blocking so the inner tokio::Mutex doesn't pin the reactor while the kernel fsyncs — channel intake no longer serializes behind the compactor. Closes #3646. - event_bus: add record_consumer_lag + recv_event_skipping_lag helper that turns broadcast RecvError::Lagged into an error! log routed through dropped_count, so lag-induced trigger misses stop being silent. Closes #3630.
github-actions
Bot
added
area/channels
4 tasks
houko
added a commit
that referenced
this pull request
Apr 29, 2026
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
Two follow-ups from review of #4104: * validate_webhook_url_resolved now returns the validated SocketAddr alongside the host, and test_webhook pins reqwest to that address via ClientBuilder::resolve. Without pinning, reqwest performs its own DNS lookup before connecting, and a low-TTL record can flip between our validate call and reqwest's lookup — the canonical DNS-rebind bypass that #3701 exists to close. Pinning forces the eventual connection to the IP we already proved safe. * message_journal::compact() now drops the inner mutex *before* the spawn_blocking await. The lock was previously held across the entire fsync window, so channel intake still serialized behind the compactor — same stall as #3646, just on the blocking thread instead of the runtime thread. Cloning path + entries out and dropping the guard is the actual fix. Note: the review also flagged dispatch-path coverage and the recv_event_skipping_lag helper as dead code. The webhook subscription store has no separate dispatcher in this codebase — test_webhook is the only call site that actually POSTs to a webhook URL — so the "dispatch path" concern was misdirected. Migrating Lagged consumers is broader than this PR's scope and is left to a follow-up.
After the prior #3646 follow-up dropped the inner mutex before the spawn_blocking fsync, compact() and record() could interleave: 1. compact() snapshots pending = {A,B,C} and drops the lock. 2. compact() begins writing A,B,C to tmp.jsonl. 3. record(D) takes the lock, appends D to journal.jsonl on disk, inserts D into pending, drops the lock. 4. compact() finishes the slow fsync and rename(tmp, journal.jsonl) — overwriting D's appended line on disk while D remains in memory. 5. Crash before D is re-appended: D is permanently lost on recovery. This was exactly the truncation race the #3967 audit established record()'s "lock-held-across-disk-write" invariant to prevent. Fix: keep the slow work (File::create + flush + sync_all) off the mutex so intake is not stalled (the goal of #3646), but re-acquire the lock before the rename and check whether any new ids were appended to pending since the snapshot. If so, abort this compaction (retry next tick) and clean up the stale tmp file. Only when no record() interleaved do we proceed with rename — and the lock then held also serializes against any record() that arrives during the rename itself. Adds compact_does_not_lose_entries_appended_during_window regression test that exercises the abort path.
forward_kernel_events used to swallow RecvError::Lagged with just a warn! and an internal `n` counter that was per-listener and never surfaced anywhere. The new EventBus::record_consumer_lag (added in this PR) gives us a global counter + rate-limited error! log, which is what #3630 wanted. Migrate forward_kernel_events to use recv_event_skipping_lag, threading &Arc<LibreFangKernel> through both call sites (lib.rs direct-boot path and connection.rs local-server path) so the helper has the EventBus reference it needs. The other three Lagged handlers in the codebase don't fit this helper: - bridge.rs uses KernelHandle (trait); plumbing EventBus through the trait would leak kernel internals the trait was created to hide. - context_engine.rs uses runtime's PluginEventBus, not kernel EventBus. - agents.rs:2485 uses StreamEvent from SessionStreamHub, different bus.
houko
added a commit
that referenced
this pull request
Apr 30, 2026
…count (#3630 follow-up) PR #4104 added EventBus::record_consumer_lag + recv_event_skipping_lag but only migrated the desktop notification listener — the bridge approval listener and the runtime on_event hook still swallowed RecvError::Lagged into per-listener warn! logs that never surfaced in any global counter. This change finishes the migration for the two remaining sites that have a sensible bus to record into: - ChannelBridgeHandle gets a default-no-op `record_consumer_lag` method. KernelBridgeAdapter overrides it to forward to EventBus, so bridge.rs approval listener lag now contributes to EventBus::dropped_count and triggers the rate-limited error! log. Test mocks inherit the no-op default, so none of the existing ChannelBridgeHandle impls in tests needed changes. - PluginEventBus gets its own dropped_count + record_consumer_lag, mirroring EventBus's design. context_engine on_event listener routes Lagged through it instead of the previous per-listener warn!. Intentionally NOT migrated: - agents.rs:2485 SSE attach uses StreamEvent from SessionStreamHub which is documented as intentionally lossy; routing those drops to error! would over-log normal SSE backpressure on slow clients.
4 tasks
houko
added a commit
that referenced
this pull request
Apr 30, 2026
* fix(ssrf,channels,mcp): close 3 followup safety gaps 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 * fix(quality): cargo fmt #4110 mcp_oauth_provider.rs + mcp_oauth.rs * docs(mcp-oauth): correct userinfo-guard rationale 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. * fix(ssrf,dingtalk): address review comments from #4110 - 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. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
houko
added a commit
that referenced
this pull request
Apr 30, 2026
…count (#3630 follow-up) PR #4104 added EventBus::record_consumer_lag + recv_event_skipping_lag but only migrated the desktop notification listener — the bridge approval listener and the runtime on_event hook still swallowed RecvError::Lagged into per-listener warn! logs that never surfaced in any global counter. This change finishes the migration for the two remaining sites that have a sensible bus to record into: - ChannelBridgeHandle gets a default-no-op `record_consumer_lag` method. KernelBridgeAdapter overrides it to forward to EventBus, so bridge.rs approval listener lag now contributes to EventBus::dropped_count and triggers the rate-limited error! log. Test mocks inherit the no-op default, so none of the existing ChannelBridgeHandle impls in tests needed changes. - PluginEventBus gets its own dropped_count + record_consumer_lag, mirroring EventBus's design. context_engine on_event listener routes Lagged through it instead of the previous per-listener warn!. Intentionally NOT migrated: - agents.rs:2485 SSE attach uses StreamEvent from SessionStreamHub which is documented as intentionally lossy; routing those drops to error! would over-log normal SSE backpressure on slow clients.
houko
added a commit
that referenced
this pull request
Apr 30, 2026
* fix(channels,runtime): route remaining broadcast lag through dropped_count (#3630 follow-up) PR #4104 added EventBus::record_consumer_lag + recv_event_skipping_lag but only migrated the desktop notification listener — the bridge approval listener and the runtime on_event hook still swallowed RecvError::Lagged into per-listener warn! logs that never surfaced in any global counter. This change finishes the migration for the two remaining sites that have a sensible bus to record into: - ChannelBridgeHandle gets a default-no-op `record_consumer_lag` method. KernelBridgeAdapter overrides it to forward to EventBus, so bridge.rs approval listener lag now contributes to EventBus::dropped_count and triggers the rate-limited error! log. Test mocks inherit the no-op default, so none of the existing ChannelBridgeHandle impls in tests needed changes. - PluginEventBus gets its own dropped_count + record_consumer_lag, mirroring EventBus's design. context_engine on_event listener routes Lagged through it instead of the previous per-listener warn!. Intentionally NOT migrated: - agents.rs:2485 SSE attach uses StreamEvent from SessionStreamHub which is documented as intentionally lossy; routing those drops to error! would over-log normal SSE backpressure on slow clients. * fix(channels,runtime,kernel): plug review gaps in #4152 Self-review of #4152 turned up three issues: * No test on PluginEventBus::record_consumer_lag — the kernel EventBus has one (record_consumer_lag_increments_dropped_count) but the new PluginEventBus copy did not, so silent drift between the two impls was not caught. Mirror the kernel test. * last_drop_warn was initialised to Instant::now(), so the FIRST lag burst after process start would only bump dropped_count and stay silent for the first 10 s — defeating the "make lag visible" goal of #3630. Backdate the timestamp by (interval + 1)s so the first burst is always logged. Apply the same fix to the kernel EventBus for parity (otherwise the two buses behave differently on cold start). * The default no-op record_consumer_lag on ChannelBridgeHandle is fine for test mocks but a future production impl would silently inherit it. Add a "Production implementations MUST override" line to the trait doc, since there is no compiler signal. Also extracts LAG_WARN_INTERVAL_SECS in PluginEventBus so the rate- limit window is named in the runtime side as well. * fix(event-bus): clarify LAG_WARN_INTERVAL_SECS sync + add dropped_count forwarding test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four reliability/security fixes for channel + event-bus paths.
validate_webhook_url_resolvedwhich performs a DNS lookup at fire-time and rejects any resolved address in private / loopback / link-local / cloud-metadata ranges. Wired into the test-webhook fetch site (routes/system.rs). Registration-time check stays as the cheap literal guard. IPv6 ULA + multicast + link-local are now also caught at the literal level (the oldis_private_ip(IpAddr::V6) → falsearm was a hole).api_send_messagenow propagatesErrwith the failing chunk index when a chunk POST returns non-2xx (and the plain-text fallback also fails), or when the network call errors.send_streaming's continuation loop returns the failure to its caller instead ofbreaking and continuing as if delivery succeeded.compact()holdingtokio::Mutexacross sync std::fs. WrappedFile::create+writeln!+flush+sync_all+renameintokio::task::spawn_blockingso the reactor isn't pinned while the kernel fsyncs the temp file; channel intake no longer serializes behind the compactor. (recordandupdate_statusalready usedspawn_blocking;compactwas the missing one.)EventBus::record_consumer_lagand a freerecv_event_skipping_laghelper. Consumers that receive fromsubscribe_agent/subscribe_allcan now routeRecvError::Lagged(n)throughrecord_consumer_lagto get anerror!log +dropped_countincrement, instead of silently skipping. Capacity bumps from a previous round (1024→4096 global, 256→2048 per-agent) stay in place.Closes #3701
Closes #3664
Closes #3646
Closes #3630
Test plan
webhook_store::testsgreen; newvalidate_webhook_url_resolved_*tests cover loopback / metadata literal / localhost hostname / IPv6 ULA literal.Errwith chunk index — verifiable by stubbingclient.postand asserting the error message containschunk N/M.message_journaltests green; compact still rewrites the file under temp dir.event_bus::record_consumer_lag_increments_dropped_countcovers the counter accounting.