Security audit: close MCP/channel auth gaps, fix critical CVE, add CI - #85
Merged
Conversation
…e vulns - Bump vitest floor from ^3.0.0 to ^3.2.7, closing the critical Vitest UI arbitrary file read/execute CVE (GHSA-5xrq-8626-4rwp) affecting <3.2.6. - Run `npm audit fix` to update package-lock.json in place (no package.json range changes beyond vitest): pulls in patched hono, @hono/node-server, fast-uri, ip-address, body-parser (all via @modelcontextprotocol/sdk), and patched next/postcss/sharp/vite/ esbuild/nanoid in packages/nextjs. - Remaining `npm audit` finding (adm-zip <0.6.0 via onnxruntime-node) is accepted risk: adm-zip is only invoked by onnxruntime-node's install-time postinstall script to unpack its own npm-hosted prebuilt binary, never on attacker-reachable input; onnxruntime-node has not yet released a version depending on adm-zip>=0.6.0. Verified: npm test (1138/1138 passing) and npm run build both clean after the bump.
Four related fixes to src/mcp/server.ts and oauth.ts, all found in a security audit of the entry-point auth/rate-limit perimeter: - GET /sse now enforces the same bearer-token check as the JSON-RPC endpoint when enableAuth is set; previously it was reachable by any unauthenticated client. - resources/subscribe notifications no longer broadcast to every connected SSE client when the subscribing request has no session id — they're now matched by exact session-id equality, closing a cross-session notification leak. - The rate limiter no longer trusts the client-supplied Mcp-Session-Id header as a bucket key unless it names a session that actually exists in the session store; otherwise it falls back to the socket's remote address. Previously a client could reset its own rate-limit bucket on every request by sending a fresh, unverified header value. - POST /oauth/token now has its own always-on rate limiter (20 rpm), independent of config.enableRateLimit, since credential-guessing protection on a token-issuance endpoint shouldn't be opt-in. - OAuthManager.authenticateClient compares secret hashes with crypto.timingSafeEqual instead of `!==`, removing a timing side channel on client-secret verification. Adds regression coverage in src/mcp/mcp.test.ts (real HTTP server, each fix exercised end-to-end) and src/mcp/oauth.test.ts. Verified: npm test (1143/1143) and npm run build both clean.
Three findings from the security audit's channel-bridge review: - The HTTP bridge's shared-secret check exempted GET /sse from auth even when httpBridgeSecret was configured. /sse streams channel events and pending tool-approval requests (including tool names and arguments), so leaving it open handed any local client a live feed of that traffic. Only /health stays unauthenticated now. - The shared-secret comparison used `!==`, a timing side channel on the secret itself; switched to a constant-time compare via crypto.timingSafeEqual (mirroring the same fix just applied to OAuthManager). - serializeChannelTag() escaped meta attribute values but embedded `content` verbatim inside the <channel>...</channel> body. A sender could include a literal `</channel>` followed by a forged `<channel source="trusted">` and have the LLM read it as a second, spoofed channel event — undermining the provenance the source attribute exists to convey. content is now XML-text-escaped (&, <, >) before being embedded, same as attribute values already were. Adds regression tests: /sse now returns 401 without the secret in channel-server.test.ts, and a forged-closing-tag case in utils.test.ts asserting the spoofed tag can't survive as real markup. Verified: npm test (1145/1145) and npm run build both clean.
The security audit flagged LocalTransport's sandbox.enabled as giving
false confidence ("Phase 1 sandbox... restricted global scope") when
it doesn't isolate the handler function's capabilities at all —
handlers are plain JS closures that keep full access to
require/process/fs regardless of the vm context they're invoked from.
Fixed the documentation (local-transport.ts, types.ts) to say so
plainly, and added a one-time runtime warning so sandbox.enabled isn't
silently trusted as a security boundary.
Digging into the sandbox path surfaced a second, more consequential
bug while writing a test for it: timeoutMs didn't actually bound a
handler that hangs without ever resolving.
- src/transport/timeout.ts's withTimeout() awaited `fn(signal)`
directly rather than racing it against the timeout timer. It relies
entirely on `fn` to notice the AbortSignal and reject itself — a
handler that never checks the signal (as LocalTransport's call site
didn't even forward one to `handler`) hung forever past timeoutMs.
Now races via Promise.race, so withTimeout always settles on time
regardless of whether the wrapped operation cooperates.
- LocalTransport's sandboxed path had the same problem one level
down: vm.Script's own `timeout` option only bounds *synchronous*
execution inside the script, not the awaited async handler it
kicks off, so a hanging handler slipped past the sandbox's timeout
too. Now races the vm promise against a timer the same way.
Both fixes preserve existing behavior for cooperating callers
(http-transport.ts, mcp-client-transport.ts, which pass the signal to
fetch and already worked correctly) and only change what happens for
callers that previously hung.
Adds regression tests: a real hang case in local-transport.test.ts
(sandboxed and non-sandboxed) and timeout.test.ts, plus a test that
pins the documented non-isolation of handler capabilities so it can't
silently start being (mis)represented as fixed without the docs
changing too.
Verified: npm test (1150/1150) and npm run build both clean.
…artifact.ts
- SCDictionary.unwrap() built its result object as a plain {} literal;
a "__proto__" key (SCDictionary keys are unrestricted) would then be
interpreted by bracket-notation assignment as a prototype write
instead of an own property, i.e. dormant prototype pollution. No
current caller feeds untrusted keys into SCDictionary, but unwrap()
is the boundary where the data becomes a plain JS object anyone can
read, so it's hardened at the source with Object.create(null).
- src/mcp/artifact.ts parsed both the compiled-artifact JSON file and
each manifest file in a directory with bare JSON.parse, unlike the
rest of the codebase's manifest/config loading (compile.ts), which
routes through safeJsonParse per the CHANGELOG's stated guarantee.
Switched both call sites to safeJsonParse so a prototype-pollution
payload is rejected/skipped the same way it already is everywhere
else JSON is loaded from disk.
Adds a prototype-pollution regression test to sc-object.test.ts, and
a new artifact.test.ts (this file previously had no direct test
coverage) covering both a minimal successful artifact load and a
directory with a __proto__-polluted manifest being silently skipped.
Verified: npm test (1153/1153) and npm run build both clean.
The dependency-hygiene audit found npm scripts for test/lint/build but no CI wired up to run them — nothing gated merges. Adds .github/workflows/ci.yml: - test job (Node 20 and 22 matrix): npm ci, lint (tsc --noEmit), test (vitest run — 1153 specs), build (root tsc), build:packages (all 6 workspace packages). - audit job: npm audit --audit-level=critical, so a newly introduced critical vulnerability fails CI. Left at "critical" rather than "high" because the current high-severity finding (adm-zip via onnxruntime-node's install-time-only postinstall script) is an accepted, documented risk rather than something to force a workaround for right now — see the audit report for details. Verified every step locally against the actual repo state (npm ci, lint, test, build, build:packages, npm audit --audit-level=critical) before adding the workflow.
- .gitignore: add *.log and coverage/ (vitest coverage output had no ignore rule and would get committed if generated). - README.md: the "~1,250+ specs" claim overstated the actual count (1,153 as of this branch); corrected to ~1,150+. - shorthand/package.json: repository.url pointed at github.com/johnnyclem/shorthand.git, which 404s — the real repo is github.com/johnnyclem/short-hand (hyphenated), per docs/ecosystem/engineering-guide.md's own note about this typo.
AUDIT.md is the full write-up for this security/code-review pass: findings by severity, what was fixed vs. deliberately deferred (and why), dependency upgrade summary, and prioritized remaining technical debt. CHANGELOG.md's [Unreleased] section gets the same information in the project's existing format, so it reads consistently with prior audit entries already there.
packages/docs carries its own separate package-lock.json (not part of the npm workspace), so the root-level npm audit fix earlier in this audit never touched it — confirmed by GitHub's push output reporting 106 total repo vulnerabilities against the 13 npm audit found at root. Ran npm audit fix (no --force) here too: 44 -> 22 vulnerabilities (2 critical and 22 high resolved), with no package.json range changes — Docusaurus stays at 3.6.3. The remaining 22 (11 moderate, 11 high) are in webpack-dev-server's transitive tree (dev-only local-preview tooling, not part of the built static site) and require forcing @docusaurus/preset-classic to 3.10.2 while @docusaurus/core stays pinned at 3.6.3 — a mismatched pair outside the stated dependency range that isn't safe to take without also coordinating the full Docusaurus 3.6->3.10 upgrade. Left as a follow-up (same reasoning as the deferred commander/better-sqlite3 bumps in AUDIT.md). While verifying the fix with an actual `docusaurus build`, found the build was already failing on a pre-existing broken link unrelated to dependencies: docs/integrations/index.md linked to LoomMCP's page with a relative `./loom-mcp`, which the `slug: /integrations` frontmatter override on the index page causes to resolve to `/docs/loom-mcp` instead of the real `/docs/integrations/loom-mcp`. Fixed the link to the absolute path so the docs site actually builds again — nothing gated this before now since it's not part of root CI. Verified: `npm run build` in packages/docs now succeeds cleanly.
packages/docs is outside the root npm workspace and has its own lockfile, so it needed its own CI coverage (install, build, audit) rather than being folded into the existing jobs — added as a third job in ci.yml. Also updates AUDIT.md and CHANGELOG.md with the packages/docs findings and fixes from the preceding commit (106 vs 13 vulnerabilities discrepancy, the broken-link build failure, and what was and wasn't resolved).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
7 tasks
johnnyclem
added a commit
that referenced
this pull request
Aug 16, 2026
* chore: remove orphaned nested lockfiles in shorthand/ and packages/examples/ Both were flagged as a deferred finding in the prior audit pass (AUDIT.md #15) pending confirmation they weren't serving some standalone-publish purpose that would make removing them unsafe. Checked both possibilities directly: - @shorthand/core is not published to npm (registry 404) despite shorthand/'s lockfile implying an independently-tracked dependency tree. - @smallchat/examples is published, but publishing doesn't consume a package-lock.json, so that's not a reason to keep one either. More importantly, `npm install` run directly inside either directory does not update its local lockfile at all — npm detects the ancestor workspace root and defers to the root package-lock.json entirely, so these files were not being maintained by any normal workflow. This had already caused real drift: shorthand/package-lock.json still resolved vitest@3.2.4, the exact version with the critical CVE fixed at the root in the prior audit pass (#85) — invisible to root-level `npm audit`/CI, the same class of blind spot as the packages/docs lockfile found in that same pass. A lockfile nobody can update and that silently reintroduces fixed CVEs is worse than no lockfile. Verified: fresh `npm install` from a clean node_modules, `npm test` (1153/1153), `npm run build`, and `npm run build:packages` all pass using only the root lockfile. * fix(docs): coordinated Docusaurus 3.6.3 -> 3.10.2 upgrade Second half of the deferred packages/docs dependency work: bumping just @docusaurus/preset-classic (via npm audit fix --force) while @docusaurus/core stayed pinned at 3.6.3 produced a mismatched, out-of-range pair that wasn't safe to ship. Bumped all four @docusaurus/* packages (core, preset-classic, module-type-aliases, types) together to the current latest, 3.10.2. - npm audit: 44 -> 24 remaining findings. All 24 are serialize-javascript/uuid/sockjs/webpack-dev-server, pulled in transitively via @docusaurus/bundler's own webpack toolchain, with "No fix available" per npm audit's own output even at Docusaurus's current latest release -- nothing further to do here until Docusaurus updates that dependency chain upstream. These are build/dev-time tooling deps, not shipped in the static site output. - Fixed the onBrokenMarkdownLinks config deprecation warning the bump surfaced (moved to markdown.hooks.onBrokenMarkdownLinks per the new Docusaurus config shape). - Aligned engines.node from >=18.0 to >=20.0 -- Docusaurus 3.10 itself now requires Node >=20, so the old floor was both already inconsistent with the rest of the monorepo and no longer accurate. Verified: `npm run build` (docusaurus build) succeeds cleanly with no warnings; root `npm test` (1153/1153) and `npm run build` unaffected (packages/docs is outside the root workspace/tsconfig). * refactor(core): decouple ToolProxy from concrete MCP transport Deferred item #13 from the security audit: ToolClass/ToolProxy are re-exported from `@smallchat/core/inference`, documented as the durable, transport-agnostic engine -- but ToolProxy statically imported MCPTransport/getTransport from src/mcp/transport.ts, ~550 lines of HTTP/JSON-RPC/SSE/gRPC wire-protocol code. Anyone importing `@smallchat/core/inference` expecting pure selection logic got concrete networking code pulled in transitively, contradicting the entry point's own stated contract in ARCHITECTURE.md. - src/core/types.ts: added ToolTransport (execute/executeStream/ executeInference), ToolTransportConnectionOptions, and a ToolTransportFactory type -- type-only, zero runtime import cost. - src/core/tool-class.ts: ToolProxy no longer imports mcp/transport.js at all. Its constructor takes an optional transportFactory; getTransport() is now a private method that uses the injected factory (lazily, same caching behavior as before) or returns null. execute()/ executeStream() return a clear "no transport configured" ToolResult error when no factory was injected, rather than silently reaching into a hardcoded implementation; executeInference() returns nothing, matching its existing "unsupported transport" no-op pattern. - src/compiler/compiler.ts (createIMP) and src/mcp/artifact.ts (hydrateRuntime) -- the only two places that construct ToolProxy -- now explicitly pass getTransport from mcp/transport.ts, preserving identical existing behavior at both call sites. - src/inference.ts and src/index.ts: export the three new types alongside the existing core type exports. Verified the fix is real, not just moved: grepped the built dist/inference.js, dist/core/*.js, and dist/runtime/*.js for any reference to mcp/transport -- none. Added src/inference.test.ts, a source-scan regression test that fails if any core/runtime file statically imports mcp/transport.ts again. Added constructor-injection tests to core/tool-class.test.ts (no-factory error path, and routing through an injected factory). Verified: npm run lint (tsc --noEmit), npm test (1156/1156), npm run build, and npm run build:packages all clean. * fix(importance): reconcile src/importance/ with @shorthand/core/importance Deferred item #21, tracked as a "Phase 0, do first" fix in docs/ecosystem/engineering-guide.md: PR #58 titled itself "Extract @shorthand/core package from compaction, CRDT, and importance modules," but only compaction and CRDT actually got extracted -- src/compaction/ and src/crdt/ don't exist as local directories, only as re-exports from @shorthand/core in src/index.ts. src/importance/ was left as a full duplicate copy of shorthand/src/importance/ instead, and it had already drifted: its types.ts defined a narrower local ConversationMessage (missing the 'tool' role option, timestamp: string | number, and normalizeTimestamp) instead of importing the canonical shared type shorthand/src/importance/types.ts already does. `diff -rq` on the two directories confirmed every other file was byte-identical -- this was purely a types.ts fork. Replaced src/importance/ with a thin re-export of @shorthand/core/importance, matching how compaction/CRDT already work, and deleted the five duplicated implementation files plus their tests (the underlying logic is exercised by @shorthand/core's own test suite). The public @smallchat/core/importance subpath is unchanged -- same exported names, same behavior -- so this is non-breaking for anyone consuming it. Making this change surfaced a real CI gap: shorthand/'s own ~260-spec test suite (compaction, CRDT, importance) was never run by root `npm test` or CI -- root vitest.config.ts only globs the root src/, and nothing wired shorthand's tests into either. This was true even before this change; deleting the local duplicate just made the gap visible rather than causing it. Added `npm test --workspace=shorthand` to CI's test job to close it. Adds src/importance/index.test.ts: a smoke test verifying the re-export barrel resolves and works end-to-end (not re-testing the underlying logic, which shorthand's own suite already covers). Verified: npm run lint (tsc --noEmit) clean, npm test (1106/1106), npm run build, npm run build:packages, and `npm test --workspace=shorthand` (260/260) all pass. * docs: update README test counts after the importance/docs follow-up work Root spec count dropped from ~1,150 to ~1,106 (removing the duplicated src/importance/ implementation files also removed their tests -- the underlying logic is now exercised by @shorthand/core's own suite instead). Also documents `npm test --workspace=shorthand`, which the audit follow-up wired into CI but wasn't mentioned anywhere for anyone running it locally. * feat(deps)!: raise Node floor to >=22, bump commander and better-sqlite3 Last deferred item from the security audit. commander@15 and better-sqlite3@13 (two majors ahead of what was pinned) both now require Node >=22 upstream -- a real breaking-change decision that wasn't this audit's to make unilaterally, so it was put to the maintainer directly: raise the floor, or hold at Node >=20 and skip the bumps. Decision: raise the floor. BREAKING CHANGE: engines.node is now >=22.0.0 (was >=20.0.0). Node 20 is no longer supported. - package.json: commander ^13.0.0 -> ^15.0.0, better-sqlite3 ^11.0.0 -> ^13.0.3, @types/better-sqlite3 ^7.6.12 -> ^9.6.0, engines.node -> >=22.0.0. - shorthand/package.json: better-sqlite3 bumped to match (it depends on the same package directly -- now deduped to one copy across the workspace instead of two divergent majors), plus vitest ^3.0.0 -> ^3.2.7 to match the critical-CVE fix applied to root earlier in this audit but missed here, and engines.node -> >=22.0.0. - packages/examples/package.json, packages/docs/package.json: engines.node -> >=22.0.0 for consistency. - src/cli/commands/init.ts: the package.json template `smallchat init` scaffolds for new projects also bumped to >=22.0.0 -- it depends on @smallchat/core, which now requires that floor anyway. - .github/workflows/ci.yml: test matrix ['20','22'] -> ['22','24']; the audit and docs jobs' single Node version '20' -> '22'. - README.md: "Requires Node.js >= 20" -> ">= 22". Verified beyond typecheck/tests (which a native-binding major bump like better-sqlite3 wouldn't necessarily catch): built and ran the actual CLI against the new versions -- `smallchat --help` renders correctly and `smallchat doctor` confirms "better-sqlite3 + sqlite-vec: working". Verified: npm run lint, npm test (1106/1106), npm run build, npm run build:packages, and npm test --workspace=shorthand (260/260) all pass on Node 22. * docs: note in AUDIT.md that all five deferred items are now resolved --------- Co-authored-by: Claude <noreply@anthropic.com>
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
2 participants
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.
Summary
Full code review + security audit per the standing brief: architecture/quality review, security audit, dependency hygiene, and targeted technical-debt cleanup, in that priority order. Full write-up in
AUDIT.md(findings by severity, what was fixed vs. deliberately deferred and why, dependency summary, remaining debt) and the[Unreleased]section ofCHANGELOG.md.The codebase was in good shape going in — no TODO/FIXME/HACK debt, no placeholder tests, and real prior hardening (CORS defaults, body-size limits, env allowlisting, prototype-pollution guards) left untouched here. What this pass found follows one consistent pattern: auth/rate-limiting primitives existed but weren't applied to every entry point of a server that already had them wired up elsewhere, plus a critical
vitestCVE and a complete absence of CI (both at the root package and, it turned out, inpackages/docs, which carries its own separate lockfile and was a total blind spot — GitHub reported 106 vulnerabilities on push against the 13npm auditfound at root before that was caught).Security fixes
src/mcp/server.ts,oauth.ts):GET /sseno longer bypasses bearer auth; resource-update notifications no longer broadcast to every SSE client when the subscribing request has no session id (cross-session leak); the rate limiter no longer trusts an unverifiedMcp-Session-Idheader as a bucket key;POST /oauth/tokengets its own always-on rate limiter; secret comparison is now constant-time.src/channel/channel-server.ts,utils.ts):GET /sseis no longer exempt from the shared-secret check; secret comparison is constant-time; channelcontentis now XML-escaped, closing a provenance-spoofing injection (a sender could forge</channel><channel source="trusted">).LocalTransport(src/transport/local-transport.ts,timeout.ts): fixed misleading docs claiming the sandbox isolates handler capabilities (it doesn't — handlers are plain closures with fullrequire/process/fsaccess regardless of thevmcontext), plus a real bug found while verifying it:timeoutMsdidn't actually bound a handler that hangs without resolving, in both the sandboxed and non-sandboxed paths.SCDictionary.unwrap()prototype-pollution guard;mcp/artifact.tsnow usessafeJsonParseconsistently.Dependencies
vitest^3.0.0→^3.2.7(GHSA-5xrq-8626-4rwp).npm audit fix(nopackage.jsonrange changes): 11 more root findings resolved;packages/docs(separate lockfile, previously unaudited): 22 of 44 resolved, including both criticals.commander/better-sqlite3majors (require Node ≥22, would silently break the declared>=20.0.0floor) and the remainingpackages/docsfindings (need a coordinated Docusaurus 3.6→3.10 bump). Both documented inAUDIT.mdas follow-ups.CI
Added
.github/workflows/ci.yml— previously nothing gated merges despitetest/lint/buildscripts existing. Three jobs: root lint/test/build/build:packages on a Node 20/22 matrix, a root dependency-audit job, and apackages/docsjob (install/build/audit) since it's outside the npm workspace. Every step was run locally against this branch before being added, including the docs build — which surfaced and required fixing a pre-existing broken link that had nothing to do with dependencies.Test plan
npm test— 1153/1153 passing (up from 1138 at the start of this branch; added regression tests for every security fix)npm run buildandnpm run build:packages— cleanpackages/docs:npm run build— clean (after the broken-link fix)Generated by Claude Code