Security audit and hardening pass across all Chronos packages - #16
Merged
Conversation
The guarded setTimeout compiled string handlers via `new Function`, turning any string that reached a forgotten global timer (a simulated peer's message payload, an untrusted capsule field) into executed code. Node's own setTimeout rejects string handlers outright, so the guard was adding an eval sink the emulated runtime does not even have. Throw the same TypeError Node throws. Also in core: - TraceLogger.summarize marked merely SHARED object references as [Circular] (a WeakSet of everything seen, rather than an ancestor stack), corrupting the summaries that send/deliver pairing keys on. - RealEnv.random() used Math.random(); production callers may use env.random() for ids or nonces, and V8's PRNG state is recoverable from observed output. Use a CSPRNG-backed 53-bit float instead (nothing in production is replayed). - InvariantViolated read 'violated at t=<detail>' — the detail is not a time. - CHRONOS_VERSION was still 0.0.0 while the packages shipped 0.1.5; pin the two together with a test so capsules stop claiming a version that never existed. Co-authored-by: Saim <contact@saimshafique.com>
A capsule is untrusted shared input, but validateCapsule stopped at the fields the Simulator reads. Everything downstream — chronos trace/stats/export and the Inspector — treats core's TraceEvent union as a guarantee, so a capsule that merely omitted trace.nodes crashed `chronos trace` with a raw TypeError, and an event with an unknown kind or a non-array `groups` white-screened the Inspector. Validate the trace envelope and every event once, here, so every renderer downstream can stay simple. Also hardens capsule I/O: - readCapsule had no size limit. MAX_EVENTS is a post-parse bound, which is too late: readFile and JSON.parse both materialize the whole file first, so a multi-GB .json was an OOM crash before any bound applied. Stat first and cap at 128 MB (CHRONOS_MAX_CAPSULE_BYTES overrides). - Reject a `__proto__` key at the parse boundary. JSON.parse alone is safe, but the parsed config and trace are spread and merged downstream, where any Object.assign or hand-rolled deep merge would trigger the setter. - writeCapsuleTo used a pid-derived temp name and a following write, so anyone who could write the capsule directory could pre-create it as a symlink and have the capsule written through it. Use an unpredictable name with an exclusive create (wx) at mode 0600, and clean up if the rename fails. - runCandidate swallowed every throw as 'did not reproduce', reporting a broken harness as 'already minimal'. Let strict-mode violations and OOM propagate. Co-authored-by: Saim <contact@saimshafique.com>
A capsule is untrusted input that the CLI renders into three sinks with three different injection grammars, and none of them was escaped: - Terminal (trace, stats, explain). `summary`/`detail`/node ids went straight to stdout, so a crafted capsule could embed ANSI escapes to erase and repaint earlier output — making a run that violated an invariant appear clean. A trace viewer that can be made to lie about what happened is worse than one that crashes. Strip ESC sequences and C0/C1 controls before printing. - CSV (export). `escapeCSV` quoted per RFC 4180 but left formula leads intact, so a summary of `=cmd|'/c calc'!A1` executes when the shared export is opened in Excel, LibreOffice, or Sheets. Prefix the standard apostrophe. - Markdown (export). Values were wrapped in backticks with no escaping, so a summary containing a backtick or pipe forged cells and rows. Also in the CLI: - explain sent the Gemini API key as `?key=`, where proxies, CDNs, access logs, and Referer all retain it. Use the x-goog-api-key header, validate the base URL's scheme, and refuse to send a key over plaintext http off loopback. - The interactive API-key prompt echoed the key into the scrollback. Mask it. - The model's reply is the exit of a prompt-injection path (the capsule shapes the prompt) and was printed raw; sanitize it too. - check walked directories with stat(), which follows symlinks, so one symlink cycle made `chronos check` — and `chronos doctor`, which calls it — recurse forever. Use lstat, skip links, cap depth. - sweep dynamic-imported (i.e. executed) any path, while replay and shrink both confine scenarios to cwd/CHRONOS_DIR. Apply the same confinement. - Bound `sweep <seeds>`; the seed list is materialized before the first run. - Serve frame-ancestors/X-Frame-Options, and stop the server CSP from intersecting away the webfonts index.html loads. - Drive every version string and the doctor's provider list from source rather than hardcoded 0.1.4/0.1.5 and NVIDIA_API_KEY, which explain never reads. Co-authored-by: Saim <contact@saimshafique.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
parseCapsule checked the trace envelope but nothing inside it, so the views — which destructure events by `kind` and index style tables with it — took the TraceEvent union on faith. Three ways a shared capsule could take down the UI: - An unknown `kind`, or a `partition` whose `groups` was missing or empty, threw mid-render and white-screened the app (spreading an empty array into Math.min/max also yields the ±Infinity that produces invalid SVG geometry). - 2M events were mapped to SVG markers with no virtualization. - Event `t` is unbounded, and both views size their SVG as `span × zoom`, so events at t=0 and t=1e12 asked the browser for a canvas ~4e13 px on a side. Validate the event union at the parse boundary (dropping bad events rather than discarding an otherwise readable trace), cap nodes at 256 to match the CLI, cap rendered events at 50k, and clamp the laid-out span. Both caps are surfaced in the summary strip — a silently partial timeline reads as a complete one. Also reject an oversized file before `file.text()` materializes it, and stop publishing source maps from the production build (they hand anyone the full unminified tree); CHRONOS_SOURCEMAP=1 keeps them for local debugging. Co-authored-by: Saim <contact@saimshafique.com>
- vercel.json served the hosted Inspector with no security headers at all, while the local `chronos open` server sent a full set. Add the matching CSP plus HSTS, X-Frame-Options, nosniff, Referrer-Policy, and Permissions-Policy. - The CI workflow declared no `permissions`, so its GITHUB_TOKEN inherited the repo default — write-all on many repos, which turns any dependency executing during install or test into push access. Pin to contents: read. - CI never ran `pnpm build`, so a tsup/vite break would only surface at publish. - .env.example advertised NVIDIA_API_KEY, which `chronos explain` never reads. Co-authored-by: Saim <contact@saimshafique.com>
52 new tests pinning the behavior each fix introduced, so a later refactor cannot quietly reopen any of them: - core: the guarded setTimeout refuses string handlers and compiles nothing; summarize distinguishes a shared reference from a real cycle and never throws on a payload JSON cannot represent; RealEnv.random stays in [0,1) and varies. - vitest: the trace envelope and all nine event kinds are validated; oversized files are refused before being read; a __proto__ key is rejected and does not reach Object.prototype. - cli: ANSI, OSC, CR/LF and bare-ESC payloads produce no control bytes on stdout and cannot forge trace lines; CSV formula leads are neutralized; Markdown cells cannot grow the table; a symlink cycle terminates the check walk; sweep refuses an out-of-tree scenario. - inspector: malformed events are dropped without discarding the trace, and event/node counts are capped and reported. - The open server sends frame-ancestors/X-Frame-Options and a CSP that does not intersect away the app's own webfonts. Two of these caught my own mistakes while being written (a CR formula lead is removed rather than prefixed; escaped pipes still split a naive column count), which is the point of writing them. Co-authored-by: Saim <contact@saimshafique.com>
Adds SECURITY.md, which writes down the assumption the whole hardening pass rests on: a failure capsule is untrusted input, because it is designed to be shared. Users need that stated to understand why capsule paths are confined and why the CLI escapes its own output. It also records the deliberate asymmetry between SimEnv.random (seeded, therefore predictable — never use it for a secret) and RealEnv.random (CSPRNG), and what is out of scope. Fixes references that had drifted from the code: the docs and Inspector chrome still said v0.0.0, and the CLI guide and README still described `chronos explain` as NVIDIA NIM with an NVIDIA_API_KEY, which it has not read for some time. PROGRESS/REMAINING now reflect the current 256-test state. Co-authored-by: Saim <contact@saimshafique.com>
…ripping The inspector may import @sx4im/chronos-core for TYPES ONLY — those are elided, which is why the build needs no node polyfills. Reading CHRONOS_VERSION from it imported a runtime VALUE, pulling core's whole barrel (and env.ts's node:async_hooks) into a browser bundle and failing `vite build`. Inject core's package.json version via a Vite define instead: same single source of truth, no runtime import. The `pnpm build` step added to CI in the previous commit is what surfaced this — typecheck alone did not. Also in sanitize.ts, replace the ad-hoc escape patterns with ECMA-48's actual byte ranges (CSI parameter 0x30-0x3F, intermediate 0x20-0x2F, final 0x40-0x7E). This catches sequences the old pattern missed — private-use forms like ESC[?25l and charset selection like ESC(B — and makes every quantified class disjoint from its neighbours, so no input can be split between two of them and the passes are provably linear on attacker-chosen text. Tests pin the C1 single-byte CSI (0x9B) and a pathological input's runtime. And bind the scheduler's macrotask barrier to the real setImmediate/setTimeout at module load. The strict guards replace globalThis.setTimeout with one that schedules into the simulator, so a barrier resolving through the patched global would enqueue its own resolution as a simulated event only this loop can run — a deadlock in the one function the scheduler awaits every step. Unreachable in Node today (setImmediate always exists), but it is the wrong thing to leave depending on that. Co-authored-by: Saim <contact@saimshafique.com>
… the README The docs site deploys from its own vercel.json, which had no headers at all. Also surfaces the capsule trust model and the SimEnv/RealEnv entropy asymmetry in the README, since 'never use env.random() for a secret in a simulated run' is the kind of thing a reader needs before they need it, not after. Co-authored-by: Saim <contact@saimshafique.com>
…pace leads A security review of the previous commits found the new escapeCsvCell was sidesteppable. Anchoring the formula test at the very first character is not enough, because a spreadsheet skips over things on its way to the `=`: ` =cmd|'/c calc'!A1`, or a zero-width space or BOM in front of it, all reached the cell unprefixed and still evaluated. Skip leading whitespace/BOM when testing for a formula lead. The zero-width variant pointed at a gap worth closing on its own: sanitizeText stripped C0/C1 controls but left the invisible and bidirectional-override characters, which are not controls in that sense but attack display integrity in exactly the same way an ANSI escape does. U+202E is Trojan Source (CVE-2021-42574) — in a capsule summary it makes `node-1 → node-2` render reversed. For a tool whose entire job is telling a human what happened, that is the injury rather than a side effect, so strip them alongside the escapes. Also from the review, two defense-in-depth items: - sanitizeText dropped the ESC introducer of DCS/SOS/PM/APC sequences but left their payload behind looking like ordinary trace text. Consume both, as the OSC pass already did. - The capsule reviver rejected __proto__ but not `constructor`/`prototype`; a legitimate capsule has no use for any of the three. - Add object-src 'none' to the open server's CSP, matching vercel.json. Co-authored-by: Saim <contact@saimshafique.com>
sx4im
marked this pull request as ready for review
August 9, 2026 09:13
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
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.
A full security and robustness audit of Chronos, following the vibe-security agent-skill methodology, with every finding fixed and pinned by a regression test. An independent security review of these changes then found one bypass in my own CSV fix, which is also fixed here.
The threat model that organizes most of this: a failure capsule is untrusted input. It is a JSON file designed to be shared — attached to a GitHub issue, produced by someone else's CI, handed to a teammate — and then fed to
chronos trace,chronos export,chronos explain, and the Inspector web UI. Validation stopped at the fields theSimulatorreads, and every renderer downstream trusted the rest.Critical / High
packages/core/src/strict.ts— code-execution sink in the strict-modesetTimeoutguard. The guard compiled string handlers withnew Function(String(handler)). Any string reaching a forgotten global timer — a simulated peer's message payload, a field of an untrusted capsule — became executed code. Node's ownsetTimeoutrejects string handlers, so the guard was adding an eval sink to the harness that the runtime it emulates does not even have. Now throws the sameTypeErrorNode throws.packages/cli/src/export.ts— CSV formula injection.escapeCSVquoted per RFC 4180 but left formula leads intact, so a capsule summary of=cmd|'/c calc'!A1executes when the exported CSV is opened in Excel, LibreOffice, or Google Sheets. The export exists to be shared, which is what makes this reachable. Cells are now prefixed — and the check skips leading whitespace and zero-width padding, because anchoring at the very first character alone was sidesteppable with a single space.packages/cli/src/{trace,stats,explain}.ts— terminal escape and bidi injection. Capsulesummary,detail, and node ids went straight to stdout. A crafted capsule could embed ANSI escapes to erase and repaint earlier output, or a U+202E bidi override (Trojan Source, CVE-2021-42574) to makenode-1 → node-2render reversed. A trace viewer that can be made to lie about what it found is worse than one that crashes.packages/vitest/src/capsule.ts— validation stopped short of the trace. A capsule that merely omittedtrace.nodescrashedchronos tracewith a rawTypeError; an event with an unknownkindor a non-arraygroupswhite-screened the Inspector. The trace envelope and all nine event kinds are now validated once at the trust boundary, so every renderer downstream can stay simple.Medium
explain.ts?key=, where proxies, CDNs, access logs, andRefererall retain it. Now anx-goog-api-keyheader, with base-URL scheme validation and a refusal to put a key on plaintexthttpoff loopback.ui.tscapsule.tsMAX_EVENTSis a post-parse bound, which is too late —readFileandJSON.parseboth materialize the whole file first.capsule.tswx, mode0600.capsule.ts__proto__,constructor, andprototypenow rejected at the parse boundary — the parsed config and trace are spread and merged downstream, where anyObject.assignwould trigger the setter.check.tsstat(), which follows symlinks, so one symlink cycle madechronos check— andchronos doctor, which calls it — recurse until the stack blew.sweep.tsreplayandshrinkboth confine scenarios to cwd/CHRONOS_DIR.real.tsMath.random()forenv.random()in the production adapter. Callers may reasonably use it for an id or a nonce, and V8's PRNG state is recoverable from observed output. Now CSPRNG-backed; nothing in production is replayed, so determinism is unaffected.server.tsframe-ancestors/X-Frame-Options, and the CSP intersected away the webfontsindex.htmlloads.span × zoomsized the SVG from an unboundedt).GITHUB_TOKENpermissions.Correctness bugs found along the way
TraceLogger.summarizemarked merely shared object references as[Circular]— it tracked every object seen rather than an ancestor stack — corrupting the summaries thatsend/deliverpairing keys on.{ a: x, b: x }is a very common message shape.globalThis.setTimeout, which the strict guards replace with one that schedules into the simulator. On the fallback path that is a deadlock in the one function the scheduler awaits every step. Now bound to the real primitives at module load.InvariantViolatedrendered asviolated at t=<detail>— the detail is not a time.CHRONOS_VERSIONwas0.0.0while packages shipped0.1.5, so every capsule claimed a version that never existed. Now pinned topackage.jsonby a test, and the CLI/Inspector/docs all read from it instead of five hardcoded copies.chronos --helpandchronos doctoradvertised AI providers (NVIDIA_API_KEY, OpenRouter, Groq, …) thatexplain.tsnever reads.runCandidateswallowed every throw as "did not reproduce", reporting a broken harness as "already minimal".pnpm buildto CI immediately caught a latent break: the Inspector must import core for types only, and reading a runtime value from it pullednode:async_hooksinto the browser bundle.Verification
pnpm typecheck,pnpm lint(zero warnings, down from 3),pnpm test, andpnpm buildall green.replayCapsulecompares the whole re-run event trace against a capsule committed before any of these changes, so nothing here perturbed scheduling, RNG draw order, or network delivery.chronos openserver was exercised end to end: security headers present, non-loopbackHost→ 403, path traversal contained, and a deliberately hostile capsule renders throughtraceandexportwith no escape bytes and a neutralized CSV formula.SECURITY.mdis new, and documents the threat model, the capsule trust boundary, the deliberateSimEnv/RealEnventropy asymmetry (seeded and predictable vs. CSPRNG), and what is out of scope.