refactor(agentex): shrink identity-link config, surface the connect link, stop leaking ids

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_names,
 )
 from src.domain.services.link_nonce_service import LinkNonceService
 from src.utils import session_jwt
@@ -70,27 +69,16 @@
 # 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``.
+
+# 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:
@@ -155,15 +143,102 @@ 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``.
+
+    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.
     """
-    raw = request.headers.get("cookie") or ""
-    for part in raw.split(";"):
+    allowed = session_cookie_names()
+    if not allowed:
+        return None
+    jar: dict[str, str] = {}
+    for part in (request.headers.get("cookie") or "").split(";"):
         name, sep, value = part.strip().partition("=")
-        if sep and name.strip() == SESSION_COOKIE_NAME:
-            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
 
 
+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.
+
+    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
@@ -187,21 +262,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",
         "<h1>Connect your Slack account?</h1>"
         "<p>This lets the agent use <strong>your</strong> connected tools "
         "(Notion, Linear, …) when you ask it something in Slack.</p>"
         "<dl>"
-        f"<dt>Slack</dt><dd>{html.escape(slack_who)}</dd>"
-        f"<dt>SGP</dt><dd>{html.escape(email or sgp_user_id)}</dd>"
+        f"<dt>Slack</dt><dd>{html.escape(slack_who or _UNKNOWN_IDENTITY)}</dd>"
+        f"<dt>SGP</dt><dd>{html.escape(email or _UNKNOWN_IDENTITY)}</dd>"
         "</dl>"
         "<form method=post>"
         f"<input type=hidden name=nonce value='{html.escape(nonce)}'>"
         "<button type=submit>Connect</button></form>"
-        "<p class=muted style='margin-top:1.5rem'>If either name above isn't you, "
-        "close this page and don't continue.</p>",
+        f"<p class=muted style='margin-top:1.5rem'>{caution}</p>",
     )
 
 
@@ -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",
+            "<h1>Those accounts don't match</h1>"
+            "<p>The Slack account this link was made for and the SGP account "
+            "you're signed in as belong to different people.</p>"
+            "<p class=muted>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.</p>",
+            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",
-                "<h1>Those accounts don't match</h1>"
-                "<p>The Slack account this link was made for and the SGP account "
-                "you're signed in as belong to different people.</p>"
-                "<p class=muted>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.</p>",
-                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",
         "<h1>You're connected</h1>"
         f"<p>The agent will now use your own tools when you ask it something in "
-        f"Slack, as <strong>{html.escape(email or sgp_user_id)}</strong>.</p>"
+        f"Slack, as <strong>{html.escape(email or 'your SGP account')}</strong>.</p>"
         f"<p class=muted>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.</p>",

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,21 +38,49 @@
 
 # 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 = "-"
 
 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,
         }
 

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