From 6be698215b05c31595bd21681b3cedd20b7f7956 Mon Sep 17 00:00:00 2001 From: yzxcj797 Date: Thu, 20 Aug 2026 21:49:00 +0800 Subject: [PATCH 1/2] feat(slack): refuse unresolved own-team members as their own state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In email identity mode (the default posture of a standard deployment — the CLI scaffold hardcodes SLACK_IDENTITY_EMAIL=1 for Fly and AWS), an own-team member whose email the directory can't currently see was classified isExternalGuest — an org owner with a just-set email masqueraded as an external guest for the ~39-minute Slack propagation window, and the refusal copy, logs, and audit all said "external" when the truth was "own-team, principal unresolved" (#626). classifyUser now keeps the third state distinct: email-less members with no Slack-side external evidence carry principalUnresolved (native flags still decide isExternalGuest, and email keying is unchanged — a restricted member with a visible email remains an email-keyed guest). The turn-handler gate refuses principalUnresolved members ahead of the external-guest gate — equally fail-closed, never a fallback Slack-ID principal — but with its own user-facing copy (names the directory refresh, tells the sender to retry) and its own log line. The external-guest refusal path now logs on success too: the incident in #626 was undiagnosable precisely because a successful refusal left no trace, leaving H1/H2/H3 indistinguishable from the record. Not included: preserving users.info ok:false at callers (defect 1b) and user_change/team_join event-driven refresh (defect 3) — separate follow-ups per the issue's fix ladder. Fixes defects 1 and 2 of #626 --- src/slack/identity.ts | 18 +++++++++++++++++- src/slack/turn-handler.ts | 26 ++++++++++++++++++++++++++ test/slack-identity.test.ts | 21 ++++++++++++++++++++- 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/slack/identity.ts b/src/slack/identity.ts index f03aaf8d1..451da0ca2 100644 --- a/src/slack/identity.ts +++ b/src/slack/identity.ts @@ -5,6 +5,14 @@ export interface ActorAssertion { isExternalGuest?: boolean; isBot?: boolean; displayName?: string; + /** + * Own-team member whose principal could not be resolved in email identity + * mode (email not currently visible in the directory). Refused like a + * guest — fail closed, never a fallback Slack-ID principal — but as its + * OWN state, so logs and the user-facing refusal say "unresolved member" + * instead of masquerading as "external guest" (#626). + */ + principalUnresolved?: true; } export interface SlackUser { @@ -53,14 +61,22 @@ export function classifyUser( ); const displayName = user.profile?.display_name || user.real_name || user.name || user.id || ""; let externalId = String(user.id ?? ""); + // Email mode keys members on their email — including guests, whose + // principal still resolves for refusal copy and audit. An email-LESS + // member with no Slack-side external evidence is own-team but + // principal-unresolved: kept distinct from an external guest (which the + // native flags above decide) so refusal copy and logs name what actually + // happened instead of masquerading as external (#626). + let principalUnresolved = false; if (identity === "email" && !user.is_bot) { const email = (user.profile?.email ?? "").trim().toLowerCase(); if (email.includes("@")) externalId = email; - else isGuest = true; + else if (!isGuest) principalUnresolved = true; } return { externalId, isExternalGuest: isGuest, + ...(principalUnresolved ? { principalUnresolved: true as const } : {}), ...(user.is_bot ? { isBot: true } : {}), ...(displayName ? { displayName } : {}), }; diff --git a/src/slack/turn-handler.ts b/src/slack/turn-handler.ts index 199d7b547..9a4c0edcb 100644 --- a/src/slack/turn-handler.ts +++ b/src/slack/turn-handler.ts @@ -296,7 +296,33 @@ export function createTurnHandler(deps: { ...(ids.botHandle ? { botHandle: ids.botHandle } : {}), }; + // A principal-unresolved own-team member (email mode, email not yet + // visible) is refused as its OWN state — fail closed like a guest, but + // named for what it is, both to the sender and in the log, so the + // incident signature is decidable (#626: the refusal used to be + // indistinguishable from external-guest in logs, and its success path + // logged nothing at all). + const unresolvedMember = audience.find((a) => a.principalUnresolved); + if (unresolvedMember) { + console.warn( + `[slack-plugin] refusing turn: own-team member principal unresolved ` + + `(email identity mode; email not visible in the directory) ` + + `user=${unresolvedMember.externalId || "?"} ch=${inc.channel} ts=${inc.ts}`, + ); + if (!inc.unprompted) { + await ephemeralOrSay( + "I can't respond here yet — I couldn't verify your team membership " + + "(your email isn't visible to me). This usually resolves within a " + + "few minutes of the directory refreshing; try again shortly.", + ); + } + return; + } + if (audience.some((a) => a.isExternalGuest) && !(await externalParticipantsEnabled())) { + console.warn( + `[slack-plugin] refusing turn: external guest ch=${inc.channel} ts=${inc.ts}`, + ); if (!inc.unprompted) { await ephemeralOrSay( "I can't respond here — this conversation isn't fully internal. Try a DM or a fully-internal channel.", diff --git a/test/slack-identity.test.ts b/test/slack-identity.test.ts index c260e7706..5c4b25ae9 100644 --- a/test/slack-identity.test.ts +++ b/test/slack-identity.test.ts @@ -63,7 +63,26 @@ test("classifyUser email mode keys members on their normalized work email", () = test("classifyUser email mode fails closed to guest when a member has no visible email", () => { const a = classifyUser({ id: "U1", team_id: TEAM }, TEAM, "email"); assert.equal(a.externalId, "U1"); - assert.equal(a.isExternalGuest, true); + // Own-team member, principal unresolved: its OWN state, not external + // guest (#626). The refusal is equally closed, but logs and copy say + // what actually happened. + assert.equal(a.isExternalGuest, false); + assert.equal(a.principalUnresolved, true); +}); + +test("classifyUser email mode: native external evidence still wins over unresolved", () => { + // A restricted member with no visible email is an EXTERNAL GUEST, not an + // unresolved own-team member — the third state never masks the flags + // (#626). + const g = classifyUser({ id: "U2", team_id: TEAM, is_restricted: true }, TEAM, "email"); + assert.equal(g.isExternalGuest, true); + assert.equal(g.principalUnresolved, undefined); +}); + +test("classifyUser slack-id mode never marks principalUnresolved", () => { + const a = classifyUser({ id: "U1", team_id: TEAM }, TEAM, "slack-id"); + assert.equal(a.isExternalGuest, false); + assert.equal(a.principalUnresolved, undefined); }); test("classifyUser email mode keeps bots on their Slack id and non-guest", () => { From bb55f087ea7e1e31ad0520938c72bedb3e16b5fc Mon Sep 17 00:00:00 2001 From: yzxcj797 Date: Thu, 20 Aug 2026 21:58:15 +0800 Subject: [PATCH 2/2] fix(slack): preserve users.info failures instead of laundering them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A transient users.info failure returned { actor: { isExternalGuest: true }, ok: false } — the failure flag existed, but classifyActor stripped it (returning only .actor) and the direct-intake path read classifyUserCached and ignored .ok too, so a single failed lookup read to every consumer as a confident "this user is an external guest" and refused the member outright. In the #626 incident an org owner was refused on exactly that shape. - classifyUserCached's failure arm now returns a lookupFailed actor (the id-based placeholder, named for what it is) instead of asserting external guest — the failure state is distinguishable from every checked state (guest, internal, principal-unresolved). - classifyActor returns ActorAssertion & { ok } — the flag rides along instead of being discarded; consumers decide what a failed lookup means. The roster path already consumed ok (resolveChannelMembership sets complete=false); the two remaining discard sites no longer can. - The direct-intake path (turn-handler) acts on it: fall back to the directory's last-known classification when the cache holds one (time-bounded by the cache TTL — failures are never cached, so the fallback is always a prior success), log the fallback, and with no prior state refuse as principalUnresolved — the third-state path from the stacked commit — never a laundered "external". Stacked on the third-state commit (principalUnresolved + refusal gates); together they cover #626's defect 1 including its 1b sub-defect. The event-driven refresh (defect 3) remains a follow-up. Fixes the ok-propagation half of #626 (defect 1b) --- src/slack/directory.ts | 29 ++++++++++++++++++++---- src/slack/identity.ts | 7 ++++++ src/slack/turn-handler.ts | 21 +++++++++++++++++ test/slack-identity.test.ts | 45 +++++++++++++++++++++++++++++++++++++ 4 files changed, 98 insertions(+), 4 deletions(-) diff --git a/src/slack/directory.ts b/src/slack/directory.ts index baa2624c7..020a3ec65 100644 --- a/src/slack/directory.ts +++ b/src/slack/directory.ts @@ -65,7 +65,11 @@ export interface Directory { getUserSnapshot(client: any): Promise<{ byId: Map; fetchedAt: number } | undefined>; forceDirectorySync(client: any, invalidateChannelId?: string, invalidatePrincipalId?: string): Promise; classifyUserCached(client: any, userId: string): Promise; - classifyActor(client: any, userId: string): Promise; + lastKnownClassification(userId: string): ActorAssertion | undefined; + classifyActor( + client: any, + userId: string, + ): Promise; getChannelInfo(client: any, channel: string): Promise; channelMembership( client: any, @@ -588,12 +592,28 @@ export function createDirectory(deps: { if (ids.ownTeamId) userCache.set(userId, classified); return { ...classified, ok: true }; } catch { - return { actor: { externalId: userId, isExternalGuest: true }, ok: false }; + // Lookup failed — say so instead of asserting the user is an external + // guest (#626): the id-based actor carries lookupFailed so consumers + // can distinguish "couldn't check" from every checked state. + return { actor: { externalId: userId, lookupFailed: true }, ok: false }; } } - async function classifyActor(client: any, userId: string): Promise { - return (await classifyUserCached(client, userId)).actor; + function lastKnownClassification(userId: string): ActorAssertion | undefined { + // The LRU entry is the last SUCCESSFUL classification (failures are + // never cached), i.e. exactly the fallback a transient users.info + // failure should fall back to (#626). Time-bounded by the cache TTL. + return userCache.get(userId)?.actor; + } + + async function classifyActor(client: any, userId: string): Promise { + // ok preserved, not discarded: a failed users.info currently reads as a + // confident "external guest" to every caller — transiently refusing a + // member whose record is merely unreachable. Callers decide what a failed + // lookup means (skip, retry, fall back to last-known); the classification + // itself must not launder the failure into an identity claim (#626). + const { actor, ok } = await classifyUserCached(client, userId); + return { ...actor, ok }; } async function getChannelInfo(client: any, channel: string): Promise { @@ -658,6 +678,7 @@ export function createDirectory(deps: { forceDirectorySync, classifyUserCached, classifyActor, + lastKnownClassification, getChannelInfo, channelMembership, allInternalRosters, diff --git a/src/slack/identity.ts b/src/slack/identity.ts index 451da0ca2..1427e3857 100644 --- a/src/slack/identity.ts +++ b/src/slack/identity.ts @@ -13,6 +13,13 @@ export interface ActorAssertion { * instead of masquerading as "external guest" (#626). */ principalUnresolved?: true; + /** + * The users.info lookup itself failed — the classification below is a + * placeholder, not an identity claim. Distinct from every checked state + * (guest, internal, principal-unresolved) so callers can retry or fall + * back to last-known rather than acting on "external" (#626). + */ + lookupFailed?: true; } export interface SlackUser { diff --git a/src/slack/turn-handler.ts b/src/slack/turn-handler.ts index 9a4c0edcb..544556ab7 100644 --- a/src/slack/turn-handler.ts +++ b/src/slack/turn-handler.ts @@ -199,6 +199,27 @@ export function createTurnHandler(deps: { ...(inc.prefetched.timezone ? { timezone: inc.prefetched.timezone } : {}), }; else classified = await classifyUserCached(client, inc.userId); + // A failed users.info must not read as a confident external-guest + // refusal: log it so the transient failure is visible, and fall back to + // the LAST-KNOWN classification when one exists (the incident in #626 + // showed a single failed lookup refusing an owner outright). With no + // prior state, fail closed but as "unverifiable", the third-state + // refusal path — never a laundered "external". + if ("ok" in classified && !classified.ok) { + const cachedAgain = await directory.lastKnownClassification?.(inc.userId); + if (cachedAgain) { + console.warn( + `[slack-plugin] users.info failed for ${inc.userId}; using last-known classification`, + ); + classified = { actor: cachedAgain }; + } else { + console.warn( + `[slack-plugin] users.info failed for ${inc.userId} with no prior classification; ` + + `refusing as unverifiable ch=${inc.channel} ts=${inc.ts}`, + ); + classified = { actor: { ...classified.actor, principalUnresolved: true as const } }; + } + } const actor = classified.actor; const timezone = classified.timezone; const text = stripMention(inc.rawText, ids.botUserId); diff --git a/test/slack-identity.test.ts b/test/slack-identity.test.ts index 5c4b25ae9..0932b9992 100644 --- a/test/slack-identity.test.ts +++ b/test/slack-identity.test.ts @@ -102,6 +102,51 @@ test("classifyUser email mode still flags restricted/other-workspace members as assert.equal(g.isExternalGuest, true); }); +test("a failed users.info asserts lookupFailed, not external guest (#626)", async () => { + // The directory's classifyUserCached currently launders a transient + // users.info failure into a confident "external guest" — the incident + // refused an owner outright on one failed lookup. The failure state must + // be distinguishable from every checked state. + const { createDirectory } = await import("../src/slack/directory.ts"); + const failingClient = { + users: { + info: async () => { throw new Error("transient"); }, + list: async () => ({ members: [] }), + }, + conversations: { list: async () => ({ channels: [] }) }, + auth: { test: async () => ({ ok: true, user_id: "UBOT", team_id: "T1" }) }, + }; + const dir = createDirectory({ + core: failingClient as never, + ids: { botUserId: "UBOT", ownTeamId: "T1", identityMode: "slack-id" } as never, + }); + const result = await dir.classifyUserCached(failingClient, "U1"); + assert.equal(result.ok, false); + assert.equal(result.actor.lookupFailed, true, "the failure is named, not guest-folded"); + assert.equal(result.actor.isExternalGuest, undefined); +}); + +test("classifyActor preserves ok instead of discarding it (#626)", async () => { + // Shape check without a live client: the interface change is the contract + // (ok rides on the assertion); the directory unit above covers behavior. + const { createDirectory } = await import("../src/slack/directory.ts"); + const okClient = { + users: { + info: async () => ({ user: { id: "U1", team_id: "T1" } }), + list: async () => ({ members: [] }), + }, + conversations: { list: async () => ({ channels: [] }) }, + auth: { test: async () => ({ ok: true, user_id: "UBOT", team_id: "T1" }) }, + }; + const dir = createDirectory({ + core: okClient as never, + ids: { botUserId: "UBOT", ownTeamId: "T1", identityMode: "slack-id" } as never, + }); + const actor = await dir.classifyActor(okClient, "U1"); + assert.equal(actor.ok, true, "ok rides on the returned assertion"); + assert.equal(actor.isExternalGuest, false); +}); + test("slackUserTimezone extracts only valid Slack user timezones", () => { assert.equal(slackUserTimezone({ id: "U1", tz: "America/Los_Angeles" }), "America/Los_Angeles"); assert.equal(slackUserTimezone({ id: "U1", profile: { tz: "Europe/London" } }), "Europe/London");