security: harden Slack approval handling

This commit is contained in:
Rohit P
2026-07-24 22:33:13 -07:00
parent 56b3c62864
commit 7656952692
21 changed files with 931 additions and 33 deletions
+25 -4
View File
@@ -357,11 +357,12 @@ export async function mockApi(page: import("@playwright/test").Page) {
mode: "relay" as "" | "relay",
account: "deeplearning.ai",
allowed_users: [] as string[], // flat list (manual Socket Mode only)
approval_owner_ids: [] as string[],
workspaces: [
// T1DL mirrors a managed install: the installer (authed_user) was pre-added
// to the allow-list on connect (UX-027) — keys the "you" chip + setup card.
{ team_id: "T1DL", account: "deeplearning.ai", domain: "dlaiteam", allowed_users: ["U_ME"] as string[], allow_all: false, allowed_user_names: {} as Record<string, string | null>, installer_user_id: "U_ME", installer_name: "Rohit Prasad" },
{ team_id: "T2AC", account: "acme-partners", domain: "acmehq", allowed_users: [] as string[], allow_all: false, allowed_user_names: {} as Record<string, string | null>, installer_user_id: "", installer_name: "" },
{ team_id: "T1DL", account: "deeplearning.ai", domain: "dlaiteam", allowed_users: ["U_ME"] as string[], allow_all: false, allowed_user_names: {} as Record<string, string | null>, approval_owner_ids: ["U_ME"] as string[], approval_owner_names: { U_ME: "Rohit Prasad" } as Record<string, string | null>, installer_user_id: "U_ME", installer_name: "Rohit Prasad" },
{ team_id: "T2AC", account: "acme-partners", domain: "acmehq", allowed_users: [] as string[], allow_all: false, allowed_user_names: {} as Record<string, string | null>, approval_owner_ids: [] as string[], approval_owner_names: {} as Record<string, string | null>, installer_user_id: "", installer_name: "" },
],
};
const slackConnector = () => ({
@@ -369,7 +370,9 @@ export async function mockApi(page: import("@playwright/test").Page) {
auth: "bot_token", two_way: true, channels: true, available: true, brand_color: "#611f69", logo: "slack",
fields: [], instructions: [], connected: slackState.connected,
account: slackState.account, enabled: slackState.connected,
allowed_users: [...slackState.allowed_users], tools: [], managed: true,
allowed_users: [...slackState.allowed_users],
approval_owner_ids: [...slackState.approval_owner_ids],
tools: [], managed: true,
managed_profile: slackState.mode === "relay", mode: slackState.mode,
workspaces: slackState.workspaces.map((w) => ({ ...w, allowed_users: [...w.allowed_users] })),
unauthorized: parked.map((x) => ({ ...x })),
@@ -911,6 +914,24 @@ export async function mockApi(page: import("@playwright/test").Page) {
if (add && b.name && ws) ws.allowed_user_names[b.user_id] = b.name;
return json({ ok: true, allowed_users: [...pool], team_id: b.team_id ?? null });
}
if (p.endsWith("/v1/connectors/slack/approval-owners/add") && m === "POST") {
const b = req.postDataJSON();
if (!slackState.approval_owner_ids.includes(b.user_id))
slackState.approval_owner_ids.push(b.user_id);
if (!slackState.allowed_users.includes(b.user_id))
slackState.allowed_users.push(b.user_id);
return json({
ok: true,
approval_owner_ids: [...slackState.approval_owner_ids],
allowed_users: [...slackState.allowed_users],
});
}
if (p.endsWith("/v1/connectors/slack/approval-owners/remove") && m === "POST") {
const b = req.postDataJSON();
const i = slackState.approval_owner_ids.indexOf(b.user_id);
if (i >= 0) slackState.approval_owner_ids.splice(i, 1);
return json({ ok: true, approval_owner_ids: [...slackState.approval_owner_ids] });
}
// Workspace rosters for the pickers (users.list / conversations.list, mocked).
if (/\/v1\/connectors\/slack\/workspaces\/[^/]+\/directory$/.test(p) && m === "GET") {
const q = (new URL(req.url()).searchParams.get("q") || "").toLowerCase();
@@ -1137,7 +1158,7 @@ export async function mockApi(page: import("@playwright/test").Page) {
// Slack managed install = add a workspace. The real flow completes in the system
// browser; the mock installs instantly so the page's poll picks it up.
if (p.includes("/connectors/slack/")) {
slackState.workspaces.push({ team_id: "T3NEW", account: "new-workspace", allowed_users: [], allow_all: false, allowed_user_names: {} });
slackState.workspaces.push({ team_id: "T3NEW", account: "new-workspace", domain: "new-workspace", allowed_users: ["U_ME"], allow_all: false, allowed_user_names: { U_ME: "Rohit Prasad" }, approval_owner_ids: ["U_ME"], approval_owner_names: { U_ME: "Rohit Prasad" }, installer_user_id: "U_ME", installer_name: "Rohit Prasad" });
slackState.connected = true;
slackState.mode = "relay";
}
+3 -3
View File
@@ -59,13 +59,13 @@ test("routing: Configure tab binds the mirror channel; Pending's status line fol
await page.getByTestId("inbox-route-configure").click();
const mirror = page.getByTestId("inbox-mirror-card");
await expect(mirror).toContainText("in-app Inbox only");
await mirror.getByPlaceholder("slack:C0123 or channel link").fill("C0777");
await mirror.getByPlaceholder("slack:C0123 or channel link").fill("slack:T1DL/C0777");
await mirror.getByRole("button", { name: "Set", exact: true }).click();
await expect(mirror).toContainText("slack:C0777");
await expect(mirror).toContainText("slack:T1DL/C0777");
// Back on Pending, the line reflects the new target immediately.
await page.getByTestId("inbox-tab-pending").click();
await expect(line).toContainText("slack:C0777");
await expect(line).toContainText("slack:T1DL/C0777");
await expect(line).toContainText("replies there resolve items here");
// Clearing (also on Configure) returns Pending to local-only delivery.
+16
View File
@@ -61,6 +61,7 @@ test("disconnect removes one workspace and keeps the rest relaying", async ({ pa
test("manual Socket Mode: one card with the flat allow-list (no regression)", async ({
page,
}) => {
let owners: string[] = [];
// Override the connectors payload AFTER mockApi so this test sees a manual-mode Slack
// (routes registered later match first).
await page.route("**/v1/connectors", (route) =>
@@ -74,6 +75,8 @@ test("manual Socket Mode: one card with the flat allow-list (no regression)", as
auth: "bot_token", two_way: true, available: true, brand_color: "#611f69",
logo: "slack", fields: [], instructions: [], connected: true, account: "acme",
enabled: true, allowed_users: ["U0OK"], allowed_user_names: { U0OK: "Rohit" },
approval_owner_ids: [...owners],
approval_owner_names: Object.fromEntries(owners.map((u) => [u, u === "U9MAYA" ? "Maya Chen" : u])),
tools: [], managed: true, managed_profile: false, mode: "", workspaces: [],
unauthorized: [],
},
@@ -81,9 +84,22 @@ test("manual Socket Mode: one card with the flat allow-list (no regression)", as
}),
}),
);
await page.route("**/v1/connectors/slack/approval-owners/add", async (route) => {
const body = route.request().postDataJSON();
owners = [...new Set([...owners, body.user_id])];
await route.fulfill({
status: 200,
contentType: "application/json",
body: JSON.stringify({ ok: true, approval_owner_ids: owners }),
});
});
await openSlackPage(page);
await expect(page.getByTestId("slack-mode-badge")).toContainText("Socket Mode");
const card = page.getByTestId("slack-manual-card");
await expect(card).toContainText("acme");
await expect(card).toContainText("Rohit"); // flat allow-list chip, named
await expect(card).toContainText("Choose at least one owner");
await page.getByTestId("add-approval-owner").click();
await page.getByTestId("pick-person-U9MAYA").click();
await expect(page.getByTestId("approval-owner-U9MAYA")).toContainText("Maya Chen");
});
+30
View File
@@ -363,6 +363,8 @@ export interface SlackWorkspace {
allowed_users: string[];
allow_all: boolean;
allowed_user_names?: Record<string, string | null>;
approval_owner_ids?: string[];
approval_owner_names?: Record<string, string | null>;
// Who installed this workspace (authed_user) — pre-added to the allow-list on
// connect (UX-027); the GUI marks their chip "you" and keys the setup card copy.
installer_user_id?: string;
@@ -443,6 +445,8 @@ export interface Connector {
mcp?: boolean; // MCP-backed one-click (vendor-hosted MCP + local OAuth — no cloud sign-in)
allowed_users: string[]; // the allow-list (managed inline in the Connectors tab)
allowed_user_names?: Record<string, string | null>; // id → display name (people directory)
approval_owner_ids?: string[]; // Manual Slack: humans allowed to resolve approvals
approval_owner_names?: Record<string, string | null>;
recent?: RecentSender[]; // recently-seen senders on a connected two-way connector
unauthorized?: ParkedMessage[]; // parked messages from unallowed senders (§19)
tools: ConnectorTool[];
@@ -1585,6 +1589,32 @@ export async function disallowUser(name: string, userId: string, teamId?: string
return res.json();
}
export async function addSlackApprovalOwner(
userId: string,
displayName?: string,
): Promise<{ ok: boolean; error?: string }> {
const res = await fetch(`${httpBase()}/v1/connectors/slack/approval-owners/add`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
user_id: userId,
...(displayName ? { name: displayName } : {}),
}),
});
return res.json();
}
export async function removeSlackApprovalOwner(
userId: string,
): Promise<{ ok: boolean; error?: string }> {
const res = await fetch(`${httpBase()}/v1/connectors/slack/approval-owners/remove`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ user_id: userId }),
});
return res.json();
}
/** Stop relaying one managed Slack workspace (the app stays installed in Slack). */
export async function disconnectSlackWorkspace(teamId: string): Promise<{ ok: boolean; error?: string; remaining_workspaces?: number }> {
const res = await fetch(
+46 -3
View File
@@ -1,5 +1,6 @@
import { useEffect, useState } from "react";
import {
getConnectors,
getDmRoute,
getInboxRouting,
getRecentChannels,
@@ -11,6 +12,7 @@ import {
subscribeChannel,
unsubscribeChannel,
type RecentChannel,
type Connector,
type Subscription,
type UnroutedItem,
} from "../api";
@@ -53,11 +55,14 @@ export function InboxConfigure() {
// the "default" route (sessions fall back to it); pick a channel separate from any you subscribe to.
function InboxRoutingCard() {
const [recent, setRecent] = useState<RecentChannel[]>([]);
const [connectors, setConnectors] = useState<Connector[]>([]);
const [target, setTarget] = useState(""); // current default-binding address, e.g. "slack:C0123"
const [draft, setDraft] = useState("");
const [error, setError] = useState<string | null>(null);
const load = () => {
getRecentChannels().then(setRecent).catch(() => setRecent([]));
getConnectors().then(setConnectors).catch(() => setConnectors([]));
getInboxRouting()
.then((bs) => {
const def = bs.find((b) => b.name === "default");
@@ -76,15 +81,43 @@ function InboxRoutingCard() {
if (!addr) return;
// "slack:C0123" → channel="slack", target="C0123"; a bare id assumes slack.
const [platform, id] = addr.includes(":") ? addr.split(":", 2) : ["slack", addr];
await setInboxBinding("default", platform, id);
const result = await setInboxBinding("default", platform, id);
if (!result.ok) {
setError(result.error || "Could not update Inbox routing.");
return;
}
setError(null);
setDraft("");
load();
};
const clear = async () => {
await setInboxBinding("default", null, "");
const result = await setInboxBinding("default", null, "");
if (!result.ok) {
setError(result.error || "Could not clear Inbox routing.");
return;
}
setError(null);
load();
};
const draftAddr = draft.trim();
const [draftPlatform, draftTarget] = draftAddr.includes(":")
? draftAddr.split(":", 2)
: ["slack", draftAddr];
const slack = connectors.find((c) => c.name === "slack");
const teamId =
draftPlatform === "slack" && draftTarget.includes("/")
? draftTarget.split("/", 1)[0]
: null;
const owners =
draftPlatform !== "slack"
? []
: teamId
? slack?.workspaces?.find((w) => w.team_id === teamId)?.approval_owner_ids ?? []
: slack?.approval_owner_ids ?? [];
const missingSlackOwner =
draftPlatform === "slack" && draftTarget.length > 0 && owners.length === 0;
// Show the channel's NAME when the recent list knows it (raw address as the fallback/tooltip).
const known = recent.find((c) => c.channel === target)?.name;
@@ -103,7 +136,11 @@ function InboxRoutingCard() {
<Icon name="plug" size={16} />
</span>
<ChannelPicker value={draft} onChange={setDraft} recent={recent} onSubmit={save} />
<button className={BTN_ACCENT_SM} disabled={!draft.trim()} onClick={save}>
<button
className={BTN_ACCENT_SM}
disabled={!draft.trim() || missingSlackOwner}
onClick={save}
>
Set
</button>
{target && (
@@ -112,6 +149,12 @@ function InboxRoutingCard() {
</button>
)}
</div>
{missingSlackOwner && (
<p className="text-[11.5px] text-warnInk mt-2">
Choose an approval owner under Integrations Slack before routing approvals here.
</p>
)}
{error && <p className="text-[11.5px] text-warnInk mt-2">{error}</p>}
</div>
);
}
@@ -1,11 +1,13 @@
import { useEffect, useRef, useState } from "react";
import {
addSlackApprovalOwner,
allowUser,
disallowUser,
disconnectSlackWorkspace,
getSlackDirectory,
getSubscriptions,
resolveUnauthorized,
removeSlackApprovalOwner,
unsubscribeChannel,
type Connector,
type ParkedMessage,
@@ -130,10 +132,17 @@ export function SlackDetail({ c, cloud, slack, onChanged }: DetailProps) {
<PeopleRow
allowed={c.allowed_users}
names={c.allowed_user_names}
protectedIds={c.approval_owner_ids}
teamId={null}
onRemove={(u) => disallowUser("slack", u).then(changed)}
onChanged={changed}
/>
<ApprovalOwnersRow
owners={c.approval_owner_ids ?? []}
names={c.approval_owner_names}
editable
onChanged={changed}
/>
{(c.unauthorized ?? [])
.filter((m) => !m.team_id)
.map((m) => (
@@ -209,24 +218,43 @@ function WorkspaceGroup({
</div>
<div className={GRP}>
{empty ? (
<div className={ROW}>
<span className="min-w-0 flex-1 text-[12.5px] text-muted flex items-center gap-2 flex-wrap">
<span>No one allowed yet mentions of the bot show up here for your OK.</span>
<PersonPicker teamId={w.team_id} allowed={[]} onChanged={onChanged} />
</span>
<DisconnectBtn teamId={w.team_id} busy={busy} onClick={disconnect} />
</div>
<>
<div className={ROW}>
<span className="min-w-0 flex-1 text-[12.5px] text-muted flex items-center gap-2 flex-wrap">
<span>No one allowed yet mentions of the bot show up here for your OK.</span>
<PersonPicker teamId={w.team_id} allowed={[]} onChanged={onChanged} />
</span>
<DisconnectBtn teamId={w.team_id} busy={busy} onClick={disconnect} />
</div>
<ApprovalOwnersRow
owners={w.approval_owner_ids ?? []}
names={w.approval_owner_names}
installerId={w.installer_user_id}
installerName={w.installer_name}
editable={false}
onChanged={onChanged}
/>
</>
) : (
<>
<PeopleRow
allowed={w.allowed_users}
names={w.allowed_user_names}
protectedIds={w.approval_owner_ids}
teamId={w.team_id}
installerId={w.installer_user_id}
installerName={w.installer_name}
onRemove={(u) => disallowUser("slack", u, w.team_id).then(onChanged)}
onChanged={onChanged}
/>
<ApprovalOwnersRow
owners={w.approval_owner_ids ?? []}
names={w.approval_owner_names}
installerId={w.installer_user_id}
installerName={w.installer_name}
editable={false}
onChanged={onChanged}
/>
{parked.map((m) => (
<WaitingRow key={m.id} m={m} onChanged={onChanged} />
))}
@@ -259,6 +287,7 @@ function DisconnectBtn({ teamId, busy, onClick }: { teamId: string; busy: boolea
function PeopleRow({
allowed,
names,
protectedIds,
teamId,
installerId,
installerName,
@@ -267,6 +296,7 @@ function PeopleRow({
}: {
allowed: string[];
names?: Record<string, string | null>;
protectedIds?: string[];
teamId: string | null; // null = manual flat list (directory queries as "default")
installerId?: string; // authed_user — pre-added on managed connect (UX-027)
installerName?: string;
@@ -296,9 +326,18 @@ function PeopleRow({
</span>
{label(u)}
{u === installerId && <span className="text-[10.5px] text-faint">· you</span>}
<button className={XBTN} title="remove" onClick={() => onRemove(u)}>
×
</button>
{protectedIds?.includes(u) ? (
<span
className="text-[10.5px] text-faint"
title="Remove approval-owner access before removing this person."
>
· owner
</span>
) : (
<button className={XBTN} title="remove" onClick={() => onRemove(u)}>
×
</button>
)}
</span>
))}
<PersonPicker teamId={teamId} allowed={allowed} onChanged={onChanged} />
@@ -314,10 +353,16 @@ function PersonPicker({
teamId,
allowed,
onChanged,
onPick,
buttonLabel = " Add person",
testId,
}: {
teamId: string | null;
allowed: string[];
onChanged: () => void;
onPick?: (member: SlackMember) => Promise<{ ok: boolean; error?: string }>;
buttonLabel?: string;
testId?: string;
}) {
const [open, setOpen] = useState(false);
const [q, setQ] = useState("");
@@ -360,7 +405,13 @@ function PersonPicker({
}, [open]);
const pick = async (m: SlackMember) => {
await allowUser("slack", m.id, teamId, m.name);
const result = onPick
? await onPick(m)
: await allowUser("slack", m.id, teamId, m.name);
if (result?.ok === false) {
setErr(result.error || "could not add person");
return;
}
setOpen(false);
setQ("");
onChanged();
@@ -372,11 +423,11 @@ function PersonPicker({
<button
ref={btn}
className="inline-flex items-center px-2 py-0.5 rounded-full border border-dashed border-line text-[12.5px] text-muted hover:text-ink hover:border-faint"
data-testid={`add-person-${teamId || "default"}`}
data-testid={testId || `add-person-${teamId || "default"}`}
title="Pick from the workspace directory"
onClick={toggle}
>
Add person
{buttonLabel}
</button>
{open && (
<div
@@ -432,6 +483,80 @@ function PersonPicker({
);
}
function ApprovalOwnersRow({
owners,
names,
installerId,
installerName,
editable,
onChanged,
}: {
owners: string[];
names?: Record<string, string | null>;
installerId?: string;
installerName?: string;
editable: boolean;
onChanged: () => void;
}) {
const [err, setErr] = useState<string | null>(null);
const label = (u: string) =>
names?.[u] || (u === installerId ? installerName || "You" : u);
const remove = async (userId: string) => {
const result = await removeSlackApprovalOwner(userId);
if (!result.ok) {
setErr(result.error || "could not remove approval owner");
return;
}
setErr(null);
onChanged();
};
return (
<div className={ROW} data-testid="slack-approval-owners">
<span className={LABEL}>Approvals</span>
<span className="min-w-0 flex-1 flex flex-wrap items-center gap-1.5">
{owners.length === 0 && (
<span className="text-[12px] text-warnInk">
Choose at least one owner before routing Inbox approvals to Slack.
</span>
)}
{owners.map((u) => (
<span
key={u}
className="inline-flex items-center gap-1.5 pl-1 pr-2 py-0.5 rounded-full bg-paper border border-line text-[12.5px]"
title={`id ${u}`}
data-testid={`approval-owner-${u}`}
>
<span className="w-5 h-5 rounded-full bg-accentSoft text-accent grid place-items-center text-[9px] font-bold">
{initials(label(u))}
</span>
{label(u)}
{u === installerId && <span className="text-[10.5px] text-faint">· installer</span>}
{editable && (
<button className={XBTN} title="remove approval owner" onClick={() => remove(u)}>
×
</button>
)}
</span>
))}
{editable && (
<PersonPicker
teamId={null}
allowed={owners}
onChanged={onChanged}
onPick={(m) => addSlackApprovalOwner(m.id, m.name)}
buttonLabel=" Add owner"
testId="add-approval-owner"
/>
)}
{!editable && owners.length > 0 && (
<span className="text-[11.5px] text-faint">Set by the workspace installer.</span>
)}
{err && <span className="basis-full text-[11.5px] text-warnInk">{err}</span>}
</span>
</div>
);
}
function WaitingRow({ m, onChanged }: { m: ParkedMessage; onChanged: () => void }) {
const act = async (action: "dismiss" | "allow" | "allow_deliver") => {
await resolveUnauthorized("slack", m.id, action);