Repository navigation
fix: security and correctness fixes from #119 - #120
stealthwhizz wants to merge 18 commits into
Conversation
…ools Run git via execFileSync with an argument array instead of a shell string so commit messages are passed literally. skill_learner update/delete now validate skill_name as kebab-case (same rule as crystallize), delete uses git rm on the skill directory and commits only that path.
Session mode passes the token as an HTTP auth header through git's GIT_CONFIG_* environment instead of embedding it in the clone URL, runs all git commands with argv, validates the session branch name, redacts the token from error text, and always resets the remote to the clean URL.
…tories Validate names/ids used to build filesystem paths for plugin remove and install, deleteSchedule, updateScheduleMeta and deleteFlowDefinition, and assert the resolved path stays inside the expected directory. Also install git plugins into the directory named by --name / the agent.yaml key so they can be found by the loader, and pass "--" before the source URL in git clone.
If a pre_tool_use hook cannot start, exits non-zero or times out, the tool call is now blocked with the hook error as the reason. Hooks for other events keep their non-blocking behavior. Documented in Documentation.md.
…turns - query() no longer finishes the message stream on each agent_end, so multi-turn prompts deliver all turns. - temperature/maxTokens are passed to every LLM call through a stream function wrapper (pi-agent-core ignores them in initialState); topP, topK and stop sequences are added to the provider payload for known APIs, with a one-time warning where unsupported. - max_turns (SDK maxTurns or runtime.max_turns) is enforced by aborting the agent when a new turn would exceed the limit; the SDK emits a system message with subtype "max_turns". - The CLI now waits for the telemetry flush before exiting.
…andling - read: paginate on whole lines within the UTF-8 byte budget so the reported range and next offset are exact; an oversized single line is cut on a character boundary. - edit / sandbox edit: insert new_string literally (no $-pattern expansion). - declarative tools: ignore stdin EPIPE; a process killed by a signal is reported as a failure (also for the cli tool). - sandbox paths: ~ resolves to the sandbox home, not the host home. - telemetry: take tool.call_id from the toolCallId argument.
…ading - extends/dependencies: reuse an existing clone (fast-forward pull, ignoring network failure) instead of dropping it on the second run; validate dependency names; pass "--" before the URL in git clone. - model strings: only "@http(s)://..." is a base-URL override; unknown models raise "Unknown model provider:id". - a malformed knowledge index, skill frontmatter or task-tracker skill file is skipped with a warning instead of aborting agent load. - scheduler: chain timers so runAt dates beyond ~24.8 days don't fire immediately.
Add a pretest script so npm test works on a fresh checkout, and tests for git argv usage, skill name validation, session token handling, path confinement, fail-closed hooks, read pagination, edit replacement, SDK multi-turn/sampling/max_turns, clone reuse, model parsing, long timers and sandbox home resolution.
- js-yaml ^4.3.2, @modelcontextprotocol/sdk ^1.31.0 - OpenTelemetry SDK packages to 0.222.x / 2.11.x, instrumentation-undici 0.32 - node-cron ^4.6.0 (drops the vulnerable uuid dependency; ships its own types, so @types/node-cron is removed) - npm update + npm audit fix for transitive packages Remaining advisories come through @mariozechner/pi-ai (proxy-agent -> basic-ftp, @anthropic-ai/sdk) and need an upstream release.
…am lines as bytes Each provider now reads only its own key (OPENAI_API_KEY or ANTHROPIC_API_KEY, no cross fallback). The key is attached only when the base URL is the provider's official HTTPS host or the endpoint the user set in GITAGENT_MODEL_BASE_URL; a @baseurl in a model string alone no longer receives it. No Authorization header is sent without a key. The SSE reader buffers raw bytes and decodes complete lines only, so multi-byte characters split across chunks are no longer corrupted.
…the local server - Generate a random API token per launch (IRA_UI_TOKEN can pin one) and inject it into the served page; the page's fetch() adds it as X-Ira-Token. /api, /v1 and /raw reject requests without it (/v1 also accepts Authorization: Bearer; /raw GETs accept ?token= for <img>). - Validate Host (127.0.0.1:<port> or localhost:<port>) and reject any foreign Origin. - /api/chat is POST + JSON (the UI streams it via fetch); other state-changing or process-launching endpoints are POST only. - Model overrides from requests may only use a loopback @baseurl; the same check applies to agent create/update. - Agent create/update edit agent.yaml as a YAML value, so unedited fields (permissions, sandbox, ...) are kept and values are escaped. - Skill install: reject non-relative subpaths, confine the chosen root to the clone, and skip symlinks/special files while copying. - /raw serves regular files only.
…rocess trees - Child processes (hooks, declarative tools, MCP servers) start in their own process group; timeout/abort kills the group, not just sh. - hooks.yaml pre_tool_use hooks fail closed: start failure, non-zero exit or timeout (GITAGENT_HOOK_TIMEOUT_SECS, default 30s) denies the call. Stdin is written concurrently with reading stdout. Gate checks now observe the run's abort. - MCP: stdout is read on a dedicated thread and every request has a deadline (GITAGENT_MCP_TIMEOUT_SECS, default 120s; connect handshake GITAGENT_MCP_CONNECT_TIMEOUT_SECS, default 30s). A server that misses it is killed and respawned on the next call, so the connection lock is never held indefinitely. - Declarative tools write stdin concurrently with reading output.
Repo mode (--repo): - Refuse to reuse an existing --dir unless it is itself the top level of a git clone; git runs with GIT_CEILING_DIRECTORIES so it never walks up into an enclosing repository. An empty directory is cloned into. - The token is passed to git via GIT_CONFIG_* (http.<origin>.extraHeader) instead of the URL: it never reaches argv, the remote URL or .git/config. Credentials embedded in --repo URLs are moved out the same way, git error text is redacted, and reusing an old clone rewrites its remote to the clean URL. - A cloned repository is untrusted by default: its mcp.yaml, hooks.yaml, tools/*.yaml and .env are not used unless --trust-repo (or GITAGENT_TRUST_REPO=1) is given. Local agents are unchanged. run_agent: - Sub-agents inherit the parent's gates (permission rules, hooks) and permission mode, apply their own manifest's tools allow-list, permission rules and max_turns. run_agent counts as mutating, so plan mode blocks it unless allowed. Permissions: - path rules are evaluated on the normalized path relative to the agent dir (symlinked ancestors resolved); a path outside it never matches an allow rule. read/write/edit reject relative paths that climb out of the agent dir with '..'. - cli allow rules are word-prefix rules and never match commands with chaining, substitution or redirection; cli deny rules also match per segment with short flags compared as sets. Limits are documented. Also: the read tool refuses non-regular files and files over 32 MiB, cli runs in its own process group (timeout/abort kills its children), and query()/open_session()/session finalize run their blocking git and MCP work via block_in_place on multi-threaded runtimes.
A second prompt() while a run is in flight now gets an immediate "agent is busy" error turn instead of interleaving its messages into the shared transcript. Each turn has its own cancel token (prompt_with_cancel); QueryStream/TurnStream cancel only their own turn when dropped, so a late drop can't abort a newer turn.
…decompression - skill_learner and agent.yaml dependencies only accept simple names ([A-Za-z0-9._-], not . or ..), so they can't write or clone outside skills/ and .gitagent/deps/. - Document extraction caps each decompressed zip entry / PDF stream at 16 MiB and the document total at 64 MiB. - The UI file viewer doesn't load files over 32 MiB.
- rustls 0.23.43 -> 0.23.45 (and rustls-webpki) in both lockfiles. - lru is only pulled in through ratatui 0.29 (lru ^0.12), so ratatui goes to 0.30 and crossterm to 0.29 (the version ratatui 0.30 uses); lru resolves to 0.18.5. No source changes were needed. - The desktop lockfile also picks up this crate's new deps. glib 0.18 (desktop only) is pinned by Tauri/GTK and is not changed.
pre_tool_use hooks already fail closed. A hook can now set fail_open: true to mark itself advisory, so its failure is logged and the tool still runs. The default stays fail_open: false.
shreyas-lyzr
left a comment
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Reviewed against the full finding list from #119 and the verification report. Every blocking issue is fixed; security pass is clean; test coverage is real and adversarial. Approving.
What I checked
Session PAT leak (Critical): Token is now injected via GIT_CONFIG_COUNT/GIT_CONFIG_KEY_0/GIT_CONFIG_VALUE_0 env vars only, never in the clone URL, argv, or .git/config. Error messages pass through redactToken() which strips both the raw token and its base64 form. The remote URL is cleaned to bare https:// before any git call. Verified in src/session.ts.
Skill-learner path traversal (Critical): assertValidSkillName() enforces /^[a-z0-9]+(-[a-z0-9]+)*$/ at lines 99-103. Delete uses git rm -- skills/ (scoped path, not git add -A). Update follows the same guard. Tests in test/security-fixes.test.ts confirm that '../outside', '..', 'a/b', and 'Bad Name' are all rejected and that delete commits only the skill directory, leaving staged and untracked files untouched.
Plugin remove / deleteSchedule / deleteFlowDefinition traversal (High): src/safe-path.ts introduces isSafePathSegment and safeChildPath. Both require the name to match /^[A-Za-z0-9_][A-Za-z0-9._-]*$/ with no '..' substring, then resolve and verify the result is a direct child of the base with no additional sep. All three deletion paths use safeChildPath. plugin remove additionally validates the name before constructing either the local or installed path.
Pre_tool_use hooks fail closed (High): runHooks() now accepts { failClosed: true }. A hook that throws, exits non-zero, or times out hits the catch block and returns { action: 'block', reason: ... } when failClosed is true and hook.fail_open !== true. wrapToolWithHooks always passes failClosed: true. A 10 s SIGTERM timeout is wired. The EPIPE crash on child.stdin is suppressed with a no-op error listener. Verified in src/hooks.ts.
SDK channel.finish() called on every agent_end (High): Fixed at sdk.ts:507-509 — the comment explicitly says 'Do not finish the channel here: in multi-turn mode more prompts follow.' Channel is finished only after the prompt loop exits (line 530), or in the finally blocks at lines 555/567/607.
edit tool dollar-sign corruption (High): String replacement now uses a function replacer: original.replace(old_string, () => new_string), which bypasses JS's
read tool byte-budget line truncation (Medium): paginateLines now counts whole lines within the byte budget, never splitting mid-line, so the reported line range matches what was actually returned.
max_turns enforcement in CLI (Medium): index.ts:602-603 calls enforceMaxTurns(agent, manifest.runtime?.max_turns, ...), wiring the manifest value to the agent loop.
setTimeout overflow for long runAt schedules (Medium): schedule-runner.ts exports MAX_TIMEOUT_MS = 2_147_483_647 and the scheduleAt function chains timers via Math.min(remaining, MAX_TIMEOUT_MS), so delays beyond 24.8 days don't overflow. Test in security-fixes.test.ts covers this.
extends: parent lost on second run (Medium): The cached-clone path in syncGitRepo now falls through to the existing directory instead of returning { manifest, parentRules: '' } on clone failure. Verified via loadAgent test.
Declarative tool EPIPE crash (Medium): child.stdin.on('error', () => {}) added in hooks.ts. Same pattern in any other spawned child.
CLAUDE.md / AGENTS.md / CONTRIBUTING.md: None present; no convention violations to check.
Security pass
Debug leftovers: One console.log added in schedule-runner.ts (scheduler status lines, dimmed, not sensitive). All others are console.error for genuine error paths. No debugger hooks, no .only tests, no debug flags.
Secrets: No hardcoded credentials in the diff.
Dependencies (resolved versions from lockfile):
- js-yaml 4.3.2: no known advisory — fixes CVE-2025-23059 (the prototype-pollution vector in 4.1.x).
- @modelcontextprotocol/sdk 1.31.0 (manifest) → lockfile resolves 1.32.1: no known advisory.
- node-cron 4.6.0: drops the vulnerable uuid transitive dep; no known advisory.
- yaml 2.9.1 (lockfile resolved): no known advisory (GHSA-48c2-rrv3-qjmp affects <= 2.8.2, fixed in 2.8.3+).
- OpenTelemetry 0.222 / 2.11: no known advisories.
Remaining 7 vulnerabilities: all trace through @mariozechner/pi-ai → @anthropic-ai/sdk and proxy-agent → basic-ftp (GHSA-c475-qrg2-pj4r, quadratic RE DoS in Client.list()). The PR description correctly identifies this as a pi-ai upstream blocker and notes pi-ai 0.73.1 did not clear them. These are pre-existing, not introduced by this PR, and the fix path requires a pi-ai release.
No injection, SSRF, authz, or crypto findings in the changed code.
Testing
test/security-fixes.test.ts is adversarial: it attempts actual shell injection via $(touch sentinel), verifies ../outside and other traversal names are rejected with the right error substring, confirms scoped commits leave staging area intact, and covers redactToken, hooks fail-closed, safeChildPath, channel delivery, scheduleAt timer chaining, and session mode with a real local git repo. 98 tests claimed; structure matches the test file count. Tests import from dist/ via the pretest build step — no stale-build risk.
This branch has not been deployed
2 participants
Fixes #119
This PR fixes the security and correctness issues in #119 for both the TypeScript CLI and the Rust port (
ira), and upgrades the vulnerable dependencies.TypeScript
Security
.git/configand error messages. The token goes to git throughGIT_CONFIG_*env as anhttp.extraHeader, and errors are redacted. The remote is reset in afinallyblock.execFileSyncand argv, so commit messages are passed literally.skill_learnerupdate and delete validate the skill name. Delete commits only that skill's path instead ofgit add -A.deleteScheduleanddeleteFlowDefinitionreject names that would leave their directory.pre_tool_usehooks fail closed. A hook that crashes, exits non-zero or times out blocks the tool. A hook can setfail_open: trueto opt out; the default isfalse.Correctness
query()delivers every turn, not just the first.temperature,maxTokens,topP,topKandstopSequencesreach the model request.max_turnsis enforced in the CLI and SDK.readpages by whole lines within the byte budget, so no lines are skipped.edittreats$&,$$and similar sequences in the replacement text literally.extends:keeps the parent on later runs by reusing the cached clone.@parse correctly, and an unknown model ID gives a clear error.runAtdates more than 24.8 days out wait instead of firing at once.plugin install --nameinstalls a plugin that loads.~maps to the sandbox home, signal kills are reported as failures, telemetry records the real tool call id and is flushed on exit, and git calls have--before URLs.npm testbuilds first, so it works on a fresh checkout.Rust (
ira)GITAGENT_MODEL_BASE_URL. The UI refuses non-loopback@baseurloverrides.run_agentsub-agents inherit the parent's gates, permission mode, tool allow-list andmax_turns.--dirthat is not a repo root, keeps the token out of URLs and errors, and needs--trust-repobefore it runs a cloned repo'smcp.yaml, hooks, tools or.env.agent.yamlas YAML and keep the fields they don't touch.readand document extraction have size caps.Dependencies and CI
js-yaml4.3.2,@modelcontextprotocol/sdk1.32.1, OpenTelemetry 0.222 / 2.11,node-cron4 (drops the vulnerableuuid), plusnpm updateandnpm audit fix.npm auditgoes from 44 to 7.@mariozechner/pi-ai(basic-ftpand@anthropic-ai/sdk) and need a new pi-ai release. Upgrading to pi-ai 0.73.1 did not clear them (see chore: bump pi-agent-core/pi-ai to 0.73.1 and update supported model versions + outdated dependencies #98).rustls0.23.45, andlru0.18 throughratatui0.30.glibis pinned by Tauri and is unchanged.Behavior changes
pre_tool_usehooks now block the tool when they fail, unless a hook setsfail_open: true.max_turnsinagent.yamlis now enforced.iraUI: external/v1clients must sendAuthorization: Bearer <token>. SetIRA_UI_TOKENfor a fixed token.ira: a model@baseurlno longer gets an API key, and there is no fallback fromOPENAI_API_KEYtoANTHROPIC_API_KEY. Custom gateways needGITAGENT_MODEL_BASE_URL.ira --reponeeds--trust-repoto run a cloned repo's MCP servers, hooks, tools and.env.RepoOptionshas a newtrust_repofield.Testing
tsc --noEmitis clean andnpm testpasses 98/98 (76 before). The new tests intest/security-fixes.test.tsfail onmainand pass here.cargo buildhas no warnings andcargo testpasses 65/65. The desktop crate builds.max_turnsstops the loop.iraUI: requests with the token work. Requests without the token, or from a foreign Origin or Host, are refused.Follow-up
A second review found some smaller hardening items. They are not regressions and will come in a separate PR.