fix(email): pairing, decline and gateway grants reach the gateway instead of dying in the adapter pre-gate
EmailAdapter._sender_accepted runs before any MessageEvent exists and read only EMAIL_ALLOWED_USERS. Unset, it dropped every sender unless allow-all was on; set, it dropped everyone not listed. The gateway's own handling therefore never ran for email: platforms.email.unauthorized_dm_behavior "pair" (the setup wizard's "Use DM pairing") and "decline" sent nothing, and a sender admitted by GATEWAY_ALLOWED_USERS or an approved pairing was dropped.bb304b4914turned the empty-allowlist branch into drop-all after #50568 had made "pair" email's explicit opt-in. The gate now keeps a sender listed by address in EMAIL_ALLOWED_USERS or GATEWAY_ALLOWED_USERS, a sender the registered gateway authorization check admits (that is the only reader of the pairing store), and, under an explicit pair or decline, an unknown sender the gateway will answer. The default "ignore" still drops unknown senders before a MessageEvent exists, so the mail-loop guard fromfd9c32c0f2holds. Three guards keep the wider gate from widening access, and close two forged-From: paths main already had: - A sender admitted only so the gateway can answer it (pair or decline) must authenticate its From:, open access or not: the pairing code or refusal is mailed back to that address. A granted sender still needs it short of open access, since a pairing grant keys on From: just as the allowlist does. Open access follows the gateway's own order: EMAIL_ALLOW_ALL_USERS wins over a list, while GATEWAY_ALLOW_ALL_USERS beside a list admits nobody extra, so it no longer exempts a listed address from From: authentication either (on main a forged From: of a listed address got through there). - Open access comes from the gateway's own verdict when a check is registered. GATEWAY_ALLOW_ALL_USERS beside a GATEWAY_ALLOWED_USERS list grants a stranger nothing there, so the env flag alone no longer exempts one from From: authentication (that path mailed a pairing code to a forged From: on main too). - A sender whose local part alone matches an allowlist entry is dropped. The gateway's check also matches an address by its bare local part (#119446), so without this, GATEWAY_ALLOWED_USERS=alice (a chat username) would admit or pair alice@<any domain>. The lists are parsed as the gateway parses them, JSON list literals included, or '["alice"]' would slip past this guard. _allowlist_in_effect only served the old condition and is removed. The scope tests now assert the same scoped reads through _sender_accepted, with GATEWAY_ALLOWED_USERS covered as well. Measured end to end with the real GatewayRunner callback wired (adapter -> gateway ingress): - pair, decline, GATEWAY_ALLOWED_USERS and an approved pairing each went from 0 events reaching the gateway to 1. pair mails a pairing code, decline mails one refusal. - An unauthenticated From: in pair mode, for a paired address or for a GATEWAY_ALLOWED_USERS address still reaches nothing. - A bare GATEWAY_ALLOWED_USERS=stranger entry lets nothing from stranger@<domain> through, under ignore or pair. Without the local-part guard that mail reached the gateway in both. - The same holds for a JSON-literal list, and a pair-mode stranger with a forged From: under allow-all beside an EMAIL_ or GATEWAY_ALLOWED_USERS list reaches nothing. - The default still drops.
This commit is contained in:
John Paul Soliva
committed by
Teknium
Teknium
parent
92eaf21eb5
commit
b5a300fe34
@@ -29,7 +29,7 @@ from gateway.platforms.helpers import cancel_task
|
||||
from gateway.platforms.event import MessageEvent, MessageType
|
||||
from gateway.config import Platform, PlatformConfig
|
||||
from utils import is_truthy_value
|
||||
from gateway.platforms._shared import get_scoped_secret as _get_secret, coerce_port, send_error
|
||||
from gateway.platforms._shared import get_scoped_secret as _get_secret, coerce_port, decode_json_list_literal, send_error
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -595,31 +595,60 @@ class EmailAdapter(BasePlatformAdapter):
|
||||
for name in ("EMAIL_ALLOW_ALL_USERS", "GATEWAY_ALLOW_ALL_USERS"))
|
||||
|
||||
@staticmethod
|
||||
def _allowlist_in_effect() -> bool:
|
||||
"""True when EMAIL_/GATEWAY_ALLOWED_USERS gates access (without one the gateway default-denies, so the spoofable From: grants nothing)."""
|
||||
return any(_get_secret(name, "").strip() for name in ("EMAIL_ALLOWED_USERS", "GATEWAY_ALLOWED_USERS"))
|
||||
def _open_access() -> bool:
|
||||
"""True when the gateway admits any sender, so a forged From: gains nothing. The gateway's own order:
|
||||
EMAIL_ALLOW_ALL_USERS wins over a list, GATEWAY_ALLOW_ALL_USERS applies only while no list is set."""
|
||||
if _get_secret("EMAIL_ALLOW_ALL_USERS", "").strip().lower() in _TRUTHY:
|
||||
return True
|
||||
return (_get_secret("GATEWAY_ALLOW_ALL_USERS", "").strip().lower() in _TRUTHY
|
||||
and not any(_get_secret(name, "").strip() for name in ("EMAIL_ALLOWED_USERS", "GATEWAY_ALLOWED_USERS")))
|
||||
|
||||
def _answers_unknown_senders(self) -> bool:
|
||||
"""True when ``platforms.email.unauthorized_dm_behavior`` opts into ``pair`` or ``decline``."""
|
||||
behavior = (self.config.extra or {}).get("unauthorized_dm_behavior")
|
||||
return isinstance(behavior, str) and behavior.strip().lower() in {"pair", "decline"}
|
||||
|
||||
def _sender_accepted(self, sender_addr: str, msg_data: Dict[str, Any]) -> bool:
|
||||
"""Pre-dispatch sender gate: self, automated, allowlist, From: authentication."""
|
||||
"""Pre-dispatch sender gate: self, automated, authorization, From: authentication."""
|
||||
if sender_addr == self._address.lower():
|
||||
return False
|
||||
if _is_automated_sender(sender_addr, {}):
|
||||
logger.debug("[Email] Dropping automated sender at dispatch: %s", sender_addr)
|
||||
return False
|
||||
# Drop senders the gateway would never authorize before a MessageEvent (and thread context) exists —
|
||||
# otherwise a dispatch/authorization race can send a reply even though the handler returned None.
|
||||
allowed_raw = _get_secret("EMAIL_ALLOWED_USERS", "").strip()
|
||||
if not allowed_raw:
|
||||
if not self._allow_all_senders():
|
||||
logger.debug("[Email] Dropping sender at dispatch — EMAIL_ALLOWED_USERS is unset and open access is not opted in: %s", sender_addr)
|
||||
return False
|
||||
elif sender_addr.lower() not in {a.strip().lower() for a in allowed_raw.split(",") if a.strip()}:
|
||||
logger.debug("[Email] Dropping non-allowlisted sender at dispatch: %s", sender_addr)
|
||||
# Parsed like the gateway's allowlists (JSON list literals included), or '["alice"]' would dodge the guard below.
|
||||
listed = set()
|
||||
for raw in (allowed_raw, _get_secret("GATEWAY_ALLOWED_USERS", "")):
|
||||
raw = decode_json_list_literal(raw)
|
||||
listed.update(str(a).strip().lower() for a in (raw if isinstance(raw, list) else str(raw).split(","))
|
||||
if str(a).strip())
|
||||
if sender_addr.lower() in listed:
|
||||
granted = True
|
||||
elif sender_addr.split("@", 1)[0].lower() in listed:
|
||||
# The gateway's check also matches an address by its bare local part (#119446), so an entry like "alice"
|
||||
# would admit, or pair, alice@<any domain>; the domain is the sender's to choose.
|
||||
logger.debug("[Email] Dropping sender whose local part alone matches an allowlist entry: %s", sender_addr)
|
||||
return False
|
||||
# Reject spoofed senders (GHSA-rxqh-5572-8m77): the allowlist keys on the attacker-controlled
|
||||
# From:. Only matters when an allowlist GRANTS access and allow-all is off; fail-closed.
|
||||
if (self._require_authenticated_sender and self._allowlist_in_effect()
|
||||
and not self._allow_all_senders() and not msg_data.get("sender_authenticated", False)):
|
||||
else:
|
||||
# Approved pairings grant access too, and only the gateway's own check sees them. Its verdict also decides
|
||||
# open access: GATEWAY_ALLOW_ALL_USERS beside a GATEWAY_ALLOWED_USERS list grants a stranger nothing there.
|
||||
verdict = self._is_sender_authorized(sender_addr, "dm", sender_addr)
|
||||
granted = verdict if verdict is not None else (not allowed_raw and self._allow_all_senders())
|
||||
# Drop senders the gateway would neither authorize nor answer (pair/decline) before a MessageEvent (and thread
|
||||
# context) exists — otherwise a dispatch/authorization race can send a reply even though the handler returned None.
|
||||
if not granted and not self._answers_unknown_senders():
|
||||
logger.debug("[Email] Dropping unauthorized sender at dispatch (unknown senders are ignored): %s", sender_addr)
|
||||
return False
|
||||
# Reject spoofed senders (GHSA-rxqh-5572-8m77): short of open access, every grant keys on the attacker-controlled
|
||||
# From:, and a pairing code or decline is mailed back to it, open access or not; fail-closed. Only a granted
|
||||
# sender's drop warns: forged mail from strangers is routine, and the opt-out hint would be wrong advice for it.
|
||||
if self._require_authenticated_sender and not msg_data.get("sender_authenticated", False):
|
||||
if not granted:
|
||||
logger.debug("[Email] Not answering unknown sender with unauthenticated From: %s (%s)",
|
||||
sender_addr, msg_data.get("auth_reason", "no verdict"))
|
||||
return False
|
||||
if self._open_access():
|
||||
return True
|
||||
logger.warning("[Email] Dropping sender with unauthenticated From: %s (%s). If your mail server does not "
|
||||
"stamp Authentication-Results, set platforms.email.require_authenticated_sender: false "
|
||||
"(or EMAIL_TRUST_FROM_HEADER=true) to accept the risk.",
|
||||
|
||||
@@ -22,6 +22,7 @@ _DEFAULT_ENV = {
|
||||
"MATRIX_ALLOWED_USERS": "@default-admin:example.org", "MATRIX_IGNORE_USER_PATTERNS": r"^@spam:.*",
|
||||
"WHATSAPP_ALLOWED_USERS": "+15550001111", "SLACK_ALLOW_BOTS": "all", "SLACK_API_HUMAN_USERS": "U0DEFAULT",
|
||||
"LINE_ALLOW_ALL_USERS": "true", "LINE_ALLOWED_USERS": "Udefault", "DINGTALK_ALLOWED_USERS": "default-admin",
|
||||
"EMAIL_ALLOWED_USERS": "bot2-admin@example.org",
|
||||
}
|
||||
|
||||
|
||||
@@ -73,6 +74,12 @@ def _dingtalk():
|
||||
return adapter
|
||||
|
||||
|
||||
def _email():
|
||||
from plugins.platforms.email.adapter import EmailAdapter
|
||||
|
||||
return EmailAdapter(PlatformConfig(enabled=True, extra={"address": "bot2@example.org"}))
|
||||
|
||||
|
||||
def _line():
|
||||
from plugins.platforms.line.adapter import LineAdapter
|
||||
|
||||
@@ -84,8 +91,11 @@ _GATES = [
|
||||
("email.allow_all", {"GATEWAY_ALLOW_ALL_USERS": "true"},
|
||||
lambda: __import__("plugins.platforms.email.adapter", fromlist=["EmailAdapter"]).EmailAdapter._allow_all_senders(),
|
||||
False, True),
|
||||
("email.allowlist", {"GATEWAY_ALLOWED_USERS": "bot2-admin"},
|
||||
lambda: __import__("plugins.platforms.email.adapter", fromlist=["EmailAdapter"]).EmailAdapter._allowlist_in_effect(),
|
||||
("email.allowlist", {"EMAIL_ALLOWED_USERS": "bot2-admin@example.org"},
|
||||
lambda: _email()._sender_accepted("bot2-admin@example.org", {"sender_authenticated": True}),
|
||||
False, True),
|
||||
("email.gateway_allowlist", {"GATEWAY_ALLOWED_USERS": "bot2-admin@example.org"},
|
||||
lambda: _email()._sender_accepted("bot2-admin@example.org", {"sender_authenticated": True}),
|
||||
False, True),
|
||||
("qqbot.open_dm", {"QQ_ALLOW_ALL_USERS": "true"},
|
||||
lambda: __import__("gateway.platforms.qqbot.adapter", fromlist=["QQAdapter"]).QQAdapter._open_dm_opted_in(object.__new__(__import__("gateway.platforms.qqbot.adapter", fromlist=["QQAdapter"]).QQAdapter)),
|
||||
|
||||
@@ -320,6 +320,106 @@ class TestDispatchMessage(unittest.TestCase):
|
||||
self.assertEqual(len(captured), 1)
|
||||
|
||||
|
||||
class TestDispatchDefersToGatewayAuthorization(unittest.TestCase):
|
||||
"""The pre-dispatch gate must not drop mail the gateway would authorize (GATEWAY_ALLOWED_USERS,
|
||||
an approved pairing) or answer itself (an explicit pair/decline unauthorized_dm_behavior)."""
|
||||
|
||||
STRANGER = "stranger@example.com"
|
||||
|
||||
def setUp(self):
|
||||
self._env = patch.dict(os.environ, {}, clear=False)
|
||||
self._env.start()
|
||||
for key in ("EMAIL_ALLOWED_USERS", "EMAIL_ALLOW_ALL_USERS", "GATEWAY_ALLOWED_USERS",
|
||||
"GATEWAY_ALLOW_ALL_USERS", "EMAIL_TRUST_FROM_HEADER"):
|
||||
os.environ.pop(key, None)
|
||||
|
||||
def tearDown(self):
|
||||
self._env.stop()
|
||||
|
||||
def _reached_gateway(self, *, extra=None, env=None, paired=False, authenticated=True):
|
||||
"""Dispatch one mail from STRANGER with the real GatewayRunner auth callback wired, as startup does;
|
||||
return the events handed to the gateway. Each call gets its own pairing store."""
|
||||
import asyncio
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
from gateway.config import GatewayConfig, Platform, PlatformConfig
|
||||
from gateway.pairing import PairingStore
|
||||
from gateway.run import GatewayRunner
|
||||
from plugins.platforms.email.adapter import EmailAdapter
|
||||
with tempfile.TemporaryDirectory() as pairing_dir, \
|
||||
patch("gateway.pairing.PAIRING_DIR", Path(pairing_dir)), \
|
||||
patch.dict(os.environ, {"EMAIL_ADDRESS": "hermes@test.com", "EMAIL_PASSWORD": "secret",
|
||||
"EMAIL_IMAP_HOST": "imap.test.com", "EMAIL_SMTP_HOST": "smtp.test.com",
|
||||
**(env or {})}):
|
||||
adapter = EmailAdapter(PlatformConfig(enabled=True, extra=dict(extra or {})))
|
||||
runner = object.__new__(GatewayRunner)
|
||||
runner.config = GatewayConfig(platforms={Platform.EMAIL: adapter.config})
|
||||
runner.adapters = {Platform.EMAIL: adapter}
|
||||
runner.pairing_store = PairingStore()
|
||||
adapter.set_authorization_check(runner._make_adapter_auth_check(Platform.EMAIL))
|
||||
if paired:
|
||||
code = runner.pairing_store.generate_code("email", self.STRANGER, "Stranger")
|
||||
self.assertIsNotNone(runner.pairing_store.approve_code("email", code))
|
||||
captured = []
|
||||
|
||||
async def capture(event):
|
||||
captured.append(event)
|
||||
|
||||
adapter.handle_message = capture
|
||||
asyncio.run(adapter._dispatch_message({
|
||||
"uid": b"301", "sender_addr": self.STRANGER, "sender_name": "Stranger", "subject": "Hello",
|
||||
"message_id": "<m301@example.com>", "in_reply_to": "", "body": "Hi there", "attachments": [],
|
||||
"date": "", "sender_authenticated": authenticated,
|
||||
"auth_reason": "dmarc=pass" if authenticated else "no Authentication-Results header"}))
|
||||
return captured
|
||||
|
||||
def test_mail_the_gateway_admits_or_answers_reaches_it(self):
|
||||
cases = {
|
||||
"pair opt-in": {"extra": {"unauthorized_dm_behavior": "pair"}},
|
||||
"decline opt-in": {"extra": {"unauthorized_dm_behavior": "decline"}},
|
||||
"GATEWAY_ALLOWED_USERS": {"env": {"GATEWAY_ALLOWED_USERS": self.STRANGER}},
|
||||
"EMAIL_ALLOWED_USERS JSON list literal": {"env": {"EMAIL_ALLOWED_USERS": f'["{self.STRANGER}"]'}},
|
||||
"approved pairing": {"paired": True},
|
||||
}
|
||||
for label, kwargs in cases.items():
|
||||
with self.subTest(label):
|
||||
self.assertEqual(len(self._reached_gateway(**kwargs)), 1)
|
||||
|
||||
def test_mail_the_gateway_would_ignore_or_that_forges_from_is_dropped(self):
|
||||
cases = {
|
||||
"default ignore": {},
|
||||
"pair opt-in, unauthenticated From": {"extra": {"unauthorized_dm_behavior": "pair"}, "authenticated": False},
|
||||
"approved pairing, unauthenticated From": {"paired": True, "authenticated": False},
|
||||
# Open access grants a stranger nothing beside a list, so a pairing code must not go to a forged From:.
|
||||
"pair opt-in, allow-all beside EMAIL list, unauthenticated From": {
|
||||
"extra": {"unauthorized_dm_behavior": "pair"}, "authenticated": False,
|
||||
"env": {"GATEWAY_ALLOW_ALL_USERS": "true", "EMAIL_ALLOWED_USERS": "boss@example.com"}},
|
||||
# GATEWAY_ALLOW_ALL_USERS is inert beside a list, so a listed address still has to authenticate its From:.
|
||||
"listed sender, GATEWAY allow-all beside the list, unauthenticated From": {
|
||||
"authenticated": False, "env": {"GATEWAY_ALLOW_ALL_USERS": "true", "EMAIL_ALLOWED_USERS": self.STRANGER}},
|
||||
"pair opt-in, allow-all beside GATEWAY list, unauthenticated From": {
|
||||
"extra": {"unauthorized_dm_behavior": "pair"}, "authenticated": False,
|
||||
"env": {"GATEWAY_ALLOW_ALL_USERS": "true", "GATEWAY_ALLOWED_USERS": "boss@example.com"}},
|
||||
}
|
||||
for label, kwargs in cases.items():
|
||||
with self.subTest(label):
|
||||
self.assertEqual(self._reached_gateway(**kwargs), [])
|
||||
|
||||
def test_bare_allowlist_entry_does_not_admit_its_local_part_at_any_domain(self):
|
||||
"""``GATEWAY_ALLOWED_USERS=stranger`` names one principal (say a chat username), not stranger@<any domain>."""
|
||||
cases = {
|
||||
"GATEWAY_ALLOWED_USERS bare entry": {"env": {"GATEWAY_ALLOWED_USERS": "stranger"}},
|
||||
"EMAIL_ALLOWED_USERS bare entry": {"env": {"EMAIL_ALLOWED_USERS": "stranger"}},
|
||||
"GATEWAY_ALLOWED_USERS bare entry, JSON list literal": {"env": {"GATEWAY_ALLOWED_USERS": '["stranger"]'}},
|
||||
"EMAIL_ALLOWED_USERS bare entry, JSON list literal": {"env": {"EMAIL_ALLOWED_USERS": '["stranger"]'}},
|
||||
"bare entry, pair opt-in": {"env": {"GATEWAY_ALLOWED_USERS": "stranger"},
|
||||
"extra": {"unauthorized_dm_behavior": "pair"}},
|
||||
}
|
||||
for label, kwargs in cases.items():
|
||||
with self.subTest(label):
|
||||
self.assertEqual(self._reached_gateway(**kwargs), [])
|
||||
|
||||
|
||||
class TestThreadContext(unittest.TestCase):
|
||||
"""Test email reply threading logic."""
|
||||
|
||||
|
||||
@@ -149,10 +149,13 @@ class TestEmailAdapterSecretScope(unittest.TestCase):
|
||||
ss.set_multiplex_active(True)
|
||||
token = ss.set_secret_scope(scoped)
|
||||
try:
|
||||
# _allowlist_in_effect reads EMAIL_ALLOWED_USERS — verify it
|
||||
# sees the scoped value, not the environ value
|
||||
# The dispatch gate must match the scoped list, not the environ one.
|
||||
from gateway.config import PlatformConfig
|
||||
from plugins.platforms.email.adapter import EmailAdapter
|
||||
self.assertTrue(EmailAdapter._allowlist_in_effect())
|
||||
adapter = EmailAdapter(PlatformConfig(enabled=True))
|
||||
authenticated = {"sender_authenticated": True}
|
||||
self.assertTrue(adapter._sender_accepted("epsilon@test.invalid", authenticated))
|
||||
self.assertFalse(adapter._sender_accepted("gamma@test.invalid", authenticated))
|
||||
finally:
|
||||
ss.reset_secret_scope(token)
|
||||
|
||||
|
||||
@@ -169,10 +169,15 @@ When enabled, attachment and inline parts are skipped before payload decoding. T
|
||||
|
||||
Email access is stricter by default than chat-style platforms:
|
||||
|
||||
1. **`EMAIL_ALLOWED_USERS` set** → only emails from those addresses are processed
|
||||
1. **`EMAIL_ALLOWED_USERS` set** → only emails from those addresses (and from `GATEWAY_ALLOWED_USERS` or an approved pairing) are processed
|
||||
2. **No allowlist set** → unknown senders are ignored silently
|
||||
3. **`EMAIL_ALLOW_ALL_USERS=true`** → any sender is accepted (use with caution)
|
||||
4. **`platforms.email.unauthorized_dm_behavior: pair`** → unknown senders receive a pairing code
|
||||
5. **`platforms.email.unauthorized_dm_behavior: decline`** → an unknown sender receives one polite refusal, then nothing more for 24 hours
|
||||
|
||||
Allowlist entries match whole addresses. A bare entry such as `alice` (a chat username in `GATEWAY_ALLOWED_USERS`, say) never admits `alice@` at any domain, and mail from such an address is dropped rather than paired or declined.
|
||||
|
||||
Unless open access is on, Hermes acts on a message only when the `Authentication-Results` header stamped by your receiving server authenticates its `From:` domain (DMARC, or aligned SPF/DKIM). `GATEWAY_ALLOW_ALL_USERS` counts as open access only while no allowlist is set, as it does for the gateway itself. Pairing codes and declines need an authenticated `From:` even with open access on, so neither is mailed to a forged address. If your mail server does not stamp that header, set `platforms.email.require_authenticated_sender: false` to accept the risk.
|
||||
|
||||
:::warning
|
||||
**Use a dedicated inbox and configure `EMAIL_ALLOWED_USERS` for normal operation.** Email pairing is opt-in because shared inboxes often contain unrelated unread messages, and Hermes should not reply to those contacts by default.
|
||||
|
||||
Reference in New Issue
Block a user