security: validate operation names in FailImproveLoop

[garagon]

Summary

FailImproveLoop is the new deterministic-first / LLM-fallback helper added in v0.10. Every call passes an operation name (e.g. "extract_mrr", "rotation_test") that the class uses directly as a filesystem segment under ~/.gbrain/fail-improve/ — one JSONL file per operation, one counts file per operation, one improvements.json per operation dir.

The method signatures take plain string for operation and do no validation. Every current in-tree caller happens to pass a hard-coded identifier, but FailImproveLoop is exported and the first consumer that forwards a request field — a recipe name, a webhook slug, a user-typed label — turns operation into an arbitrary filesystem-write primitive. Because path.join preserves ../, a value like "../../../../../tmp/owned" lands outside ~/.gbrain/fail-improve/ entirely: anywhere the gbrain process has write permission. On a dev box that's $HOME; on a CI runner that can be /tmp or worse.

This isn't a live exploit today — no caller passes user input there yet — but it's a latent gap one PR away from mattering, in code that exists specifically to chain into ingest pipelines (skills/data-research, skills/meeting-ingestion) which do take external input.

The vulnerable shape repeats across four internal sites:

// src/core/fail-improve.ts  (trimmed)
private getLogPath(operation: string): string {
  return join(this.logDir, `${operation}.jsonl`);
}
private getCallCountPath(operation: string): string {
  return join(this.logDir, `${operation}.counts.json`);
}
logImprovement(operation: string, description: string): void {
  const filePath = join(this.logDir, operation, 'improvements.json');
  /* ensureDir + writeFileSync */
}
private getImprovements(operation: string): Array<…> {
  const filePath = join(this.logDir, operation, 'improvements.json');
  /* readFileSync */
}

This PR closes the door at the boundary, not at the caller — one validator runs before any path is constructed.

Fix

A private assertSafeOperation that accepts identifier-shaped names in any script:

//   \p{L} — letter (any script)
//   \p{N} — number (any script)
//   \p{M} — combining mark (Arabic fatha/shadda, Hebrew niqqud,
//           Devanagari matras, Thai tone marks)
const VALID_OPERATION = /^[\p{L}\p{N}][\p{L}\p{N}\p{M}_-]{0,63}$/u;

function assertSafeOperation(operation: string): void {
  if (typeof operation !== 'string' || !VALID_OPERATION.test(operation)) {
    throw new Error(
      `FailImproveLoop: invalid operation name '${operation}'. ` +
      `Must be 1-64 characters, starting with a Unicode letter or digit, ` +
      `containing only letters, digits, '_' or '-'. No path separators, ` +
      `no parent-dir segments, no leading dot, no whitespace.`
    );
  }
}

Applied at the four internal sites and at logImprovement (public method). Every in-tree caller ("extract_mrr", "rotation_test", etc.) passes, and so do names like 田中-enrich, иван_stats, 抽出_mrr, مَهَمَّة, mañana — which matters because gbrain skills enrich entity pages whose slugs are derived from user text in any language.

Why Unicode letter/number/mark classes, not ASCII-only

An ASCII charset would reject every legitimate non-Latin name. \p{L}\p{N}\p{M} is the tight union that

• includes every letter + digit + combining mark in every script Unicode knows about (CJK, Cyrillic, Arabic, Hebrew, Devanagari, Thai, Greek, …), AND
• still excludes \p{Z} (whitespace), \p{P} (punctuation other than the whitelisted _ and -), and \p{C} (controls + format characters — U+202E RTL override, U+200C/D ZW[N]J, U+0000 NUL). The attack shapes stay closed.

Why a regex allow-list, not path.basename

• basename("../../../tmp/owned") returns "owned" — silently rewrites the caller's intent. A future reader of the code would have no hint the input was hostile.
• path.resolve + prefix check works (that's what we used for R6-F007 on check-resolvable.ts), but there it made sense because the untrusted value is a full relative path. Here operation is semantically an identifier — it's never supposed to have slashes at all. The regex matches the actual shape.

Reproduction

Before the fix, from a hypothetical future caller that forwards user input:

import { FailImproveLoop } from './src/core/fail-improve';
const loop = new FailImproveLoop();          // writes under ~/.gbrain/fail-improve/
await loop.execute(
  '../../../../../tmp/owned',                // <= user-controlled value
  'payload',
  () => null,
  async () => 'llm',
);
// After: /tmp/owned.jsonl exists. The process wrote a JSONL file
// outside logDir because path.join normalized the `..` segments.

Sibling review

Grepped src/ for every callsite that forwards a caller-supplied string into path.join under an app-owned directory:

File	Line	Input	Status
src/core/fail-improve.ts	getLogPath	operation	fixed
src/core/fail-improve.ts	getCallCountPath	operation	fixed
src/core/fail-improve.ts	logImprovement	operation	fixed
src/core/fail-improve.ts	getImprovements	operation	fixed
src/core/check-resolvable.ts	various	skill.path	covered in #156
src/commands/doctor.ts:206	conformance	skill.path	covered in #156

No other in-tree callers of FailImproveLoop — today it's only exercised by its test file, so no consumer needs to be updated.

Test results

test/fail-improve.test.ts — 31 tests, all passing (15 pre-existing, 16 new).

New regression tests

1. Table-driven rejects (17 payloads) — each BAD operation name throws /invalid operation name/. Covers the ASCII attack surface (../../../tmp/owned, ../owned, /tmp/owned, a/b, a\b, ok\0evil, .hidden, "", ..., 65-char overflow) AND the non-ASCII one (田中/.., ../田中, مَهَمَّة\0x, .иван, 🚀_op, ok\u202eevil RTL override, has space). Confirms that adding Unicode support did not open a hole.
2. Path-traversal sentinel test — uses a unique-per-run sentinel payload (../gbrain-escape-${Date.now()}-${rand}), asserts execute() throws AND no sibling file was ever created next to tempDir. Robust against leftover tmp pollution from prior runs.
3. Benign names still work (14 values) — 6 ASCII (extract_mrr, rotation_test, op-1, ABC_123, a, 64-char max) + 8 non-ASCII (田中-enrich, extract_タスク, иван_stats, 抽出_mrr, مَهَمَّة, mañana, 任務1, Δ_delta). Each returns a result and places the JSONL file inside logDir.
4. logImprovement coverage — separate check because it has its own operation parameter path.

Negative control

Reverting src/core/fail-improve.ts to master and rerunning the same test file fails 8 of the 46 tests — every injected traversal / null-byte / spoof payload goes through, confirming the pre-fix code really did write outside logDir and accept format characters.

Full suite

bun test
 874 pass
 131 skip     (E2E without DATABASE_URL / API keys)
   0 fail
Ran 1005 tests across 52 files.

What stayed in place

• Public API: FailImproveLoop.execute(), logImprovement(), and every accessor keep the same signatures. The only behaviour change is a thrown Error on invalid input — which is the intended outcome for the hostile case and the reason no valid-path caller hits it.
• The MAX_ENTRIES = 1000 rotation, JSONL format, and directory layout are unchanged.
• No new dependency. One file touched in src/, one file touched in test/.

Files

 src/core/fail-improve.ts  |  39 +++++++++++++++++++++++
 test/fail-improve.test.ts | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 153 insertions(+)

How to verify

git checkout security/fail-improve-path-traversal
bun install
bun test test/fail-improve.test.ts   # 46 pass
bun test                              # 874 pass, 0 fail