fix: security, correctness & robustness sweep across lib/commands#87
Merged
Conversation
Bug-review sweep findings, each verified against current main and covered by typecheck + tests. No behaviour change for legitimate inputs. Security: - pr-poster: guard writeFilesToWorktree against path traversal (../ escape) - scan-plugins: reject absolute / ".." plugin manifest skills paths - credentials-sync: atomic tmp+rename write of OAuth credentials - cwd-resolver: try/catch malformed repo-defaults.json instead of throwing Correctness: - skill-dependencies: parse YAML block-sequence requires_mcps (not just inline arrays) so block-form deps are no longer silently dropped (+ unit tests) - skill-linter: slugify R008 anchor refs to match heading slugs (no false positives on headings with & ( ) etc.) Robustness: - runtime-materializer: don't stub-rewrite .claude.json on a transient read error (EMFILE/EACCES) — only on ENOENT / parse error — to preserve auth state - telemetry-ingest, shared-profiles: atomic tmp+rename writes - shared-profiles: bound profile fetch with a 10s AbortSignal timeout - status: use homedir() fallback instead of literal "~" - skills, init: handle non-zero subprocess exits instead of crashing/ignoring
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
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
A bug-review sweep of
src/libandsrc/commandssurfaced via 4 read-only audit agents, then verified individually against currentmainand covered by typecheck + tests. Every change is a defensive guard or a correctness fix with no behaviour change for legitimate inputs.13 files, +131/−28. Independent review confirmed the 6 security/correctness fixes correct with no regressions.
Security
writeFilesToWorktreenow refuses paths that escape the worktree (../../.bashrc); theroot + sepboundary also blocks the${root}-evilsibling-prefix trick...-containingmanifest.skillsfrom an untrustedplugin.json(wasresolve(dir, manifest.skills)→ arbitrary-dir scan).rename) so a crash/concurrent reader never sees a half-written credentials file.repo-defaults.jsonis caught instead of throwing out ofresolveProfileForCwd.Correctness
parseExplicitDepsnow parses the YAML block-sequencerequires_mcps:form, not only inline arrays. Block-form deps were silently dropped → MCP-dependency starvation. Exported + 7 unit tests.&/(/).Robustness
syncMcpsIntoClaudeJsonno longer stub-rewrites.claude.jsonon a transient read error (EMFILE/EACCES); it rewrites only on ENOENT / parse error, preserving session/auth state.AbortSignal.timeout.homedir()fallback instead of the literal"~"(whichjoinnever expands).Test plan
bun run typecheck— cleanbun teston all touched suites — 226/226 pass, incl. 7 newskill-dependenciesparser testsNotes
Each fix was re-verified against current
main(the audit originally ran on a stale branch). One audit finding — apair-suggestions.tstypecheck break — was already fixed on main and is intentionally excluded. Three other audit findings were judged false-positives (inheritance depth-vs-width, an intentional cluster sort, a cosmetic regex) and intentionally not changed.🤖 Generated with Claude Code