Repository navigation
fix(cli): sanitize parse errors, validate flags, safe writes, no-network CI check - #24
Merged
Merged
Conversation
publish.yml previously ran npm install -g npm@latest (unpinned mutable tag) then npm install (ignoring the committed lockfile) in a job that holds id-token: write immediately before npm publish --provenance. A compromised transitive dependency resolved fresh at publish time would get a valid provenance attestation. Pins the npm CLI to the version the file's own comment already names (11.5.1), switches both publish.yml and ci.yml to npm ci --ignore-scripts against the audited lockfile, and adds/moves npm audit --audit-level=high to run immediately after install rather than last (or not at all, in publish.yml's case). Deliberately not doing in this branch: SHA-pinning actions/checkout, actions/setup-node, and codeql-action, or adding an environment approval gate. Both are sound hygiene but defend against GitHub/npm Inc. itself being compromised, lower urgency than the lockfile-bypass issue above for a single-maintainer repo, and the environment gate needs an actual GitHub Environment created first. Verified locally end to end: npm ci --ignore-scripts (194 packages, 0 vulnerabilities), lint, typecheck, coverage (24 tests), build, demo all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The first version of this branch failed real CI: npm ci --ignore-scripts on ubuntu-latest left @esbuild/linux-x64 unresolved (vite/vitest/tsx all depend on esbuild transitively), because --ignore-scripts also disables esbuild's postinstall fallback that resolves the platform binary when npm's own optionalDependencies resolution doesn't land it. Local verification on Windows didn't catch this, esbuild resolved fine there, which is exactly why the real Linux CI run is the actual check, not a local pass. Keeping npm ci (the real fix: pins to the audited lockfile instead of re-resolving ^ ranges) and npm install -g npm@11.5.1 --ignore-scripts (installing the npm CLI binary itself has no legitimate need for scripts and isn't affected by this). Dropping --ignore-scripts only from the npm ci steps. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ork CI check
Non-breaking hardening pass. Severities re-checked against an adversarial
pass before writing this up, several of the review's original framings
were softer than stated (documented per-item below and in the PR).
- cli.ts parseResponses: unified the array/object/JSONL branching into
try-whole-document-first, fall back to JSONL on failure. Fixes two
bugs in one rewrite: a pretty-printed single JSON object was
previously misrouted to the JSONL parser (a bare newline check), and
JSONL line numbers were wrong when blank lines were present (index
captured after .filter). The JSONL fallback's catch discards the
native SyntaxError entirely rather than surfacing it, closing the
path where a malformed payload's V8 error (which can embed a
fragment of the input around the syntax error) reached stderr
verbatim.
- cli.ts parseArgs: a flag's value must not itself look like another
flag (reject a next token starting with "-"), and a "--" terminator
forces everything after it to be treated as the positional source
argument. Closes the argv[++i] confused-deputy path where
--model --out victim.json silently consumed --out's name as
--model's value; the prior behavior only silently mis-normalized
data when a file happened to already exist at the swallowed path,
otherwise it already failed loudly with ENOENT.
- cli.ts write path: { flag: "wx" } plus an explicit --force flag, so
--out refuses to silently overwrite an existing file. The write is
now wrapped and returns exit code 2 on any failure, previously an
unwritable --out threw out of run() and exited 1, the same code
documented for "some records failed to normalize", so a pipeline
reading exit codes couldn't tell total write failure from partial
parse failure.
- cli.ts: added a statSync size cap (256MB) before reading the input
file. Framed as a capacity ceiling for the operator's own input, not
a security boundary, this CLI has no untrusted-input trust boundary.
- adapters.ts, normalize.ts: no functional change here (see the two
fix/* branches for that), just consistency with this branch's
Object.hasOwn preference is left to those branches since they own
that code in this release.
- SECURITY.md: corrected "does not parse or retain prompt or
completion content" (JSON.parse necessarily deserializes the whole
payload) to describe what's actually true: content isn't copied into
the output or written to disk, and --out writes wherever it's
pointed. Scoped the "no network calls" claim to the current version
and backed it with scripts/check-no-network.mjs, now a CI step, so
the claim fails loudly instead of quietly rotting.
Verified locally: npm ci, lint, typecheck, coverage (31/31 tests),
build, demo, check:no-network, audit --audit-level=high all pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mizcausevic-dev
added a commit
that referenced
this pull request
Sep 13, 2026
…or-changes docs(changelog): disclose #24's CLI behavior changes in the 0.3.0 entry
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.
Summary
Branch 3 of the review's fix plan. Non-breaking. Based on top of #21 (Branch 0, CI hardening), so this diff includes those commits too until #21 merges; the actual new content here is
src/cli.ts,SECURITY.md,scripts/check-no-network.mjs, and the CLI test additions.Real, but several items landed softer than the review's original framing after an adversarial re-check, documented per item:
parseResponsesrewrite: try the whole document as JSON first, fall back to JSONL on failure. Fixes two bugs at once: a pretty-printed single object used to be misrouted to the JSONL parser (a bare newline check), and JSONL line numbers were wrong when blank lines were present. The JSONL fallback discards the nativeSyntaxErrorentirely rather than surfacing it, this is the actual fix for the payload-leak path, a malformed input can make V8's own error message embed a fragment of the source around the syntax break.parseArgs: a flag's value can no longer itself look like another flag (rejects a next token starting with-), plus a--terminator for a source path that itself starts with-. This closes the--model --out victim.jsonconfused-deputy path, worth noting the failure mode is a latent footgun rather than guaranteed corruption: it only silently normalizes the wrong file when something already exists at the swallowed path, otherwise it already failed loudly withENOENT.--outwrites:{ flag: "wx" }plus an explicit--force, so it refuses to silently overwrite. Wrapped and returns exit code 2 on any write failure, previously indistinguishable from exit code 1 ("some records failed to normalize").statSynccap before reading the input file, framed as a capacity ceiling for the operator's own input, not a security boundary. This CLI has no untrusted-input trust model, the invoker already controls every flag including the file path.SECURITY.md: corrected "does not parse or retain prompt or completion content" (JSON.parsenecessarily deserializes the whole payload) to what's actually true, content isn't copied into the output or written to disk, and--outwrites wherever it's pointed. Scoped "no network calls" to the current version and backed it withscripts/check-no-network.mjs, now wired into CI, so that claim fails loudly instead of quietly rotting the next time a dependency changes.Test plan
npm cinpm run lintnpm run typechecknpm run coverage— 31/31 testsnpm run buildnpm run demonpm run check:no-networknpm audit --audit-level=high🤖 Generated with Claude Code