Skip to content

feat(trust): Phase 1 (gateway) — route gateway ingress through the shared gate#1267

Merged
thepagent merged 2 commits into
mainfrom
feat/trust-phase1a-carriers
Jul 1, 2026
Merged

feat(trust): Phase 1 (gateway) — route gateway ingress through the shared gate#1267
thepagent merged 2 commits into
mainfrom
feat/trust-phase1a-carriers

Conversation

@chaodu-agent

Copy link
Copy Markdown
Collaborator

Phase 1 of #1264 (plan), scoped to the gateway path (Telegram/LINE/Feishu/WeCom/Google Chat/Teams). Builds on Phase 0 (#1266).

Behavior-preserving — no message that's accepted today is denied, and vice-versa.

What this does

  • AdapterRouter gains a PlatformTrustConfigs registry (via a with_trust() builder, so new()'s signature and all its callers stay unchanged) + a single gate_incoming(platform, channel_id, is_dm, sender_id) -> Decision ingress gate.
  • process_gateway_event() now enforces L2 (scope) + L3 (identity) via router.gate_incoming(). should_skip_event() keeps only its structural checks (bot filter + @mention gating); its channel/user checks are neutered in the unified path (passed allow-all).
  • The registry is built at startup from the same GATEWAY_* env the bridge already uses, keyed per gateway platform.

Why it addresses F1 (from #1266 review)

The gateway's single choke point is process_gateway_event — the gate now runs there for every gateway event before any dispatch path. Discord/Slack (two-entry submit-vs-handle_message) + making the dispatch entries pub(crate) follow in the next PR.

Behavior-preserving details

  • Registry defaults mirror today's should_skip_event (allow_all_* default true).
  • is_dm = false in Phase 1 → gateway DMs are evaluated against the channel allowlist exactly as today; the allow_dm surface semantics arrive with the per-platform trust flip.
  • One intentional micro-hardening: an empty sender_id is now denied even under allow_all_users (Phase-0 F7 fail-closed). Gateway events always carry a sender, so no practical impact.

Deferred

  • Discord/Slack routing through gate_incoming + privatizing submit/handle_message (next PR)
  • Removing should_skip_event's now-neutered L2/L3 + unused GatewayEventContext fields (Phase 1c)
  • Deny-all default flip (Phase 3)

Testing

  • cargo clippy --features unified -D warnings clean
  • 9/9 gateway tests pass; full openab-core suite green except the pre-existing macOS-only secrets::resolve_exec_nonzero_exit

Note: branch name (…phase1a-carriers) is a leftover from the original carrier-change plan; this PR takes the lower-churn ingress-parameter approach instead (no ChannelRef.is_dm field — avoids touching 47 constructors).

Refs #1264

… trust gate

Wires the shared L2/L3 gate (#1266) into the unified gateway path:
- AdapterRouter gains a PlatformTrustConfigs registry (via with_trust
  builder — new()'s signature unchanged) + gate_incoming() ingress gate
- process_gateway_event now enforces L2/L3 via router.gate_incoming();
  should_skip_event keeps only bot-filter + @mention gating (its
  channel/user checks are neutered in the unified path)
- registry built at startup from GATEWAY_* env, keyed per gateway platform

Behavior-preserving: registry defaults mirror today's should_skip_event
(allow-all default); is_dm passed false so DMs are evaluated as channels
exactly as today. Discord/Slack routing + dispatch-path privatization +
should_skip_event L2/L3 removal follow in later PRs. Deny-flip is Phase 3.

Refs #1264
@chaodu-agent chaodu-agent requested a review from thepagent as a code owner July 1, 2026 01:22
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@thepagent thepagent merged commit 22a3cb2 into main Jul 1, 2026
22 checks passed
@chaodu-agent

This comment has been minimized.

chaodu-agent added a commit that referenced this pull request Jul 1, 2026
…F1/F2)

F1 (blocker): Discord's is_denied_user has a !is_bot bypass (bot admission
is handled by allow_bot_messages + trusted_bot_ids). The shared L3 gate is
human-identity only, so running it on bots wrongly dropped trusted
bot-to-bot messages when allow_all_users=false (multi-agent). Guard the
gate with !sender.is_bot.

F2: pass the in-scope is_dm instead of hardcoded false (benign today with
the L2-open discord entry, but avoids latent risk).

Note: #1267 (gateway) is unaffected — should_skip_event's user check never
had a bot bypass, so the gateway gate already matched it for bots.
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

LGTM ✅ — Phase 1 trust gate correctly wires the shared ingress gate into the gateway path. Behavior-preserving confirmed by independent review across correctness, architecture, docs/UX, security/CI, and spec compliance.

What This PR Does

Wires the Phase 0 trust module (PlatformTrustConfigs) into the gateway ingress path via AdapterRouter::gate_incoming(), establishing L2 (scope) + L3 (identity) enforcement at the single gateway choke point (process_gateway_event). Scoped to 6 gateway platforms (Telegram/LINE/Feishu/WeCom/Google Chat/Teams); Discord/Slack follow in #1269.

How It Works

  1. AdapterRouter gains a trust: PlatformTrustConfigs field + builder with_trust() + gate_incoming() method — defaults to deny-all via Default, so new() callers are unaffected.
  2. process_gateway_event() passes allow_all=true / empty sets to should_skip_event (neutering L2/L3 there), then evaluates router.gate_incoming(platform, channel_id, false, sender_id) — returning early on deny.
  3. main.rs builds the registry from GATEWAY_* env vars, defaulting allow_all_channels=true / allow_all_users=true → behavior-preserving with today's open gateway.

Findings

# Severity Finding Location
1 🟡 env_bool empty string = trueGATEWAY_ALLOW_ALL_USERS="" evaluates as true (because "" != "0" and !"".eq_ignore_ascii_case("false")). Behavior-preserving today (default is true), but becomes a fail-open vector after Phase 3 deny-flip. One-line fix: add || v.is_empty() to falsy check. main.rs:270
2 🟡 Neutered params clone-and-drift riskshould_skip_event called with allow_all=true + empty HashSet::new(). If future code copy-pastes this pattern without following up with gate_incoming, it fails open. Suggest // SAFETY: gate_incoming below enforces L2+L3 comment. gateway.rs:1150-1155
3 🟡 GATEWAY_ALLOWED_USERS without ALLOW_ALL=false doesn't auto-restrict — Unlike Discord/Slack's resolve_allow_all (list non-empty → auto-tighten), gateway path requires explicit GATEWAY_ALLOW_ALL_USERS=false. Asymmetric UX; tracked for Phase 3 convergence. main.rs:279
4 🟡 TrustConfig::new(None, ...) defaults — Phase 0's TrustConfig::new() defaults allow_all_users to false (deny-all), but Phase 1 overrides with Some(true). API contract gap between "library default" and "Phase 1 runtime default" needs ADR/rustdoc clarification. main.rs:279 vs trust.rs
5 🟢 Per-event HashSet::new() allocation for neutered params — micro-optimization opportunity (use static or const), not a correctness issue. gateway.rs:1157
6 🟢 Deny tracing at info level will be noisy after Phase 3 deny-flip under high DenyScope volume. Suggest DenyScope → debug, DenyIdentity → info for production. gateway.rs:1174-1180
7 🟢 gate_incoming() docstring says "dispatch paths should only be reachable after Allow" but doesn't enforce at type level. Suggest adding # Contract section to rustdoc. adapter.rs:489-500
8 🟢 GATEWAY_ALLOWED_CHANNELS / GATEWAY_ALLOWED_USERS env vars not yet documented in README or .env.example. Non-blocking for Phase 1 (allow-all default). main.rs:275-277
9 🟢 Builder pattern with_trust() is elegant — avoids modifying new() signature, protects all existing call sites. adapter.rs:485
10 🟢 Case-insensitive platform key normalization (lowercase) prevents bypass via casing mismatch. trust.rs:167
11 🟢 Behavior-preserving confirmed: allow_all_*=true matches old should_skip_event defaults; no traffic newly denied or admitted.
12 🟢 gate_incoming placement correct: structural filter (bot + @mention) → trust gate → dispatch. Single choke point. gateway.rs:1161→1165→1185+
13 🟢 Empty sender_id fails closed at L3 even under allow_all_users=true — intentional micro-hardening, no practical impact. trust.rs:116
14 🟢 Unregistered platforms → PlatformTrustConfigs::default() → L3 deny-all. Fail-closed by construction. trust.rs
Finding Details

🟡 F1: env_bool empty string handling

// Current:
let env_bool = |k: &str, default: bool| {
    std::env::var(k)
        .map(|v| v != "0" && !v.eq_ignore_ascii_case("false"))
        .unwrap_or(default)
};

// Suggested (one-line addition):
        .map(|v| !v.is_empty() && v != "0" && !v.eq_ignore_ascii_case("false"))

Today this is behavior-preserving (default is true anyway). But after Phase 3 deny-flip, GATEWAY_ALLOW_ALL_USERS="" would be a fail-open. Low cost to fix now.

🟡 F2: SAFETY comment for neutered params

The allow_all_channels: true + empty allowed_channels pattern works because gate_incoming runs immediately after. A // SAFETY: comment prevents future copy-paste drift.

🟡 F3: resolve_allow_all asymmetry

Discord/Slack auto-derives allow_all=false when a non-empty allowlist is set. Gateway path requires explicit env. Tracked for Phase 3 convergence — not a blocker today since both paths are allow-all.

🟡 F4: API contract documentation

TrustConfig::new(None, ...) → library default allow_all_users=false (deny-all). Phase 1 runtime passes Some(true). This is correct and intentional, but the gap between "library safe default" and "Phase 1 runtime config" should be documented in the ADR or rustdoc to prevent confusion in Phase 3 migration.

What's Good (🟢)
  • Low-churn design: with_trust() builder avoids touching 47 constructors and keeps new() stable.
  • Behavior-preserving defaults: allow_all_channels=true + allow_all_users=true exactly mirrors today's should_skip_event defaults.
  • Clean layering: should_skip_event retains structural role (bot/mention); L2/L3 moves to shared gate.
  • Fail-closed safety: unregistered platforms get deny-all; empty sender_id always denied.
  • Case normalization: platform keys lowercased prevents casing bypass.
  • Good observability: structured tracing with platform/sender/channel/decision fields.
  • Correct gate placement: structural → trust → dispatch ordering verified.
Addressing Previous Review Findings
# Previous Finding Resolution
🟡 F1 (Round 1) Gateway-only scope not documented in ADR Tracked in #1269
🟡 F2 (Round 1) is_dm=false hardcode leaves allow_dm untestable Addressed in bc49f0a — TODO comment added
🟡 F3 (Round 1) Batched dispatch paths ungated Tracked in #1269 — clarified: gateway Thread/Lane modes ARE gated (gate runs before dispatcher.submit in process_gateway_event)
Spec Alignment Summary

ADR Contract Compliance:

Phase 0→1 Contract:

  • ✅ API: all Phase 0 exports consumed correctly
  • ✅ Behavior: defaults match; fail-closed invariants preserved
  • ✅ Deferred items tracked with blocking relationship to Phase 3
Baseline Check

Verdict: LGTM ✅ — Reviewed by 5 independent reviewers across correctness, architecture, docs/UX, security/CI, and spec compliance. Four new 🟡 findings are all non-blocking (env edge case, comment hygiene, Phase 3 prep documentation). Core behavior-preserving invariant confirmed. Previous round's 3 🟡 findings all addressed (TODO in bc49f0a, tracking in #1269). Ready to merge; 🟡 items can be addressed in follow-up or Phase 3 prep.

thepagent pushed a commit that referenced this pull request Jul 1, 2026
…canary before merge] (#1270)

* feat(trust): Phase 1 (discord) — gate L3 identity via shared gate

Routes Discord ingress through AdapterRouter::gate_incoming for the L3
(identity) layer, keyed under "discord" in the trust registry.

- registry "discord" entry: L2 open + allow_dm=true (Discord's richer
  channel/thread/DM logic stays in the adapter — the flat allowed_channels
  model can't express thread-by-parent admission); L3 mirrors resolved
  [discord].allow_all_users/allowed_users
- gate call added at the Discord dispatch spawn, redundant-but-matching
  with Discord's existing pre-dispatch user check → non-regressive by
  construction (cannot deny what already passed)

Behavior-preserving. Phase 1c makes the gate authoritative and removes the
scattered user check; richer Discord L2 modeling + dispatch privatization
tracked in #1269.

Refs #1264 #1269

* fix(trust): Discord gate skips bots + passes real is_dm (review #1270 F1/F2)

F1 (blocker): Discord's is_denied_user has a !is_bot bypass (bot admission
is handled by allow_bot_messages + trusted_bot_ids). The shared L3 gate is
human-identity only, so running it on bots wrongly dropped trusted
bot-to-bot messages when allow_all_users=false (multi-agent). Guard the
gate with !sender.is_bot.

F2: pass the in-scope is_dm instead of hardcoded false (benign today with
the L2-open discord entry, but avoids latent risk).

Note: #1267 (gateway) is unaffected — should_skip_event's user check never
had a bot bypass, so the gateway gate already matched it for bots.

* test(trust): name + test the Discord L3 bot-bypass (review #1270 F4)

Extract the gate's bot-skip into l3_gate_applies(is_bot) and add a
regression test (l3_gate_skips_bots_admits_humans) locking in that bots
bypass the shared L3 gate (mirrors is_denied_user's !is_bot), so
trusted/mode-admitted bots aren't denied when allow_all_users=false.

---------

Co-authored-by: chaodu-agent <chaodu-agent@users.noreply.github.com>
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