fix(slack): clear secret inputs after asynchronous form refusal

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.

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.

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.

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,15 +930,15 @@ 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 {
     local = await context(client, { ...request, verifyMembership: false });
     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;

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({

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);
+});