This lets the agent use your connected tools " "(Notion, Linear, …) when you ask it something in Slack.
" "If either name above isn't you, " - "close this page and don't continue.
", + f"{caution}
", ) @@ -267,47 +351,37 @@ async def slack_link_confirm(request: Request, nonce: str = Form("")) -> HTMLRes status=409, ) - if _REQUIRE_EMAIL_MATCH: - # Local import: the gateway module owns the Slack token and HTTP calls, and - # importing it at module load would pull the use case into the route's import - # graph for a feature that is off by default. - from src.domain.use_cases.slack_gateway_use_case import slack_user_profile - - slack_email = (await slack_user_profile(link_request.external_user_id)).get( - "email" + if await _email_mismatch(link_request.external_user_id, email): + logger.warning( + "identity link refused: Slack/SGP email mismatch", + extra={ + "sgp_user_id": sgp_user_id, + "external_user_id": link_request.external_user_id, + }, + ) + return _page( + "Accounts don't match", + "The Slack account this link was made for and the SGP account " + "you're signed in as belong to different people.
" + "If someone sent you this link, don't use it — it " + "would let their Slack messages run as you. Mention the agent in " + "Slack yourself to get your own link.
", + status=403, ) - # An unreadable email is treated as a mismatch, not as "skip the check". - # Failing open here would silently disable the only defence against a - # forwarded link the moment the Slack scope lapsed. - if not slack_email or not email or slack_email.lower() != email.lower(): - logger.warning( - "identity link refused: Slack/SGP email mismatch", - extra={ - "sgp_user_id": sgp_user_id, - "external_user_id": link_request.external_user_id, - "slack_email_known": bool(slack_email), - }, - ) - return _page( - "Accounts don't match", - "The Slack account this link was made for and the SGP account " - "you're signed in as belong to different people.
" - "If someone sent you this link, don't use it — it " - "would let their Slack messages run as you. Mention the agent in " - "Slack yourself to get your own link.
", - status=403, - ) secret = _session_credential(request) if not secret: # The middleware authenticated this caller somehow, but not by a session - # cookie — an api-key or bearer caller, or a cookie under a different name. - # There is nothing here we can act through later, so refuse rather than - # store an empty credential. + # cookie — an api-key or bearer caller, a cookie under a different name, or + # cookie delegation switched off entirely. There is nothing here we can act + # through later, so refuse rather than store an empty credential. logger.warning( "identity link refused: no session cookie on an authenticated request", - extra={"sgp_user_id": sgp_user_id, "cookie": SESSION_COOKIE_NAME}, + extra={ + "sgp_user_id": sgp_user_id, + "accepted_cookies": session_cookie_names(), + }, ) return _page( "Couldn't read your session", @@ -391,7 +465,7 @@ async def slack_link_confirm(request: Request, nonce: str = Form("")) -> HTMLRes "Connected", "The agent will now use your own tools when you ask it something in " - f"Slack, as {html.escape(email or sgp_user_id)}.
" + f"Slack, as {html.escape(email or 'your SGP account')}." f"This connection is valid until {html.escape(when)}, after " "which the agent will ask you to reconnect. You can close this page and go " "back to Slack.
", diff --git a/agentex/src/domain/services/identity_link_service.py b/agentex/src/domain/services/identity_link_service.py index 811d476c..dfb19b56 100644 --- a/agentex/src/domain/services/identity_link_service.py +++ b/agentex/src/domain/services/identity_link_service.py @@ -23,12 +23,12 @@ from __future__ import annotations import json -import os from datetime import UTC, datetime from typing import Annotated, Any from fastapi import Depends +from src.domain.delegation_headers import session_cookie_names_to_forward from src.domain.entities.identity_links import IdentityLinkEntity, IdentityProvider from src.domain.repositories.identity_link_repository import DIdentityLinkRepository from src.utils.credential_encryption import CredentialEncryptionError @@ -38,8 +38,8 @@ # Positive entries are stable — a link changes only on an explicit link/unlink, and # both paths invalidate. Negatives expire fast so linking feels immediate. -_CACHE_TTL_S = int(os.getenv("IDENTITY_LINK_CACHE_TTL", "300")) -_NEGATIVE_CACHE_TTL_S = int(os.getenv("IDENTITY_LINK_NEGATIVE_CACHE_TTL", "30")) +_CACHE_TTL_S = 300 +_NEGATIVE_CACHE_TTL_S = 30 # Distinguishes "cached: known to be unlinked" from "not in cache". _UNLINKED = "-" @@ -47,12 +47,40 @@ HEADER_COOKIE = "cookie" HEADER_SELECTED_ACCOUNT_ID = "x-selected-account-id" -# The stored credential is the linking user's own session cookie, so it goes back -# out as a cookie. ``build_delegation_headers`` filters a Cookie header down to its -# allowlisted names and re-emits it as ``x-acting-user-cookie``, so this name has to -# match that allowlist (``AGENTEX_DELEGATION_SESSION_COOKIE_NAMES``, default -# ``_identityJwt``) or the credential is silently dropped on the way to the agent. -SESSION_COOKIE_NAME = os.getenv("IDENTITY_LINK_SESSION_COOKIE_NAME", "_identityJwt") + +def session_cookie_names() -> tuple[str, ...]: + """Every cookie name a session may arrive under, in preference order. + + **Derived from the delegation allowlist, never separately configurable.** The + stored credential leaves as a Cookie header that ``build_delegation_headers`` + filters down to these same names before re-emitting as ``x-acting-user-cookie``. + If the two could disagree, the credential would be dropped in transit and every + linked turn would silently lose its acting identity — with a stored, valid, + apparently-healthy link. One source of truth removes that failure mode. + + Callers reading an inbound request must accept **any** of these, not just the + first: the allowlist is a set of names the deployment considers valid, so a + session arriving under the second one is exactly as legitimate as the first. + Empty means cookie delegation is disabled. + """ + return session_cookie_names_to_forward() + + +def session_cookie_name() -> str | None: + """The single name to **emit** under, or None if cookie delegation is off. + + Distinct from ``session_cookie_names()`` on purpose. Reading accepts any + allowlisted name; writing has to choose one, and the first is the deployment's + canonical choice. + + A stored credential is a session JWT, validated downstream on its own contents + rather than on the label it travels under, so emitting a value that arrived + under a later allowlisted name as the canonical one is safe. If a downstream + ever became name-sensitive this would need the originating name stored + alongside the credential. + """ + names = session_cookie_names() + return names[0] if names else None def _cache_key( @@ -172,8 +200,18 @@ async def acting_headers(self, identity: ResolvedIdentity) -> dict[str, str] | N extra={"sgp_user_id": identity.sgp_user_id}, ) return None + cookie_name = session_cookie_name() + if cookie_name is None: + # Cookie delegation is disabled, so build_delegation_headers would strip + # whatever we emit. Refuse rather than hand back headers that get + # silently dropped between here and the agent. + logger.warning( + "identity_link_cookie_delegation_disabled", + extra={"sgp_user_id": identity.sgp_user_id}, + ) + return None return { - HEADER_COOKIE: f"{SESSION_COOKIE_NAME}={credential}", + HEADER_COOKIE: f"{cookie_name}={credential}", HEADER_SELECTED_ACCOUNT_ID: identity.sgp_account_id, } diff --git a/agentex/src/domain/services/link_nonce_service.py b/agentex/src/domain/services/link_nonce_service.py index b6041eed..b1b4e9c6 100644 --- a/agentex/src/domain/services/link_nonce_service.py +++ b/agentex/src/domain/services/link_nonce_service.py @@ -40,7 +40,6 @@ from __future__ import annotations import json -import os import secrets from dataclasses import asdict, dataclass, field, replace from typing import Annotated, Any @@ -53,7 +52,7 @@ # Long enough for a human to switch windows and sign in, short enough that an # abandoned link stops being interesting. -_TTL_S = int(os.getenv("IDENTITY_LINK_NONCE_TTL", "600")) +_TTL_S = 600 # 32 bytes of urlsafe randomness. Guessing is not a threat model at this size, but # the token is still consumed on first use rather than relying on entropy alone. @@ -63,7 +62,7 @@ # reuse the live nonce rather than minting another, so this caps DM noise without # multiplying live tokens. Past the cap the caller should fall back to an ephemeral # in-channel notice rather than going silent. -_MAX_SENDS = int(os.getenv("IDENTITY_LINK_MAX_DMS", "2")) +_MAX_SENDS = 2 _KEY_PREFIX = "link_nonce:" # identity -> its one live token, so a second mention finds the first nonce instead diff --git a/agentex/src/domain/use_cases/slack_gateway_use_case.py b/agentex/src/domain/use_cases/slack_gateway_use_case.py index 867abaeb..b7f938a3 100644 --- a/agentex/src/domain/use_cases/slack_gateway_use_case.py +++ b/agentex/src/domain/use_cases/slack_gateway_use_case.py @@ -141,7 +141,7 @@ # caps DMs at 2 per live nonce, but a nonce only lives ~10 minutes, so without this a # persistent mentioner re-arms that budget every 10 minutes. Worst case becomes ~2 # DMs an hour instead of ~12. -_LINK_OFFER_COOLDOWN_S = int(os.getenv("SLACK_LINK_OFFER_COOLDOWN_S", "3600")) +_LINK_OFFER_COOLDOWN_S = 3600 # Slack's HTTP Events API is at-least-once — it retries a delivery (up to ~3x, with an # X-Slack-Retry-Num header) if we don't 200 within ~3s. Dedup on the envelope's @@ -373,20 +373,20 @@ def _agent_text(messages) -> str | None: async def slack_user_profile(user_id: str) -> dict[str, str | None]: - """``{"display_name": …, "email": …}`` for a Slack user, best-effort. + """``{"display_name": …, "email": …, "error": …}`` for a Slack user. - Both fields can be None and callers must cope: ``display_name`` is only used to - make the confirmation page legible, and ``email`` requires the - ``users:read.email`` scope, which is *not* granted by default. A missing email is - therefore "unknown", never "does not match" — see the email-match check in the - link route, which refuses rather than assuming when it can't read one. + All three can be None. ``display_name`` only makes the confirmation page legible. + ``email`` requires the ``users:read.email`` scope, which may not be granted — + ``error`` is how a caller tells "Slack won't tell us" apart from "Slack told us + and there's no email", which the link route needs to decide whether it can + perform its identity check at all. Never raises: an identity-link attempt shouldn't fail because a Slack lookup hiccupped. """ token = os.getenv("SLACK_BOT_TOKEN", "") if not token or not user_id: - return {"display_name": None, "email": None} + return {"display_name": None, "email": None, "error": "no_token"} try: async with httpx.AsyncClient(timeout=10) as client: resp = await client.get( @@ -397,18 +397,19 @@ async def slack_user_profile(user_id: str) -> dict[str, str | None]: body = resp.json() except Exception: # noqa: BLE001 - best-effort lookup logger.warning("[slack] users.info failed for %s", user_id, exc_info=True) - return {"display_name": None, "email": None} + return {"display_name": None, "email": None, "error": "request_failed"} if not body.get("ok"): - # missing_scope here means users:read.email isn't granted; that's expected - # until the app is reinstalled, so it's info rather than a warning. + # missing_scope means users:read.email isn't granted — expected until the app + # is reinstalled, so info rather than warning. logger.info("[slack] users.info -> %s", body.get("error")) - return {"display_name": None, "email": None} + return {"display_name": None, "email": None, "error": body.get("error")} user = body.get("user") or {} profile = user.get("profile") or {} handle = user.get("name") return { "display_name": f"@{handle}" if handle else profile.get("real_name"), "email": profile.get("email"), + "error": None, } @@ -1296,16 +1297,10 @@ async def _offer_link(self, inbound: InboundSlack) -> bool: logger.warning("[slack] link offer failed to mint a nonce", exc_info=True) return False - if not allowed: - # Already DMed about this link. Acknowledge in-channel (ephemerally) so - # the user isn't left wondering, but don't send another DM. - await self._post_ephemeral( - inbound, - "I've already sent you a DM with a link to connect your account — " - "check your direct messages with me.", - ) - return False - + # Opened before the send-cap check because BOTH branches need the channel id + # to build the deep link below — telling someone to "check your DMs" without + # taking them there is the failure this method exists to avoid. Idempotent: + # conversations.open returns the existing DM rather than creating another. opened = await self._slack_api("conversations.open", {"users": inbound.user}) dm_channel = ( (opened.get("channel") or {}).get("id") if opened.get("ok") else None @@ -1318,7 +1313,38 @@ async def _offer_link(self, inbound: InboundSlack) -> bool: ) return False + # Slack files bot conversations under "Apps", NOT in the Direct messages + # list, so a person told to check their DMs looks in the one place the + # message isn't. Observed in the field: the first real offer was delivered + # correctly, confirmed present via conversations.history, and still reported + # as never received. app_redirect works on web and desktop, unlike a + # slack:// URI. + dm_deeplink = f"https://slack.com/app_redirect?channel={dm_channel}&team={inbound.team_id}" url = f"{_PUBLIC_BASE_URL}/integrations/slack/link?nonce={token}" + + # The connect link goes in the ephemeral as well as the DM. An ephemeral is + # single-viewer — Slack renders it for one user, keeps it out of channel + # history and out of search — so it has exactly the same audience as the DM, + # and putting the link there costs a round trip through a conversation people + # cannot find. The DM stays because ephemerals are transient: reload Slack + # before clicking and it's gone, and the offer cooldown would then block a + # retry for an hour. + # + # This is NOT licence to put the link in an ordinary channel message. The + # nonce is a bearer token; the first reader of a broadcast could bind this + # user's Slack identity to their own SGP account. Single-viewer is the + # property that makes the ephemeral safe, not "it's in the channel anyway". + if not allowed: + # Past the DM cap — but an ephemeral costs nothing and is what the user + # is actually looking at, so still hand them the live link. + await self._post_ephemeral( + inbound, + f"<{url}|Connect your SGP account> to let me use your own tools " + f"when you ask me things here.\n" + f"_Only you can see this. The same link is in " + f"<{dm_deeplink}|our DM>._", + ) + return False posted = await self._slack_api( "chat.postMessage", { @@ -1347,8 +1373,10 @@ async def _offer_link(self, inbound: InboundSlack) -> bool: ) await self._post_ephemeral( inbound, - "I've DM'd you a link to connect your SGP account — once you do, I'll " - "use your own tools when you ask me things here.", + f"<{url}|Connect your SGP account> and I'll use your own tools " + f"(Notion, Linear, …) when you ask me things here.\n" + f"_Only you can see this message. I've also sent the link to " + f"<{dm_deeplink}|our DM>, in case this one disappears._", ) return True diff --git a/agentex/tests/unit/api/test_integrations_routes.py b/agentex/tests/unit/api/test_integrations_routes.py index ed0d5dca..c31ea3eb 100644 --- a/agentex/tests/unit/api/test_integrations_routes.py +++ b/agentex/tests/unit/api/test_integrations_routes.py @@ -10,6 +10,7 @@ """ import base64 +import html import json from datetime import UTC, datetime, timedelta from types import SimpleNamespace @@ -260,77 +261,234 @@ async def test_unconfigured_encryption_key_is_reported_not_a_500(self, wiring): @pytest.mark.unit @pytest.mark.asyncio class TestEmailMatch: - """The flagged defence against a *forwarded* link. + """The self-enabling defence against a *forwarded* link. The nonce stops an attacker forging someone else's Slack identity. It does not - stop them sending their OWN link to a victim: if the 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. Comparing the two - accounts' emails is what closes that. - - Off by default: it needs the `users:read.email` Slack scope, which isn't granted - until the app is reinstalled. + stop them sending their OWN link to a victim: clicking it while signed in binds + the attacker's Slack identity to the victim's SGP account, and from then on the + attacker's Slack messages run as the victim with the victim's integrations. + Comparing the two accounts' emails is what closes that. + + There is deliberately no flag. The check needs the `users:read.email` Slack + scope, and gating it on config means the flag and the scope must be flipped + together — flipping one alone either refuses every link or silently protects + nothing. Instead it enforces whenever Slack answers with an email and stands + down when it won't, so granting the scope turns the protection on by itself. + + The asymmetry that follows is the thing to pin: **verified different -> refuse, + unverifiable -> allow**. """ - def _slack_email(self, monkeypatch, email): + def _slack_profile(self, monkeypatch, **profile): import src.domain.use_cases.slack_gateway_use_case as sg - monkeypatch.setattr( - sg, "slack_user_profile", AsyncMock(return_value={"email": email}) - ) - - async def test_disabled_by_default_does_not_call_slack(self, wiring, monkeypatch): - import src.domain.use_cases.slack_gateway_use_case as sg - - probe = AsyncMock(return_value={"email": "someone.else@example.com"}) + full = {"display_name": "@x", "email": None, "error": None, **profile} + probe = AsyncMock(return_value=full) monkeypatch.setattr(sg, "slack_user_profile", probe) - monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", False) - - resp = await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok") - assert resp.status_code == 200 - probe.assert_not_awaited() + return probe async def test_matching_emails_link_successfully(self, wiring, monkeypatch): - monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", True) - self._slack_email(monkeypatch, _SGP_EMAIL) + self._slack_profile(monkeypatch, email=_SGP_EMAIL) resp = await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok") assert resp.status_code == 200 wiring.repo.upsert_link.assert_awaited_once() - async def test_match_is_case_insensitive(self, wiring, monkeypatch): - monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", True) - self._slack_email(monkeypatch, _SGP_EMAIL.upper()) + async def test_match_ignores_case_and_surrounding_space(self, wiring, monkeypatch): + self._slack_profile(monkeypatch, email=f" {_SGP_EMAIL.upper()} ") resp = await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok") assert resp.status_code == 200 async def test_mismatch_is_refused_and_stores_nothing(self, wiring, monkeypatch): - monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", True) - self._slack_email(monkeypatch, "attacker@example.com") + self._slack_profile(monkeypatch, email="attacker@example.com") resp = await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok") assert resp.status_code == 403 wiring.repo.upsert_link.assert_not_awaited() - # The nonce survives, so a legitimate owner can still use their own link. + # The nonce survives a refusal, so the link's legitimate owner can still use + # it — refusing the wrong clicker must not burn the right one's link. wiring.nonce.consume.assert_not_awaited() body = resp.body.decode().lower() assert "don't use it" in body or "don't use it" in body - async def test_unreadable_slack_email_fails_closed(self, wiring, monkeypatch): - # Missing scope, deleted user, API hiccup -> None. Treating that as "skip the - # check" would silently disable the defence the moment the scope lapsed. - monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", True) - self._slack_email(monkeypatch, None) + async def test_missing_scope_allows_the_link(self, wiring, monkeypatch): + # The current production state: users:read.email isn't granted, so Slack + # won't tell us. Refusing here would block every link in the workspace, so + # the check stands down rather than failing closed. + self._slack_profile(monkeypatch, email=None, error="missing_scope") + resp = await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok") + assert resp.status_code == 200 + wiring.repo.upsert_link.assert_awaited_once() + async def test_lookup_failure_allows_the_link(self, wiring, monkeypatch): + # Same rule for a transient failure. Failing closed on an unreadable email + # would make linking fail randomly, and the gap isn't attacker-reachable: + # nobody outside our infrastructure decides whether our Slack call succeeds. + self._slack_profile(monkeypatch, email=None, error="request_failed") resp = await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok") - assert resp.status_code == 403 - wiring.repo.upsert_link.assert_not_awaited() + assert resp.status_code == 200 - async def test_missing_sgp_email_fails_closed(self, wiring, monkeypatch): - monkeypatch.setattr(mod, "_REQUIRE_EMAIL_MATCH", True) - self._slack_email(monkeypatch, "someone@example.com") - principal = {**_PRINCIPAL, "raw_user": {}} + async def test_missing_sgp_email_allows_the_link(self, wiring, monkeypatch): + # Slack told us, but the SGP principal carries no email: nothing to compare + # against, so the same "can't verify, don't block" rule applies. + self._slack_profile(monkeypatch, email="someone@example.com") + resp = await mod.slack_link_confirm( + _request({**_PRINCIPAL, "raw_user": {}}), nonce="tok" + ) + assert resp.status_code == 200 - resp = await mod.slack_link_confirm(_request(principal), nonce="tok") - assert resp.status_code == 403 - wiring.repo.upsert_link.assert_not_awaited() + async def test_the_check_runs_on_every_confirm(self, wiring, monkeypatch): + # No flag means no way to accidentally leave it off: granting the scope is + # the only thing standing between here and enforcement. + probe = self._slack_profile(monkeypatch, email=_SGP_EMAIL) + await mod.slack_link_confirm(_request(_PRINCIPAL), nonce="tok") + probe.assert_awaited_once() + assert probe.await_args.args[0] == "U1" + + +@pytest.mark.unit +@pytest.mark.asyncio +class TestNoRawIdentifiersOnThePage: + """The confirmation page never prints an internal id. + + The identity rows exist so someone can answer "is this *my* account?". Nobody + recognises their own Slack `U…` id or SGP uuid, so falling back to one doesn't + make the check possible — it exposes an internal identifier while making an + unanswerable question look answered. + """ + + def _no_slack_lookup(self, monkeypatch, name=None): + import src.domain.use_cases.slack_gateway_use_case as sg + + monkeypatch.setattr( + sg, "slack_user_profile", AsyncMock(return_value={"display_name": name}) + ) + + async def test_named_identities_are_shown(self, wiring, monkeypatch): + self._no_slack_lookup(monkeypatch) + body = ( + await mod.slack_link_page(_request(_PRINCIPAL), nonce="tok") + ).body.decode() + assert _SLACK_HANDLE in body and _SGP_EMAIL in body + + async def test_missing_slack_name_does_not_fall_back_to_the_member_id( + self, wiring, monkeypatch + ): + wiring.nonce.peek = AsyncMock(return_value=_link_request(display_name="")) + self._no_slack_lookup(monkeypatch) # live lookup also comes back empty + + body = ( + await mod.slack_link_page(_request(_PRINCIPAL), nonce="tok") + ).body.decode() + + assert "U1" not in body + assert html.escape(mod._UNKNOWN_IDENTITY) in body + + async def test_missing_slack_name_is_re_read_live(self, wiring, monkeypatch): + # A transient Slack failure when the nonce was minted shouldn't permanently + # degrade the page — users:read is enough to recover the handle. + wiring.nonce.peek = AsyncMock(return_value=_link_request(display_name="")) + self._no_slack_lookup(monkeypatch, name="@recovered") + + body = ( + await mod.slack_link_page(_request(_PRINCIPAL), nonce="tok") + ).body.decode() + assert "@recovered" in body + + async def test_missing_email_does_not_fall_back_to_the_uuid( + self, wiring, monkeypatch + ): + self._no_slack_lookup(monkeypatch) + body = ( + await mod.slack_link_page( + _request({**_PRINCIPAL, "raw_user": {}}), nonce="tok" + ) + ).body.decode() + + assert _SGP_USER not in body + assert html.escape(mod._UNKNOWN_IDENTITY) in body + + async def test_unnamed_identity_says_the_check_is_unavailable( + self, wiring, monkeypatch + ): + # Don't imply someone verified a match they had no way to verify. + self._no_slack_lookup(monkeypatch) + body = ( + await mod.slack_link_page( + _request({**_PRINCIPAL, "raw_user": {}}), nonce="tok" + ) + ).body.decode() + assert "couldn't be identified" in body or "couldn't be identified" in body + + async def test_success_page_does_not_print_the_uuid(self, wiring, monkeypatch): + self._no_slack_lookup(monkeypatch) + resp = await mod.slack_link_confirm( + _request({**_PRINCIPAL, "raw_user": {}}), nonce="tok" + ) + assert resp.status_code == 200 + assert _SGP_USER not in resp.body.decode() + + +@pytest.mark.unit +class TestMultiNameCookieAllowlist: + """A session may arrive under ANY allowlisted cookie name. + + REGRESSION: this used to read only the first entry of + AGENTEX_DELEGATION_SESSION_COOKIE_NAMES. With a multi-name allowlist, a session + carried by a later name was rejected — linking failed with "couldn't read your + session" for a cookie the delegation layer would have forwarded quite happily. + The allowlist is a set of names the deployment treats as valid, so second is as + legitimate as first. + """ + + def _request(self, cookie: str): + return SimpleNamespace(headers={"cookie": cookie}) + + def _allow(self, monkeypatch, names: str): + from src.domain.delegation_headers import ENV_SESSION_COOKIE_NAMES + + monkeypatch.setenv(ENV_SESSION_COOKIE_NAMES, names) + + def test_session_under_a_later_name_is_accepted(self, monkeypatch): + self._allow(monkeypatch, "_identityJwt,_jwt") + assert mod._session_credential(self._request("_jwt=BBB")) == "BBB" + + def test_allowlist_order_wins_when_several_are_present(self, monkeypatch): + # Deterministic, and prefers the deployment's canonical cookie — which is + # also the name acting_headers() emits under. + self._allow(monkeypatch, "_identityJwt,_jwt") + got = mod._session_credential(self._request("_jwt=BBB; _identityJwt=AAA")) + assert got == "AAA" + + def test_later_name_survives_a_messy_browser_header(self, monkeypatch): + self._allow(monkeypatch, "_identityJwt,_jwt") + cookie = "_ga=GA1.2.x; __utmzz=(not set); _jwt=BBB; fs_uid=a#b" + assert mod._session_credential(self._request(cookie)) == "BBB" + + def test_names_outside_the_allowlist_are_still_rejected(self, monkeypatch): + # Widening to "any cookie that looks like a session" would let an attacker + # nominate the credential we store. + self._allow(monkeypatch, "_identityJwt") + assert mod._session_credential(self._request("_jwt=BBB")) is None + + def test_empty_allowlist_reads_nothing(self, monkeypatch): + self._allow(monkeypatch, "") + assert mod._session_credential(self._request("_identityJwt=AAA")) is None + + def test_blank_value_is_not_a_session(self, monkeypatch): + self._allow(monkeypatch, "_identityJwt,_jwt") + assert ( + mod._session_credential(self._request("_identityJwt=; _jwt=BBB")) == "BBB" + ) + + def test_emitted_name_stays_canonical(self, monkeypatch): + # Reading accepts many; writing must choose one. The stored value is a JWT + # validated on its contents, not on the label it travels under. + from src.domain.services.identity_link_service import ( + session_cookie_name, + session_cookie_names, + ) + + self._allow(monkeypatch, "_identityJwt,_jwt") + assert session_cookie_names() == ("_identityJwt", "_jwt") + assert session_cookie_name() == "_identityJwt" diff --git a/agentex/tests/unit/services/test_identity_link_service.py b/agentex/tests/unit/services/test_identity_link_service.py index 45d6b8bc..ce207fd2 100644 --- a/agentex/tests/unit/services/test_identity_link_service.py +++ b/agentex/tests/unit/services/test_identity_link_service.py @@ -245,3 +245,67 @@ async def test_no_redis_pool_is_database_only(self, monkeypatch): monkeypatch.setattr(type(service), "_redis", IdentityLinkService._redis) assert await service.resolve(IdentityProvider.SLACK, "T1", "U1") is not None repo.get_active_by_external_user.assert_awaited_once() + + +@pytest.mark.unit +class TestSessionCookieName: + """The cookie name has exactly one source of truth. + + It used to be separately configurable here AND in delegation_headers. If the two + diverged, acting_headers() would emit a Cookie header under a name the delegation + allowlist filters out — the agent would get no acting credential at all, silently, + while the link itself looked stored, valid and healthy. Deriving it removes the + possibility rather than documenting it. + """ + + def test_follows_the_delegation_allowlist(self, monkeypatch): + from src.domain.delegation_headers import ENV_SESSION_COOKIE_NAMES + from src.domain.services.identity_link_service import session_cookie_name + + monkeypatch.setenv(ENV_SESSION_COOKIE_NAMES, "_sgpSession") + assert session_cookie_name() == "_sgpSession" + + def test_defaults_with_the_delegation_module(self, monkeypatch): + from src.domain.delegation_headers import ENV_SESSION_COOKIE_NAMES + from src.domain.services.identity_link_service import session_cookie_name + + monkeypatch.delenv(ENV_SESSION_COOKIE_NAMES, raising=False) + assert session_cookie_name() == "_identityJwt" + + def test_none_when_cookie_delegation_is_disabled(self, monkeypatch): + from src.domain.delegation_headers import ENV_SESSION_COOKIE_NAMES + from src.domain.services.identity_link_service import session_cookie_name + + monkeypatch.setenv(ENV_SESSION_COOKIE_NAMES, "") + assert session_cookie_name() is None + + +@pytest.mark.unit +@pytest.mark.asyncio +class TestActingHeadersFollowDelegation: + async def test_emitted_cookie_survives_the_delegation_filter(self, monkeypatch): + """The end-to-end property: whatever name is configured, what acting_headers + emits is what build_delegation_headers forwards.""" + from src.domain.delegation_headers import ( + ENV_SESSION_COOKIE_NAMES, + build_delegation_headers, + ) + + monkeypatch.setenv(ENV_SESSION_COOKIE_NAMES, "_sgpSession") + service, _ = _service(_link(), credential="jwt.value.sig") + + headers = await service.acting_headers(ResolvedIdentity(_link())) + assert headers["cookie"] == "_sgpSession=jwt.value.sig" + + forwarded = build_delegation_headers({"user_id": "u"}, dict(headers)) + # Not dropped in transit — the whole point of deriving the name. + assert forwarded["x-acting-user-cookie"] == "_sgpSession=jwt.value.sig" + + async def test_refuses_when_cookie_delegation_is_disabled(self, monkeypatch): + # Emitting a credential the delegation layer will strip would look like a + # working link and deliver nothing, so refuse instead. + from src.domain.delegation_headers import ENV_SESSION_COOKIE_NAMES + + monkeypatch.setenv(ENV_SESSION_COOKIE_NAMES, "") + service, _ = _service(_link(), credential="jwt.value.sig") + assert await service.acting_headers(ResolvedIdentity(_link())) is None diff --git a/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py b/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py index 2b4dc740..1a48b5d1 100644 --- a/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py +++ b/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py @@ -1745,14 +1745,31 @@ async def test_dms_the_link_and_acknowledges_in_channel(self, monkeypatch): # onboarding. assert calls["chat.postEphemeral"]["user"] == "U1" - async def test_the_link_never_goes_to_the_origin_channel(self, monkeypatch): + async def test_the_link_is_never_broadcast(self, monkeypatch): + """The invariant is single-viewer, not "not in the channel". + + The nonce is a bearer token, so the first reader of a channel-visible message + could bind this user's Slack identity to their own SGP account. It may ride + in the DM and in an ephemeral — both of which exactly one person can see — + but a chat.postMessage must never carry it anywhere but that user's own DM. + """ uc, api, _ = self._wire(monkeypatch) await uc._offer_link(_inbound(channel="C_PUBLIC")) + for method, payload in (c.args for c in api.await_args_list): - if payload.get("channel") == "C_PUBLIC": - assert "TOKEN123" not in str( - payload - ), f"{method} leaked the nonce into the origin channel" + if method == "chat.postMessage": + assert ( + payload["channel"] == "D_DM" + ), f"the nonce was posted to {payload['channel']}, not the DM" + # Nothing that lands in channel history mentions it. + broadcast = [ + p + for m, p in (c.args for c in api.await_args_list) + if m == "chat.postMessage" + ] + assert all( + "TOKEN123" not in str(p) or p["channel"] == "D_DM" for p in broadcast + ) async def test_dm_warns_against_forwarding(self, monkeypatch): uc, api, _ = self._wire(monkeypatch) @@ -1782,7 +1799,10 @@ async def test_send_cap_reached_says_so_without_a_second_dm(self, monkeypatch): assert await uc._offer_link(_inbound()) is False methods = [m for m, _ in (c.args for c in api.await_args_list)] # Acknowledge in-channel rather than going silent, but send no new DM. - assert methods == ["chat.postEphemeral"] + # conversations.open still runs first — it's idempotent, and this branch + # needs the channel id to deep-link the user to the DM they can't find. + assert methods == ["conversations.open", "chat.postEphemeral"] + assert "chat.postMessage" not in methods async def test_cooldown_suppresses_the_offer_entirely(self, monkeypatch): uc, api, nonce = self._wire(monkeypatch, cooldown_ok=False) @@ -1803,3 +1823,96 @@ async def test_pending_turn_is_stashed_for_later_replay(self, monkeypatch): assert req.external_user_id == "U1" assert req.pending_turn["text"] == "what's in my linear?" assert req.pending_turn["channel"] == "C9" + + +@pytest.mark.unit +@pytest.mark.asyncio +class TestLinkOfferDiscoverability: + """The offer has to be findable, not merely delivered. + + Observed in production on the first real offer: the DM was posted successfully + and confirmed present via conversations.history, and the recipient still reported + never receiving it — because Slack files bot conversations under "Apps" rather + than in the Direct messages list. "Check your DMs" points at the one place the + message isn't, so the ephemeral carries a deep link into the conversation. + """ + + def _wire(self, monkeypatch, *, allowed=True): + uc = SlackGatewayUseCase() + monkeypatch.setattr(sg, "_PUBLIC_BASE_URL", "https://agentex.example.com") + monkeypatch.setattr( + sg, "slack_user_profile", AsyncMock(return_value={"display_name": "@ada"}) + ) + monkeypatch.setattr(uc, "_claim_offer_cooldown", AsyncMock(return_value=True)) + nonce = MagicMock() + nonce.create_or_reuse = AsyncMock(return_value=("TOKEN123", False)) + nonce.claim_send = AsyncMock(return_value=allowed) + monkeypatch.setattr( + "src.domain.services.link_nonce_service.LinkNonceService", + lambda *a, **k: nonce, + ) + api = AsyncMock( + side_effect=lambda m, p: ( + {"ok": True, "channel": {"id": "D_DM"}} + if m == "conversations.open" + else {"ok": True} + ) + ) + monkeypatch.setattr(uc, "_slack_api", api) + return uc, api + + def _ephemeral(self, api): + return next( + p + for m, p in (c.args for c in api.await_args_list) + if m == "chat.postEphemeral" + ) + + async def test_ephemeral_points_at_the_durable_copy(self, monkeypatch): + # The link is inline now, so the deep link is no longer how you reach it — + # it's the pointer to the copy that survives a reload, since ephemerals + # don't. app_redirect rather than a slack:// URI, which fails on web. + uc, api = self._wire(monkeypatch) + await uc._offer_link(_inbound(team_id="T_ACME")) + + text = self._ephemeral(api)["text"] + assert "https://slack.com/app_redirect?channel=D_DM&team=T_ACME" in text + assert "TOKEN123" in text + + async def test_the_ephemeral_carries_the_link_itself(self, monkeypatch): + # Same audience as the DM (one person), and it's where the user is already + # looking — which is the whole point, since bot DMs are filed under "Apps" + # where people don't think to look. + uc, api = self._wire(monkeypatch) + await uc._offer_link(_inbound()) + eph = self._ephemeral(api) + assert "TOKEN123" in eph["text"] + assert eph["user"] == "U1" + # Still says where the durable copy is, since ephemerals vanish on reload. + assert "app_redirect" in eph["text"] + + async def test_send_cap_ephemeral_also_deep_links(self, monkeypatch): + # The "already sent" path is exactly when someone can't find the DM, so it + # needs the link more than the happy path does. + uc, api = self._wire(monkeypatch, allowed=False) + assert await uc._offer_link(_inbound(team_id="T_ACME")) is False + + text = self._ephemeral(api)["text"] + # Past the DM cap the user still gets the live link — the cap limits DMs, + # not what we're allowed to show the person in front of us. + assert "TOKEN123" in text + assert "https://slack.com/app_redirect?channel=D_DM&team=T_ACME" in text + # But no second DM. + methods = [m for m, _ in (c.args for c in api.await_args_list)] + assert "chat.postMessage" not in methods + + async def test_unreachable_dm_offers_nothing(self, monkeypatch): + # If we can't open the DM we can't link to it either, so pointing someone at + # a conversation that doesn't exist would be worse than staying quiet. + uc, api = self._wire(monkeypatch) + monkeypatch.setattr( + uc, + "_slack_api", + AsyncMock(return_value={"ok": False, "error": "cannot_dm_bot"}), + ) + assert await uc._offer_link(_inbound()) is False