Skip to content

fix(security): fail closed when an own-policy gateway adapter has no allowlist - #45444

Closed
Que0x wants to merge 1 commit into
NousResearch:mainfrom
Que0x:fix/gateway-own-policy-fail-open
Closed

fix(security): fail closed when an own-policy gateway adapter has no allowlist#45444
Que0x wants to merge 1 commit into
NousResearch:mainfrom
Que0x:fix/gateway-own-policy-fail-open

Conversation

@Que0x

@Que0x Que0x commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

What

Own-policy gateway adapters (WhatsApp, WeCom, Weixin, QQBot, Yuanbao)
default dm_policy/group_policy to open, which forwards every
sender. The adapter-trust shortcut in _is_user_authorized then
blanket-trusted those platforms whenever no env allowlist was set, so
enabling one with only credentials authorized the entire external
network.

This is the fail-open SECURITY.md §2.6 names a code bug: "an allowlist
is required for every enabled network-exposed adapter ... code paths
that fail open when no allowlist is configured are code bugs."

Fix

Trust the adapter only when its effective policy for the chat type is
an actual allowlist restriction (the config-only allowlist case
#34515 protects). open / pairing / anything else falls through to
default-deny, where {PLATFORM}_ALLOW_ALL_USERS,
GATEWAY_ALLOW_ALL_USERS and the pairing flow stay the explicit
opt-ins to broader access.

  • gateway/authz_mixin.py — allowlist-only trust; new
    _adapter_group_policy helper for group/forum/channel traffic.
  • gateway/platforms/base.py — docstring corrected.

Config-only dm_policy: allowlist / allow_from keeps working; only
the silent open fail-open is removed.

Tests

scripts/run_tests.sh tests/gateway/test_config_driven_access_policy.py

  • Reworked the two tests that asserted open + no-allowlist →
    authorized (they encoded the fail-open) into fail-closed assertions.
  • Added regression tests: default open DM, explicit open DM, and
    open group all deny without an allowlist; allowlist policy stays
    trusted (DM + group).

Results

Suite Result
test_config_driven_access_policy.py 43 passed
Adjacent auth suites (unauthorized-dm, qqbot, whatsapp, yuanbao, bot-auth-bypass) 437 passed
Full tests/gateway/ no regressions vs base

…allowlist

Own-policy adapters (WhatsApp, WeCom, Weixin, QQBot, Yuanbao) default dm_policy/group_policy to "open", which forwards every sender. The gateway's adapter-trust shortcut in _is_user_authorized blanket-trusted those platforms when no env allowlist was set, so an operator who enabled one with only credentials authorized the entire external network -- the fail-open SECURITY.md section 2.6 forbids ("an allowlist is required for every enabled network-exposed adapter").

Trust the adapter only when its effective policy for the chat type is an actual "allowlist" restriction (the case NousResearch#34515 was protecting). "open"/"pairing"/anything else falls through to default-deny, where {PLATFORM}_ALLOW_ALL_USERS / GATEWAY_ALLOW_ALL_USERS and the pairing flow remain the explicit opt-ins.
@liuhao1024

Copy link
Copy Markdown
Contributor

Reviewed the full diff — this is a clean and well-structured security fix.

What it does: Changes the _is_user_authorized gate so that enforces_own_access_policy adapters are only trusted when their effective policy for the chat type is "allowlist", not "open". Previously, any adapter with dm_policy: open (the default) was treated as "already authorized at intake," which silently admitted every external sender with no operator-configured allowlist — a textbook fail-open.

Verified:

  • The new _adapter_group_policy helper mirrors _adapter_dm_policy exactly (same fallback chain: live adapter → config.extra).
  • The gate logic correctly dispatches on chat_type (group/forum/channel → group policy, else DM policy).
  • "pairing" DM policy falls through to default-deny (the pairing-store check above already denied unpaired senders).
  • GATEWAY_ALLOW_ALL_USERS and {PLATFORM}_ALLOW_ALL_USERS remain as explicit opt-ins.
  • Tests added for open-DM deny, open-group deny, default-open deny (no config), allowlist-DM trust, allowlist-group trust, and the pairing carve-out with allowlisted group.
  • Test docstrings updated to reflect the new semantics (no stale assertions).

No issues found. LGTM.

@liuhao1024

Copy link
Copy Markdown
Contributor

Verification review — LGTM.

Reviewed the full diff of this security fix. Key observations:

  1. Correct fail-closed semantics: The previous behavior trusted any enforces_own_access_policy adapter as "already authorized," which was a fail-open when the adapter's default policy was "open" (forwarding every sender). This fix correctly gates trust on effective_policy == "allowlist" — the only policy where the adapter actually restricted senders before forwarding.

  2. Symmetric DM/group handling: The new _adapter_group_policy() method mirrors _adapter_dm_policy() for group/forum/channel traffic, ensuring both chat types follow the same allowlist-or-deny logic. The source.chat_type in {"group", "forum", "channel"} dispatch selects the right policy reader.

  3. Pairing correctly handled: The old pairing carve-out (dm_policy == "pairing" → fall through) is no longer needed as a special case — pairing's effective DM policy is "pairing" (not "allowlist"), so the effective_policy == "allowlist" check naturally denies it. The test test_pairing_dm_policy_group_chat_still_trusted now uses group_policy: "allowlist" instead of "open", confirming group traffic is governed by its own policy.

  4. Test coverage is thorough: New tests cover dm_policy: open denied, group_policy: open denied, default open denied (via mock adapter _dm_policy attribute), and allowlist accepted for both DM and group. All own-policy platforms are parametrized.

  5. Comment updates are accurate: The enforces_own_access_policy docstring now correctly describes the flag as "trustworthy only under allowlist" rather than a blanket "already authorized" pass.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Security fix: own-policy gateway adapters with dm_policy: open or group_policy: open (the defaults) were previously trusted at the gateway level, constituting a fail-open. The fix changes the trust logic to only accept allowlist policy as authorization signal — open, pairing, and disabled all fall through to the default-deny path.

Security

  • Before: adapters defaulting to dm_policy: open forwarded every sender but were trusted by the gateway, silently allowing the whole external network (SECURITY.md violation)
  • After: only dm_policy: allowlist / group_policy: allowlist is trusted; open → default-deny; explicit opt-in still via GATEWAY_ALLOW_ALL_USERS / {PLATFORM}_ALLOW_ALL_USERS

Code Quality

  • New _adapter_group_policy() method mirrors _adapter_dm_policy() for group/forum/channel traffic
  • Well-documented with SECURITY.md §2.6 cross-references throughout
  • New tests cover: allowlist authorized, open DM denied, default open fails closed, group allowlist trusted, group open denied — all parametrized across all own-policy platforms

Looks Good

  • Clean, well-scoped security fix with comprehensive test coverage

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation labels Jun 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #45634 with your authorship preserved by rebase-merge.

Your original fail-closed hardening commit was cherry-picked onto current main, with a maintainer follow-up for the WeCom groups.<group_id>.allow_from case.

Merged PR: #45634

@teknium1 teknium1 closed this Jun 13, 2026

@aelrobaiy-source aelrobaiy-source left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do u recommend?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants