Skip to content

Commit ee8104a

Browse files
feat(agentex): DM unlinked Slack users a link to connect their account
Completes the identity-link flow. Everything else shipped in #409/#410 and was reachable only through a dev script, because nothing ever handed a user a link. Now an unlinked mention offers one. On a mention where the turn is NOT running as a person -- unlinked, or linked with a credential we can't use (expired session, undecryptable ciphertext) -- the gateway: 1. mints (or reuses) a nonce carrying the Slack identity Slack's HMAC just verified, plus the message that triggered it 2. conversations.open -> chat.postMessage: DMs that user the link 3. chat.postEphemeral: tells them in-channel that a DM is waiting A dead credential is offered the same fix as no credential, since re-linking is the remedy for both. The link is DMed and NEVER posted in a channel. The nonce is a bearer token: whoever opens it gets linked to that Slack identity by signing in as themselves. In a channel, the first person to read it could bind someone else's Slack identity to their own SGP account. So when conversations.open or the DM fails we log and stop rather than falling back anywhere visible -- there is a test asserting the token never appears in a payload addressed to the origin channel. Entirely best-effort. The turn is already proceeding (as the shared bot, or being refused just after), and no failure in here changes that: a Redis outage, a missing scope, a closed DM all end in "no offer" and an unaffected turn. Rate limiting is two layers, for two different problems: - claim_send caps DMs about one live link at 2, so a re-mention re-sends the same link rather than going quiet. - a cooldown key (SLACK_LINK_OFFER_COOLDOWN_S, default 1h) stops a fresh nonce from re-arming that budget every time the old one expires. Without it a persistent mentioner would collect ~12 DMs an hour instead of ~2. It fails OPEN on a Redis error, since never offering is worse and claim_send still bounds it. Offers require SLACK_GATEWAY_PUBLIC_BASE_URL. Unset means no offers at all: the host has to be browser-reachable AND a sibling subdomain of the SGP host or the session cookie never arrives, and a link that cannot work is worse than none. Also adds the email-match defence, OFF by default. The nonce stops an attacker forging someone else's Slack identity. It does not stop them forwarding their OWN link: if a victim clicks it while signed in, the attacker's Slack identity binds to the victim's SGP account, and from then on the attacker's Slack messages run as the victim with the victim's integrations. The confirmation page naming both identities catches a mis-click but reduces to user vigilance against a deliberate attempt. Comparing the Slack account's email to the signed-in SGP account's closes it. It ships disabled because it needs the users:read.email Slack scope, which is not granted (verified: users.lookupByEmail returns missing_scope). Enable IDENTITY_LINK_REQUIRE_EMAIL_MATCH and the scope together -- the check treats an unreadable email as a MISMATCH, not as "skip", so turning the flag on without the scope refuses every link. That direction is deliberate: failing open would silently disable the only defence the moment the scope lapsed. Not included: replaying the stashed turn after linking. pending_turn is recorded for it, but the confirmation page still says "ask me again", and wiring the route back into the gateway is left for a follow-up. Testing: 15 new unit tests. The link-offer ones concentrate on where the token must not go (origin channel, on DM failure) and on the offer never affecting the turn; the email-match ones on failing closed -- unreadable Slack email, missing SGP email -- and on the nonce surviving a refusal so a legitimate owner can still use their own link. Full unit suite: 685 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent c0cad3c commit ee8104a

4 files changed

Lines changed: 462 additions & 0 deletions

File tree

agentex/src/api/routes/integrations.py

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,26 @@
7272
# holding one indefinitely, so an unknown expiry becomes a short known one.
7373
_FALLBACK_TTL_DAYS = int(os.getenv("IDENTITY_LINK_FALLBACK_TTL_DAYS", "30"))
7474

75+
# Require the Slack account's email to match the signed-in SGP account's.
76+
#
77+
# This is the only real defence against a *forwarded* link. The nonce stops an
78+
# attacker forging someone else's Slack identity, but nothing stops them sending
79+
# their OWN link to a victim: if the victim clicks it while signed in, the
80+
# attacker's Slack identity binds to the victim's SGP account, and thereafter the
81+
# attacker's Slack messages run as the victim, using the victim's integrations. The
82+
# confirmation page names both identities, which catches a mis-click but reduces to
83+
# user vigilance against a deliberate attempt.
84+
#
85+
# OFF by default because it needs the ``users:read.email`` Slack scope, which is not
86+
# granted until the app is reinstalled. Enabling it without the scope would refuse
87+
# every link (the check treats an unreadable email as a mismatch, deliberately), so
88+
# the flag and the scope have to be turned on together.
89+
_REQUIRE_EMAIL_MATCH = os.getenv("IDENTITY_LINK_REQUIRE_EMAIL_MATCH", "").lower() in (
90+
"1",
91+
"true",
92+
"yes",
93+
)
94+
7595

7696
def _page(title: str, body: str, *, status: int = 200) -> HTMLResponse:
7797
"""Minimal self-contained page. No external assets — this renders inside
@@ -247,6 +267,38 @@ async def slack_link_confirm(request: Request, nonce: str = Form("")) -> HTMLRes
247267
status=409,
248268
)
249269

270+
if _REQUIRE_EMAIL_MATCH:
271+
# Local import: the gateway module owns the Slack token and HTTP calls, and
272+
# importing it at module load would pull the use case into the route's import
273+
# graph for a feature that is off by default.
274+
from src.domain.use_cases.slack_gateway_use_case import slack_user_profile
275+
276+
slack_email = (await slack_user_profile(link_request.external_user_id)).get(
277+
"email"
278+
)
279+
# An unreadable email is treated as a mismatch, not as "skip the check".
280+
# Failing open here would silently disable the only defence against a
281+
# forwarded link the moment the Slack scope lapsed.
282+
if not slack_email or not email or slack_email.lower() != email.lower():
283+
logger.warning(
284+
"identity link refused: Slack/SGP email mismatch",
285+
extra={
286+
"sgp_user_id": sgp_user_id,
287+
"external_user_id": link_request.external_user_id,
288+
"slack_email_known": bool(slack_email),
289+
},
290+
)
291+
return _page(
292+
"Accounts don't match",
293+
"<h1>Those accounts don't match</h1>"
294+
"<p>The Slack account this link was made for and the SGP account "
295+
"you're signed in as belong to different people.</p>"
296+
"<p class=muted>If someone sent you this link, don't use it — it "
297+
"would let their Slack messages run as you. Mention the agent in "
298+
"Slack yourself to get your own link.</p>",
299+
status=403,
300+
)
301+
250302
secret = _session_credential(request)
251303
if not secret:
252304
# The middleware authenticated this caller somehow, but not by a session

agentex/src/domain/use_cases/slack_gateway_use_case.py

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,18 @@
131131
"Connect your account and try again."
132132
)
133133

134+
# Public origin for the link we DM. Must be a host the user's browser can reach AND
135+
# a sibling subdomain of the SGP host, or their session cookie never arrives and the
136+
# callback can't tell who they are. Unset => no offers (a broken link is worse than
137+
# no link).
138+
_PUBLIC_BASE_URL = os.getenv("SLACK_GATEWAY_PUBLIC_BASE_URL", "").rstrip("/")
139+
140+
# How long before an unlinked user is offered the link again. ``claim_send`` already
141+
# caps DMs at 2 per live nonce, but a nonce only lives ~10 minutes, so without this a
142+
# persistent mentioner re-arms that budget every 10 minutes. Worst case becomes ~2
143+
# DMs an hour instead of ~12.
144+
_LINK_OFFER_COOLDOWN_S = int(os.getenv("SLACK_LINK_OFFER_COOLDOWN_S", "3600"))
145+
134146
# Slack's HTTP Events API is at-least-once — it retries a delivery (up to ~3x, with an
135147
# X-Slack-Retry-Num header) if we don't 200 within ~3s. Dedup on the envelope's
136148
# ``event_id`` via Redis with a short TTL so a retry can't start a duplicate turn.
@@ -360,6 +372,46 @@ def _agent_text(messages) -> str | None:
360372
return "\n\n".join(parts) if parts else None
361373

362374

375+
async def slack_user_profile(user_id: str) -> dict[str, str | None]:
376+
"""``{"display_name": …, "email": …}`` for a Slack user, best-effort.
377+
378+
Both fields can be None and callers must cope: ``display_name`` is only used to
379+
make the confirmation page legible, and ``email`` requires the
380+
``users:read.email`` scope, which is *not* granted by default. A missing email is
381+
therefore "unknown", never "does not match" — see the email-match check in the
382+
link route, which refuses rather than assuming when it can't read one.
383+
384+
Never raises: an identity-link attempt shouldn't fail because a Slack lookup
385+
hiccupped.
386+
"""
387+
token = os.getenv("SLACK_BOT_TOKEN", "")
388+
if not token or not user_id:
389+
return {"display_name": None, "email": None}
390+
try:
391+
async with httpx.AsyncClient(timeout=10) as client:
392+
resp = await client.get(
393+
"https://slack.com/api/users.info",
394+
headers={"Authorization": f"Bearer {token}"},
395+
params={"user": user_id},
396+
)
397+
body = resp.json()
398+
except Exception: # noqa: BLE001 - best-effort lookup
399+
logger.warning("[slack] users.info failed for %s", user_id, exc_info=True)
400+
return {"display_name": None, "email": None}
401+
if not body.get("ok"):
402+
# missing_scope here means users:read.email isn't granted; that's expected
403+
# until the app is reinstalled, so it's info rather than a warning.
404+
logger.info("[slack] users.info -> %s", body.get("error"))
405+
return {"display_name": None, "email": None}
406+
user = body.get("user") or {}
407+
profile = user.get("profile") or {}
408+
handle = user.get("name")
409+
return {
410+
"display_name": f"@{handle}" if handle else profile.get("real_name"),
411+
"email": profile.get("email"),
412+
}
413+
414+
363415
# --------------------------------------------------------------------------- use case
364416

365417

@@ -614,6 +666,15 @@ async def _run_turn(self, inbound: InboundSlack) -> None:
614666
# so an unlinked user still gets a working turn, just without their
615667
# personal integrations. Prompting them to link is a separate concern.
616668
principal, auth_headers, sgp_user_id = await self._turn_identity(inbound)
669+
670+
if sgp_user_id is None:
671+
# Not running as a person: either unlinked, or linked with a
672+
# credential we can't use (expired session, undecryptable). Offer the
673+
# link either way — a dead credential needs the same fix as no
674+
# credential. Rate-limited and best-effort; it never affects the turn,
675+
# which continues as the bot below (or is refused just after).
676+
await self._offer_link(inbound)
677+
617678
if principal is None and auth_headers is None:
618679
# Only reachable when linking is mandatory and this user hasn't.
619680
await self._deliver(inbound, _UNLINKED_MESSAGE)
@@ -1180,6 +1241,156 @@ async def _fetch_bot_token(self) -> str:
11801241
# Bot token from env / k8s-secret.
11811242
return os.getenv("SLACK_BOT_TOKEN", "")
11821243

1244+
async def _offer_link(self, inbound: InboundSlack) -> bool:
1245+
"""DM the invoking user a one-time link to connect their SGP account.
1246+
1247+
Returns True only when a DM actually went out. Entirely best-effort: this runs
1248+
alongside a turn that is already proceeding (as the shared bot, or being
1249+
refused), and no failure here may change that outcome.
1250+
1251+
**The link is DMed, never posted in channel.** The nonce is a bearer token —
1252+
whoever holds it gets linked to this Slack identity by signing in as
1253+
themselves. In a channel, the first person to click it would bind *this*
1254+
user's Slack identity to *their own* SGP account. So if the DM can't be sent
1255+
we say so and stop, rather than falling back to somewhere visible.
1256+
1257+
Rate limiting is two-layer and deliberately so: ``claim_send`` caps DMs about
1258+
one live link (default 2, so a re-mention re-sends rather than going quiet),
1259+
and a cooldown key stops a fresh nonce from re-arming that budget on every
1260+
mention once the old one expires.
1261+
"""
1262+
if not _PUBLIC_BASE_URL:
1263+
logger.info(
1264+
"[slack] link offer skipped: SLACK_GATEWAY_PUBLIC_BASE_URL is unset"
1265+
)
1266+
return False
1267+
1268+
from src.domain.services.link_nonce_service import LinkNonceService, LinkRequest
1269+
1270+
if not await self._claim_offer_cooldown(inbound):
1271+
logger.info(
1272+
"[slack] link offer suppressed by cooldown for %s", inbound.user
1273+
)
1274+
return False
1275+
1276+
profile = await slack_user_profile(inbound.user)
1277+
request = LinkRequest(
1278+
provider="slack",
1279+
external_team_id=inbound.team_id,
1280+
external_user_id=inbound.user,
1281+
display_name=profile.get("display_name") or inbound.user,
1282+
# Stored so a later change can answer the original question; nothing
1283+
# replays it yet.
1284+
pending_turn={
1285+
"text": inbound.text,
1286+
"channel": inbound.channel,
1287+
"thread_ts": inbound.thread_ts,
1288+
},
1289+
)
1290+
1291+
service = LinkNonceService()
1292+
try:
1293+
token, reused = await service.create_or_reuse(request)
1294+
allowed = await service.claim_send(request)
1295+
except Exception: # noqa: BLE001 - Redis down: no offer, turn unaffected
1296+
logger.warning("[slack] link offer failed to mint a nonce", exc_info=True)
1297+
return False
1298+
1299+
if not allowed:
1300+
# Already DMed about this link. Acknowledge in-channel (ephemerally) so
1301+
# the user isn't left wondering, but don't send another DM.
1302+
await self._post_ephemeral(
1303+
inbound,
1304+
"I've already sent you a DM with a link to connect your account — "
1305+
"check your direct messages with me.",
1306+
)
1307+
return False
1308+
1309+
opened = await self._slack_api("conversations.open", {"users": inbound.user})
1310+
dm_channel = (
1311+
(opened.get("channel") or {}).get("id") if opened.get("ok") else None
1312+
)
1313+
if not dm_channel:
1314+
logger.warning(
1315+
"[slack] conversations.open failed for %s: %s",
1316+
inbound.user,
1317+
opened.get("error"),
1318+
)
1319+
return False
1320+
1321+
url = f"{_PUBLIC_BASE_URL}/integrations/slack/link?nonce={token}"
1322+
posted = await self._slack_api(
1323+
"chat.postMessage",
1324+
{
1325+
"channel": dm_channel,
1326+
"unfurl_links": False,
1327+
"text": (
1328+
"Connect your SGP account and I'll use *your* tools "
1329+
"(Notion, Linear, …) when you ask me things in Slack.\n\n"
1330+
f"<{url}|Connect my account>\n\n"
1331+
"This link is just for you and expires in a few minutes. "
1332+
"Don't forward it — anyone who opens it could connect your "
1333+
"Slack identity to their own SGP account."
1334+
),
1335+
},
1336+
)
1337+
if not posted.get("ok"):
1338+
logger.warning(
1339+
"[slack] link DM failed for %s: %s", inbound.user, posted.get("error")
1340+
)
1341+
return False
1342+
1343+
logger.info(
1344+
"[slack] link offer DMed to %s (nonce %s)",
1345+
inbound.user,
1346+
"reused" if reused else "new",
1347+
)
1348+
await self._post_ephemeral(
1349+
inbound,
1350+
"I've DM'd you a link to connect your SGP account — once you do, I'll "
1351+
"use your own tools when you ask me things here.",
1352+
)
1353+
return True
1354+
1355+
async def _claim_offer_cooldown(self, inbound: InboundSlack) -> bool:
1356+
"""True if we may offer this user a link now, and records the offer.
1357+
1358+
Fails *open* on a Redis problem: the alternative is never offering, and the
1359+
per-link ``claim_send`` cap still bounds the damage.
1360+
"""
1361+
key = f"slack:link_offer:{inbound.team_id}:{inbound.user}"
1362+
try:
1363+
pool = GlobalDependencies().redis_pool
1364+
if pool is None:
1365+
return True
1366+
import redis.asyncio as redis
1367+
1368+
client = redis.Redis(connection_pool=pool)
1369+
# SET NX: only the first caller in the window wins.
1370+
return bool(await client.set(key, "1", nx=True, ex=_LINK_OFFER_COOLDOWN_S))
1371+
except Exception: # noqa: BLE001 - see docstring
1372+
logger.warning("[slack] link offer cooldown check failed", exc_info=True)
1373+
return True
1374+
1375+
async def _post_ephemeral(self, inbound: InboundSlack, text: str) -> None:
1376+
"""Post a message only the invoking user sees. Best-effort.
1377+
1378+
Ephemeral so a channel isn't cluttered with onboarding nudges aimed at one
1379+
person — and Slack rejects it outside a channel context (e.g. an assistant
1380+
pane), which we swallow.
1381+
"""
1382+
body = await self._slack_api(
1383+
"chat.postEphemeral",
1384+
{
1385+
"channel": inbound.channel,
1386+
"user": inbound.user,
1387+
"thread_ts": inbound.thread_ts,
1388+
"text": text,
1389+
},
1390+
)
1391+
if not body.get("ok"):
1392+
logger.info("[slack] postEphemeral -> %s", body.get("error"))
1393+
11831394
async def _set_status(self, inbound: InboundSlack, status: str) -> None:
11841395
"""AI-app 'thinking…' indicator (assistant.threads.setStatus). Shows in the
11851396
assistant pane while the turn runs; cleared when the reply is posted. Best-effort:

agentex/tests/unit/api/test_integrations_routes.py

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,3 +255,82 @@ async def test_unconfigured_encryption_key_is_reported_not_a_500(self, wiring):
255255
)
256256
# Nonce preserved so the link works once the key is configured.
257257
wiring.nonce.consume.assert_not_awaited()
258+
259+
260+
@pytest.mark.unit
261+
@pytest.mark.asyncio
262+
class TestEmailMatch:
263+
"""The flagged defence against a *forwarded* link.
264+
265+
The nonce stops an attacker forging someone else's Slack identity. It does not
266+
stop them sending their OWN link to a victim: if the victim clicks it while
267+
signed in, the attacker's Slack identity binds to the victim's SGP account, and
268+
from then on the attacker's Slack messages run as the victim. Comparing the two
269+
accounts' emails is what closes that.
270+
271+
Off by default: it needs the `users:read.email` Slack scope, which isn't granted
272+
until the app is reinstalled.
273+
"""
274+
275+
def _slack_email(self, monkeypatch, email):
276+
import src.domain.use_cases.slack_gateway_use_case as sg
277+
278+
monkeypatch.setattr(
279+
sg, "slack_user_profile", AsyncMock(return_value={"email": email})
280+
)
281+
282+
async def test_disabled_by_default_does_not_call_slack(self, wiring, monkeypatch):
283+
import src.domain.use_cases.slack_gateway_use_case as sg
284+
285+
probe = AsyncMock(return_value={"email": "someone.else@example.com"})
286+
monkeypatch.setattr(sg, "slack_user_profile", probe)
287+
monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", False)
288+
289+
resp = await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok")
290+
assert resp.status_code == 200
291+
probe.assert_not_awaited()
292+
293+
async def test_matching_emails_link_successfully(self, wiring, monkeypatch):
294+
monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", True)
295+
self._slack_email(monkeypatch, _SGP_EMAIL)
296+
resp = await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok")
297+
assert resp.status_code == 200
298+
wiring.repo.upsert_link.assert_awaited_once()
299+
300+
async def test_match_is_case_insensitive(self, wiring, monkeypatch):
301+
monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", True)
302+
self._slack_email(monkeypatch, _SGP_EMAIL.upper())
303+
resp = await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok")
304+
assert resp.status_code == 200
305+
306+
async def test_mismatch_is_refused_and_stores_nothing(self, wiring, monkeypatch):
307+
monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", True)
308+
self._slack_email(monkeypatch, "attacker@example.com")
309+
310+
resp = await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok")
311+
312+
assert resp.status_code == 403
313+
wiring.repo.upsert_link.assert_not_awaited()
314+
# The nonce survives, so a legitimate owner can still use their own link.
315+
wiring.nonce.consume.assert_not_awaited()
316+
body = resp.body.decode().lower()
317+
assert "don&#x27;t use it" in body or "don't use it" in body
318+
319+
async def test_unreadable_slack_email_fails_closed(self, wiring, monkeypatch):
320+
# Missing scope, deleted user, API hiccup -> None. Treating that as "skip the
321+
# check" would silently disable the defence the moment the scope lapsed.
322+
monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", True)
323+
self._slack_email(monkeypatch, None)
324+
325+
resp = await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok")
326+
assert resp.status_code == 403
327+
wiring.repo.upsert_link.assert_not_awaited()
328+
329+
async def test_missing_sgp_email_fails_closed(self, wiring, monkeypatch):
330+
monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", True)
331+
self._slack_email(monkeypatch, "someone@example.com")
332+
principal = {**_PRINCIPAL, "raw_user": {}}
333+
334+
resp = await mod.slack_link_confirm(_request(principal), nonce="tok")
335+
assert resp.status_code == 403
336+
wiring.repo.upsert_link.assert_not_awaited()

0 commit comments

Comments
 (0)