fix(security): deep audit remediation — 5 high + 6 medium findings - #22
Open
ARHAEEM wants to merge 6 commits into
Open
fix(security): deep audit remediation — 5 high + 6 medium findings#22ARHAEEM wants to merge 6 commits into
ARHAEEM wants to merge 6 commits into
Conversation
undici 7.24.5 was in range for a TLS certificate validation bypass via dropped requestTls. This is not a theoretical transitive dependency: src/proxy.js constructs undici's EnvHttpProxyAgent for every proxied environment, which is the exact affected code path — a corporate-proxy user could have had certificate validation silently skipped. It also shipped further than npm. undici is not in the esbuild `external` list in scripts/bundle-mcp.mjs, so the vulnerable copy was compiled into dist/mcp/index.mjs and frozen into all 8 platform VSIXes; extension users could not have fixed it by updating anything. Reaching them needs a rebuild + republish. Raise the floor ^7.0.0 -> ^7.28.0 (resolves 7.29.0). Diff is deliberately undici-only: bumping the hono/@hono/node-server advisories that @modelcontextprotocol/sdk carries pulled 24 unrelated upgrades, including patchright 1.59.4 -> 1.61.1 (the Chromium driver behind the default auth path), and those advisories are unreachable anyway — their vulnerable code is serve-static, CORS middleware and JSX, while the SDK only uses @hono/node-server's request/response conversion. Verified: 1438 mcp-server + 395 extension + 88 webview tests pass, check:tool-sync green, and all 16 pinned platform packages still match their lockfile-verified tarballs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects found by audit, each reproduced before being fixed.
client.js: all seven row-template methods interpolated a caller-supplied
templateId into the API URL path with no assertAirtableId, while the file
defines AIRTABLE_ID_RE with the comment "Prevents path traversal" and
applies it at 128 other call sites. The value arrives straight from tool
args and auth._rawApiCall passes an absolute URL through unchanged, so a
crafted id reached arbitrary internal endpoints under the live session
cookie. applyRowTemplate's body is byte-identical to what deleteTable
posts to table/{id}/destroy, so a table-write-only caller could destroy a
table while skipping deleteTable's expectedName confirmation.
createRowTemplate was never affected -- it generates its own id.
vendor/formula-{beautifier,minifier}-v2.js: the default formatter deleted
lowercase function names and wrote the result to disk. Airtable function
names are case-insensitive but both tokenizers matched /[A-Z_]/ with no /i
flag, so a-z fell through to the operator catch-all and vanished.
Measured: lower({Email}) -> ({Email}), If({A},1,0) -> I({A},1,0). The
mangled output still parses, so the try/catch never fired -- format-on-save
corrupted buffers silently and the bulk path wrote unopened files with no
undo stack. Now matches case-insensitively and canonicalises only for the
FUNCTIONS/CONSTANTS lookup, preserving the user's casing.
vendor/formula-{beautifier,minifier}.js: the v1 identifier fallback was the
only tokenizer branch with no unconditional advance, so an unrecognized
character (';', '%', '[', a pasted smart quote) spun the loop allocating
until V8 aborted, killing the extension host. Now throws, engaging the
callers' existing try/catch so the formula is returned unchanged.
Verified: 1438 mcp-server + 416 extension (incl. 21 new regression cases)
+ 88 webview tests pass, check:tool-sync green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
…adratic diagnostics Six medium findings from the audit. Each is pinned by a regression test. sync/records.js: pruneRecords' only "don't prune after a failure" gate was RUN-wide, so one converged row in any other table (counted as skipped) disarmed it. A FIELD_FORBIDDEN 403 is a per-field permission property that fails every row of exactly one table by design, so a wholly-failed table is routine -- and under mirror+confirmDeletions its pre-existing dest rows were deleted while the replacement data was never written, reporting phase=done. Pass 1 now records per-table outcomes and prune skips a table that wrote nothing and failed, emitting RECORDS_FAILED_PRUNE_SKIPPED. sync/prune-schema.js: pruneSchema deleted the fieldMappings targets apply() had just validated. A target must exist on the dest but not on the source -- the documented injection pattern (source autoNumber Code -> dest text field InjectID) is dest-only by construction and so always an orphan. The records job then re-validated, threw FIELD_MAP_INVALID and synced zero records: a destination column destroyed and no data written. sync/index.js: the drift guard fingerprints only the DESTINATION, but plan.orphans is a statement about the SOURCE. A source-side addition between plan and apply was invisible -- dest untouched so the fingerprint matched, no DRIFT, no create action in the stale plan -- and pruneSchema then deleted the dest field that now legitimately matched. expectedName cannot help; the field kept its name. apply() now re-reads the source before any schema deletion and drops orphans that regained a counterpart; if that read fails it skips all schema deletion rather than trusting a stale list. daemon/launcher.js: stopDaemon --force SIGKILLed a lockfile pid with no identity check. isStale()'s bare process.kill(pid,0) cannot tell our daemon from a process that recycled the pid, and on Windows SIGTERM maps to TerminateProcess -- an unsavable hard kill of a stranger. Now probes /daemon/health and requires the uuid to echo the lockfile's, mirroring the extension's _verifyDaemonIdentity. Unproven means release the lock and leave the process alone. sync/records.js: buildUpdateCells never compared against the destination, so every mapped scalar cell of every mapped row was re-posted on each re-sync -- updateRecords issues one serialized POST per CELL, i.e. 20,000 sequential requests for a 1000-row x 20-field no-op run. Converged cells are now skipped. language-services diagnostics.ts: isInsideExclusionRange was a linear scan run once per character by three checkers, so cost was chars x field-refs with no debounce and no size cap -- and in --tcp daemon mode that blocking is shared by every attached editor. Now a binary search over the already-ascending ranges. Measured on the repo's largest shipped example (38,830 chars / 741 refs): 83.3ms -> 9.6ms. On an 87KB/4000-ref synthetic: 592ms -> 10ms. Diagnostic output identical. Verified: 1451 mcp-server + 162 language-services + 88 webview tests pass, check:tool-sync green, pnpm build succeeds. One extension test (session-backup "rejects backups larger than 200 MB") fails with ENOSPC -- the disk is 100% full and that test truncates a 201 MB file. It passed earlier this session and neither it nor packages/extension is touched by this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
download_formula_field and download_base_formulas READ from Airtable but WRITE .formula files to a caller-chosen path, so they did not belong in the `read` category -- the read-only profile is advertised as "Schema inspection, formula validation, and record reading only". Correcting the readOnlyHint annotation (previous commit) restored the MCP client's consent prompt, but the profile itself still claimed a file-writing tool was read-only. New `local-write` category, included in safe-write and full, so those tool sets are unchanged (54 / 72). Only read-only changes: 12 -> 10. Deliberately NOT added to LEGACY_CATEGORIES_DEFAULT_ON, per the invariant documented there -- a pre-existing on-disk `custom` profile must not silently widen to include a category it never opted into. Propagated through every mirror the sync guard checks: tool-config.js, tool-profile.ts, shared types, extension package.json (new mcp.categories.localWrite setting + enumDescriptions), webview store + Settings toggle + store test, CLAUDE.md, all three READMEs, skill templates and the three banner SVGs. Verified: check:tool-sync green at 10/54/72 with 17 labels, 1451 mcp-server + 421 extension + 88 webview tests pass, pnpm build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+1773
to
+1775
| const tableCounts = result.perTable?.[t.name]; | ||
| if (tableCounts && tableCounts.failed > 0 | ||
| && tableCounts.created === 0 && tableCounts.updated === 0 && tableCounts.skipped === 0) { |
There was a problem hiding this comment.
Bug: The per-table failure check in pruneRecords uses the destination table name for a lookup, but the statistics are keyed by the source table name, bypassing the check when names differ.
Severity: HIGH
Suggested Fix
The result.perTable statistics dictionary should be keyed by a consistent identifier in both the write and read locations. Using the destination table ID would be a robust solution, as tables are matched by ID throughout the sync process. This ensures the lookup succeeds even when table names differ.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/mcp-server/src/sync/records.js#L1773-L1775
Potential issue: A safety gate in `pruneRecords` intended to prevent data deletion is
silently bypassed when source and destination tables have different names. The sync
process writes per-table operation counts to `result.perTable` using the source table's
name as the key. However, the check in `pruneRecords` reads from this map using the
destination table's name. Since the sync system matches tables by ID, differing names
are a supported scenario. When names differ, the lookup fails, and the safety check is
skipped. This could lead to the deletion of all records in a destination table if all
write operations for that table failed during the sync.
Also affects:
packages/mcp-server/src/sync/records.js:759~759
Did we get this right? 👍 / 👎 to inform future reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remediates the findings from a multi-agent security/performance/leak/conflict audit of the monorepo (51 raw findings → 29 survived adversarial verification; 13 refuted, 3 contested).
Every fix here was reproduced before being written and is pinned by a regression test that fails against the old code.
HIGH
disable-tunneland the 401-burst tripwire each stopped the wrong handle while the UI, lockfile and/daemon/healthall reported "off"daemon/server.js,daemon/launcher.jsdownload_*annotatedreadOnlyHint: truewhile writing files at a caller-supplied path — the signal MCP clients use to auto-approve without promptingindex.jstemplateIdinto the API URL pathclient.jsunconfigureMcpToml/unconfigureHelixtruncated the user's config from our marker to EOFauto-config/lsp-config.tsvendor/formula-*-v2.jsMeasured before the fix —
lower({Email})→({Email}),If({A},1,0)→I({A},1,0). The mangled output still parses, so the try/catch never fired: plaineditor.formatOnSavecorrupted buffers silently, and the bulk path writes unopened files with no undo stack.Measured against the old
lsp-config.ts— aconfig.tomlholding one extra MCP server was reduced to a single newline.MEDIUM
pruneRecordsdeleted dest rows in a table whose every write failed (the gate was run-wide, so one converged row elsewhere disarmed it)pruneSchemadeleted thefieldMappingstargetsapply()had just validated — dest-only by construction, hence always orphansstopDaemon --forceSIGKILLed a lockfile pid with no identity check (recycled pid → kills a stranger; on WindowsSIGTERM=TerminateProcess)buildUpdateCellsnever diffed against the destination — 20,000 sequential POSTs for a 1000×20 no-op re-syncTwo more mediums fell out for free alongside the above: the v1 tokenizer infinite loop (unrecognized char → V8 OOM, killed the extension host) and the boot tunnel's unconsumed
waitUntilReadyrejection.Dependencies
undici ^7.0.0→^7.28.0, closing a TLS certificate-validation bypass on the exactEnvHttpProxyAgentpathproxy.jsuses. It is not in the esbuildexternallist, so the vulnerable copy was compiled intodist/mcp/index.mjsand frozen into all 8 platform VSIXes — extension users could not have fixed it themselves. Reaching them needs a rebuild + republish.Diff is deliberately undici-only: bumping the
honoadvisories pulled 24 unrelated upgrades includingpatchright1.59.4 → 1.61.1 (the Chromium driver behind the default auth path), and those advisories are unreachable anyway — their vulnerable code isserve-static, CORS middleware and JSX, while the SDK only uses@hono/node-server's request/response conversion.Verification
check:tool-syncgreen ·pnpm buildsucceedssession-backup→ "rejects backups larger than 200 MB") currently fails withENOSPC. The build machine's disk is 100% full and that test truncates a 201 MB file. It passed earlier in the same session, and neither it norpackages/extensionis touched by the final commit — but it should be re-run on a machine with free space before merge.Deliberately not changed
outputPath/outputDirare still honoured as given — they are the user's explicit choice, and confining them tocwdwould break legitimate "save to my Desktop" use. The consent step is restored by the annotation fix instead.download_*remain in thereadcategory. The annotation is fixed so clients prompt, but a file-writing tool still sits in the profile advertised as "reading only" — recategorising changes the published profile counts (12/54/72) and is a product decision.🤖 Generated with Claude Code