Repository navigation
fix: security and correctness fixes from #107 - #108
stealthwhizz wants to merge 15 commits into
Conversation
Add a shared path-safety helper that resolves a relative path inside a base directory (rejecting absolute paths, '..' escapes and symlinks that lead outside) and use it for always_load knowledge documents in every adapter. Documents outside knowledge/ are skipped with a warning. Also tolerate an empty knowledge/index.yaml instead of crashing.
…ent dir Dependency mount paths must now be relative paths strictly inside the agent directory; absolute paths, '..' escapes and the agent root itself are rejected. Dependency names are confined to .gitagent/deps. Local source/extends paths must resolve inside the agent directory unless the new --allow-external-sources flag is passed. A local source that already lives at its mount point is left in place instead of being removed and re-copied. Pass '--' before the URL in git clone.
- Confine hooks[].script to the hooks/ directory and shell-quote the script path in the generated hook command so paths with spaces work. - Pass system prompts larger than 100 KB via --append-system-prompt-file instead of argv to avoid E2BIG; smaller prompts are still inlined. - Validate session options before writing any temp files and always remove temp files on every return path.
Default user_id is now LYZR_USER_ID if set, otherwise a stable identifier derived from a SHA-256 hash of the key. --user-id still overrides.
Use execFileSync with argument arrays for all git and gh calls (the PR body is passed as a single argument), validate the agent name against ^[a-z][a-z0-9-]*$, parse and normalize the repository URL with URL(), and remove the temp clone with rmSync. Fixes submissions from paths containing spaces.
Invoke cmd.exe explicitly with every argument quoted and refuse arguments containing characters cmd.exe still interprets inside quotes. Set process.exitCode instead of calling process.exit so callers can clean up.
Create config directories with 0700 and credential files with 0600, merge the generated provider/profile into an existing OpenClaw or Nanobot config instead of replacing it, and abort with an error rather than overwriting a config file that cannot be parsed.
Previously only llm-wiki created --dir, so minimal/standard/full failed for a new directory. Unknown templates are rejected before anything is created.
- Create the target directory before importing. - Copy Claude skill directories recursively (skipping symlinks) instead of reading every entry as a file. - parseSections keeps text before the first heading and ignores headings inside fenced code blocks. - Accept CrewAI's top-level agents.yaml map as well as an agents: key, and sanitize sub-agent directory names.
Parent (extends) skills are now merged in memory and referenced from .gitagent/parent/skills instead of being copied into skills/.
Branch on whether the exported config has an agents map rather than on whether sub-agent directories exist, always set agents.main.workspace, and give every declared sub-agent a temp workspace.
Normalize line endings before matching frontmatter (also for Cursor .mdc files) and accept allowed-tools as either a space-delimited string or a YAML list.
- gitclaw runner copies hook scripts, knowledge documents and full skill directories into the temp workspace. - GitHubProvider validates owner/repo references, skill sub-paths and install directory names. - run/git runner: avoid process.exit inside try/finally, register temp clone cleanup on exit, and reuse the already-resolved clone for 'run -a git' instead of cloning twice. - opencode runner sets process.exitCode instead of exiting. - Pass '--' before repository URLs and paths in git invocations.
npm update + npm audit fix (no --force); npm audit reports 0 vulnerabilities.
shreyas-lyzr
left a comment
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Reviewed against the full finding list from #107 and the verification report. All six security findings and the functional bugs are fixed; test coverage is real and adversarial. Approving.
What I checked
Knowledge path traversal → LLM exfil (Critical): src/utils/path-safety.ts introduces resolveInside(), which resolves the path lexically, rejects absolute paths and '..' escapes, then follows symlinks (realpathSync on the deepest existing ancestor) and verifies the real target also stays inside the base. All 9 adapters now call loadKnowledgeDocument(knowledgeDir, doc.path) instead of join + readFileSync. The null guard on index?.documents also fixes the empty-index crash (finding #15). Verified in src/utils/path-safety.ts.
Install mount path traversal with rm -rf (Critical): src/commands/install.ts introduces resolveInstallTarget() which calls resolveInside(agentDir, dep.mount); an absolute path, '..', '.' or a path that resolves outside agentDir returns an error string and the install is aborted before any rmSync is called. Local source: / extends: paths outside the agent dir now require --allow-external-sources. Dependency names go through resolveInside(depsDir, dep.name) similarly.
Lyzr API key as user_id (High): resolveLyzrUserId() in src/runners/lyzr.ts: precedence is explicit userId arg, then LYZR_USER_ID env, then a sha256 hex digest of the API key (first 16 chars) prefixed 'gitagent-'. The raw API key is never sent as user_id. Verified in src/runners/lyzr.ts.
Registry shell injection (High): src/commands/registry.ts fully replaces all execSync template-string calls with execFileSync + argv arrays. Agent names are validated against /^[a-z][a-z0-9-]*$/ and repo URLs are parsed with new URL() and checked for https, github.com hostname, exactly two path segments, no auth/port/query/fragment. Verified in src/commands/registry.ts.
Gemini runner shell: true on Windows (High): shell: true is removed entirely. On non-Windows, spawnSync('gemini', args) is called directly. On Windows, cmd.exe is invoked explicitly via /d /s /c with windowsVerbatimArguments: true. Before reaching that path, every argument is checked by isSafeWindowsCmdArg() which rejects characters cmd.exe interprets inside quotes (", %, !, \n). Verified in src/runners/gemini.ts.
Credential files world-readable (High): src/utils/auth-provision.ts writes credential files with writeFileSync(path, ..., { mode: 0o600 }) and creates their parent directories with mkdirSync(dirname(path), { recursive: true, mode: 0o700 }). The temp prompt file in the Claude runner also uses mode 0o600. Verified at auth-provision.ts:41-42 and runners/claude.ts:138.
init -d ENOENT (correctness): init.ts:538 now calls mkdirSync(dir, { recursive: true }) before any createFile() call. Verified.
Claude runner E2BIG (correctness): System prompts > 100 KB are written to a temp file and passed via --append-system-prompt-file, bypassing ARG_MAX. Verified in src/runners/claude.ts.
Skill-loader CRLF and list-form allowed-tools (correctness): cursor.ts parseMdcFile now normalizes CRLF before the regex match. skill-loader.ts coerces allowed-tools to a string via Array.isArray(tools) ? tools.join(', ') : tools before calling .trim(). Both verified in the respective files.
CLAUDE.md / AGENTS.md / CONTRIBUTING.md: None present; no convention violations to check.
Security pass
Debug leftovers: No console.log, debugger hooks, .only tests, or debug flags added in the diff.
Secrets: No hardcoded credentials in the diff.
Dependencies (resolved versions from lockfile):
- js-yaml 4.3.2: no known advisory — fixes CVE-2025-23059.
- ajv 8.20.0 (lockfile resolved): no known advisory (GHSA-2g4f-4pwh-qvx6 affects 8.17.1, fixed before 8.20.0).
- fast-uri 3.1.8: no known advisory.
- npm audit: 0 vulnerabilities.
No injection, SSRF, authz, or crypto findings in the changed code.
Testing
src/utils/path-safety.test.ts tests isPathInside, resolveInside, and loadKnowledgeDocument with actual symlinks pointing outside the base, and then runs all 9 adapters against an agent with a traversal path and a symlink escape in knowledge/index.yaml, asserting the in-tree marker is included and the outside/symlink markers are not. The install tests cover mount traversal, external-source rejection, and valid installs. The registry tests cover name and URL validation. 104 tests claimed; test file structure is consistent with that count.
Fixes #107
This PR fixes the security and correctness issues in #107 and upgrades the vulnerable dependencies.
Security
knowledge/, with symlinks resolved. A sharedresolveInsidehelper is used by all 9 adapters. Claude runner hook scripts are confined tohooks/the same way.install: amountmust be a relative path inside the agent dir, and absolute paths,..and.are refused. Dependency names stay inside.gitagent/deps. Localsource:andextends:paths outside the agent dir need the new--allow-external-sourcesflag.user_id: the API key is never used asuser_id. The order is--user-id, thenLYZR_USER_ID, then a non-reversible id derived from a SHA-256 hash of the key.registry: git and gh run throughexecFileSyncwith argv. Agent names must match^[a-z][a-z0-9-]*$, and the repo URL is parsed and checked (https, github.com, owner/repo only).shell: trueis removed. On Windows it callscmd.exewith quoted arguments and refuses arguments that cmd.exe can't pass safely.Correctness
--append-system-prompt-file, which avoidsE2BIG.initandimport:init -dandimportcreate the target dir.import:parseSectionskeeps the preamble and respects code fences.export -f claude-code: it no longer writes into the repo. Parent skills are merged in memory and referenced at.gitagent/parent/skills/....SKILL.mdfiles load, andallowed-toolsaccepts a string or a list.knowledge/index.yamlno longer crashes the adapters.process.exitinsidetry/finally.run -a gitdoesn't clone twice.--before URLs.Dependencies and CI
js-yaml4.3.2,ajv8.20.0,fast-uri3.1.8.npm auditreports 0 vulnerabilities.Behavior changes
install: local sources orextendsoutside the agent dir need--allow-external-sources. A mount that is absolute, escapes the agent dir or equals its root is refused.user_idchanged, so old sessions keyed by the raw key won't match new ones.registryrefuses agent names that aren't lowercase kebab-case, and repo URLs with extra path segments, query strings or fragments.",%,!or a line break.export -f claude-codepoints inherited skills at.gitagent/parent/skills/..., so a fresh clone needsopengap installfirst.Testing
tsc --noEmitis clean, and the test suite passes 104/104 (51 before). The new init, skill-loader and adapter tests fail onmainand pass here.initfor every template.validate.exportin all 15 formats for 13 agents (195 outputs). The output is byte-identical tomain, and no agent dir is modified.importfrom 6 tools.installwith good and bad entries.runwith stubclaudeandgeminibinaries.Follow-up
A second review found some smaller hardening items. They are not regressions and will come in a separate PR.