From beac3a516136f5c2f5cc953f4f18e15977536da1 Mon Sep 17 00:00:00 2001 From: Michael Chou Date: Sat, 29 Aug 2026 22:13:43 -0700 Subject: [PATCH 1/4] refactor(agentex): cut the identity-link config surface from 8 vars to 1 The identity-link work accumulated eight environment variables. One was a real hazard, one was a trap, and six were knobs nobody has ever turned. Removing them also removes two failure modes. The hazard: IDENTITY_LINK_SESSION_COOKIE_NAME The session cookie name was configurable in two places -- here and AGENTEX_DELEGATION_SESSION_COOKIE_NAMES in delegation_headers. acting_headers() emits a Cookie header that build_delegation_headers then filters down to its allowlist, so if the two ever disagreed the credential would be stripped in transit and every linked turn would silently lose its acting identity, while the link sat in the database looking stored, valid and healthy. It is now derived: session_cookie_name() reads the delegation allowlist. One source of truth, so divergence is unrepresentable rather than merely documented. Empty allowlist (cookie delegation disabled) returns None and acting_headers() refuses, since emitting a credential that will certainly be stripped is worse than admitting we cannot act. The trap: IDENTITY_LINK_REQUIRE_EMAIL_MATCH The email check needs the users:read.email Slack scope, which isn't granted. The flag existed to keep it off until the scope lands -- but flag and scope then had to be flipped together: the flag alone refused every link (unreadable email treated as mismatch), and the scope alone protected nothing. It now enables itself. _email_mismatch() enforces whenever Slack answers with an email and stands down when it won't, so granting the scope switches the protection on with no config change and no ordering hazard. That inverts the unverifiable case from refuse to allow, which is weaker, and deliberately so: with no flag to distinguish "scope missing" from "Slack had a bad minute", failing closed would make linking fail at random. The gap is not attacker-reachable -- nobody outside our infrastructure influences whether our own Slack lookup succeeds -- and the previous shipped state (flag off) verified nothing at all, so this is strictly stronger than what it replaces. The knobs -> module constants IDENTITY_LINK_NONCE_TTL, _MAX_DMS, _CACHE_TTL, _NEGATIVE_CACHE_TTL, _FALLBACK_TTL_DAYS and SLACK_LINK_OFFER_COOLDOWN_S are now constants at their former defaults. Each was a config surface and a branch carrying a value that has never been set to anything else, and therefore never tested at anything else. They crept in by pattern-matching the surrounding file, which isn't a reason. What remains: AGENTEX_CREDENTIAL_ENCRYPTION_KEY and SLACK_GATEWAY_PUBLIC_BASE_URL, both deployment-specific with no sensible default, plus the pre-existing SLACK_GATEWAY_REQUIRE_LINKED_USER, which is a genuine product choice. No behavior change at current settings: every constant equals the default it replaced, and the email check's effective behavior in production (no scope, flag off -> no verification) is unchanged until the scope is granted. Testing: 10 new unit tests. The cookie-name ones assert the end-to-end property that whatever name is configured, what acting_headers emits is what build_delegation_headers forwards -- and that a disabled allowlist refuses rather than emitting something that gets stripped. The email ones pin both directions of the asymmetry: verified-different refuses (and leaves the nonce intact for the legitimate owner), while missing scope, lookup failure and a principal without an email all allow. Full unit suite 691 passed; the 14 Redis integration tests still pass against a real Redis after the TTL constants moved. Co-Authored-By: Claude Opus 5 (1M context) --- agentex/src/api/routes/integrations.py | 141 +++++++++++------- .../domain/services/identity_link_service.py | 42 ++++-- .../src/domain/services/link_nonce_service.py | 5 +- .../use_cases/slack_gateway_use_case.py | 25 ++-- .../unit/api/test_integrations_routes.py | 97 ++++++------ .../services/test_identity_link_service.py | 64 ++++++++ 6 files changed, 247 insertions(+), 127 deletions(-) diff --git a/agentex/src/api/routes/integrations.py b/agentex/src/api/routes/integrations.py index d48bd0fb..25f546a3 100644 --- a/agentex/src/api/routes/integrations.py +++ b/agentex/src/api/routes/integrations.py @@ -41,7 +41,6 @@ from __future__ import annotations import html -import os from datetime import UTC, datetime, timedelta from fastapi import APIRouter, Form, Request @@ -55,8 +54,8 @@ from src.domain.entities.identity_links import IdentityLinkMethod, IdentityProvider from src.domain.repositories.identity_link_repository import IdentityLinkRepository from src.domain.services.identity_link_service import ( - SESSION_COOKIE_NAME, IdentityLinkService, + session_cookie_name, ) from src.domain.services.link_nonce_service import LinkNonceService from src.utils import session_jwt @@ -70,27 +69,10 @@ # Used only when the session token doesn't declare its own expiry. Never "no # expiry": storing a credential with an unbounded lifetime is how you end up # holding one indefinitely, so an unknown expiry becomes a short known one. -_FALLBACK_TTL_DAYS = int(os.getenv("IDENTITY_LINK_FALLBACK_TTL_DAYS", "30")) - -# Require the Slack account's email to match the signed-in SGP account's. -# -# This is the only real defence against a *forwarded* link. The nonce stops an -# attacker forging someone else's Slack identity, but nothing stops 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 thereafter the -# attacker's Slack messages run as the victim, using the victim's integrations. The -# confirmation page names both identities, which catches a mis-click but reduces to -# user vigilance against a deliberate attempt. -# -# OFF by default because it needs the ``users:read.email`` Slack scope, which is not -# granted until the app is reinstalled. Enabling it without the scope would refuse -# every link (the check treats an unreadable email as a mismatch, deliberately), so -# the flag and the scope have to be turned on together. -_REQUIRE_EMAIL_MATCH = os.getenv("IDENTITY_LINK_REQUIRE_EMAIL_MATCH", "").lower() in ( - "1", - "true", - "yes", -) +_FALLBACK_TTL_DAYS = 30 + +# Email matching has no flag: it enforces itself whenever Slack will tell us the +# email, and stands down when it won't. See ``_email_mismatch``. def _page(title: str, body: str, *, status: int = 200) -> HTMLResponse: @@ -155,15 +137,71 @@ def _session_credential(request: Request) -> str | None: chokes on, silently dropping every morsel after the first bad one — which can include the session cookie itself. The same reasoning (and the same approach) applies in ``delegation_headers``. + + The name comes from the delegation allowlist, so what we store here is by + construction what the delegation layer will forward later. None when cookie + delegation is disabled: there would be no way to act through the credential, so + there is no point storing one. """ + wanted = session_cookie_name() + if wanted is None: + return None raw = request.headers.get("cookie") or "" for part in raw.split(";"): name, sep, value = part.strip().partition("=") - if sep and name.strip() == SESSION_COOKIE_NAME: + if sep and name.strip() == wanted: return value.strip() or None return None +async def _email_mismatch(external_user_id: str, sgp_email: str | None) -> bool: + """True when Slack and SGP demonstrably identify different people. + + This is the only real 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, who — clicking it while signed in — would bind the + attacker's Slack identity to their SGP account, after which the attacker's Slack + messages run as them with their integrations. + + **Self-enabling, with no flag.** The check needs the ``users:read.email`` Slack + scope, which may not be granted. Rather than gate that on configuration — where + the flag and the scope must be flipped together, and flipping one alone either + breaks every link or silently protects nothing — it simply enforces whenever + Slack answers with an email and stands down when it won't. Granting the scope + turns the protection on by itself. + + So the asymmetry is deliberate: **verified different -> refuse; unverifiable -> + allow and warn.** Failing closed on an unreadable email would be stronger, but + with no flag to distinguish "scope missing" from "Slack had a bad minute" it + would make linking fail randomly. The gap it leaves is not attacker-reachable: + nobody outside our infrastructure influences whether our own Slack lookup + succeeds. + """ + from src.domain.use_cases.slack_gateway_use_case import slack_user_profile + + profile = await slack_user_profile(external_user_id) + slack_email = profile.get("email") + if not slack_email: + logger.warning( + "identity link: email not verified (Slack would not tell us)", + extra={ + "external_user_id": external_user_id, + "slack_error": profile.get("error"), + "hint": "grant users:read.email to enable this check", + }, + ) + return False + if not sgp_email: + # Slack gave us an email but the SGP session didn't. Nothing to compare, so + # the same rule applies: can't verify, don't block. + logger.warning( + "identity link: email not verified (no email on the SGP principal)", + extra={"external_user_id": external_user_id}, + ) + return False + return slack_email.strip().lower() != sgp_email.strip().lower() + + @router.get("/slack/link", summary="Confirm linking a Slack identity to SGP") async def slack_link_page(request: Request, nonce: str = "") -> HTMLResponse: """Render the confirmation screen. Does NOT consume the nonce, so a refresh or @@ -267,47 +305,34 @@ 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", + "

Those 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", - "

Those 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, "cookie": session_cookie_name()}, ) return _page( "Couldn't read your session", diff --git a/agentex/src/domain/services/identity_link_service.py b/agentex/src/domain/services/identity_link_service.py index 811d476c..fc488f1c 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,24 @@ 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_name() -> str | None: + """The cookie name to store and emit, or None if cookie delegation is off. + + **Derived from the delegation allowlist, never separately configurable.** The + stored credential leaves as a Cookie header that ``build_delegation_headers`` + filters down to its allowlisted names before re-emitting as + ``x-acting-user-cookie``. If this name and that allowlist 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 entirely. + + None when the allowlist is empty (cookie delegation explicitly disabled): there + is then no way to act through a stored session, so callers must refuse rather + than store or emit something that cannot work. + """ + names = session_cookie_names_to_forward() + return names[0] if names else None def _cache_key( @@ -172,8 +184,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..f45f7b44 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, } diff --git a/agentex/tests/unit/api/test_integrations_routes.py b/agentex/tests/unit/api/test_integrations_routes.py index ed0d5dca..6f4323ba 100644 --- a/agentex/tests/unit/api/test_integrations_routes.py +++ b/agentex/tests/unit/api/test_integrations_routes.py @@ -260,77 +260,86 @@ 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): - 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): + def _slack_profile(self, monkeypatch, **profile): 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" 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 From 556b3cb44bdd4ac292aae0f4212cecce611751f7 Mon Sep 17 00:00:00 2001 From: Michael Chou Date: Sat, 29 Aug 2026 22:42:17 -0700 Subject: [PATCH 2/4] fix(agentex): put the connect link where the user is actually looking The first real link offer in production was delivered correctly and reported as never received. Both were true: chat.postMessage returned ok, conversations.history confirmed the message sitting in the DM channel -- and Slack files bot conversations under "Apps", not in the Direct messages list, so "I've DM'd you a link" pointed at the one place it wasn't. The link now goes in the ephemeral as well as the DM. An ephemeral has exactly the same audience as a DM: Slack renders it for one user, keeps it out of channel history and out of search. So the exposure argument that made this DM-only never applied to an ephemeral -- and routing someone through a conversation they can't find, to click a link we could have handed them directly, bought nothing. 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. So the ephemeral carries the link plus a deep link to the DM as the durable copy. The security invariant is unchanged but is now stated precisely, because this change moves the line: the nonce is a bearer token, so it may go anywhere exactly one person can see it (the DM, an ephemeral) and nowhere that lands in channel history. The test that used to assert "never in a payload addressed to the origin channel" now asserts "never in a chat.postMessage outside the user's own DM", which is the property that actually matters -- the old wording would have failed this change while the real risk was untouched. conversations.open moved above the send-cap check, since both branches now need the channel id for the deep link. It's idempotent (returns the existing DM), and past the cap the user still gets the live link -- the cap limits DMs, not what we can show the person in front of us. Deep links use slack.com/app_redirect rather than a slack:// URI, which doesn't work on the web client. Testing: 3 new tests plus 3 rewritten. The rewrites are the interesting ones -- they invert assertions that encoded the old design (nonce must not appear in the ephemeral; nothing addressed to the origin channel may carry it) into the ones that encode the new invariant. 93 in the gateway suite, 689 across unit. Co-Authored-By: Claude Opus 5 (1M context) --- .../use_cases/slack_gateway_use_case.py | 51 +++++-- .../use_cases/test_slack_gateway_use_case.py | 125 +++++++++++++++++- 2 files changed, 158 insertions(+), 18 deletions(-) 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 f45f7b44..b7f938a3 100644 --- a/agentex/src/domain/use_cases/slack_gateway_use_case.py +++ b/agentex/src/domain/use_cases/slack_gateway_use_case.py @@ -1297,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 @@ -1319,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", { @@ -1348,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/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 From de65803560a54caf0e7cc15295184f5fe77875c6 Mon Sep 17 00:00:00 2001 From: Michael Chou Date: Sat, 29 Aug 2026 22:53:13 -0700 Subject: [PATCH 3/4] fix(agentex): stop printing internal ids on the connect page The confirmation page fell back to raw identifiers when it couldn't name an identity: slack_who = link_request.display_name or link_request.external_user_id # U0B01457V24
{email or sgp_user_id}
# 5da8f784-... That leaks an internal id, and it doesn't even buy anything in exchange. The two identity rows exist for one purpose: so the person clicking can answer "is this MY Slack account?" and stop if it isn't -- the only defence against a link that was forwarded to them. Nobody recognises their own Slack member id or SGP uuid, so the fallback never made that question answerable. It just made an unanswerable question look answered, which is worse than showing nothing. Now: - The Slack side prefers the name captured when the nonce was minted, and re-reads it live when that came back empty -- a transient Slack failure at offer time shouldn't permanently degrade the page, and users.info needs only users:read, which is granted. - Neither side ever falls back to an id. An unnameable identity renders a placeholder. - When either side is unnamed, the caution line changes from "if either name above isn't you, don't continue" to saying the match can't be confirmed here and to continue only if you just asked for the link yourself. Claiming someone verified something they had no way to verify is the actual harm. - The success page likewise stops printing the uuid when there's no email. Testing: 6 new unit tests -- named identities still shown; a missing Slack name does not fall back to the member id; a missing name IS recovered by a live lookup; a missing email does not fall back to the uuid; the unnamed case says the check is unavailable; the success page prints no uuid. 701 unit tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- agentex/src/api/routes/integrations.py | 47 +++++++++-- .../unit/api/test_integrations_routes.py | 84 +++++++++++++++++++ 2 files changed, 125 insertions(+), 6 deletions(-) diff --git a/agentex/src/api/routes/integrations.py b/agentex/src/api/routes/integrations.py index 25f546a3..1952d90b 100644 --- a/agentex/src/api/routes/integrations.py +++ b/agentex/src/api/routes/integrations.py @@ -74,6 +74,12 @@ # Email matching has no flag: it enforces itself whenever Slack will tell us the # email, and stands down when it won't. See ``_email_mismatch``. +# Shown when an identity can't be named. Deliberately not the underlying id: a raw +# Slack member id or SGP uuid is meaningless to the person being asked to check it, +# so printing one exposes an internal identifier while making an unanswerable +# question look answered. +_UNKNOWN_IDENTITY = "(couldn't identify this account)" + def _page(title: str, body: str, *, status: int = 200) -> HTMLResponse: """Minimal self-contained page. No external assets — this renders inside @@ -154,6 +160,26 @@ def _session_credential(request: Request) -> str | None: return None +async def _slack_display_name(link_request) -> str | None: + """A human-recognisable name for the Slack side, or None. + + Never falls back to the Slack member ID. The identity rows exist so someone can + answer "is this *my* account?", and nobody recognises their own ``U…`` id — so a + raw identifier doesn't make the check possible, it only makes an unanswerable + check look answered, while exposing an internal id for nothing. + + The name is normally captured when the nonce is minted. This re-reads it when + that lookup came back empty (a transient Slack failure at offer time shouldn't + permanently degrade the page), which needs only ``users:read``. + """ + if link_request.display_name: + return link_request.display_name + from src.domain.use_cases.slack_gateway_use_case import slack_user_profile + + profile = await slack_user_profile(link_request.external_user_id) + return profile.get("display_name") or None + + async def _email_mismatch(external_user_id: str, sgp_email: str | None) -> bool: """True when Slack and SGP demonstrably identify different people. @@ -225,21 +251,30 @@ async def slack_link_page(request: Request, nonce: str = "") -> HTMLResponse: status=401, ) - slack_who = link_request.display_name or link_request.external_user_id + slack_who = await _slack_display_name(link_request) + unnamed = slack_who is None or not email + caution = ( + # When a side can't be named, the check the user is being asked to perform + # is not available to them. Say that, rather than implying they verified + # something they couldn't. + "One of these accounts couldn't be identified, so you can't confirm the " + "match here. Only continue if you just asked for this link yourself." + if unnamed + else "If either name above isn't you, close this page and don't continue." + ) return _page( "Connect your account", "

Connect your Slack account?

" "

This lets the agent use your connected tools " "(Notion, Linear, …) when you ask it something in Slack.

" "
" - f"
Slack
{html.escape(slack_who)}
" - f"
SGP
{html.escape(email or sgp_user_id)}
" + f"
Slack
{html.escape(slack_who or _UNKNOWN_IDENTITY)}
" + f"
SGP
{html.escape(email or _UNKNOWN_IDENTITY)}
" "
" "
" f"" "
" - "

If either name above isn't you, " - "close this page and don't continue.

", + f"

{caution}

", ) @@ -416,7 +451,7 @@ async def slack_link_confirm(request: Request, nonce: str = Form("")) -> HTMLRes "Connected", "

You're connected

" f"

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/tests/unit/api/test_integrations_routes.py b/agentex/tests/unit/api/test_integrations_routes.py index 6f4323ba..62c9de77 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 @@ -343,3 +344,86 @@ async def test_the_check_runs_on_every_confirm(self, wiring, monkeypatch): 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() From bbceae918f4288e6c22499e5638ee9d8932aeed4 Mon Sep 17 00:00:00 2001 From: Michael Chou Date: Sat, 29 Aug 2026 23:02:40 -0700 Subject: [PATCH 4/4] fix(agentex): accept a session under any allowlisted cookie name session_cookie_name() returned only the FIRST entry of AGENTEX_DELEGATION_SESSION_COOKIE_NAMES, and _session_credential() matched against that alone. With a multi-name allowlist, a session carried by any later name was rejected: linking failed with "couldn't read your session" for a cookie the delegation layer would have forwarded quite happily. Nothing was wrong with the request; the read was just too narrow. The allowlist is the set of cookie names a deployment treats as valid sessions, so one arriving under the second entry is exactly as legitimate as the first. Reading now accepts any of them. Reading and writing genuinely differ, so they are now separate functions rather than one doing double duty -- which is how the bug got in: session_cookie_names() every accepted name, in preference order (READ) session_cookie_name() the single canonical name to emit under (WRITE) Allowlist order beats header order when a request carries several, so the stored credential is the deployment's canonical cookie whenever it is present -- which is also the name acting_headers() emits under, keeping the common case exact. Emitting a value that arrived under a later name as the canonical one is safe: the credential is a session JWT, validated downstream on its contents rather than on the label it travels under. Noted in the docstring, because if a downstream ever became name-sensitive this would need the originating name stored alongside the credential -- a schema change, not worth making speculatively. Parsing mirrors delegation_headers._minimal_session_cookie: split on ';', first occurrence of a name wins, never trust a non-allowlisted morsel. Widening to "any cookie that looks like a session" would let a caller nominate which of their cookies we store. Testing: 7 new unit tests -- a session under a later name is accepted; allowlist order wins when several are present; a later name survives a realistic browser header full of analytics morsels; names outside the allowlist are still rejected; an empty allowlist reads nothing; a blank value is not a session; and the emitted name stays canonical. 708 unit tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- agentex/src/api/routes/integrations.py | 38 +++++++---- .../domain/services/identity_link_service.py | 40 ++++++++---- .../unit/api/test_integrations_routes.py | 65 +++++++++++++++++++ 3 files changed, 119 insertions(+), 24 deletions(-) diff --git a/agentex/src/api/routes/integrations.py b/agentex/src/api/routes/integrations.py index 1952d90b..3aa534bd 100644 --- a/agentex/src/api/routes/integrations.py +++ b/agentex/src/api/routes/integrations.py @@ -55,7 +55,7 @@ from src.domain.repositories.identity_link_repository import IdentityLinkRepository from src.domain.services.identity_link_service import ( IdentityLinkService, - session_cookie_name, + session_cookie_names, ) from src.domain.services.link_nonce_service import LinkNonceService from src.utils import session_jwt @@ -144,19 +144,30 @@ def _session_credential(request: Request) -> str | None: include the session cookie itself. The same reasoning (and the same approach) applies in ``delegation_headers``. - The name comes from the delegation allowlist, so what we store here is by - construction what the delegation layer will forward later. None when cookie - delegation is disabled: there would be no way to act through the credential, so - there is no point storing one. + Accepts **any** name on the delegation allowlist, not only the first. The + allowlist is the set of cookie names this deployment treats as valid sessions, + so a session arriving under the second entry is exactly as legitimate as one + under the first — matching only the first would reject a perfectly good session + and fail the link with "couldn't read your session", while the delegation layer + would happily have forwarded it. + + Allowlist order wins over header order when a request carries more than one, so + the credential we keep is the deployment's canonical cookie whenever it's + present. None when cookie delegation is disabled: there would be no way to act + through the credential, so there is no point storing one. """ - wanted = session_cookie_name() - if wanted is None: + allowed = session_cookie_names() + if not allowed: return None - raw = request.headers.get("cookie") or "" - for part in raw.split(";"): + jar: dict[str, str] = {} + for part in (request.headers.get("cookie") or "").split(";"): name, sep, value = part.strip().partition("=") - if sep and name.strip() == wanted: - return value.strip() or None + if sep: + # First occurrence wins, matching delegation_headers' parsing. + jar.setdefault(name.strip(), value.strip()) + for name in allowed: + if jar.get(name): + return jar[name] return None @@ -367,7 +378,10 @@ async def slack_link_confirm(request: Request, nonce: str = Form("")) -> HTMLRes # 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", diff --git a/agentex/src/domain/services/identity_link_service.py b/agentex/src/domain/services/identity_link_service.py index fc488f1c..dfb19b56 100644 --- a/agentex/src/domain/services/identity_link_service.py +++ b/agentex/src/domain/services/identity_link_service.py @@ -48,22 +48,38 @@ HEADER_SELECTED_ACCOUNT_ID = "x-selected-account-id" -def session_cookie_name() -> str | None: - """The cookie name to store and emit, or None if cookie delegation is off. +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 its allowlisted names before re-emitting as - ``x-acting-user-cookie``. If this name and that allowlist 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 entirely. - - None when the allowlist is empty (cookie delegation explicitly disabled): there - is then no way to act through a stored session, so callers must refuse rather - than store or emit something that cannot work. + 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_to_forward() + names = session_cookie_names() return names[0] if names else None diff --git a/agentex/tests/unit/api/test_integrations_routes.py b/agentex/tests/unit/api/test_integrations_routes.py index 62c9de77..c31ea3eb 100644 --- a/agentex/tests/unit/api/test_integrations_routes.py +++ b/agentex/tests/unit/api/test_integrations_routes.py @@ -427,3 +427,68 @@ async def test_success_page_does_not_print_the_uuid(self, wiring, monkeypatch): ) 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"