From 46febba27f948ebb02c0039c631dae1f0e7e6569 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Mon, 13 Jul 2026 14:43:16 -0400 Subject: [PATCH 1/2] feat(gateway)!: route standalone WS path through the shared trust gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1c prerequisite (#1356): the standalone-gateway WebSocket path (run_gateway_adapter) now enforces L2 (channel scope) and L3 (identity) via the same per-platform trust registry as the unified path, instead of its own inline allowlist checks. - gateway.rs: extract gate_gateway_event — the shared ingress gate (gate_incoming + DenyIdentity throttled request-access echo + DenyScope silent drop) used by BOTH process_gateway_event (unified) and the WS loop. Gate runs before slash handling so untrusted senders cannot execute /reset|/cancel|/config. should_skip_event on the WS path is neutered to structural gating only (bot admission + @mention), matching the unified path. - main.rs: seed the registry with the [gateway] section's trust for its platform (gateway_section_trust — same resolve_allow_all semantics the old inline filter used). Precedence: GATEWAY_* env (uniform seed) < [gateway] section < [] section (platform_trust_override). - Dead code removed: GatewayParams and GatewayEventContext channel/user allowlist fields (unified ones were already unread post-#1297; WS ones move to the registry), plus the unified block's GATEWAY_ALLOW_ALL_* env reads (the registry's uniform seed already consumes them). - Docs: the six mode-scoping callouts (line/wecom/google-chat/teams/ config-reference ×2) now describe the consolidated trust resolution. BREAKING CHANGE: in standalone-gateway deployments that define BOTH [gateway] allowlists AND a first-class [] trust section, the platform section now wins on the WS path (documented target state: config-first, platform section is the most specific). Previously the platform section had no effect on that path. Migration: if your WS deployment relies on [gateway].allowed_users while also carrying a [] section with different values, align the platform section (it is now authoritative for that platform). Tests: gateway_section_trust_mirrors_ws_filter_semantics + gateway_trust_seed_precedence_env_lt_gateway_lt_platform (main.rs) + ws_path_filter_is_structural_only (gateway.rs). Refs #1356. --- crates/openab-core/src/gateway.rs | 191 +++++++++++++++++++++--------- docs/config-reference.md | 4 +- docs/google-chat.md | 2 +- docs/line.md | 2 +- docs/msteams-selfhosted.md | 2 +- docs/wecom.md | 2 +- src/main.rs | 164 ++++++++++++++++++------- 7 files changed, 265 insertions(+), 102 deletions(-) diff --git a/crates/openab-core/src/gateway.rs b/crates/openab-core/src/gateway.rs index a95840f10..e67bdd88c 100644 --- a/crates/openab-core/src/gateway.rs +++ b/crates/openab-core/src/gateway.rs @@ -745,15 +745,15 @@ impl ChatAdapter for GatewayAdapter { // --- Run the gateway adapter (connects to gateway WS, routes events to AdapterRouter) --- /// Resolved gateway configuration passed to the adapter at startup. +/// Channel/user allowlists are NOT carried here anymore: L2/L3 enforcement for +/// the WebSocket path moved to the shared per-platform trust registry +/// (`AdapterRouter::gate_incoming`), seeded in main.rs with precedence +/// `GATEWAY_*` env < `[gateway]` section < `[]` section (#1356). pub struct GatewayParams { pub url: String, pub platform: String, pub token: Option, pub bot_username: Option, - pub allow_all_channels: bool, - pub allowed_channels: Vec, - pub allow_all_users: bool, - pub allowed_users: Vec, pub allow_bot_messages: bool, pub trusted_bot_ids: Vec, pub streaming: bool, @@ -774,10 +774,6 @@ pub async fn run_gateway_adapter( // Append auth token as query param if configured let gateway_url = params.url; let bot_username = params.bot_username; - let allow_all_channels = params.allow_all_channels; - let allowed_channels: HashSet = params.allowed_channels.into_iter().collect(); - let allow_all_users = params.allow_all_users; - let allowed_users: HashSet = params.allowed_users.into_iter().collect(); let allow_bot_messages = params.allow_bot_messages; let trusted_bot_ids: HashSet = params.trusted_bot_ids.into_iter().collect(); // Cosmetic streaming edits a placeholder in place. On platforms without an @@ -850,12 +846,17 @@ pub async fn run_gateway_adapter( let slash_ws_tx = ws_tx.clone(); // for fire-and-forget slash command responses let mut tasks: tokio::task::JoinSet<()> = tokio::task::JoinSet::new(); - // Hoist filter params outside loop — all fields are loop-invariant + // Hoist filter params outside loop — all fields are loop-invariant. + // Structural gating (bot filter + @mention) stays in should_skip_event. + // L2 (channel) + L3 (identity) are enforced by the shared ingress gate + // (`gate_gateway_event`) below — same registry as the unified path — + // so channel/user checks are neutered here by passing allow-all. + let no_ids: HashSet = HashSet::new(); let filter = EventFilterParams { - allow_all_channels, - allowed_channels: &allowed_channels, - allow_all_users, - allowed_users: &allowed_users, + allow_all_channels: true, + allowed_channels: &no_ids, + allow_all_users: true, + allowed_users: &no_ids, allow_bot_messages, trusted_bot_ids: &trusted_bot_ids, bot_username: bot_username.as_deref(), @@ -884,6 +885,15 @@ pub async fn run_gateway_adapter( continue; } + // Shared ingress trust gate (L2 scope + L3 + // identity) — same per-platform registry as + // the unified path. Placed before slash + // handling so untrusted senders cannot + // execute /reset|/cancel|/config. + if !gate_gateway_event(&router, adapter.as_ref(), &event).await { + continue; + } + info!( platform = %event.platform, sender = %event.sender.name, @@ -1205,7 +1215,6 @@ pub async fn run_gateway_adapter( } // outer reconnect loop } - // --- Public API for unified mode (Phase 2) --- /// Context required to process a gateway event without a WebSocket connection. @@ -1214,10 +1223,6 @@ pub struct GatewayEventContext { pub adapter: Arc, pub dispatcher: Arc, pub router: Arc, - pub allow_all_channels: bool, - pub allowed_channels: HashSet, - pub allow_all_users: bool, - pub allowed_users: HashSet, pub allow_bot_messages: bool, pub trusted_bot_ids: HashSet, pub bot_username: Option, @@ -1254,42 +1259,30 @@ fn echo_allowed(key: &str) -> bool { } } -pub async fn process_gateway_event( - event_json: &str, - ctx: &GatewayEventContext, -) -> anyhow::Result { - let event: GatewayEvent = serde_json::from_str(event_json) - .map_err(|e| anyhow::anyhow!("invalid gateway event JSON: {e}"))?; - - // Structural gating (bot filter + @mention) stays in should_skip_event. - // L2 (channel) + L3 (identity) are now enforced by the shared ingress gate - // (`router.gate_incoming`) below, so we neuter should_skip_event's channel/user - // checks here by passing allow-all for them. - let no_ids: HashSet = HashSet::new(); - let filter = EventFilterParams { - allow_all_channels: true, - allowed_channels: &no_ids, - allow_all_users: true, - allowed_users: &no_ids, - allow_bot_messages: ctx.allow_bot_messages, - trusted_bot_ids: &ctx.trusted_bot_ids, - bot_username: ctx.bot_username.as_deref(), - }; - if should_skip_event(&event, &filter) { - return Ok(false); - } - - // Shared ingress trust gate (L2 scope + L3 identity), keyed by platform. - // Phase 1: `is_dm = false` preserves today's behavior where gateway DMs are - // evaluated against the channel allowlist like any other channel (the - // `allow_dm` surface semantics arrive with the per-platform trust flip). - // TODO(phase-2): derive is_dm from the event/ChannelRef carrier so the - // `allow_dm` L2 surface can be enforced and tested for gateway platforms. +/// Shared ingress trust gate for gateway events — used by BOTH the standalone +/// WebSocket path (`run_gateway_adapter`) and the unified path +/// (`process_gateway_event`), so L2 (channel scope) and L3 (identity) are +/// enforced by the same per-platform registry regardless of deployment mode +/// (#1356 Phase 1c prerequisite). +/// +/// Returns `true` if the event may proceed. On `DenyIdentity`, sends the +/// throttled request-access echo via `adapter`; on `DenyScope` (and any future +/// variant), drops silently — scope is not a security boundary, so no echo. +/// +/// Phase 1: `is_dm = false` preserves today's behavior where gateway DMs are +/// evaluated against the channel allowlist like any other channel (the +/// `allow_dm` surface semantics arrive with the per-platform trust flip). +/// TODO(phase-2): derive is_dm from the event/ChannelRef carrier so the +/// `allow_dm` L2 surface can be enforced and tested for gateway platforms. +async fn gate_gateway_event( + router: &crate::adapter::AdapterRouter, + adapter: &dyn ChatAdapter, + event: &GatewayEvent, +) -> bool { let decision = - ctx.router - .gate_incoming(&event.platform, &event.channel.id, false, &event.sender.id); + router.gate_incoming(&event.platform, &event.channel.id, false, &event.sender.id); match decision { - crate::trust::Decision::Allow => {} + crate::trust::Decision::Allow => true, crate::trust::Decision::DenyIdentity => { // L3 identity deny → echo the sender their ID so they can request // access (throttled to avoid amplification). Bots never reach here @@ -1313,9 +1306,9 @@ pub async fn process_gateway_event( "⚠️ You are not on this bot's trusted list.\nYour ID: {}\nAsk the admin to add it to allowed_users.", event.sender.id ); - let _ = ctx.adapter.send_message(&echo_channel, &echo).await; + let _ = adapter.send_message(&echo_channel, &echo).await; } - return Ok(false); + false } // DenyScope (and any future variant) → silent drop (scope is not a // security boundary; no request-access echo). @@ -1327,9 +1320,40 @@ pub async fn process_gateway_event( ?decision, "gateway event denied (scope); silent" ); - return Ok(false); + false } } +} + +pub async fn process_gateway_event( + event_json: &str, + ctx: &GatewayEventContext, +) -> anyhow::Result { + let event: GatewayEvent = serde_json::from_str(event_json) + .map_err(|e| anyhow::anyhow!("invalid gateway event JSON: {e}"))?; + + // Structural gating (bot filter + @mention) stays in should_skip_event. + // L2 (channel) + L3 (identity) are now enforced by the shared ingress gate + // (`router.gate_incoming`) below, so we neuter should_skip_event's channel/user + // checks here by passing allow-all for them. + let no_ids: HashSet = HashSet::new(); + let filter = EventFilterParams { + allow_all_channels: true, + allowed_channels: &no_ids, + allow_all_users: true, + allowed_users: &no_ids, + allow_bot_messages: ctx.allow_bot_messages, + trusted_bot_ids: &ctx.trusted_bot_ids, + bot_username: ctx.bot_username.as_deref(), + }; + if should_skip_event(&event, &filter) { + return Ok(false); + } + + // Shared ingress trust gate (L2 scope + L3 identity), keyed by platform. + if !gate_gateway_event(&ctx.router, ctx.adapter.as_ref(), &event).await { + return Ok(false); + } tracing::info!( platform = %event.platform, @@ -1628,6 +1652,63 @@ mod tests { } } + #[test] + fn ws_path_filter_is_structural_only() { + // The WS path's hoisted filter neuters channel/user checks (allow-all) + // because L2/L3 moved to the shared trust registry (#1356 Phase 1c + // prerequisite). This pins the two properties that combination relies + // on: unknown channels/users PASS the structural filter (the gate + // decides), while bot admission and @mention gating still apply. + let no_ids: HashSet = HashSet::new(); + let trusted: HashSet = ["good-bot".to_string()].into_iter().collect(); + let filter = EventFilterParams { + allow_all_channels: true, + allowed_channels: &no_ids, + allow_all_users: true, + allowed_users: &no_ids, + allow_bot_messages: false, + trusted_bot_ids: &trusted, + bot_username: Some("mybot"), + }; + + // Unknown human in unknown channel: structural filter passes it through. + let ev = make_event( + false, + "stranger", + "unlisted-channel", + "private", + None, + vec!["mybot"], + ); + assert!(!should_skip_event(&ev, &filter)); + + // Untrusted bot still skipped (structural, stays on this path). + let ev = make_event( + true, + "evil-bot", + "unlisted-channel", + "private", + None, + vec![], + ); + assert!(should_skip_event(&ev, &filter)); + + // Trusted bot admitted. + let ev = make_event( + true, + "good-bot", + "unlisted-channel", + "private", + None, + vec![], + ); + assert!(!should_skip_event(&ev, &filter)); + + // Group without @mention still skipped (structural, stays on this path). + let ev = make_event(false, "stranger", "group-1", "group", None, vec![]); + assert!(should_skip_event(&ev, &filter)); + } + #[test] fn echo_allowed_throttles_repeat_within_window() { // Unique key so we don't collide with other tests touching the global map. diff --git a/docs/config-reference.md b/docs/config-reference.md index 6722a1e9b..f83456bc2 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -139,7 +139,7 @@ Custom Gateway adapter for platforms like Telegram, LINE, Feishu/Lark, and Googl First-class LINE section — credentials, connection, and L3 identity trust (config-first parity, #1376). Replaces the uniform `GATEWAY_ALLOW_ALL_USERS` / `GATEWAY_ALLOWED_USERS` env vars for LINE trust — relying on those for LINE is deprecated and warns at startup. -> **Mode scoping:** takes effect on the **embedded/unified adapter path** (see the note under `[wecom]` / `[googlechat]` / `[teams]` below — the same applies here). +> **Trust resolution:** applies in **both** deployment modes (see the note under `[wecom]` / `[googlechat]` / `[teams]` below — the same applies here). Each field resolves: config value → `LINE_*` env var → default (deny-all). @@ -214,7 +214,7 @@ Full first-class Teams section (config-first parity, #1380) — credentials, con First-class L3 identity trust — same shape and semantics as `[line]`. Each section replaces the uniform `GATEWAY_ALLOW_ALL_USERS` / `GATEWAY_ALLOWED_USERS` env vars for its platform (deprecated: warns at startup, becomes an error in Phase 2). Platform credentials remain on the gateway env vars (`TEAMS_APP_ID`/`TEAMS_APP_SECRET`) until the Teams config-first parity slice lands (#1380). -> **Mode scoping:** these sections (like `[line]`) take effect on the **embedded/unified adapter path**, where events pass the shared ingress trust gate. Deployments using the standalone `openab-gateway` companion over WebSocket enforce trust via `[gateway].allow_all_users` / `allowed_users` instead; Phase 1c consolidates the two paths. +> **Trust resolution:** these sections (like `[line]`) apply in **both** deployment modes — the embedded/unified adapter path and the broker's WebSocket path to the standalone `openab-gateway` companion both consult the shared per-platform trust registry. Precedence per platform: `GATEWAY_*` env < `[gateway]` section < `[]` section (the platform section wins when both are set). Each field resolves: config value → `TEAMS_*` env var → default (deny-all). diff --git a/docs/google-chat.md b/docs/google-chat.md index ac29e4449..dcee33d0e 100644 --- a/docs/google-chat.md +++ b/docs/google-chat.md @@ -166,7 +166,7 @@ allowed_users = ["users/123456789"] ### User Trust (`[googlechat]` section) -> **Mode scoping:** the `[googlechat]` section applies when the Google Chat adapter is **embedded in the OAB binary** (unified mode). Enable it with `[googlechat] enabled = true`; `GOOGLE_CHAT_ENABLED=true` remains the env-only fallback. In the standalone-gateway mode shown above, trust is enforced by `[gateway].allow_all_users` / `allowed_users` instead — the `[googlechat]` section has no effect on that path yet (Phase 1c consolidates the two). +> **Trust resolution:** the `[googlechat]` section's trust settings apply in **both** deployment modes (enable the embedded adapter with `[googlechat] enabled = true`; `GOOGLE_CHAT_ENABLED=true` remains the env-only fallback). Broker-side enforcement goes through the shared per-platform trust registry with precedence `GATEWAY_*` env < `[gateway]` section < `[googlechat]` section — in the standalone-gateway mode, the broker's WebSocket path consults the same registry, so a `[googlechat]` section overrides `[gateway].allow_all_users` / `allowed_users` for this platform. Identity trust defaults to **deny-all** (identity-trust-none ADR): unknown senders are rejected until explicitly admitted. Configure trust with a first-class `[googlechat]` section: diff --git a/docs/line.md b/docs/line.md index 4d80a6083..9d39a897a 100644 --- a/docs/line.md +++ b/docs/line.md @@ -104,7 +104,7 @@ platform = "line" ### User Trust (`[line]` section) -> **Mode scoping:** the `[line]` section applies when the LINE adapter is **embedded in the OAB binary** (unified mode, `LINE_CHANNEL_SECRET` env set on the OAB container). In the standalone-gateway mode shown above, trust is enforced by `[gateway].allow_all_users` / `allowed_users` instead — the `[line]` section has no effect on that path yet (Phase 1c consolidates the two). +> **Trust resolution:** the `[line]` section's trust settings apply in **both** deployment modes. Broker-side enforcement goes through the shared per-platform trust registry with precedence `GATEWAY_*` env < `[gateway]` section < `[line]` section — in the standalone-gateway mode, the broker's WebSocket path consults the same registry, so a `[line]` section overrides `[gateway].allow_all_users` / `allowed_users` for this platform. Identity trust defaults to **deny-all** (identity-trust-none ADR): unknown senders are rejected until explicitly admitted. Configure trust with a first-class `[line]` section: diff --git a/docs/msteams-selfhosted.md b/docs/msteams-selfhosted.md index 93c43ef6c..43cc395da 100644 --- a/docs/msteams-selfhosted.md +++ b/docs/msteams-selfhosted.md @@ -55,7 +55,7 @@ allowed_users = ["29:1abc..."] # Bot Framework activity.from.id values Each field falls back to its `TEAMS_ALLOW_ALL_USERS` / `TEAMS_ALLOWED_USERS` env var when unset. To find a user's `29:…` ID, check the OAB logs — the sender ID is logged for each incoming message. -> **Mode scoping:** the `[teams]` section applies to this unified (embedded) mode. In the legacy standalone-gateway mode below, trust is enforced by `[gateway].allow_all_users` / `allowed_users` instead — the `[teams]` section has no effect on that path yet (Phase 1c consolidates the two). +> **Trust resolution:** the `[teams]` section's trust settings apply in **both** deployment modes. Broker-side enforcement goes through the shared per-platform trust registry with precedence `GATEWAY_*` env < `[gateway]` section < `[teams]` section — in the standalone-gateway mode, the broker's WebSocket path consults the same registry, so a `[teams]` section overrides `[gateway].allow_all_users` / `allowed_users` for this platform. > ⚠️ **Deprecated:** driving Teams trust through the uniform `GATEWAY_ALLOW_ALL_USERS` / `GATEWAY_ALLOWED_USERS` env vars still works but logs a startup warning; it will become a startup error in a later phase. Migrate to `[teams]` (or `TEAMS_*` env vars). diff --git a/docs/wecom.md b/docs/wecom.md index ae969fb86..15bd6d491 100644 --- a/docs/wecom.md +++ b/docs/wecom.md @@ -145,7 +145,7 @@ allowed_users = ["zhangsan", "lisi"] ### User Trust (`[wecom]` section) -> **Mode scoping:** the `[wecom]` section applies when the WeCom adapter is **embedded in the OAB binary** (unified mode, `WECOM_CORP_ID` env set on the OAB container). In the standalone-gateway mode shown above, trust is enforced by `[gateway].allow_all_users` / `allowed_users` instead — the `[wecom]` section has no effect on that path yet (Phase 1c consolidates the two). +> **Trust resolution:** the `[wecom]` section's trust settings apply in **both** deployment modes. Broker-side enforcement goes through the shared per-platform trust registry with precedence `GATEWAY_*` env < `[gateway]` section < `[wecom]` section — in the standalone-gateway mode, the broker's WebSocket path consults the same registry, so a `[wecom]` section overrides `[gateway].allow_all_users` / `allowed_users` for this platform. Identity trust defaults to **deny-all** (identity-trust-none ADR): unknown senders are rejected until explicitly admitted. Configure trust with a first-class `[wecom]` section: diff --git a/src/main.rs b/src/main.rs index d30cf1f9d..ac54067e2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -156,6 +156,26 @@ fn has_unified_wecom_config(cfg: &config::Config) -> bool { }) } +/// Trust view of the `[gateway]` section for the standalone WebSocket platform. +/// Same `resolve_allow_all` semantics the WS path's inline allowlist filter used +/// before L2/L3 moved to the shared registry (#1356 Phase 1c prerequisite): +/// explicit flag wins; otherwise a non-empty list implies deny-by-default. +fn gateway_section_trust(gw: &config::GatewayConfig) -> openab_core::trust::TrustConfig { + openab_core::trust::TrustConfig::new( + Some(config::resolve_allow_all( + gw.allow_all_channels, + &gw.allowed_channels, + )), + gw.allowed_channels.clone(), + None, // allow_dm unused in Phase 1 (is_dm passed as false) + Some(config::resolve_allow_all( + gw.allow_all_users, + &gw.allowed_users, + )), + gw.allowed_users.clone(), + ) +} + /// Apply a platform's first-class trust section to the registry, or — when the /// platform is active but still trust-driven by the deprecated uniform /// `GATEWAY_ALLOW_ALL_USERS`/`GATEWAY_ALLOWED_USERS` env — log the Phase 1 @@ -412,6 +432,17 @@ async fn main() -> anyhow::Result<()> { ); } + // [gateway] section seed for the standalone WS platform: the WebSocket + // path now enforces L2/L3 via this registry (gate_gateway_event) instead + // of its own inline allowlist checks, so the section's values must land + // here (behavior-preserving: same resolve_allow_all semantics the old + // inline filter used). Precedence: GATEWAY_* env (uniform seed above) + // < [gateway] (this insert) < [] section + // (platform_trust_override below, which runs in both modes). + if let Some(ref gw) = cfg.gateway { + reg.insert(&gw.platform, gateway_section_trust(gw)); + } + // Discord: gate L3 (identity) only via the shared gate. Discord's L2 is // richer than the flat allowed_channels model (threads are admitted by // *parent* channel, DMs by allow_dm), so we leave channel/DM enforcement @@ -836,16 +867,6 @@ async fn main() -> anyhow::Result<()> { platform: gw_cfg.platform, token: gw_cfg.token, bot_username: gw_cfg.bot_username, - allow_all_channels: config::resolve_allow_all( - gw_cfg.allow_all_channels, - &gw_cfg.allowed_channels, - ), - allowed_channels: gw_cfg.allowed_channels, - allow_all_users: config::resolve_allow_all( - gw_cfg.allow_all_users, - &gw_cfg.allowed_users, - ), - allowed_users: gw_cfg.allowed_users, allow_bot_messages: gw_cfg.allow_bot_messages, trusted_bot_ids: gw_cfg.trusted_bot_ids, streaming: gw_cfg.streaming, @@ -1099,27 +1120,9 @@ async fn main() -> anyhow::Result<()> { unified_adapter::UnifiedGatewayAdapter::new(gw_state.clone()), ); - // Read security gating from env (mirrors [gateway] config section) - let gw_allow_all_channels = std::env::var("GATEWAY_ALLOW_ALL_CHANNELS") - .map(|v| v != "0" && !v.eq_ignore_ascii_case("false")) - .unwrap_or(true); - let gw_allowed_channels: std::collections::HashSet = - std::env::var("GATEWAY_ALLOWED_CHANNELS") - .unwrap_or_default() - .split(',') - .map(|s| s.trim().to_string()) - .filter(|s| !s.is_empty()) - .collect(); - let gw_allow_all_users = std::env::var("GATEWAY_ALLOW_ALL_USERS") - .map(|v| v != "0" && !v.eq_ignore_ascii_case("false")) - .unwrap_or(true); - let gw_allowed_users: std::collections::HashSet = - std::env::var("GATEWAY_ALLOWED_USERS") - .unwrap_or_default() - .split(',') - .map(|s| s.trim().to_string()) - .filter(|s| !s.is_empty()) - .collect(); + // Bot gating still reads env here (structural, not L2/L3): + // channel/user gating moved to the shared trust registry, seeded + // from GATEWAY_* env / [gateway] / [] at startup. let gw_bot_username = std::env::var("GATEWAY_BOT_USERNAME").ok(); let gw_allow_bot_messages = std::env::var("GATEWAY_ALLOW_BOT_MESSAGES") @@ -1139,16 +1142,6 @@ async fn main() -> anyhow::Result<()> { adapter: unified_adapter, dispatcher: unified_dispatcher, router: router.clone(), - allow_all_channels: config::resolve_allow_all( - Some(gw_allow_all_channels), - &gw_allowed_channels.iter().cloned().collect::>(), - ), - allowed_channels: gw_allowed_channels, - allow_all_users: config::resolve_allow_all( - Some(gw_allow_all_users), - &gw_allowed_users.iter().cloned().collect::>(), - ), - allowed_users: gw_allowed_users, allow_bot_messages: gw_allow_bot_messages, trusted_bot_ids: gw_trusted_bot_ids, bot_username: gw_bot_username, @@ -1573,6 +1566,95 @@ mod tests { clear_all(); } + #[test] + fn gateway_section_trust_mirrors_ws_filter_semantics() { + use openab_core::trust::Decision; + + // Explicit lists → deny-by-default with listed entries admitted + // (resolve_allow_all: no flag + non-empty list = false). + let gw = config::parse_config_str( + r#" +[gateway] +url = "ws://gw:8080/ws" +platform = "telegram" +allowed_channels = ["c1"] +allowed_users = ["u1"] +"#, + "test", + ) + .unwrap() + .gateway + .unwrap(); + let trust = gateway_section_trust(&gw); + assert_eq!(trust.decide("c1", false, "u1"), Decision::Allow); + assert_ne!(trust.decide("c1", false, "u2"), Decision::Allow); + assert_ne!(trust.decide("c2", false, "u1"), Decision::Allow); + + // Empty lists → allow-all (matching the old inline filter default). + let gw_open = config::parse_config_str( + "[gateway]\nurl = \"ws://gw:8080/ws\"\n", + "test", + ) + .unwrap() + .gateway + .unwrap(); + let trust_open = gateway_section_trust(&gw_open); + assert_eq!(trust_open.decide("any", false, "anyone"), Decision::Allow); + } + + #[test] + fn gateway_trust_seed_precedence_env_lt_gateway_lt_platform() { + use openab_core::trust::{Decision, PlatformTrustConfigs, TrustConfig}; + + let mut reg = PlatformTrustConfigs::new(); + // 1. uniform GATEWAY_* env seed: deny-all users + reg.insert( + "telegram", + TrustConfig::new(Some(true), vec![], None, Some(false), vec![]), + ); + assert_ne!( + reg.get("telegram").decide("c", false, "alice"), + Decision::Allow + ); + + // 2. [gateway] section seed overwrites the env seed for its platform + let gw = config::parse_config_str( + r#" +[gateway] +url = "ws://gw:8080/ws" +platform = "telegram" +allowed_users = ["alice"] +"#, + "test", + ) + .unwrap() + .gateway + .unwrap(); + reg.insert("telegram", gateway_section_trust(&gw)); + assert_eq!( + reg.get("telegram").decide("c", false, "alice"), + Decision::Allow + ); + assert_ne!( + reg.get("telegram").decide("c", false, "bob"), + Decision::Allow + ); + + // 3. [telegram] platform section (platform_trust_override) wins last + reg.insert( + "telegram", + TrustConfig::new(Some(true), vec![], None, Some(false), vec!["bob".into()]), + ); + assert_eq!( + reg.get("telegram").decide("c", false, "bob"), + Decision::Allow + ); + assert_ne!( + reg.get("telegram").decide("c", false, "alice"), + Decision::Allow + ); + } + #[test] fn complete_wecom_section_enables_unified_startup_without_env() { let cfg = config::parse_config_str( From dff3b3352426069183bb65305e3910c1d026d4a2 Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Mon, 13 Jul 2026 15:11:04 -0400 Subject: [PATCH 2/2] fix(gateway): deny-echo must not be awaited inside the WS event loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review catch: the gate helper awaited send_message inline in the WS select arm. In streaming mode send_gateway_reply registers a pending request and waits (5s timeout) for a GatewayResponse — which is dispatched by that same loop arm. Every DenyIdentity would therefore stall ALL event processing for the full reply timeout; distinct untrusted senders on a public bot could chain these stalls into a DoS. Restructure: gate_gateway_event is now a sync decision helper returning GateOutcome { Allow | Deny { echo } } — the caller chooses delivery. The WS path spawns the echo onto the JoinSet (never blocking the loop, matching the existing fire-and-forget pattern for slash responses); the unified path (axum/bridge task context) awaits it directly as before. Rationale documented on the enum and both call sites. --- crates/openab-core/src/gateway.rs | 75 ++++++++++++++++++++++--------- 1 file changed, 55 insertions(+), 20 deletions(-) diff --git a/crates/openab-core/src/gateway.rs b/crates/openab-core/src/gateway.rs index e67bdd88c..caac2441e 100644 --- a/crates/openab-core/src/gateway.rs +++ b/crates/openab-core/src/gateway.rs @@ -889,9 +889,25 @@ pub async fn run_gateway_adapter( // identity) — same per-platform registry as // the unified path. Placed before slash // handling so untrusted senders cannot - // execute /reset|/cancel|/config. - if !gate_gateway_event(&router, adapter.as_ref(), &event).await { - continue; + // execute /reset|/cancel|/config. The echo + // is SPAWNED, never awaited here: streaming + // send_message waits for a GatewayResponse + // that only this loop can dispatch, so an + // inline await would stall all event + // processing for the reply timeout. + match gate_gateway_event(&router, &event) { + GateOutcome::Allow => {} + GateOutcome::Deny { echo } => { + if let Some((echo_channel, msg)) = echo { + let echo_adapter = adapter.clone(); + tasks.spawn(async move { + let _ = echo_adapter + .send_message(&echo_channel, &msg) + .await; + }); + } + continue; + } } info!( @@ -1259,30 +1275,39 @@ fn echo_allowed(key: &str) -> bool { } } +/// Outcome of the shared ingress trust gate. `Deny { echo }` carries the +/// throttled request-access echo payload (channel + message) when the deny is +/// an identity deny and the per-sender throttle admits it; the CALLER decides +/// how to deliver it. Delivery must NOT be awaited inline inside the WS event +/// loop: `GatewayAdapter::send_message` (streaming mode) waits for a +/// `GatewayResponse` that is dispatched by that same loop — awaiting there +/// would stall all event processing for the reply timeout. The WS path spawns +/// the echo; the unified path (axum/bridge task) awaits it directly. +enum GateOutcome { + Allow, + Deny { echo: Option<(ChannelRef, String)> }, +} + /// Shared ingress trust gate for gateway events — used by BOTH the standalone /// WebSocket path (`run_gateway_adapter`) and the unified path /// (`process_gateway_event`), so L2 (channel scope) and L3 (identity) are /// enforced by the same per-platform registry regardless of deployment mode /// (#1356 Phase 1c prerequisite). /// -/// Returns `true` if the event may proceed. On `DenyIdentity`, sends the -/// throttled request-access echo via `adapter`; on `DenyScope` (and any future -/// variant), drops silently — scope is not a security boundary, so no echo. +/// On `DenyIdentity`, returns the throttled request-access echo payload; on +/// `DenyScope` (and any future variant), denies silently — scope is not a +/// security boundary, so no echo. /// /// Phase 1: `is_dm = false` preserves today's behavior where gateway DMs are /// evaluated against the channel allowlist like any other channel (the /// `allow_dm` surface semantics arrive with the per-platform trust flip). /// TODO(phase-2): derive is_dm from the event/ChannelRef carrier so the /// `allow_dm` L2 surface can be enforced and tested for gateway platforms. -async fn gate_gateway_event( - router: &crate::adapter::AdapterRouter, - adapter: &dyn ChatAdapter, - event: &GatewayEvent, -) -> bool { +fn gate_gateway_event(router: &crate::adapter::AdapterRouter, event: &GatewayEvent) -> GateOutcome { let decision = router.gate_incoming(&event.platform, &event.channel.id, false, &event.sender.id); match decision { - crate::trust::Decision::Allow => true, + crate::trust::Decision::Allow => GateOutcome::Allow, crate::trust::Decision::DenyIdentity => { // L3 identity deny → echo the sender their ID so they can request // access (throttled to avoid amplification). Bots never reach here @@ -1294,7 +1319,7 @@ async fn gate_gateway_event( "gateway event denied (identity); echoing request-access" ); let throttle_key = format!("{}:{}", event.platform, event.sender.id); - if echo_allowed(&throttle_key) { + let echo = if echo_allowed(&throttle_key) { let echo_channel = ChannelRef { platform: event.platform.clone(), channel_id: event.channel.id.clone(), @@ -1302,13 +1327,15 @@ async fn gate_gateway_event( parent_id: None, origin_event_id: Some(event.event_id.clone()), }; - let echo = format!( + let msg = format!( "⚠️ You are not on this bot's trusted list.\nYour ID: {}\nAsk the admin to add it to allowed_users.", event.sender.id ); - let _ = adapter.send_message(&echo_channel, &echo).await; - } - false + Some((echo_channel, msg)) + } else { + None + }; + GateOutcome::Deny { echo } } // DenyScope (and any future variant) → silent drop (scope is not a // security boundary; no request-access echo). @@ -1320,7 +1347,7 @@ async fn gate_gateway_event( ?decision, "gateway event denied (scope); silent" ); - false + GateOutcome::Deny { echo: None } } } } @@ -1351,8 +1378,16 @@ pub async fn process_gateway_event( } // Shared ingress trust gate (L2 scope + L3 identity), keyed by platform. - if !gate_gateway_event(&ctx.router, ctx.adapter.as_ref(), &event).await { - return Ok(false); + // Awaiting echo delivery here is safe: this runs on the axum/bridge task, + // not inside the WS event loop. + match gate_gateway_event(&ctx.router, &event) { + GateOutcome::Allow => {} + GateOutcome::Deny { echo } => { + if let Some((echo_channel, msg)) = echo { + let _ = ctx.adapter.send_message(&echo_channel, &msg).await; + } + return Ok(false); + } } tracing::info!(