Skip to content

fix(discord): isolate multiplex access policy per profile - #76166

Open
OneNoted wants to merge 1 commit into
NousResearch:mainfrom
OneNoted:fix/discord-multiplex-access-policy
Open

fix(discord): isolate multiplex access policy per profile#76166
OneNoted wants to merge 1 commit into
NousResearch:mainfrom
OneNoted:fix/discord-multiplex-access-policy

Conversation

@OneNoted

@OneNoted OneNoted commented Aug 1, 2026

Copy link
Copy Markdown

What does this PR do?

Under multiplexing, Discord authorization settings are bridged through process-global state. Depending on the setting and initialization path, one profile's values can therefore be observed by another profile's adapter, causing unauthorized acceptance or silent rejection.

This change captures a DiscordAccessPolicy for each adapter while that adapter's profile scope is active, then carries the receiving profile and its policy through admission, routing, persistence/restoration, voice, interactive views, and the gateway's second authorization gate. Multiplex config handoff uses PlatformConfig.extra instead of mutating process-global admission state. Explicit empty and false values remain authoritative, and each setting retains its existing environment/YAML precedence within the owning profile.

Transport provenance is persisted separately from the runtime routed profile so restored sessions continue to authorize through the bot/profile that received them. Explicit allowed/ignored channel boundaries remain hard constraints, including parent/thread and normalized channel context. Pairing remains profile-owned and fails closed when an explicitly stamped profile has no corresponding store.

Single-profile behavior remains compatible, including live environment/config reads and existing direct helper/view construction.

Why this scope is necessary

The receiving profile's authorization decision crosses several asynchronous boundaries after ingress. Each changed path preserves that same owner rather than introducing a separate behavior:

  • the Discord adapter performs the first user, role, bot, and channel gate;
  • the gateway performs a second authorization gate and resolves the owning pairing store;
  • persisted/restored session sources must retain transport ownership without persisting an evergreen role grant;
  • interactive component callbacks execute after the original message callback and must reuse the owning policy and channel boundary;
  • voice callbacks execute later and must retain the concrete receiving adapter and bot.

Leaving any one of these paths process-global would reopen the same cross-profile authorization boundary after the initial message was admitted.

Related Issue

Fixes #72348

This follows the already-public, security-labeled issue above; it does not disclose a new private vulnerability report.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Added an adapter-local Discord policy for users, roles, channels, gateway grants, pairing, bots, mentions, approval mentions, and history/missed-message backfill (plugins/platforms/discord/adapter.py).
  • Stopped multiplex YAML/profile loading from writing Discord admission values into shared process environment while preserving the legacy single-profile bridge.
  • Routed messages, slash commands, all six interactive view producers, username resolution, voice callbacks, retries/reconnects, and secondary adapters through the owning policy/bot.
  • Enforced allowed/ignored channel boundaries on interactive component clicks before user, role, or pairing grants.
  • Persisted the bot discriminator, transport-owning profile, parent channel, and adapter-verified authorization channel keys (gateway/session.py, gateway/platforms/base.py).
  • Resolved restored and secondary-default sources to their actual owning adapter and pairing store (gateway/authz_mixin.py, gateway/run.py).
  • Revalidated restored role-only sources against current membership through the transport-owning adapter rather than persisting an evergreen role grant.
  • Added synthetic two-profile regressions plus a temp-home two-profile config-loader/production-factory handoff path (tests/gateway/test_discord_multiplex_access_policy.py).

How to Test

  1. Focused policy and startup coverage:
    scripts/run_tests.sh tests/gateway/test_discord_multiplex_access_policy.py -q
  2. Discord, multiplex, pairing, authorization, session, and voice surface:
    files=$(python3 - <<'PY'
    from pathlib import Path
    patterns = ['test_discord*.py', 'test_*multiplex*.py', 'test_*pairing*.py', 'test_authorization.py', 'test_authz*.py', 'test_voice_command.py', 'test_session*.py']
    root = Path('tests/gateway')
    print(' '.join(str(p) for p in sorted({p for pattern in patterns for p in root.glob(pattern)})))
    PY
    ); scripts/run_tests.sh $files -q
  3. Complete gateway suite:
    scripts/run_tests.sh tests/gateway -q

Observed locally on Arch Linux / Python 3.11 after a clean merge with upstream main at 470cf66b0:

  • Focused policy suite: 37 passed.
  • Related 85-file selection: 596 passed.
  • Complete gateway suite: 4,517 passed; 2 WeCom callback tests fail because optional XML support is unavailable. The same 2 failures reproduce unchanged on upstream main at 470cf66b0 (4 passed, 2 failed in that module).
  • Shadowed-test-definition guard: 5 passed.
  • Ruff check, Python bytecode compilation, and git diff --check pass for all changed files.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Arch Linux, Python 3.11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A; no user-facing keys or setup steps were added
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide; changes use platform-neutral config/session paths
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

N/A — authorization behavior is covered by regression tests.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins platform/discord Discord bot adapter area/auth Authentication, OAuth, credential pools area/profiles Multi-profile isolation, HERMES_HOME scoping needs-decision Awaiting maintainer decision before any implementation sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades P2 Medium — degraded but workaround exists needs-repro Bug needs reproduction steps labels Aug 1, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #75970 and #72427 address the same Discord multiplex policy family using narrower scoped-read/per-adapter-extra approaches. This PR carries adapter policy and transport provenance through restored sessions and interaction paths, so it is a competing mechanism rather than a duplicate. Maintainer decision needed.

@egilewski

Copy link
Copy Markdown
Contributor

too large to review safely

This PR changes 1487 production lines before tests and docs. Please split it or add a focused justification if it should stay together.

Signed: GPT-5.6-terra-low in Codex

@teknium1

teknium1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for tracing the policy across ingress, restored-session, interaction, and voice paths. The underlying defect is confirmed on current main: Discord still reads process-global authorization inputs in plugins/platforms/discord/adapter.py:1158, :4418, :4487, and :8013, while its YAML bridge writes DISCORD_ALLOWED_USERS to os.environ at :9678.

The PR's diff addresses those concrete current-main paths with an adapter-local policy and carries the policy through the second gateway gate and persisted source metadata. I did not identify a specific blocking correctness defect from the read-only diff review.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit label Aug 1, 2026
@alt-glitch alt-glitch removed the needs-repro Bug needs reproduction steps label Aug 1, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Three PRs address #72348: #72427 starts replacing process-global Discord gate reads with per-adapter accessors, #75970 broadens that approach across Discord and Telegram, and #76166 introduces a fuller Discord policy object carried through ingress, gateway authorization, interactions, voice, and restored sessions. The diffs overlap on adapter-local gate isolation but differ materially in how they handle the downstream gateway gate and transport provenance.

Related pull requests

Duplicates

#72427 and #75970 substantially overlap on moving Discord authorization gates from process-global environment reads to per-adapter state; #75970 superseded #72427. #76166 overlaps with both on gate isolation but is not a pure duplicate because it additionally carries transport provenance and authorization context through the gateway, persistence, interactions, and voice paths.

Suggested consolidation

author action: rebase #76166 onto main, or split out the part that can merge. Preserve the adapter-owned downstream gateway authorization and persisted transport/channel provenance that address the reviewed gap left after #75970, while dropping code already implemented by merged #75970; this is consistent with the automated keep_open review's medium salvageability, while explicitly addressing the contributor's blocking review-size concern. Keep #72427 closed as superseded by #75970; #75970 is already merged and should remain the reference for the narrower adapter-local and Telegram work.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I72348(["issue #72348 (closed)"])
    P76166["PR #76166 (open)"]
    P76166 -->|best fix| I72348
    class I72348 closed
    class P76166 open
    class P76166 best
    class P76166 target
    click I72348 "https://github.com/NousResearch/hermes-agent/issues/72348"
    click P76166 "https://github.com/NousResearch/hermes-agent/pull/76166"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 3 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 229 kB of PR diffs, 20 kB of issue/PR text, 34 kB of discussion (17 comments), 7 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@alt-glitch alt-glitch removed the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Aug 3, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #75970 merged the core per-profile Discord admission gate. This conflicting PR retains broader restored-session and voice-policy propagation, so it needs a scoped rebase and maintainer decision rather than being treated as a duplicate.

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

Labels

area/auth Authentication, OAuth, credential pools area/profiles Multi-profile isolation, HERMES_HOME scoping comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists platform/discord Discord bot adapter sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Discord adapter allow/deny gates are process-global, breaking per-profile isolation under multiplex_profiles

5 participants