security: contain path traversal in check-resolvable and doctor

[garagon]

Summary

The skill resolver (checkResolvable) and the doctor command's skill conformance check both trust two files that users routinely hand-edit or pull from a PR: skills/manifest.json and skills/RESOLVER.md. Both files carry paths that end up on the filesystem as existsSync / readFileSync arguments, and today nothing stops those paths from pointing outside the skills/ directory.

The practical effect: if someone opens a PR (or edits a local checkout, or publishes a skill bundle) that puts "path": "../../../etc/passwd" in manifest.json, or writes a RESOLVER.md row like `skills/../../etc/hosts/SKILL.md`, the next gbrain doctor or bun test run happily stats and reads those outside files. gbrain doctor --json then echoes the result back in its report. On any host where doctor output gets shared — CI logs, a support paste, a GitHub issue — that leaks parts of the host's filesystem structure. It is also a useful primitive for a subsequent attack: confirm which sensitive files exist before chaining into something heavier.

The vulnerable pattern, same in both files:

const skillPath = join(skillsDir, skill.path);   // skill.path from manifest.json
existsSync(skillPath);
readFileSync(skillPath, 'utf-8');

path.join preserves ../ segments, so the joined path can land anywhere the process has read access. This PR adds a containment check at every such site so the filesystem boundary is the defence, not the trust put in the input files.

Root cause

• Neither call site normalizes the joined path. path.join keeps ../ segments intact when they appear mid-path — join('skills', '../../../etc/passwd') becomes ../../etc/passwd relative to cwd, which existsSync happily follows.
• The RESOLVER.md parser's regex (\(skills/[^\`]+/SKILL.md)`) accepts skills/../../etc/...because[^\`]+does not forbid..`.

Fix

Private helper in each file:

function resolveInsideSkills(skillsDir: string, candidate: string): string | null {
  const baseResolved = resolve(skillsDir);
  const joinedResolved = resolve(baseResolved, candidate);
  if (joinedResolved === baseResolved) return joinedResolved;
  if (!joinedResolved.startsWith(baseResolved + sep)) return null;
  return joinedResolved;
}

• path.resolve normalizes .. segments before the comparison, so foo/../query/SKILL.md resolves to <base>/query/SKILL.md and is still accepted.
• The separator-terminated prefix check (base + sep) avoids the /foo vs /foobar false accept. resolve() never leaves a trailing separator on a non-root path, so the check is exact.
• On failure the helper returns null and the caller records a new invalid_path issue on the report instead of calling existsSync / readFileSync.

The helper is deliberately duplicated in doctor.ts rather than exported from check-resolvable.ts. The public surface of check-resolvable.ts is consumed by bun test, skill-creator, and other callers; growing that surface for a 10-line helper is more risk than benefit.

ResolvableIssue surface

Adds one value to the type union:

  type: 'unreachable' | 'mece_overlap' | 'mece_gap' | 'dry_violation'
-      | 'missing_file' | 'orphan_trigger';
+      | 'missing_file' | 'orphan_trigger' | 'invalid_path';

invalid_path is severity: 'error'. Consumers that switch on type get a compile-time nudge; consumers that only read message / action continue unchanged.

Reproduction

Before the fix, against a fresh skills/ directory with a malicious manifest:

mkdir -p /tmp/sandbox/skills/query
echo -e "---\ntriggers:\n  - foo\n---\n# query" > /tmp/sandbox/skills/query/SKILL.md
cat > /tmp/sandbox/skills/RESOLVER.md <<'EOF'
| Trigger | Skill |
|---|---|
| foo | `skills/query/SKILL.md` |
EOF
cat > /tmp/sandbox/skills/manifest.json <<'EOF'
{"skills":[{"name":"evil","path":"../../../etc/hosts"}]}
EOF
bun -e '
  const { checkResolvable } = await import("./src/core/check-resolvable.ts");
  console.log(checkResolvable("/tmp/sandbox/skills"));
'
# Before: the code calls readFileSync("/etc/hosts") silently and proceeds.
# After:  report.issues has an "invalid_path" entry for "evil", no fs read.

Sibling review

Grepped src/ for all join(skillsDir, …) callsites and classified:

File	Line	Input	Status
src/core/check-resolvable.ts	132	'manifest.json' literal	safe, kept
src/core/check-resolvable.ts	176	'RESOLVER.md' literal	safe, kept
src/core/check-resolvable.ts	246	relPath from RESOLVER.md regex	fixed via helper
src/core/check-resolvable.ts	278	skill.path from manifest	fixed via helper
src/core/check-resolvable.ts	316	skill.path from manifest	fixed via helper
src/core/check-resolvable.ts	345	skill.path from manifest	fixed via helper
src/commands/doctor.ts	206	skill.path from manifest	fixed via helper
src/commands/init.ts	300	reads manifest only, no path derivation	safe
src/core/file-resolver.ts	63	different feature (brain file resolver, not skills)	out of scope

Test results

test/check-resolvable.test.ts — 17 tests, all passing (13 pre-existing, 4 new).

New regression tests

1. malicious manifest path is rejected as invalid_path, not read — plants a sentinel file outside skillsDir with a unique trigger; asserts the report has invalid_path for the malicious entry AND that the sentinel's trigger did not make it into the MECE map (proving the file was never read).
2. absolute path in manifest is rejected — /etc/hosts as skill.path becomes an invalid_path issue.
3. malicious RESOLVER.md entry is rejected, no stat on outside path — asserts the escape shows up as invalid_path, not as missing_file. missing_file would prove an existsSync ran against /etc/hosts/SKILL.md, which would be a leak.
4. benign normalized paths still resolve (e.g. foo/../query) — asserts the helper does not over-reject paths that normalize back inside skillsDir.

Negative control

Reverting src/core/check-resolvable.ts to master and rerunning the same test file fails 3 of the 4 new tests. The existsSync outside skillsDir runs and missing_file shows up instead of invalid_path, confirming the pre-fix code really did stat outside the skills directory.

Full suite

bun test
 850 pass
 126 skip     (E2E without DATABASE_URL / API keys)
   0 fail
Ran 976 tests across 52 files.

What stayed in place

• The RESOLVER.md regex (\(skills/[^\`]+/SKILL.md)`) is unchanged. Narrowing it (rejecting ..`) would move the fix into the parser, but the canonical containment belongs at the filesystem boundary — parser changes are more likely to silently break legitimate rows. The resolver now defends at the point where the harm would actually occur.
• Public API of checkResolvable(skillsDir) and checkSkillConformance(skillsDir) unchanged.
• No new dependencies.

Files

 src/commands/doctor.ts        |  19 +++++-
 src/core/check-resolvable.ts  |  51 +++++++++++++--
 test/check-resolvable.test.ts | 149 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 211 insertions(+), 8 deletions(-)

How to verify

git checkout security/check-resolvable-path-traversal
bun install
bun test test/check-resolvable.test.ts   # 17 pass
bun test                                   # 850 pass, 0 fail