diff --git a/CHANGELOG.md b/CHANGELOG.md index f8765a4d..9d6ad0de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,10 @@ product overview. ## Unreleased +- Clear typed tokens when Slack Settings reopens a form after an asynchronous save refusal, + including a custom MCP address rejected by DNS validation. Non-secret fields remain filled; + leaving an existing connection's token blank still keeps its saved token. + - Keep cancelled Slack turns from restoring the loading indicator while their engine shuts down. Stop and cold-engine steering close progress immediately and ignore late output; cancellation during preflight cannot start a new indicator. diff --git a/FEATURES.md b/FEATURES.md index acdf62ab..ad13abd9 100644 --- a/FEATURES.md +++ b/FEATURES.md @@ -280,7 +280,9 @@ control-plane status. open the footer reads **Save** / **Close**; saving is the view submission, so a refusal appears under its own field. Each form's field ids are keyed by the form, so Slack never carries a typed value from one form into another. The open form and any pending confirmation ride in the view's - metadata; changing page clears both. + metadata; changing page clears both. After an asynchronous save refusal, the reopened form uses + a fresh identity for its token or variable-value input so Slack clears the typed secret, while + non-secret fields remain filled. A blank token on an existing connection still keeps the saved one. - **Explainers removed:** the permission paragraph, per-field footnotes and the long Access note. What remains is the 🔒 line shown where the viewer cannot edit something. → TEST-PLAN: Slack Settings redesign. diff --git a/TEST-PLAN.md b/TEST-PLAN.md index edda8f6e..55cdeb2b 100644 --- a/TEST-PLAN.md +++ b/TEST-PLAN.md @@ -63,7 +63,7 @@ ## Slack Settings redesign (2026-10-15) ### Regression -- [x] Automated: `node --test test/slack-settings-redesign.test.js test/channel-settings-modal.test.js test/access-settings.test.js test/slack-secret-actions.test.js test/slack-custom-mcp-settings.test.js test/slack-vpn-settings.test.js`. +- [x] Automated: `node --test test/slack-settings-redesign.test.js test/slack-settings-secret-reset.test.js test/channel-settings-modal.test.js test/access-settings.test.js test/slack-secret-actions.test.js test/slack-custom-mcp-settings.test.js test/slack-vpn-settings.test.js`. - **Variables ⋯ menu, end to end on the store:** - Add with Secret ticked → hidden. - Allowed servers prefills the current list, replaces it, and refuses a malformed host under @@ -92,6 +92,10 @@ an expired save cannot pass. Authority is re-checked after those checks; a manager demoted meanwhile writes nothing. A refusal only the save can find reopens the form with the error under its field, the non-secret fields refilled and the token or value box empty. + - Model Slack's retention of input by block/action identity: repeated asynchronous custom MCP + refusals clear the token in both scopes without clearing name/URL. Retried submissions read + only the new token field, local errors target it, and blank edits keep the stored token even + if stale input state is supplied. Duplicate variable refusals also clear the value field. - `beta`'s hardened pushed-form handlers (pre-redesign views) stay covered with explicit legacy buttons in `test/slack-custom-mcp-settings.test.js`. - **Access:** @@ -136,6 +140,10 @@ Fixture: a disposable channel; one admin, one approved non-manager member, one u - [ ] **MCP:** Edit Composio (the label is prefilled and the token box empty) and Save; Connect the Make toolbox with a bad URL. Pass: the error appears under URL and nothing is saved. Add a custom MCP as a manager; the member sees no Edit on it. +- [ ] **Async refusal:** submit a custom MCP whose hostname resolves to a denied address. On + the reopened native form, the token box is empty while name/URL remain. Repeat the refusal, + then correct the address and retype the token; saving succeeds. Repeat in personal scope and + verify that leaving an existing connection's token blank keeps its saved token. - [ ] **Variables:** on a test variable run all four ⋯ actions. Pass: - Replace keeps the type and servers. - Allowed servers lists the current servers and saves the new ones; `list_secrets` agrees. diff --git a/src/slack/app.js b/src/slack/app.js index 1f0ff356..56a1ce3c 100644 --- a/src/slack/app.js +++ b/src/slack/app.js @@ -1,4 +1,5 @@ import { assertEngineSelectable } from "../engines/selection.js"; +import { randomUUID } from "node:crypto"; import { assertNewMcpSelections } from "../gateway/mcp-selection.js"; // Slack Socket Mode gateway. Receives message events across DM / group DM / public & private // channels, applies the gating rules (DM → no mention needed; everywhere else → require an @@ -929,7 +930,7 @@ export async function handleSettingsFormSubmission({ ack, body, view, client }, await ack(); return; } - const values = readSettingsForm(view, state.form); + const values = readSettingsForm(view, state.form, state.formSecretId); const request = { channelId: state.channelId, userId: clicker, expectedSlug: state.slug }; let local; try { @@ -937,7 +938,7 @@ export async function handleSettingsFormSubmission({ ack, body, view, client }, assertSettingsFormAllowed(state.form, { ...local, clicker }); const problem = localSettingsFormProblem(state.form, values); if (problem) { - await ack({ response_action: "errors", errors: { [settingsFieldBlockId(state.form, problem.field)]: problem.error } }); + await ack({ response_action: "errors", errors: { [settingsFieldBlockId(state.form, problem.field, state.formSecretId)]: problem.error } }); return; } } catch (error) { @@ -971,7 +972,7 @@ export async function handleSettingsFormSubmission({ ack, body, view, client }, clearTimeout(timer); const meta = (await getChannelMeta(fresh.entry.slug)) || fresh.meta; if (result.error) { - await client.views.update({ view_id: view.id, view: await rootView(fresh.entry, meta, { ...state, formError: result, formDraft: draft }, fresh.userIsAdmin, {}) }); + await client.views.update({ view_id: view.id, view: await rootView(fresh.entry, meta, { ...state, formSecretId: randomUUID(), formError: result, formDraft: draft }, fresh.userIsAdmin, {}) }); return; } saved = true; diff --git a/src/slack/channel-settings.js b/src/slack/channel-settings.js index 7b2dede1..8a217c52 100644 --- a/src/slack/channel-settings.js +++ b/src/slack/channel-settings.js @@ -191,6 +191,7 @@ export function settingsMetadata(state = {}) { p: normalizeTab(state.tab), // The inline form open on the page (`f`) and the in-place confirmation waiting (`x`). ...(state.form ? { f: String(state.form).slice(0, 200) } : {}), + ...(state.form && state.formSecretId ? { fs: String(state.formSecretId).slice(0, 36) } : {}), ...(state.confirm ? { x: String(state.confirm).slice(0, 200) } : {}), // The person the Access page's search picked (`y`). ...(state.memberFocus ? { y: String(state.memberFocus).slice(0, 32) } : {}), @@ -212,6 +213,7 @@ export function parseSettingsMetadata(raw) { ownerId: String(value.u), tab: normalizeTab(value.p), form: typeof value.f === "string" ? value.f : "", + ...(typeof value.fs === "string" && /^[0-9a-f-]{36}$/.test(value.fs) ? { formSecretId: value.fs } : {}), confirm: typeof value.x === "string" ? value.x : "", memberFocus: typeof value.y === "string" && /^[UW][A-Z0-9]+$/.test(value.y) ? value.y : "", }; @@ -419,8 +421,11 @@ function pageNav(state, active, { isDM = false } = {}) { // metadata (`f`); its input block ids are keyed by the form, so a value typed into one form can // never be carried into another by Slack's state retention. export const SETTINGS_FIELD_ACTION_ID = "v"; -export function settingsFieldBlockId(form, field) { - return `cgf_${field}_${createHmac("sha256", "cg-settings-form").update(String(form)).digest("hex").slice(0, 8)}`; +export function settingsFieldBlockId(form, field, secretId = "") { + // Slack preserves typed values when both input identities survive views.update. A refused + // async save rotates only write-only inputs; labels, URLs and other draft fields stay put. + const key = secretId && (field === "token" || field === "value") ? `${form}:${secretId}` : String(form); + return `cgf_${field}_${createHmac("sha256", "cg-settings-form").update(key).digest("hex").slice(0, 8)}`; } // Which scope a form or row key names: c = this conversation, o = organization, p = the viewer. @@ -509,11 +514,11 @@ export function variableEntry(snapshot = {}, scope, name) { } // The values a submitted form carries, by field name, read from the root view's state. -export function readSettingsForm(view = {}, key) { +export function readSettingsForm(view = {}, key, secretId = "") { const values = view?.state?.values || {}; const out = {}; for (const field of ["label", "token", "url", "name", "value", "hosts", "secret"]) { - const entry = values[settingsFieldBlockId(key, field)]?.[SETTINGS_FIELD_ACTION_ID]; + const entry = values[settingsFieldBlockId(key, field, secretId)]?.[SETTINGS_FIELD_ACTION_ID]; if (!entry) continue; out[field] = field === "secret" ? (entry.selected_options || []).some((choice) => choice?.value === "secret") @@ -531,7 +536,7 @@ function formBlocks(spec, state) { const error = state.formError?.error ? state.formError : null; for (const raw of spec.fields) { const field = Object.hasOwn(draft, raw.name) && raw.name !== "token" && raw.name !== "value" ? { ...raw, initial: draft[raw.name] } : raw; - const blockId = settingsFieldBlockId(spec.key, field.name); + const blockId = settingsFieldBlockId(spec.key, field.name, state.formSecretId); if (field.type === "checkbox") { const choice = option(field.label, "secret", field.option || ""); blocks.push({ diff --git a/test/slack-settings-secret-reset.test.js b/test/slack-settings-secret-reset.test.js new file mode 100644 index 00000000..dd2e16b2 --- /dev/null +++ b/test/slack-settings-secret-reset.test.js @@ -0,0 +1,130 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { ensureTestEnv } from "./helpers.js"; + +ensureTestEnv(); + +const { handleSettingsFormSubmission, saveSettingsForm } = await import("../src/slack/app.js"); +const { buildChannelSettingsView, parseSettingsMetadata } = await import("../src/slack/channel-settings.js"); +const store = await import("../src/config/store.js"); +const entry = await store.upsertChannelEntry("C_SECRET_RESET", { name: "secret-reset", type: "channel", isDM: false }); +await store.saveChannelMeta(entry.slug, store.defaultChannelMeta({ channelId: "C_SECRET_RESET", name: "secret-reset", type: "channel", isDM: false })); +await store.setUser("U_EDITOR", { name: "Editor", approved: true }); +const context = async () => ({ entry, meta: await store.getChannelMeta(entry.slug), clicker: "U_EDITOR", userIsAdmin: true, userIsApproved: true }); + +// Slack retains user input by block_id + action_id when views.update reopens a form. Keeping +// an input's initial_value out of the payload alone does not reset the native control. +function modal(form, snapshot = {}) { + const state = { channelId: "C_SECRET_RESET", slug: entry.slug, ownerId: "U_EDITOR", tab: form.endsWith(":p") || form.includes(":p:") ? "you" : "mcp", form }; + const rootView = async (_entry, _meta, nextState) => buildChannelSettingsView(snapshot, nextState, { tab: nextState.tab, canManageCustomMcp: true }); + const retained = new Map(); + let view; + const identity = (block) => `${block.block_id}:${block.element.action_id}`; + const input = (field) => view.blocks.find((block) => block.block_id?.startsWith(`cgf_${field}_`)); + function render(next) { + view = next; + for (const block of view.blocks.filter((b) => b.type === "input")) { + if (!retained.has(identity(block))) retained.set(identity(block), block.element.type === "checkboxes" + ? { selected_options: block.element.initial_options || [] } + : { value: block.element.initial_value || "" }); + } + } + render(buildChannelSettingsView(snapshot, state, { tab: state.tab, canManageCustomMcp: true })); + return { + input, + value: (field) => retained.get(identity(input(field))).value, + type: (field, value) => retained.set(identity(input(field)), { value }), + view: () => view, + async submit(save, extraValues = {}) { + const values = Object.fromEntries(view.blocks.filter((b) => b.type === "input").map((b) => [b.block_id, { [b.element.action_id]: retained.get(identity(b)) }])); + const acks = []; + await handleSettingsFormSubmission({ + body: { user: { id: "U_EDITOR" } }, + view: { ...view, id: "V_RESET", state: { values: { ...extraValues, ...values } } }, + ack: async (response) => { acks.push(response); if (response?.view) render(response.view); }, + client: { views: { update: async ({ view: next }) => render(next) } }, + }, { context, rootView, save }); + return acks; + }, + }; +} + +for (const scope of ["c", "p"]) { + test(`async custom MCP refusal clears native token retention on every retry (${scope})`, async () => { + const ui = modal(`cm-add:${scope}`); + ui.type("name", "example"); + ui.type("url", "https://mcp.example.com/mcp"); + const nameId = ui.input("name").block_id; + const urlId = ui.input("url").block_id; + for (let attempt = 0; attempt < 2; attempt++) { + const token = `dummy-rejected-token-${attempt}`; + ui.type("token", token); + const tokenId = ui.input("token").block_id; + await ui.submit(async (_key, values) => { + assert.equal(values.token, token, "retry reads the current native token input"); + return { field: "url", error: "URL resolved to a loopback address." }; + }); + assert.equal(ui.value("token"), "", "Slack must not retain the refused token"); + assert.notEqual(ui.input("token").block_id, tokenId); + assert.equal(ui.input("name").block_id, nameId); + assert.equal(ui.input("url").block_id, urlId); + assert.equal(ui.value("name"), "example"); + assert.equal(ui.value("url"), "https://mcp.example.com/mcp"); + assert.match(JSON.stringify(ui.view()), /loopback/); + assert.equal(JSON.stringify(ui.view()).includes(token), false, "neither blocks nor metadata contain the token"); + assert.equal(parseSettingsMetadata(ui.view().private_metadata).form, `cm-add:${scope}`); + } + ui.type("token", "dummy-corrected-token"); + await ui.submit(async (_key, values) => { + assert.equal(values.token, "dummy-corrected-token"); + return { notice: "Saved" }; + }); + assert.equal(parseSettingsMetadata(ui.view().private_metadata).form, ""); + }); + + test(`blank custom MCP edit after an async refusal keeps the stored token (${scope})`, async () => { + const name = `existing-${scope}`; + const original = "dummy-saved-token"; + await saveSettingsForm(`cm-add:${scope}`, { name, url: "https://93.184.215.14/mcp", token: original }, await context()); + const ui = modal(`cm:${scope}:${name}`, { customMcps: { [scope === "c" ? "channel" : "personal"]: [{ name, serverName: name, url: "https://93.184.215.14/mcp" }] } }); + ui.type("token", "dummy-rejected-replacement"); + const staleId = ui.input("token").block_id; + await ui.submit(async () => ({ field: "url", error: "URL resolved to a loopback address." })); + assert.equal(ui.value("token"), ""); + ui.type("url", "https://93.184.215.14/v2"); + await ui.submit(async (key, values, ctx) => { + assert.equal(values.token, "", "a stale input identity cannot supply the replacement"); + return saveSettingsForm(key, values, ctx); + }, { [staleId]: { v: { value: "dummy-rejected-replacement" } } }); + const saved = scope === "c" ? await store.getChannelMeta(entry.slug) : await store.getUser("U_EDITOR"); + assert.equal(saved.customMcps.find((row) => row.name === name).token, original); + assert.equal(saved.customMcps.find((row) => row.name === name).url, "https://93.184.215.14/v2"); + assert.equal(parseSettingsMetadata(ui.view().private_metadata).form, ""); + }); +} + +test("local token validation after an async refusal targets the new visible input", async () => { + const ui = modal("cm-add:p"); + ui.type("name", "example"); + ui.type("url", "https://mcp.example.com/mcp"); + ui.type("token", "dummy-rejected-token"); + await ui.submit(async () => ({ field: "url", error: "Address refused." })); + ui.type("token", "short"); + const visibleId = ui.input("token").block_id; + const acks = await ui.submit(async () => assert.fail("local validation must prevent saving")); + assert.equal(acks[0].response_action, "errors"); + assert.deepEqual(Object.keys(acks[0].errors), [visibleId]); + assert.match(acks[0].errors[visibleId], /valid token/); +}); + +test("a duplicate variable refusal clears the native value but retains non-secret fields", async () => { + const ui = modal("var-add:p", { personalSecrets: [] }); + ui.type("name", "EXISTING_KEY"); + ui.type("hosts", "api.example.com"); + ui.type("value", "dummy-rejected-value"); + await ui.submit(async () => ({ field: "name", error: "EXISTING_KEY already exists." })); + assert.equal(ui.value("value"), ""); + assert.equal(ui.value("name"), "EXISTING_KEY"); + assert.equal(ui.value("hosts"), "api.example.com"); + assert.equal(JSON.stringify(ui.view()).includes("dummy-rejected-value"), false); +});