Skip to content

fix(mattermost): bridge allow_from config key to MATTERMOST_ALLOWED_USERS - #35531

Open
AhmetArif0 wants to merge 1 commit into
NousResearch:mainfrom
AhmetArif0:fix/mattermost-allow-from-yaml-bridge
Open

fix(mattermost): bridge allow_from config key to MATTERMOST_ALLOWED_USERS#35531
AhmetArif0 wants to merge 1 commit into
NousResearch:mainfrom
AhmetArif0:fix/mattermost-allow-from-yaml-bridge

Conversation

@AhmetArif0

Copy link
Copy Markdown
Contributor

What does this PR do?

Root cause: _apply_yaml_config() in the Mattermost adapter translates config.yaml keys into env vars, but it was missing the allow_fromMATTERMOST_ALLOWED_USERS mapping. Users who set mattermost.allow_from: [user1, user2] in config.yaml got no user filtering — the adapter read MATTERMOST_ALLOWED_USERS as empty and left access open to all users (fail-open security issue).

Fix: Add the allow_fromMATTERMOST_ALLOWED_USERS translation using the same list-join pattern as allowed_channels. Env var takes precedence over YAML (guarded by not os.getenv()), matching all other keys in the function.

Parity fix — Discord received the identical allow_from bridge in PR #35329 today; Mattermost was missed.

Related Issue

Parity fix — same pattern as PR #35329 (Discord allow_from bridge).

Type of Change

  • 🔒 Security fix
  • 🐛 Bug fix

Changes Made

  • plugins/platforms/mattermost/adapter.py: add 5-line allow_fromMATTERMOST_ALLOWED_USERS block to _apply_yaml_config()
  • tests/gateway/test_mattermost.py: add TestApplyYamlConfig with 7 tests covering list, single string, env precedence, absent key, and regression for existing keys

How to Test

pytest tests/gateway/test_mattermost.py::TestApplyYamlConfig -v --override-ini="addopts="

Checklist

  • Contributing Guide read | Conventional Commits | No duplicate PR
  • Single logical change | Tests added | Platform: macOS
  • Docs — N/A | Cross-platform — N/A

…SERS

_apply_yaml_config() translated require_mention, free_response_channels,
and allowed_channels from config.yaml to env vars, but omitted allow_from
→ MATTERMOST_ALLOWED_USERS. Users who set mattermost.allow_from in
config.yaml got no user filtering: the adapter read the empty env var and
left access open to everyone.

Fix: add allow_from → MATTERMOST_ALLOWED_USERS translation using the
same list-join pattern as the sibling keys. Env var still takes
precedence over YAML (guarded by not os.getenv). Parity fix — Discord
received the identical allow_from bridge in PR NousResearch#35329.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists comp/plugins Plugin system and bundled plugins comp/gateway Gateway runner, session dispatch, delivery area/config Config system, migrations, profiles labels May 30, 2026

@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

Bridges the allow_from config key to MATTERMOST_ALLOWED_USERS environment variable for the Mattermost adapter. Small, correct, well-tested.


Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks — current main still needs the Mattermost allow_from YAML-to-env bridge.

Problems

  • The stated fail-open impact no longer matches current main. Mattermost authorization reads MATTERMOST_ALLOWED_USERS (gateway/authz_mixin.py:365-375, 458-465); with no allowlist it falls through to default deny unless GATEWAY_ALLOW_ALL_USERS is explicitly enabled (gateway/authz_mixin.py:467-514). This is therefore a fail-closed config-parity bug: configured users remain unable to use the bot.
  • The new tests invoke the hook directly. The production behavior depends on registered hook dispatch in gateway/config.py:1274-1304; add a loader-level regression test, following tests/gateway/test_config.py:696-720, that writes Mattermost allow_from into temporary config.yaml and asserts MATTERMOST_ALLOWED_USERS is populated.

Suggested changes

  • Keep the proposed mapping, but revise the security description to reflect fail-closed behavior.
  • Cover the load_gateway_config() path as above.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added 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 sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants