fix(security): tunnel revocation, read-only mislabel, config truncation

Three HIGH findings from the audit. Each was reproduced before fixing and
each is pinned by a regression test that fails against the old code.

daemon: the daemon held TWO independent activeTunnel handles -- one in
server.js (filled by POST /daemon/enable-tunnel) and one in launcher.js
(filled by the boot auto-start) -- and neither closure could see the other.
disable-tunnel was a silent no-op on a boot-started tunnel: it skipped
stop(), nulled the lockfile tunnelUrl and returned ok while cloudflared
kept serving the public hostname. The 401-burst tripwire delegated to the
launcher callback and so could never stop a dashboard-enabled tunnel. And
enable-tunnel's stop-the-existing guard was equally blind, orphaning a
second cloudflared. getHealth() reads the server closure, so /daemon/health
and manage_daemon status corroborated the false state. /mcp still required
the 256-bit timing-safe bearer, so this is failed revocation rather than
open access -- but with /mcp?token= secret URLs the URL is the credential.
startDaemonServer now exposes adoptTunnel()/getActiveTunnel(), the launcher
hands its handle over, and the tripwire stops that single handle directly.

Also consumes the boot tunnel's waitUntilReady rejection. It is created
eagerly in tunnel.js and rejected when cloudflared exits before publishing
a URL; nothing consumed it and there is no unhandledRejection handler, so
Node's default throw killed the daemon ~1s after it began serving.

index.js: download_formula_field and download_base_formulas were annotated
readOnlyHint:true while writing files at a caller-supplied path.
readOnlyHint is the signal MCP clients use to auto-approve without
prompting, so the annotation removed the consent step from a filesystem
write. Now readOnlyHint:false, destructiveHint:true. Separately, the
filename sanitiser stripped separators but not '.', so a table named '..'
wrote every file one directory above outputDir -- table names come from the
base and are attacker influenced. Both segments now go through
confineToDir(). fieldName is newline-stripped in the # AT: header, which
description already was.

lsp-config.ts: unconfigureMcpToml/unconfigureHelix truncated the user's
config from our marker to EOF instead of removing our block. Both
configureMcpToml and `codex mcp add` append at EOF, so a second MCP server
added after Setup sat below ours and was destroyed by Unconfigure -- no
confirmation, no backup, no error. Measured on the old code, a config.toml
with one extra server was reduced to a single newline. Now removes only
our own sections; HELIX_BLOCK is four tables, not one, and the header
matcher will not mistake a multi-line array continuation for a header.

Verified: 1442 mcp-server + 421 extension + 88 webview tests pass,
check:tool-sync green, pnpm build succeeds.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: ARHAEEM

CHANGELOG.md

@@ -6,6 +6,21 @@ Check [Keep a Changelog](http://keepachangelog.com/) for recommendations on how
 
 ## [Unreleased]
 
+### Fixed — Unconfigure destroyed unrelated config in Codex / Helix files (2026-07-31)
+
+- **`unconfigureMcpToml` and `unconfigureHelix` truncated the user's config file from our marker
+  to EOF.** Both did `existing.slice(0, indexOf(MARKER))`, discarding everything below our block
+  rather than removing only our block. Since `configureMcpToml` appends at EOF and `codex mcp add`
+  does too, any second MCP server or `[model_providers.*]` section the user added after running
+  Setup sat below ours — and clicking **Unconfigure** in the dashboard (no confirmation prompt)
+  destroyed it silently, with no backup and no error. Measured against the old code, a
+  `config.toml` holding one extra MCP server was reduced to a **single newline**.
+  Both now remove only the sections we own, preserving everything else verbatim.
+  Note `HELIX_BLOCK` is *four* top-level tables, not one, so a naive "delete to the next `[`
+  header" would have orphaned three `[[language]]` blocks; the header matcher is also strict
+  enough not to mistake a continuation line of a multi-line array (`matrix = [\n[1,2],\n]`) for a
+  table header. Pinned by `src/test/lsp-config-toml.test.ts`.
+
 ### Fixed — formatter tokenizers silently corrupted formulas / hung the extension host (2026-07-31)
 
 Found by audit, both reproduced before fixing.

packages/extension/src/auto-config/lsp-config.ts

@@ -80,10 +80,16 @@ async function unconfigureHelix(configPath: string): Promise<void> {
   let existing = '';
   try { existing = await fs.promises.readFile(configPath, 'utf8'); } catch { return; }
   if (!existing.includes(HELIX_MARKER)) return;
-  // Remove everything from HELIX_MARKER to the next blank-line-separated section
-  const idx = existing.indexOf(HELIX_MARKER);
-  const before = existing.slice(0, idx).trimEnd();
-  await writeTextAtomic(configPath, before + '\n');
+  // HELIX_BLOCK is FOUR top-level tables, not one: the language-server plus a
+  // [[language]] per file type. Remove each of ours and nothing else — a
+  // "delete to the next header" cut would orphan the three [[language]] blocks,
+  // and the old "delete to EOF" cut destroyed the user's own config below ours.
+  const next = removeTomlSections(existing, (s) => {
+    if (s.header === null) return false;
+    if (s.header === HELIX_MARKER) return true;
+    return s.header === '[[language]]' && s.text.includes(`language-servers = ["${LSP_SERVER_KEY}"]`);
+  });
+  await writeTextAtomic(configPath, next);
 }
 
 async function isHelixConfigured(configPath: string): Promise<boolean> {
@@ -175,9 +181,12 @@ export async function unconfigureMcpToml(configPath: string): Promise<void> {
   let existing = '';
   try { existing = await fs.promises.readFile(configPath, 'utf8'); } catch { return; }
   if (!existing.includes(CODEX_MARKER)) return;
-  const idx = existing.indexOf(CODEX_MARKER);
-  const before = existing.slice(0, idx).trimEnd();
-  await writeTextAtomic(configPath, before + '\n');
+  // Remove only our own table. This used to truncate the file from the marker to
+  // EOF — and since both `configureMcpToml` and `codex mcp add` append at EOF,
+  // any MCP server or [model_providers.*] block the user added after Setup sat
+  // below ours and was silently destroyed by clicking Unconfigure.
+  const next = removeTomlSections(existing, (s) => s.header === CODEX_MARKER);
+  await writeTextAtomic(configPath, next);
 }
 
 export async function isMcpTomlConfigured(configPath: string): Promise<boolean> {
@@ -187,6 +196,51 @@ export async function isMcpTomlConfigured(configPath: string): Promise<boolean>
   } catch { return false; }
 }
 
+// ── Shared TOML section surgery ─────────────────────────────────────────────
+
+/**
+ * A complete top-level TOML table header on its own line: `[table]`, `[[array]]`,
+ * optionally dotted/quoted, optionally trailing comment.
+ *
+ * Deliberately strict. A loose `/^\s*\[/` also matches a continuation line of a
+ * multi-line array (`matrix = [\n[1,2],\n]`), which would split a user's value in
+ * half and corrupt the file — the exact class of bug this helper exists to fix.
+ */
+const TOML_HEADER_RE = /^[ \t]*\[\[?[A-Za-z0-9_.\-"' ]+\]\]?[ \t]*(?:#.*)?$/;
+
+interface TomlSection { header: string | null; text: string }
+
+/** Split a TOML document into a preamble plus one entry per top-level header. */
+function splitTomlSections(text: string): TomlSection[] {
+  const sections: TomlSection[] = [];
+  let current: TomlSection = { header: null, text: '' };
+  for (const line of text.split('\n')) {
+    if (TOML_HEADER_RE.test(line)) {
+      sections.push(current);
+      current = { header: line.trim(), text: line + '\n' };
+    } else {
+      current.text += line + '\n';
+    }
+  }
+  sections.push(current);
+  return sections.filter((s, i) => i === 0 || s.header !== null);
+}
+
+/**
+ * Drop only the sections we own, preserving everything else verbatim.
+ *
+ * The previous implementation did `existing.slice(0, indexOf(MARKER))` — i.e. it
+ * truncated the user's file from our marker to EOF, destroying any config that
+ * happened to sit below ours. Since `configureMcpToml` appends at EOF and
+ * `codex mcp add` does too, a second MCP server added after Setup was silently
+ * deleted by clicking Unconfigure.
+ */
+function removeTomlSections(text: string, isOurs: (s: TomlSection) => boolean): string {
+  const kept = splitTomlSections(text).filter((s) => !isOurs(s));
+  const out = kept.map((s) => s.text).join('').replace(/\n{3,}/g, '\n\n').trimEnd();
+  return out === '' ? '' : out + '\n';
+}
+
 // ── Shared text-file atomic writer ──────────────────────────────────────────
 
 async function writeTextAtomic(filePath: string, content: string): Promise<void> {

packages/extension/src/test/lsp-config-toml.test.ts

@@ -0,0 +1,82 @@
+import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import * as fs from 'node:fs';
+import * as os from 'node:os';
+import * as path from 'node:path';
+import {
+  configureMcpToml,
+  unconfigureMcpToml,
+  isMcpTomlConfigured,
+} from '../auto-config/lsp-config.js';
+
+let dir: string;
+let cfg: string;
+
+beforeEach(() => {
+  dir = fs.mkdtempSync(path.join(os.tmpdir(), 'atf-toml-'));
+  cfg = path.join(dir, 'config.toml');
+});
+afterEach(() => {
+  fs.rmSync(dir, { recursive: true, force: true });
+});
+
+describe('unconfigureMcpToml preserves user config', () => {
+  // The bug: unconfigure did `existing.slice(0, indexOf(MARKER))`, truncating the
+  // file from our marker to EOF. Both configureMcpToml and `codex mcp add`
+  // append at EOF, so anything the user added after Setup sat below ours.
+  it('keeps a second MCP server added BELOW our block', async () => {
+    await configureMcpToml(cfg);
+    fs.appendFileSync(
+      cfg,
+      '\n[mcp_servers.some-other-server]\ntype = "stdio"\ncommand = "other"\nargs = ["--flag"]\n',
+    );
+
+    await unconfigureMcpToml(cfg);
+    const out = fs.readFileSync(cfg, 'utf8');
+
+    expect(out).toContain('[mcp_servers.some-other-server]');
+    expect(out).toContain('command = "other"');
+    expect(out).not.toContain('airtable-user-mcp');
+    expect(await isMcpTomlConfigured(cfg)).toBe(false);
+  });
+
+  it('keeps config that sits ABOVE our block', async () => {
+    fs.writeFileSync(cfg, 'model = "gpt-5"\n\n[model_providers.openai]\nbase_url = "https://x"\n');
+    await configureMcpToml(cfg);
+    await unconfigureMcpToml(cfg);
+    const out = fs.readFileSync(cfg, 'utf8');
+
+    expect(out).toContain('model = "gpt-5"');
+    expect(out).toContain('[model_providers.openai]');
+    expect(out).toContain('base_url = "https://x"');
+    expect(out).not.toContain('airtable-user-mcp');
+  });
+
+  it('does not split a multi-line array whose continuation lines start with [', async () => {
+    // A loose /^\s*\[/ header match would treat `[1, 2],` as a table header and
+    // cut the user's value in half.
+    fs.writeFileSync(cfg, 'matrix = [\n[1, 2],\n[3, 4],\n]\n');
+    await configureMcpToml(cfg);
+    await unconfigureMcpToml(cfg);
+    const out = fs.readFileSync(cfg, 'utf8');
+
+    expect(out).toContain('matrix = [');
+    expect(out).toContain('[1, 2],');
+    expect(out).toContain('[3, 4],');
+    expect(out).not.toContain('airtable-user-mcp');
+  });
+
+  it('round-trips to an equivalent file', async () => {
+    const original = 'model = "gpt-5"\n\n[model_providers.openai]\nbase_url = "https://x"\n';
+    fs.writeFileSync(cfg, original);
+    await configureMcpToml(cfg);
+    expect(await isMcpTomlConfigured(cfg)).toBe(true);
+    await unconfigureMcpToml(cfg);
+    expect(fs.readFileSync(cfg, 'utf8').trim()).toBe(original.trim());
+  });
+
+  it('is a no-op when we were never configured', async () => {
+    fs.writeFileSync(cfg, 'model = "gpt-5"\n');
+    await unconfigureMcpToml(cfg);
+    expect(fs.readFileSync(cfg, 'utf8')).toBe('model = "gpt-5"\n');
+  });
+});

packages/mcp-server/CHANGELOG.md

@@ -2,6 +2,51 @@
 
 ## [Unreleased]
 
+### Fixed (2026-07-31 — tunnel revocation reached only half the cases)
+
+- **The daemon held TWO independent `activeTunnel` handles**, one in `daemon/server.js` (filled by
+  `POST /daemon/enable-tunnel`) and one in `daemon/launcher.js` (filled by the boot auto-start).
+  Neither closure could see the other, so every path that stops a tunnel reached only half the
+  cases: `POST /daemon/disable-tunnel` was a **silent no-op on a boot-started tunnel** — it
+  skipped `stop()`, nulled the lockfile `tunnelUrl`, published `daemon:tunnel-stopped` and
+  returned `{ok:true}` while cloudflared kept serving the public hostname; the **401-burst
+  tripwire** delegated to the launcher's callback and so could never stop a dashboard-enabled
+  tunnel (it latched `tunnelAutoDisabled`, the UI showed "Auto-disabled", the URL stayed live);
+  and `enable-tunnel`'s stop-the-existing guard was equally blind, leaving **two cloudflared
+  children and two public URLs**. `getHealth()` reads the server closure, so `/daemon/health` and
+  `manage_daemon action=status` corroborated the false state. `/mcp` still required the 256-bit
+  timing-safe bearer throughout, so this was a **failed revocation control**, not open access —
+  but with `/mcp?token=` secret URLs the URL *is* the shared credential. `startDaemonServer()` now
+  exposes `adoptTunnel()`/`getActiveTunnel()`, the launcher hands its boot-started handle over,
+  and the tripwire stops that single handle directly. Pinned by `test/test-tunnel-ownership.test.js`.
+- **A boot-auto-started tunnel could kill the daemon by unhandled rejection.** `waitUntilReady` is
+  created eagerly in `tunnel.js` and rejected when cloudflared exits before publishing a URL
+  (offline boot, blocked egress, a trycloudflare 429). Nothing consumed it and the repo installs
+  no `process.on('unhandledRejection')`, so Node's default throw took the daemon down ~1s after it
+  began serving, killing in-flight MCP requests and orphaning the non-detached LSP child. Now
+  consumed and logged.
+
+### Security (2026-07-31 — `download_*` tools declared read-only while writing files)
+
+- **`download_formula_field` and `download_base_formulas` were annotated `readOnlyHint: true`,
+  `destructiveHint: false`** while writing (and overwriting) files at a caller-supplied path.
+  `readOnlyHint` is exactly the signal MCP clients use to **auto-approve a call without
+  prompting**, so the annotation removed the user's consent step from a filesystem write. Both are
+  now `readOnlyHint: false, destructiveHint: true`. *(Their `read` category is unchanged and
+  remains a judgement call — see the note below.)*
+- **A table named `..` escaped the chosen output directory.** The filename sanitiser stripped
+  separators (`/\:*?"<>|`) but not `.`, so `download_base_formulas` wrote every formula file one
+  directory *above* `outputDir`. Table and field names come from the base, so they are attacker
+  influenced by anyone who can edit a base the user can read. Both segments now go through
+  `confineToDir()`, which rejects dot-only segments and asserts the resolved path stays under the
+  chosen directory.
+- **`fieldName` was interpolated into the `# AT:` header with only quotes escaped**, so a newline
+  in a field name injected arbitrary header lines (`description` was already stripped). Both sites
+  now use `headerSafe()`.
+- Not changed: `outputPath`/`outputDir` themselves are still honoured as given. They are the
+  user's explicit choice, and confining them to `cwd` would break legitimate "save to my Desktop"
+  use; the consent step is restored by the annotation fix instead.
+
 ### Security (2026-07-31 — row-template IDs were missing the path-traversal guard)
 
 - **All seven row-template client methods interpolated a caller-supplied `templateId` into the

packages/mcp-server/src/daemon/launcher.js

@@ -364,7 +364,12 @@ export async function startDaemon(options = {}) {
           syncLockfile(nextToken.bearerToken);
         },
         onTunnelAutoDisable: async ({ failures, windowMs, ip }) => {
-          // 401-burst auto-disable callback (D-06): update settings + clear lockfile tunnelUrl
+          // 401-burst auto-disable callback (D-06): update settings + clear lockfile tunnelUrl.
+          // The SERVER now stops the tunnel before invoking this — it owns the
+          // single handle, so it reaches dashboard-enabled tunnels too, which
+          // this callback never could. The stop below is a defensive no-op
+          // (tunnel.js stop() returns early once `stopping` is set); it stays so
+          // the mirror is cleared and an un-adopted handle is still torn down.
           if (activeTunnel) {
             await activeTunnel.stop().catch(() => undefined);
             activeTunnel = null;
@@ -453,6 +458,23 @@ export async function startDaemon(options = {}) {
               }
             },
           });
+
+          // Hand ownership to the server. Until this existed, a boot-started
+          // tunnel lived ONLY in this closure, so /daemon/disable-tunnel saw
+          // null, skipped stop(), and still returned ok + nulled the lockfile —
+          // the public URL kept serving while every UI surface said "off".
+          server.adoptTunnel?.(activeTunnel);
+
+          // waitUntilReady is created eagerly in tunnel.js and rejected when
+          // cloudflared exits before publishing a URL (offline boot, blocked
+          // egress, trycloudflare 429). Nothing consumed it, and there is no
+          // process-level unhandledRejection handler, so Node's default throw
+          // killed the daemon ~1s after it began serving.
+          activeTunnel?.waitUntilReady?.catch?.((err) => {
+            console.error(
+              `[airtable-mcp] tunnel never became ready: ${err instanceof Error ? err.message : String(err)}`,
+            );
+          });
         } catch (err) {
           // Non-fatal: daemon continues without tunnel (D-04: no auto-restart)
           console.error(`[airtable-mcp] Tunnel auto-start failed: ${err instanceof Error ? err.message : String(err)}`);

packages/mcp-server/src/daemon/server.js

@@ -339,6 +339,18 @@ export async function startDaemonServer(options = {}) {
       const ip = req.headers?.['cf-connecting-ip']
         ?? req.headers?.['x-forwarded-for']?.split(',')[0]?.trim()
         ?? null;
+      // Stop the tunnel HERE, on the one handle this module owns. The launcher's
+      // onTunnelAutoDisable callback used to be the only thing that stopped it,
+      // and it could only see a tunnel the LAUNCHER had started at boot — so a
+      // tunnel enabled from the dashboard (which fills this closure instead)
+      // survived the tripwire entirely: `tunnelAutoDisabled` latched, the UI said
+      // "Auto-disabled", and cloudflared kept serving the public hostname.
+      // The callback is now bookkeeping only (settings + lockfile).
+      if (activeTunnel) {
+        const stopping = activeTunnel;
+        activeTunnel = null;
+        void stopping.stop().catch(() => undefined);
+      }
       publishEvent('daemon:tunnel-auto-disabled', { failures: authFailureCount, windowMs: BURST_WINDOW_MS, ip });
       options.onTunnelAutoDisable?.({ failures: authFailureCount, windowMs: BURST_WINDOW_MS, ip });
     }
@@ -886,5 +898,24 @@ export async function startDaemonServer(options = {}) {
     stop,
     publishEvent,
     getHealth,
+    /**
+     * Hand a tunnel started elsewhere (the launcher's boot auto-start) to this
+     * module, so there is exactly ONE owner of the running tunnel.
+     *
+     * Without this the launcher kept its own `activeTunnel` that this closure
+     * could not see, and the three paths that stop a tunnel each reached only
+     * half the cases: `/daemon/disable-tunnel` was a silent no-op on a
+     * boot-started tunnel (it reported ok and nulled the lockfile while
+     * cloudflared kept serving), `/daemon/enable-tunnel`'s stop-the-existing
+     * guard missed it and orphaned a second cloudflared, and the 401-burst
+     * tripwire could only stop boot-started ones.
+     */
+    adoptTunnel(handle) {
+      activeTunnel = handle;
+    },
+    /** The tunnel handle this module currently owns (read-only; null when none). */
+    getActiveTunnel() {
+      return activeTunnel;
+    },
   };
 }