fix(gateway): read adapter allowlists through profile secret scope - #88559
JiuYue0820 wants to merge 7 commits into
Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
CR 88559 — this still needs changes before merge.
The intended correction is sound: secondary profiles must not borrow the default profile’s allowlists or allow-all flags from os.environ. But this patch repairs only the final read. Matrix still transports profile-local YAML through the process-global environment, which causes named-profile allowlists to disappear. Signal also deliberately converts a missing profile allowlist into "*", opening its pre-auth reaction surface. The new tests exercise helper functions rather than the production behavior they are supposed to secure.
- [P1] Named-profile Matrix YAML allowlists are silently erased
The changed constructor now reads MATRIX_ALLOWED_ROOMS and MATRIX_ALLOWED_USERS through _startup_env_secret(). Under an active multiplex scope, that is authoritative: if the key is absent from the profile scope, get_secret() returns the supplied default and deliberately does not fall through to the process environment. Profile scopes are built from the profile’s .env and external secret providers—not its config.yaml.
Matrix’s YAML loader still converts matrix.allowed_users and matrix.allowed_rooms into process-global os.environ values and returns None, so neither value is seeded into PlatformConfig.extra. The hook explicitly says “everything flows through env.” Those two designs no longer compose.
For a named profile configured only in config.yaml:
matrix:
allowed_users:
- "@operator:example.org"
allowed_rooms:
- "!private:example.org"the profile scope does not contain either MATRIX_* key. The new helper therefore returns "", ignoring the YAML-derived value in os.environ. allowed_rooms becomes empty, so the room whitelist disappears; allowed_users is likewise lost at this adapter layer.
This needs an end-to-end profile-local source path. _apply_yaml_config should seed these effective values into PlatformConfig.extra (or another profile-local config carrier), while retaining legacy env bridging only where needed for single-profile callers. Then both constructor reads should prefer config.extra and use the scope-aware environment lookup only as fallback.
The regression needs to create a named profile with YAML-only Matrix allowlists, call load_gateway_config() under that profile’s runtime scope, construct MatrixAdapter, and assert the effective _allowed_rooms and _allowed_user_ids.
- [P2] A missing Signal profile allowlist becomes pre-auth allow-all
The new test explicitly locks in this behavior:
signal_secret("SIGNAL_ALLOWED_USERS", "*") == "*"when the secondary profile has no SIGNAL_ALLOWED_USERS. The constructor parses "" into dm_allow_from. Its own comment says "" means all users and explains this local gate exists because reactions execute before the central authorization gate.
That is the wrong scoped-miss default. Signal’s policy distinguishes:
- explicit SIGNAL_ALLOWED_USERS => restricted DMs
- no allowlist => pairing/denial for unknown senders
- SIGNAL_ALLOW_ALL_USERS=true => explicit open access
This does not grant full agent execution because central auth still exists, but it does allow any sender to receive the pre-auth 👀 reaction, revealing that the bot is live and monitoring the conversation.
The scoped default should be empty, with any legacy unscoped/default-profile compatibility handled separately. One default argument should not represent both scoped-miss semantics and unscoped startup fallback.
Add a real regression that instantiates SignalAdapter, builds an unauthorized sender event, and verifies _reactions_enabled(event) is false when the active profile lacks an allowlist.
- Blocking coverage gap: the tests never execute the changed consumers
Every added test calls a helper alias directly. None of them constructs MatrixAdapter or SignalAdapter, loads a profile config.yaml, checks MATRIX_ALLOWED_ROOMS, invokes Signal’s _reactions_enabled, invokes Email’s _allow_all_senders/_allowlist_in_effect/_dispatch_message, invokes WhatsApp’s _open_dm_opted_in, exercises the unscoped default-profile fallback, or proves that actual intake behavior differs between two profiles.
For an authorization-boundary change, the minimum matrix should cover:
- scoped profile value wins over conflicting process env;
- scoped miss does not borrow process env;
- YAML-only named-profile value survives into the adapter;
- unscoped default-profile startup keeps documented compatibility behavior;
- actual allowed and unauthorized events produce the expected adapter decision.
The missed other side of the shape is that the architectural unit is not merely “replace os.getenv with get_secret.” It is:
profile-local source -> profile-local normalized configuration -> profile-local runtime scope -> adapter authorization decision
This PR repairs only the final arrow for .env-backed values. Matrix’s YAML bridge crosses into process-global state before the new helper runs, so changing the reader alone cannot close the class. The source propagation and effective adapter behavior have to be tested together.
The Email and WhatsApp substitutions themselves look correct; I did not find a corresponding functional defect in those two hunks.
Re-review — one blocking precedence regression remains at
|
|
Addressed the remaining precedence regression at 5ad3eb58. Matrix allowlists now resolve as:
A scoped New regressions in
Local: |
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review at head 3086d363 (the 5ad3eb58 precedence fix). Two independent adversarial passes plus live probes against a temp HERMES_HOME under a real secondary-profile scope, and the real focused suites.
What is genuinely closed at this head (verified)
- Matrix adapter-construction precedence:
scoped secret > YAML config.extra > unscoped process env, fail-closed on total miss (empty set → deny), explicit-empty scoped value beats YAML. Probe-verified; named-profile YAML loads run inside_profile_runtime_scopeso they stay profile-local. - Signal: all four reads (
SIGNAL_GROUP_ALLOWED_USERS,SIGNAL_REQUIRE_MENTION,SIGNAL_ALLOWED_USERS,SIGNAL_REACTIONS) via_startup_env_secret; scoped miss → empty (no more silent"*"default). - Email: all three sites now route
GATEWAY_ALLOW_ALL_USERS/GATEWAY_ALLOWED_USERSthrough_get_secret. - WhatsApp:
_open_dm_opted_innow_get_wsecret;_live_dm_allow_fromalready scoped (#87698). - Tests:
test_adapter_allowlist_secret_scope.py13/13; the broader focused set (signal/email/multiplex/whatsapp-scope/matrix-approval/matrix) 110–112 per file; all green.
But the class is not closed as claimed
- Primary admission gate still borrows the default profile's env (HIGH). The real authorization gate
_is_user_authorizedreads allowlists viagateway/authz_mixin.py::_auth_env(31-43), which on a scoped miss — or an explicitly-empty scoped value — falls through toos.environ(the default profile's bridged env). End-to-end probe against the real method under a profile-B scope with no allowlist keys: the default profile's MATRIX/SIGNAL/TELEGRAM allowlisted users are authorized under profile B; with the default env carrying onlyGATEWAY_ALLOW_ALL_USERS=true, any user on any platform is authorized under profile B. This is the exact class this PR claims to fix, on the gate where authorization actually happens._auth_envis untouched by this PR. The new construction-time gates don't rescue: Matrix has no per-message user gate of its own (room gate only), and Signal explicitly delegates DM auth to_is_user_authorized. - Same-file and sibling bare
GATEWAY_ALLOW_ALL_USERSreads remain (HIGH). Two remain inplugins/platforms/matrix/adapter.py— the file this PR modified:_on_invite:3828and_validate_matrix_prompt_reactor:4199(auto-join/presence exposure and attacker-controlled prompt-reactor under allow-all), plus bare sibling keysMATRIX_FREE_RESPONSE_ROOMS:1293andMATRIX_IGNORE_USER_PATTERNS:1406. Tree-wide, five more adapters readGATEWAY_ALLOW_ALL_USERSbare in DM-intake/_open_dm_opted_ingates:weixin.py:1540,qqbot/adapter.py:3198,yuanbao.py:1285,feishu/adapter.py:4375,wecom/adapter.py:896. With the default profile setting the flag, any secondary profile's DM gate returns True →dm_policyallowlist bypassed, every inbound DM reaches the agent — and the secondary cannot opt out because its own.envvalue is never consulted by the bare read. The PR body's "remaining intake call sites" is an overstatement of coverage. - Precedence contract is inconsistent (MED). (a) Signal's precedence is the mirror of Matrix:
signal.py:292-298readsextra.get("allowed_users")before the scoped secret, so for Signal a YAMLplatforms.signal.extravalue beats a scoped secret — opposite of Matrix. (b) YAMLconfig.extranow beats a pre-set process env everywhere, including multiplex-off single-profile deployments, inverting the documented "Env vars take precedence over YAML" behavior — a wider stale YAML allowlist can shadow a tighter env override with no multiplex involved. - Fail-loud → fail-silent wrapper downgrade (MED, design). The new wrappers (
_startup_env_secret,_get_esecret,_get_wsecret) catchUnscopedSecretErrorand convert it into a permissiveos.getenvread, instead of the secret_scope module's documented crash-loud-on-unscoped-multiplex contract. It holds today only becauserun.pywraps every secondary adapter construction in_profile_runtime_scope; any future unscoped construction of a secondary adapter silently re-opens the leak with no error, and no test exercises that fallback undermultiplex_active=True.
Coverage gap
The two new regressions (test_scoped_env_beats_yaml_extra, test_yaml_extra_beats_unscoped_process_env) pin construction-time precedence only. Neither touches _is_user_authorized/_auth_env — the path where the leak lives. No test covers the in-file Matrix allow-all sites, the sibling adapters, or free_response_rooms/ignore_user_patterns.
Claim correction
The PR body's "104 passed" for its own 5-file command is wrong at this head: real count is 111 (allowlist_scope 13 + matrix_recovery_key_scope 6 + email 39 + signal 52 + matrix_approval_reaction 1).
Bottom line
The four touched surfaces are fixed correctly and fail-closed, and the precedence regression at 5ad3eb58 is real progress. But as a class fix ("a secondary profile could inherit the default profile's bridged env allowlists") it is incomplete: the primary admission gate (_auth_env) still leaks the default profile's allowlists/allow-all to every secondary profile, and 8 bare allow-all reads plus Matrix sibling keys remain — two in the same file this PR edited. Recommend closing the _auth_env fallback (or routing it through _platform_gate_env's authoritative semantics) and migrating the remaining bare reads — especially the in-file Matrix ones — before this is presented as closing the class. The same-class reference to #69090/#59739 is accurate for the four migrated surfaces, not for the class as a whole.
Thank you for your help! |
|
suggesting changes The adapter allowlist reads are profile-scoped, but a primary shared-transport message routed to a secondary profile is still authorized using the default profile secret scope. A user allowed only by the default profile allowlist can therefore cross the profile boundary.
Security evidence:
Review setup: I reviewed a run-owned local rebase or patch replay against current GitHub Not checked:
Signed: GPT-5.6-luna-max in Codex |
Matrix, Signal, Email and WhatsApp still used os.getenv for some allowlist and allow-all gates. Under multiplex that borrows the default profile's bridged env. Route those reads through the existing scope-aware helpers so a secondary profile cannot inherit another profile's authorization. Co-authored-by: FirmamentalSpring <287222957+FirmaSpring@users.noreply.github.com>
Review on NousResearch#88559: Matrix YAML allowlists must seed PlatformConfig.extra instead of disappearing into a scoped getenv miss. Signal's scoped-miss default is empty, not "*", so unauthorized senders do not get a pre-auth reaction. Tests now construct the adapters and exercise those decisions. Co-authored-by: FirmamentalSpring <287222957+FirmaSpring@users.noreply.github.com>
Cover the named-profile path the review asked for: config.yaml only, scoped load_gateway_config(), then MatrixAdapter effective allowlists. Co-authored-by: FirmamentalSpring <287222957+FirmaSpring@users.noreply.github.com>
A named-profile secret scope must outrank config.extra so a secondary profile cannot inherit another profile's YAML allowlist. YAML extra still beats the unscoped process environment. Co-authored-by: FirmamentalSpring <287222957+FirmaSpring@users.noreply.github.com>
Co-authored-by: FirmamentalSpring <287222957+FirmaSpring@users.noreply.github.com>
Co-authored-by: FirmamentalSpring <287222957+FirmaSpring@users.noreply.github.com>
Resolve the target profile before entering the primary shared-transport message handler so authorization reads the routed profile's allowlist instead of the default profile's secrets. Co-authored-by: FirmamentalSpring <287222957+FirmaSpring@users.noreply.github.com>
797877c to
4102f30
Compare
|
Addressed the remaining P1 on the primary shared-transport path.
Added a Discord integration regression that uses the real
Verification after rebasing the full branch onto current
|
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed exact current head 4102f30bb602f2a7d5ed9d5c965fc0b563f20a16 against the findings from my 3086d363 review.
The two load-bearing class holes I previously called out are materially repaired at this head:
- The primary admission gate no longer uses
_auth_envfor platform/global allow-all and allowlist decisions._is_user_authorized()now routes those reads through_platform_gate_env, so an installed multiplex profile scope is authoritative and a scoped miss no longer borrows the default profile's bridged process env. - The remaining bare DM/intake allow-all reads have been migrated across the sibling surfaces touched by this follow-up (including Weixin, QQBot, Yuanbao, Feishu, plus the Email/WhatsApp paths already in the original slice). Signal's pre-auth reaction gate is also closed on a scoped miss instead of manufacturing
"*". - The Signal/Matrix precedence work is now much closer to one contract: scoped profile value first, profile-local YAML next, unscoped process env only for legacy startup. The routed default-profile handler now resolves the profile home from the source before entering
_handle_message, which is the right place to keep admission reads inside the owning runtime scope.
I do not see a new code blocker in the current delta equivalent to the HIGH findings from the previous review.
One residual should stay explicit rather than disappear from the architecture discussion: several adapter helper wrappers still catch UnscopedSecretError and fall back to os.environ. The current production routing/construction paths are now doing the work required to install the profile scope before those reads, so I am not reopening this head on a hypothetical unscoped call. But that fallback is not itself a security boundary; future multiplex call sites must preserve the scope invariant or fail closed instead of assuming the helper will save them.
Exact-head repository verification is still absent: CI, Docker, Nix, and the label-rerun workflows for this SHA all completed as action_required with no executable test receipt. GitHub also currently reports the PR non-mergeable, so current-main composition plus a green exact-head matrix remain merge gates.
Net: prior authorization-class blockers are closed in source at 4102f30b; rebase/mergeability and hosted exact-head verification remain. I would not duplicate the earlier blocker review onto this head.
What does this PR do?
Several adapter-level allowlist / allow-all reads still used a bare
os.getenvafter sibling paths (Matrix recovery key, Feishu, EmailEMAIL_*, WhatsApp_get_wsecret) had already moved to a scope-aware helper. Undergateway.multiplex_profilesthe process environment can hold the default profile's bridged value, so a secondary profile would inherit that profile'sMATRIX_ALLOWED_USERS,SIGNAL_*allowlists, orGATEWAY_ALLOW_ALL_USERS/GATEWAY_ALLOWED_USERS.This is the same root cause as #69090 / #59739, at the remaining intake call sites. One PR, because the helper already exists and every site is the same "don't borrow another profile's env" change.
Related Issue
Same class as #69090 (Matrix recovery key) and #87132 / #72657 (Telegram / Slack). Those PRs do not cover these call sites. WhatsApp
_live_dm_allow_fromwas addressed in #87698; this PR only changes the leftoverGATEWAY_ALLOW_ALL_USERSread in_open_dm_opted_in.Fixes #
Type of Change
Changes Made
plugins/platforms/matrix/adapter.py:MATRIX_ALLOWED_USERS/MATRIX_ALLOWED_ROOMSvia existing_startup_env_secretgateway/platforms/signal.py: new_startup_env_secret; used forSIGNAL_GROUP_ALLOWED_USERS,SIGNAL_REQUIRE_MENTION,SIGNAL_ALLOWED_USERSplugins/platforms/email/adapter.py:GATEWAY_ALLOW_ALL_USERS/GATEWAY_ALLOWED_USERSvia existing_get_secretgateway/platforms/whatsapp_common.py:GATEWAY_ALLOW_ALL_USERSvia existing_get_wsecrettests/gateway/test_adapter_allowlist_secret_scope.py: scoped secondary sees its own value; scoped miss does not fall through to the default profile's environHow to Test
python -m pytest tests/gateway/test_adapter_allowlist_secret_scope.py tests/gateway/test_matrix_recovery_key_scope.py tests/gateway/test_email.py tests/gateway/test_signal.py tests/gateway/test_matrix_approval_reaction_fail_closed.py -q— 104 passedMATRIX_ALLOWED_USERS, process env set to@default:example.org→ helper returns empty (no borrow)Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -q.Documentation & Housekeeping
For New Skills
N/A
Screenshots / Logs
N/A
AI assistance disclosure: found during a code review of adapter allowlist reads; the fix, tests, and this description were prepared with AI assistance (Grok 4.6 via PokeAPI) and reviewed by the human submitter (FirmaSpring), who verified the tests locally.