Skip to content

feat(gateway)!: route standalone WS path through the shared trust gate#1391

Merged
thepagent merged 2 commits into
mainfrom
feat/1356-ws-path-shared-gate
Jul 13, 2026
Merged

feat(gateway)!: route standalone WS path through the shared trust gate#1391
thepagent merged 2 commits into
mainfrom
feat/1356-ws-path-shared-gate

Conversation

@chaodu-agent

@chaodu-agent chaodu-agent commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

What problem does this solve?

The standalone-gateway WebSocket path was the last ingress that bypassed the shared trust registry (#1356 Phase 1c prerequisite). run_gateway_adapter enforced channel/user allowlists via its own inline filter fed only by [gateway] values — so the per-platform [<platform>] trust sections (shipped across #1297#1385) had no effect on standalone deployments, and six doc callouts had to carry mode-scoping caveats explaining that gap.

Before vs after

BEFORE                                      AFTER
======                                      =====
unified path:                               unified path:
  should_skip_event (structural)              should_skip_event (structural)
  → gate_incoming (registry)                  → gate_gateway_event (shared helper)
                                                └ gate_incoming (registry)
WS path:                                    WS path:
  should_skip_event with [gateway]            should_skip_event (structural only)
  channel/user allowlists                     → gate_gateway_event (same helper,
  (registry NOT consulted;                      same registry, deny-echo included)
   [<platform>] sections ignored)
                                            registry precedence (per platform):
                                              GATEWAY_* env  <  [gateway]  <  [<platform>]

How It Works

  1. gate_gateway_event (new shared helper in gateway.rs): gate_incoming + DenyIdentity → throttled request-access echo / DenyScope → silent drop. Both process_gateway_event (unified) and the WS loop call it — single deny-handling implementation, no drift between modes. On the WS path it runs before slash handling, so untrusted senders cannot execute /reset//cancel//config.
  2. Registry seeding (main.rs): the [gateway] section's trust values are inserted for its platform via gateway_section_trust() — same resolve_allow_all semantics the old inline filter used (behavior-preserving for the common case). Seeding order gives the documented precedence: GATEWAY_* env (uniform seed) < [gateway] < [<platform>] (platform_trust_override, which already runs in both modes).
  3. WS filter neutered to structural gating (bot admission + @mention), exactly like the unified path since feat(telegram): add allow_all_users/allowed_users to [telegram] config #1297.
  4. Dead code removed: GatewayParams/GatewayEventContext channel/user fields (the unified ones were already unread; the WS ones moved to the registry) and the unified block's GATEWAY_ALLOW_ALL_* env reads (the registry's uniform seed already consumes those vars).
  5. Docs: all six mode-scoping callouts (line/wecom/google-chat/msteams/config-reference ×2) rewritten — the sections now apply in both deployment modes with the precedence spelled out.

⚠️ Breaking Change

In standalone-gateway deployments that define both [gateway] allowlists and a first-class [<platform>] trust section with different values, the platform section now wins on the WS path (previously it was silently ignored there). This is the documented target state — config-first, most-specific section is authoritative. Migration: align the [<platform>] section values; they now govern that platform in both modes.

Known edge (documented, not changed): a standalone gateway forwarding events for platforms other than [gateway].platform — those events now resolve against the uniform GATEWAY_* env seed (or that platform's own section) instead of the [gateway] allowlists. Multi-platform standalone setups should use per-platform sections.

Self-review (round 2 commit)

Adversarial pass over my own round-1 implementation caught a real bug:

  • WS-loop deadlock/DoS via deny-echo (fixed): round 1 awaited send_message inline inside the WS select arm. In streaming mode send_gateway_reply waits (5s timeout) on a GatewayResponse dispatched by that same loop arm — every DenyIdentity would stall all event processing for the full timeout, and distinct untrusted senders could chain stalls into a DoS. gate_gateway_event is now a sync decision helper returning GateOutcome::{Allow, Deny{echo}}; the WS path spawns the echo onto its JoinSet (matching the existing fire-and-forget pattern for slash responses), the unified path awaits directly (axum/bridge context, safe). Rationale documented at the enum and both call sites.
  • Also verified: [gateway] registry seed runs after secret substitution (line 443 vs 347); registry insert/lookup both lowercase-normalize (no casing mismatch between [gateway].platform and event labels); the gate's spawned echo uses the same throttle (echo_allowed) as before, so amplification behavior is unchanged.

Validation

  • macmini: all-features core clippy, unified clippy, default clippy — all clean under -D warnings
  • cargo test -p openab-core --all-features — 672 pass (1 pre-existing macOS-only failure: secrets::tests::resolve_exec_nonzero_exit)
  • cargo test --features unified --bin openab — 18 pass (includes the 3 new tests)
  • New tests: gateway_section_trust_mirrors_ws_filter_semantics (section → TrustConfig semantics), gateway_trust_seed_precedence_env_lt_gateway_lt_platform (pins the three-layer precedence), ws_path_filter_is_structural_only (unknown users/channels pass the structural filter; bot/@mention gating stays)
  • rustfmt: zero new drift vs per-file baseline (repo has pre-existing drift)

Refs #1356 (Phase 1c prerequisite — unblocks #1357 double-gate elimination).

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 < [<platform>] 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 [<platform>] 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
[<platform>] 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.
@chaodu-agent

This comment has been minimized.

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.
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Note

LGTM ✅ — Clean architecture consolidation that unifies trust enforcement across both deployment modes, with excellent self-review catching a real deadlock/DoS vector.

What This PR Does

Routes the standalone-gateway WebSocket path through the same shared per-platform trust registry that the unified path already uses, eliminating the last ingress that bypassed it. This closes the gap where [<platform>] trust sections had no effect on standalone deployments.

How It Works

  1. gate_gateway_event — new sync helper returning GateOutcome::{Allow, Deny{echo}} that both WS and unified paths call, ensuring single deny-handling implementation with no drift.
  2. Registry seedinggateway_section_trust() seeds [gateway] values into the shared registry with documented precedence: GATEWAY_* env < [gateway] < [<platform>].
  3. WS filter neuteredshould_skip_event now handles structural gating only (bot admission + @mention); L2/L3 delegated to gate_gateway_event.
  4. Dead code removed — channel/user fields pruned from GatewayParams/GatewayEventContext; unified path env reads eliminated (registry already consumes them).
  5. WS-loop DoS fix — deny-echo spawned (not awaited) in WS path, preventing deadlock from streaming send_message waiting on the same loop.

Findings

# Severity Finding Location
1 🟢 Excellent self-review catching WS-loop deadlock/DoS before merge gateway.rs:885-915
2 🟢 Clean GateOutcome enum design — caller-decides-delivery pattern avoids tight coupling gateway.rs:1275-1280
3 🟢 Three well-targeted tests pin the exact properties the refactor relies on gateway.rs:1687+, main.rs:1566+
4 🟢 Docs updated consistently across all six platform pages docs/*
5 🟢 Breaking change documented with clear migration path PR description
What's Good (🟢)
  • Deadlock prevention: The round-2 self-review caught a genuine DoS vector — awaiting send_message inside the WS select arm would stall all event processing. The spawn-on-JoinSet fix is correct and matches the existing pattern for slash command responses.
  • Single source of truth: Both paths now call the same gate_gateway_eventgate_incoming chain. No more drift risk between deployment modes.
  • Precedence correctness: The seeding order in main.rs (env → [gateway][<platform>]) is enforced by insertion order with reg.insert overwriting, and pinned by a dedicated test.
  • Clean dead code removal: 4 fields removed from GatewayParams, 4 from GatewayEventContext, and the 18-line unified-path env-reading block — all provably unreachable after the registry handles L2/L3.
  • GateOutcome API: Making delivery the caller's responsibility is the right abstraction. The WS path spawns; the axum path awaits. Both are documented at the call site with rationale for why.
  • Test coverage: All three new tests target real invariants rather than implementation details — they will break correctly if the contract changes.
Baseline Check
  • PR opened: 2026-07-13
  • Main already has: shared trust registry (PlatformTrustConfigs), gate_incoming, per-platform sections, unified path using registry, WS path with inline allowlist filter
  • Net-new value: WS path routed through the same registry; [gateway] section trust seeded into registry; dead inline filter code removed; six doc callouts updated; WS-loop deadlock vector eliminated

5️⃣ Three Reasons We Might Not Need This PR

  1. Could wait for Phase 2 — The full trust flip (feat(feishu): migrate Feishu trust config to first-class [feishu] section (Phase 1) #1357) is the ultimate goal; this intermediate step adds a migration cliff for users with divergent [gateway]/[<platform>] configs. Counter: Phase 1c is a prerequisite for feat(feishu): migrate Feishu trust config to first-class [feishu] section (Phase 1) #1357 and the breaking change is the documented target state with clear migration guidance.

  2. Single-platform assumptiongateway_section_trust seeds only gw.platform; multi-platform standalone gateways are an acknowledged edge case rather than first-class. Counter: Multi-platform standalone is rare and documented; per-platform sections already cover it.

  3. Spawned echo is fire-and-forget — If the echo fails (WS disconnected mid-send), the denied user gets no feedback. Counter: This matches the existing slash-command pattern; the throttle already limits echo frequency; and a failed echo for an untrusted sender is an acceptable degradation.

All three are reasonably addressed — the PR is justified.

@thepagent thepagent merged commit ef12dcd into main Jul 13, 2026
38 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants