Skip to content

Commit dde4596

Browse files
ARHAEEMclaude
andcommitted
fix: sweep audit low-severity findings
Twelve of the sixteen low findings. Two were already fixed in earlier commits (the '..' output-dir escape, folded into confineToDir), and two are left open deliberately (see below). Credentials and permissions: - logout claimed "Browser session cleared" and exited 0 while credentials.json / login.json still held a live session, so the next start logged straight back in. Now reports them and exits non-zero. - login no longer accepts --password / --otp-secret. argv is world-readable via /proc/<pid>/cmdline for the ~5 minute poll and a captured base32 TOTP seed is a permanent 2FA bypass. Env vars or login.json instead. - config dir 0700; session-backup archive 0600 (it holds the cookie jar, daemon.token and daemon.lock); IDE config writes carrying a PAT 0600 -- the atomic rename replaces the inode, resetting any mode the user set. Correctness: - sync_base validates planId/diffId. They are interpolated into on-disk filenames and join() normalises '..', so diffId "x/../../tools-config" overwrote tools-config.json -- which load() merges over a defaultConfig() whose activeProfile is 'full'. - an unverifiable view no longer reads as clean. snapshotViewFiltered is the only signal suppressing pruneRecords and was set only when every view's filter state was KNOWN, so a getView that threw made a filtered row set look complete and mirror could delete real records as false orphans. - attachment fetch checks r.ok. A 403/404 body was uploaded as the file, counted as success, and written into the persisted dedupe map, so a re-run skipped the corrupted cell instead of repairing it. - an unreadable daemon.lock is no longer deleted with no liveness check -- acquire() publishes it empty and fills it a moment later, so a racer could evict a lock a live daemon was mid-write. - apply.lock expires instead of wedging forever on a recycled pid, and APPLY_LOCKED now names the lock path. - the exit-intent slot only consumes an intent staged during this request. Performance: - offsetToPosition memoises line starts and binary-searches instead of rescanning from 0 per diagnostic: ')'.repeat(20000) ~1000ms -> 28ms, 'IF('.repeat(10000) ~3500ms -> 484ms. - showToolStatus reuses one OutputChannel. Left open deliberately: createRecords re-resolving the table schema per chunk, and reapplyViewFilters prefetching view configs the next loop discards. Both are bounded micro-optimisations the verifier measured as saving ~1 request per 50 creates and "nearly nothing" respectively; neither is worth the regression risk in this pass. Verified: 1451 mcp-server + 421 extension + 162 language-services + 88 webview tests pass, check:tool-sync green at 10/54/72, pnpm build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 1eced78 commit dde4596

16 files changed

Lines changed: 222 additions & 34 deletions

File tree

packages/extension/src/auto-config/ide-detection.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,10 @@ export async function writeConfigAtomic(filePath: string, config: Record<string,
8282
const tmp = `${filePath}.${crypto.randomBytes(6).toString('hex')}.tmp`;
8383
await fs.promises.mkdir(path.dirname(filePath), { recursive: true });
8484
try {
85-
await fs.promises.writeFile(tmp, JSON.stringify(config, null, 2) + '\n', 'utf8');
85+
// 0600 — this file carries the Airtable PAT (Authorization: Bearer pat…). The
86+
// rename below replaces the destination inode, so without an explicit mode any
87+
// restrictive permissions the user had set are reset to 0644. No-op on Windows.
88+
await fs.promises.writeFile(tmp, JSON.stringify(config, null, 2) + '\n', { encoding: 'utf8', mode: 0o600 });
8689
await fs.promises.rename(tmp, filePath);
8790
} catch (err) {
8891
await fs.promises.unlink(tmp).catch(() => {});

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,8 @@ async function writeTextAtomic(filePath: string, content: string): Promise<void>
247247
const tmp = `${filePath}.${crypto.randomBytes(6).toString('hex')}.tmp`;
248248
await fs.promises.mkdir(path.dirname(filePath), { recursive: true });
249249
try {
250-
await fs.promises.writeFile(tmp, content, 'utf8');
250+
// 0600 for the same reason as writeConfigAtomic — these configs can carry a token.
251+
await fs.promises.writeFile(tmp, content, { encoding: 'utf8', mode: 0o600 });
251252
await fs.promises.rename(tmp, filePath);
252253
} catch (err) {
253254
await fs.promises.unlink(tmp).catch(() => {});

packages/extension/src/extension.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,13 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
161161
// Live-stream debug events to the VS Code Output panel
162162
const debugOutput = vscode.window.createOutputChannel('Airtable Formula: Debug Log');
163163
context.subscriptions.push(debugOutput);
164+
165+
// Created once here, not per invocation: VS Code mints a DISTINCT channel per
166+
// createOutputChannel call, so creating it inside the showToolStatus handler
167+
// left one identically-named entry in the Output dropdown per invocation and
168+
// never disposed any of them.
169+
const toolStatusChannel = vscode.window.createOutputChannel('Airtable Formula: MCP Tools', 'markdown');
170+
context.subscriptions.push(toolStatusChannel);
164171
debugCollector.onEvent = (ev) => {
165172
const time = ev.ts.slice(11, 23); // HH:MM:SS.mmm
166173
const tag = `[${ev.source}] ${ev.event}`;
@@ -565,7 +572,11 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
565572
dashboardProvider.refresh();
566573
}),
567574
vscode.commands.registerCommand('airtable-formula.showToolStatus', async () => {
568-
const channel = vscode.window.createOutputChannel('Airtable Formula: MCP Tools', 'markdown');
575+
// Created once and reused. VS Code mints a DISTINCT channel per
576+
// createOutputChannel call, so doing this inside the handler left N
577+
// identically-named entries in the Output dropdown with no way to tell
578+
// which was current — the channel.clear() below shows reuse was intended.
579+
const channel = toolStatusChannel;
569580
channel.clear();
570581
channel.appendLine(toolProfileManager.renderStatusReport());
571582
channel.show();

packages/extension/src/mcp/session-backup.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,15 @@ const MAX_UNZIPPED_BYTES = 500 * 1024 * 1024; // 500 MB post-extraction
2222
export async function backupSession(destPath: string, password?: string): Promise<void> {
2323
const zipBuffer = await createZipBuffer(CONFIG_DIR);
2424

25+
// 0600: the archive contains the Chrome cookie jar, daemon.token and daemon.lock
26+
// (both hold the plaintext bearer). Those files are hardened individually and
27+
// secureDirectory() runs after RESTORE — but not after backup, so the archive was
28+
// created 0666&~umask. No-op on Windows.
2529
if (password) {
2630
const encrypted = encrypt(zipBuffer, password);
27-
await fs.writeFile(destPath, encrypted);
31+
await fs.writeFile(destPath, encrypted, { mode: 0o600 });
2832
} else {
29-
await fs.writeFile(destPath, zipBuffer);
33+
await fs.writeFile(destPath, zipBuffer, { mode: 0o600 });
3034
}
3135
}
3236

packages/language-services/src/engines/formula/diagnostics.ts

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,40 @@ import {
2121
// Position helpers (replace document.positionAt / new vscode.Range)
2222
// ---------------------------------------------------------------------------
2323

24+
/**
25+
* Line-start offsets for one document, memoised on the document text.
26+
*
27+
* offsetToPosition used to walk from offset 0 on EVERY call, and makeRange calls it
28+
* twice per diagnostic — so emitting N diagnostics cost O(N x document). Measured
29+
* before: `')'.repeat(20000)` took ~1.0s and `'IF('.repeat(10000)` ~3.5s. Only one
30+
* document is ever in flight per call, so a single-entry cache is enough; keeping it
31+
* keyed on the text means a stale entry is impossible.
32+
*/
33+
let lineStartsText: string | null = null;
34+
let lineStartsCache: number[] = [];
35+
36+
function lineStarts(text: string): number[] {
37+
if (lineStartsText === text) return lineStartsCache;
38+
const starts = [0];
39+
for (let i = 0; i < text.length; i++) {
40+
if (text[i] === '\n') starts.push(i + 1);
41+
}
42+
lineStartsText = text;
43+
lineStartsCache = starts;
44+
return starts;
45+
}
46+
2447
function offsetToPosition(text: string, offset: number): { line: number; character: number } {
25-
let line = 0;
26-
let lastNewline = -1;
27-
for (let i = 0; i < offset && i < text.length; i++) {
28-
if (text[i] === '\n') { line++; lastNewline = i; }
48+
const starts = lineStarts(text);
49+
// Binary search for the last line start <= offset.
50+
let lo = 0;
51+
let hi = starts.length - 1;
52+
while (lo < hi) {
53+
const mid = (lo + hi + 1) >> 1;
54+
if (starts[mid] <= offset) lo = mid;
55+
else hi = mid - 1;
2956
}
30-
return { line, character: offset - lastNewline - 1 };
57+
return { line: lo, character: offset - starts[lo] };
3158
}
3259

3360
function makeRange(text: string, start: number, end: number): LsRange {

packages/mcp-server/CHANGELOG.md

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,44 @@
22

33
## [Unreleased]
44

5+
### Fixed (2026-08-01 — audit low-severity sweep)
6+
7+
- **Credentials.** `logout` wiped the browser profile, printed "Browser session cleared." and exited
8+
0 while `credentials.json` / `login.json` still held a live session — the next server start read
9+
them and was logged straight back in. It now reports those files and exits non-zero rather than
10+
claiming a cleared session (they are user-authored, so it does not delete them). `login` no longer
11+
accepts `--password` / `--otp-secret`: argv lands in world-readable `/proc/<pid>/cmdline` and shell
12+
history for the ~5-minute login poll, and a captured base32 TOTP seed is a permanent 2FA bypass —
13+
use the env vars or `login.json`. The config directory is created 0700 (it holds those files), and
14+
the session-backup archive 0600 (it contains the cookie jar, `daemon.token` and `daemon.lock`).
15+
IDE config writes that carry an Airtable PAT are 0600 — the atomic rename replaces the destination
16+
inode, so without a mode any restrictive permissions the user had set were reset. All no-ops on Windows.
17+
- **`sync_base` ids are validated.** `planId`/`diffId` are interpolated into on-disk filenames and
18+
`join()` normalises `..`, so `diffId: "x/../../tools-config"` overwrote
19+
`~/.airtable-user-mcp/tools-config.json` — which `ToolConfigManager.load()` then merges over a
20+
`defaultConfig()` whose `activeProfile` is `full`.
21+
- **An unverifiable view no longer reads as "clean".** `snapshotViewFiltered` is the only signal that
22+
suppresses `pruneRecords`, and it was set only when every candidate view's filter state was KNOWN —
23+
a `getView` that threw left the view `unknown` and the row set was treated as complete, so mirror
24+
could delete real records as false orphans. Unverified picks are now flagged (`unverified: true`).
25+
- **Attachment fetch checks `r.ok`.** A 403/404 body was uploaded to the destination cell as the file,
26+
counted as success, and written into the persisted dedupe map — so a re-run SKIPPED the corrupted
27+
cell instead of repairing it. Source signed URLs expire mid-job, which is exactly when this happened.
28+
- **An unreadable `daemon.lock` is no longer deleted.** `acquire()` publishes the file with
29+
`openSync(...,'wx')` and fills it a moment later, so it legitimately exists with zero bytes in
30+
between — and the parse-failure branch removed it with no liveness check, letting a racing acquirer
31+
evict a lock a live daemon was mid-write. It now fails the attempt and lets the retry loop re-probe.
32+
- **`apply.lock` no longer wedges forever on a recycled pid**, and the `APPLY_LOCKED` error finally
33+
names the lock file's path.
34+
- **The daemon exit-intent slot** only consumes an intent staged during the current request, so a
35+
concurrent `/mcp` response no longer truncates a stop/restart caller's confirmation. (Narrowed, not
36+
closed — see the `ponytail:` note; the exact fix needs request identity threaded through.)
37+
- **`offsetToPosition` no longer rescans from offset 0 for every diagnostic.** `makeRange` calls it
38+
twice per diagnostic, so N diagnostics cost O(N x document). Line starts are memoised per document
39+
and binary-searched: `')'.repeat(20000)` **~1000 ms → 28 ms**, `'IF('.repeat(10000)` **~3500 ms → 484 ms**.
40+
- **`showToolStatus` reuses one OutputChannel** instead of minting a new identically-named one per
41+
invocation and never disposing it.
42+
543
### Changed (2026-08-01 — new `local-write` category; read-only is 12 → 10 tools)
644

745
- **`download_formula_field` and `download_base_formulas` moved out of `read` into a new

packages/mcp-server/src/cli.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,30 @@ export async function runCli(args) {
203203
} catch {
204204
// ignore
205205
}
206+
207+
// The browser profile is only ONE of the credential sources. In byo /
208+
// direct-login mode the session lives in credentials.json / login.json, which
209+
// nothing here ever removed — so logout wiped a profile those modes never used,
210+
// printed "Browser session cleared." and exited 0, and the next server start
211+
// read the file and was logged straight back in. These are user-authored files,
212+
// so deleting them silently would destroy input the user typed; report them
213+
// instead, and do not claim the session is cleared while one still resolves.
214+
const stillLive = [];
215+
for (const name of ['credentials.json', 'login.json']) {
216+
const p = path.join(getConfigDir(), name);
217+
try {
218+
await (await import('node:fs/promises')).access(p);
219+
stillLive.push(p);
220+
} catch { /* absent — nothing to report */ }
221+
}
222+
if (stillLive.length) {
223+
process.stdout.write(
224+
'\nThese files still hold live credentials and were NOT removed:\n' +
225+
stillLive.map((p) => ` ${p}\n`).join('') +
226+
'You are still logged in through them. Delete them to finish logging out.\n',
227+
);
228+
process.exitCode = 1;
229+
}
206230
return true;
207231
}
208232

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

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ export function acquire(record, options = {}) {
2424
const lockPath = options.lockPath ?? getLockfilePath();
2525
const normalized = normalizeRecord(record);
2626

27-
mkdirSync(dirname(lockPath), { recursive: true });
27+
mkdirSync(dirname(lockPath), { recursive: true, mode: 0o700 });
2828

2929
for (let attempt = 0; attempt < 2; attempt++) {
3030
let fd;
@@ -65,12 +65,14 @@ function tryReclaimStale(lockPath) {
6565
try {
6666
existing = read({ lockPath });
6767
} catch {
68-
try {
69-
rmSync(lockPath, { force: true });
70-
return true;
71-
} catch {
72-
return false;
73-
}
68+
// Do NOT delete an unreadable lockfile. `acquire()` publishes the file with
69+
// openSync(...,'wx') and only fills it a moment later, so between those two
70+
// steps it legitimately exists with ZERO bytes — and read() throws on that.
71+
// Deleting here (with no liveness check at all) let a racing acquirer evict a
72+
// lock that a live daemon was in the middle of writing, so both callers
73+
// returned true. Fail this attempt instead and let startDaemon's existing
74+
// retry loop re-probe once the winner has finished writing.
75+
return false;
7476
}
7577
if (!isStale(existing)) {
7678
return false;
@@ -121,7 +123,7 @@ export function replace(record, options = {}) {
121123
}
122124
}
123125

124-
mkdirSync(dirname(lockPath), { recursive: true });
126+
mkdirSync(dirname(lockPath), { recursive: true, mode: 0o700 });
125127
safeAtomicWriteFileSync(lockPath, serialize(normalized), { encoding: 'utf8', mode: 0o600 });
126128
applyPrivatePermissions(lockPath);
127129
return true;

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

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import { AirtableClient } from '../client.js';
1919
import { ToolConfigManager } from '../tool-config.js';
2020
import { withToolDispatchContext } from '../page-scheduler.js';
2121
import { ensureToken, rotateToken, getTokenPath, onTokenRotate } from './token.js';
22-
import { takeDaemonExit } from './exit-intent.js';
22+
import { takeDaemonExit, peekDaemonExit } from './exit-intent.js';
2323
import { clearStopSentinel, writeStopSentinel } from './stop-sentinel.js';
2424
import { setInjectedCredentials } from './cred-store.js';
2525
import { getTunnelProvider, writeTunnelSettings } from './tunnel-providers/index.js';
@@ -745,9 +745,23 @@ export async function startDaemonServer(options = {}) {
745745
// only point at which shutting the process down cannot truncate the answer
746746
// the model is waiting for. Registered BEFORE handleRequest so it is armed
747747
// no matter how fast the handler completes; a no-op when nothing staged.
748+
// Only consume an intent that appeared DURING this request. The slot is a
749+
// process-global single slot with no association to the request that staged
750+
// it, so a concurrent /mcp response finishing in the window between
751+
// stageExit() and the stop/restart caller's own flush ran the exit on the
752+
// wrong hook and truncated the stop caller's confirmation.
753+
// ponytail: comparing against the pre-handler snapshot closes the common case
754+
// (an intent staged before we started is not ours) but not the exact one — a
755+
// request that began earlier still wins a same-instant stage. Threading the
756+
// request identity through requestDaemonExit (AsyncLocalStorage) is the real
757+
// fix; the residual window is sub-millisecond and browser-mode stop() awaits
758+
// Chromium teardown before touching activeMcpClosers, so the response wins.
759+
const intentBefore = peekDaemonExit();
748760
res.on('finish', () => {
749-
const intent = takeDaemonExit();
750-
if (intent) void runExitIntent(intent);
761+
const intent = peekDaemonExit();
762+
if (intent && intent !== intentBefore) {
763+
void runExitIntent(takeDaemonExit());
764+
}
751765
});
752766

753767
const mcpServer = new Server(

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,10 @@ export function rotateToken(options = {}) {
8484
function writeToken(record, options = {}) {
8585
const tokenPath = options.tokenPath ?? getTokenPath();
8686
const normalized = normalizeRecord(record);
87-
mkdirSync(dirname(tokenPath), { recursive: true });
87+
// 0700: this directory holds the daemon token AND the hand-authored
88+
// credentials.json / login.json. The files are chmod 0600 individually, but the
89+
// containing directory was created traversable. No-op on Windows.
90+
mkdirSync(dirname(tokenPath), { recursive: true, mode: 0o700 });
8891
const tmp = tokenPath + '.tmp';
8992
writeFileSync(tmp, JSON.stringify(normalized, null, 2) + '\n', { encoding: 'utf8', mode: 0o600 });
9093
renameSync(tmp, tokenPath);

0 commit comments

Comments
 (0)