refactor(agentex): shrink identity-link config, surface the connect link, stop leaking ids - #414
Merged
Merged
Conversation
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) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+66
to
+67
| names = session_cookie_names_to_forward() | ||
| return names[0] if names else None |
There was a problem hiding this comment.
If AGENTEX_DELEGATION_SESSION_COOKIE_NAMES contains multiple names and the authenticated session uses a non-first cookie, session_cookie_name() selects only the first entry and _session_credential() rejects the valid session, causing Slack linking to fail with “Couldn't read your session” even though the cookie is explicitly allowlisted for delegation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: agentex/src/domain/services/identity_link_service.py
Line: 66-67
Comment:
**Multi-cookie allowlist gets narrowed**
If `AGENTEX_DELEGATION_SESSION_COOKIE_NAMES` contains multiple names and the authenticated session uses a non-first cookie, `session_cookie_name()` selects only the first entry and `_session_credential()` rejects the valid session, causing Slack linking to fail with “Couldn't read your session” even though the cookie is explicitly allowlisted for delegation.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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) <noreply@anthropic.com>
michael-chou359
changed the title
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
<dd>{email or sgp_user_id}</dd> # 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) <noreply@anthropic.com>
michael-chou359
changed the title
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four fixes to the identity-link work. Three were found by using it — the config surface
was too large, the connect link was somewhere people don't look, the confirmation page
printed internal ids — and the fourth is a P1 the first one introduced, caught in review.
1. Config surface: 8 vars → 0
#409 → #412 accumulated eight environment variables. One was a hazard, one a trap,
six were knobs nobody has ever turned.
No behavior change at current settings — every constant equals the default it
replaced.
The hazard: a cookie name with two sources of truth
acting_headers()emits aCookieheader thatbuild_delegation_headersfilters downto its allowlist. Had the two disagreed, the credential would be stripped in transit
— every linked turn silently losing its acting identity, while the link sat in the
database looking stored, valid and healthy.
Now derived, so divergence is unrepresentable rather than warned about in a comment. An
empty allowlist returns
Noneandacting_headers()refuses, since emitting acredential that will certainly be stripped is worse than admitting we can't act.
The trap: a flag that had to move in lockstep with a Slack scope
The email check needs
users:read.email, which isn't granted. The flag kept it offuntil then — but flag and scope had to be flipped together: the flag alone refused
every link, the scope alone protected nothing.
It now enables itself: enforces whenever Slack answers with an email, stands down when
it won't. Granting the scope switches the protection on by itself.
no flag to distinguish "scope missing" from "Slack had a bad minute", failing closed
would make linking fail at random. The gap isn't attacker-reachable: nobody outside our
infrastructure influences whether our Slack lookup succeeds. Against what's deployed
today (flag off, verifying nothing) it's strictly stronger.
What remains
AGENTEX_CREDENTIAL_ENCRYPTION_KEYSLACK_GATEWAY_PUBLIC_BASE_URLSLACK_GATEWAY_REQUIRE_LINKED_USER2. The connect link was invisible
The first real offer in production was delivered correctly and reported as never
received. Both were true:
chat.postMessagereturnedokandconversations.historyconfirmed the message in the DM channel — and Slack files botconversations under Apps, not in the Direct messages list. "I've DM'd you a link"
pointed at the one place it wasn't.
Why this doesn't weaken anything
An ephemeral has exactly the same audience as a DM: rendered for one user, absent
from channel history and search. The exposure argument that made this DM-only never
applied to an ephemeral — so routing someone through a conversation they can't find, to
click a link we could hand them directly, bought nothing.
The DM stays because ephemerals are transient: reload 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 invariant, stated properly
The test that asserted "never in a payload addressed to the origin channel" now
asserts "never in a
chat.postMessageoutside the user's own DM". That matters — theold wording would have blocked this change while the actual risk was never touched by
it. A test encoding the implementation rather than the property blocks correct changes.
Also
conversations.openmoved above the send-cap check (idempotent; both branches needthe channel id).
we can show the person in front of us.
slack.com/app_redirect, notslack://, which fails on the web client.3. The connect page printed internal ids
It fell back to raw identifiers when it couldn't name an identity:
That leaks an internal id and buys nothing for it. The two identity rows exist so the
person clicking can answer "is this my Slack account?" and stop if it isn't — the
only defence against a link forwarded to them. Nobody recognises their own Slack member
id or SGP uuid, so the fallback never made that question answerable. It made an
unanswerable question look answered, which is worse than showing nothing.
nonce was minted shouldn't permanently degrade the page, and
users.infoneeds onlyusers:read, which is granted.you, don't continue" to saying the match can't be confirmed here. Claiming someone
verified something they couldn't is the actual harm.
4. A multi-name cookie allowlist got narrowed (P1, introduced by §1)
Deriving the cookie name fixed one hazard and introduced another.
session_cookie_name()returned only the first entry of
AGENTEX_DELEGATION_SESSION_COOKIE_NAMES, and theread path matched against that alone — so 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.
The allowlist is the set of names a deployment treats as valid sessions, so one arriving
under the second entry is exactly as legitimate as the first.
Reading and writing genuinely differ, so they're now separate functions rather than one
doing double duty — which is how the bug got in:
Allowlist order beats header order when several are present, so the stored credential is
the canonical cookie whenever it's there — which is also the name we emit 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, not
on its label. Flagged in the docstring — if a downstream ever became name-sensitive this
would need the originating name stored alongside the credential, which is a schema change
and not worth making speculatively.
Testing
26 new, 6 rewritten. The rewrites are the interesting ones — they invert assertions
that encoded superseded designs:
New coverage: the cookie name follows the delegation allowlist and the emitted cookie
survives the delegation filter under a non-default name;
acting_headersrefuses whendelegation is off; the ephemeral carries the link and points at the durable copy; an
unreachable DM offers nothing.
708 unit tests pass. The 14 Redis integration tests still pass against a real Redis
after the TTL constants moved.
ruffclean.Not fixed here
The nonce is still 10 minutes — sized assuming prompt discovery, which this incident
showed was optimistic. With the link now inline the find-it delay largely disappears,
so I'd rather see whether it's still a problem than change two things at once.
🤖 Generated with Claude Code
Greptile Summary
The PR simplifies identity-link configuration, improves Slack link discoverability, removes raw identifiers from confirmation pages, and fixes session extraction for multi-cookie delegation allowlists.
Confidence Score: 5/5
The PR appears safe to merge because the previously reported multi-cookie allowlist failure is fixed and no blocking eligible failure remains.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Request[Authenticated link confirmation] --> Parse[Parse inbound cookies] Allowlist[Delegation cookie allowlist] --> Parse Parse --> Select[Select first present allowlisted cookie] Select --> Store[Encrypt and store session JWT] Store --> Resolve[Resolve linked Slack identity] Allowlist --> Canonical[Choose canonical first cookie name] Resolve --> Emit[Emit JWT under canonical cookie name] Emit --> Delegate[Delegation filter forwards acting-user cookie]Reviews (4): Last reviewed commit: "fix(agentex): accept a session under any..." | Re-trigger Greptile