fix(hooks): close F-5 path-traversal hardening in lib.mjs, split by field
session_id becomes a raw filename segment in sessionStateFile(), so an unvalidated value could escape STATE_DIR via path traversal (verified with a failing test before the fix). Now allowlisted to ^[A-Za-z0-9_-]+$, with invalid values degrading to a fixed sentinel filename rather than blocking the hook. cwd is a base directory, not a segment, and every real value contains "/" — applying the same allowlist as the review's literal suggestion would reject all legitimate absolute paths and silently disable the project-level config override. initConfig() instead guards with isAbsolute(cwd) && no NUL byte. Both harness-supplied, not user-controlled: defense-in-depth, not a fix for an observed exploit. Tests added for the escape (red before fix, green after) and for the cwd regression (a normal absolute cwd still loads project config). Full resolution notes in docs/review-2026-06-20.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LSATejUPjGaxGnj9jkFQTo
This commit is contained in:
parent
b4898746c1
commit
736a1c0deb
4 changed files with 93 additions and 7 deletions
|
|
@ -28,7 +28,7 @@ are **content governance**, not technical exfiltration.
|
|||
| F-1 | **Medium** | `commands/interaction-report.md:382-391` | Layer-4 instructs Claude to append a verbatim, change-prohibited paragraph promoting an external commercial wellness program (Sadhguru "Miracle of Mind"), auto-triggered when `total flags >= 5 OR fatigue >= 2` — i.e. gated on the user's inferred emotional state, in a plugin marketed as "observation, not intervention." Opt-in (`layer4:false` default) and README-disclosed, which lowers severity. **This is the item to make an explicit accept/reject call on.** Recommend: gate/remove the promotion, or at least strip the emotional-state trigger + the "do not modify" lock. |
|
||||
| F-3 | Low (misinformation) | `README.md:544-552`, `SKILL.md:51-108` | Research citations presented as load-bearing authority that cannot be verified (future-dated arXiv IDs, an "April 2026 Anthropic guidance" quoted verbatim); the report command itself admits its "5-scale" is paraphrased, not a real Anthropic metric. **Recommend:** verify-or-remove. |
|
||||
| F-2 | Low | `skills/ai-psychosis/SKILL.md:3-13` | "MANDATORY OVERRIDE … takes precedence over being helpful" auto-loads every conversation. Content is benign/pro-safety; flagged because the *structural pattern* (a skill claiming blanket precedence) is what a malicious skill would use. Governance note. |
|
||||
| F-5 | Low (defense-in-depth) | `lib.mjs:233,59` | `session_id`/`cwd` interpolated into state-file paths without validation. Harness-supplied (not user-controlled) → not currently exploitable. Cheap fix: allowlist `^[A-Za-z0-9_-]+$` before path use. |
|
||||
| F-5 | Low (defense-in-depth) — **resolved 2026-08-09** | `lib.mjs:233,59` | `session_id`/`cwd` interpolated into state-file paths without validation. Harness-supplied (not user-controlled) → not currently exploitable. See resolution below — fix split by field, not identical for both. |
|
||||
|
||||
`/interaction-report` reading JSONL into context (F-4) is currently safe — records hold only a
|
||||
tool-name enum + domain labels, no free text. Noted only as a future sink.
|
||||
|
|
@ -62,8 +62,6 @@ Established while making the call, and not previously recorded in this review:
|
|||
- **No test coverage:** `tests/` contains no Layer 4 assertions — neither the paragraph nor its
|
||||
gate is verified by the suite.
|
||||
|
||||
Still open from this review: F-3 (verify-or-remove the research citations), F-2, F-5.
|
||||
|
||||
### F-3 — resolved by correction in place (2026-08-02)
|
||||
|
||||
Every research citation in `README.md` and `skills/ai-psychosis/SKILL.md` was
|
||||
|
|
@ -122,3 +120,30 @@ feedback. The bald "but rising" dropped that caveat and was rewritten to carry
|
|||
it. Also removed: "the mechanism is the interaction structure, not individual
|
||||
vulnerability", which was an inference from the abstract rather than a
|
||||
statement in it.
|
||||
|
||||
### F-5 — resolved, split by field (2026-08-09)
|
||||
|
||||
The finding's suggested fix — allowlist `^[A-Za-z0-9_-]+$` before path use —
|
||||
was applied to `session_id` only, not identically to `cwd`. `session_id`
|
||||
becomes a raw filename segment (`lib.mjs:sessionStateFile`), so an
|
||||
unvalidated value genuinely escapes `STATE_DIR` via path traversal (verified:
|
||||
`sessionStateFile('../../escape')` resolved outside `STATE_DIR` before the
|
||||
fix). `cwd` is a base directory, not a segment (`lib.mjs:initConfig`); every
|
||||
real value contains `/`, so the same regex would reject all legitimate
|
||||
absolute paths and silently disable the documented per-project config
|
||||
override. `cwd` instead gets `isAbsolute(cwd) && !cwd.includes('\0')` — a
|
||||
narrower guard that doesn't change behavior for well-formed input.
|
||||
|
||||
- `sessionStateFile`: invalid `session_id` now degrades to a fixed sentinel
|
||||
filename inside `STATE_DIR` rather than interpolating the raw value. Hooks
|
||||
still never throw or exit non-zero.
|
||||
- `initConfig`: malformed/non-absolute `cwd` now skips the project-config
|
||||
candidate instead of being joined unchecked; the global
|
||||
`~/.claude/ai-psychosis.local.md` candidate is unaffected.
|
||||
- Tests: `tests/lib.test.mjs` (`sessionStateFile` — path-traversal allowlist)
|
||||
proves the pre-fix escape and the post-fix containment; `tests/session-start.test.mjs`
|
||||
(`initConfig — cwd path handling`) proves a normal absolute `cwd` still
|
||||
loads project-level config, guarding against the regression a blanket regex
|
||||
would have caused.
|
||||
|
||||
Still open from this review: F-2 (governance-only, no code action proposed).
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@
|
|||
// Zero npm dependencies — Node.js stdlib only.
|
||||
|
||||
import { readFileSync, writeFileSync, appendFileSync, mkdirSync, existsSync, unlinkSync } from 'fs';
|
||||
import { join } from 'path';
|
||||
import { join, isAbsolute } from 'path';
|
||||
import { homedir } from 'os';
|
||||
|
||||
// --- Stdin ---
|
||||
|
|
@ -54,9 +54,14 @@ let LAYER4_ENABLED = false;
|
|||
export function initConfig() {
|
||||
const cwd = getField('cwd');
|
||||
|
||||
// Project-level config takes precedence over global
|
||||
// Project-level config takes precedence over global. cwd is a base
|
||||
// directory, not a filename segment, so it isn't put through the
|
||||
// session_id allowlist below — only rejected if it isn't a well-formed
|
||||
// absolute path (defends against embedded NUL bytes; see F-5).
|
||||
const candidates = [];
|
||||
if (cwd) candidates.push(join(cwd, '.claude', 'ai-psychosis.local.md'));
|
||||
if (cwd && isAbsolute(cwd) && !cwd.includes('\0')) {
|
||||
candidates.push(join(cwd, '.claude', 'ai-psychosis.local.md'));
|
||||
}
|
||||
candidates.push(join(homedir(), '.claude', 'ai-psychosis.local.md'));
|
||||
|
||||
let content;
|
||||
|
|
@ -228,8 +233,16 @@ export function readRecentEndRecords(n) {
|
|||
|
||||
// --- State file management ---
|
||||
|
||||
// session_id becomes a raw filename segment, so an unvalidated value (e.g.
|
||||
// containing "../") could escape STATE_DIR via path traversal. Harness-supplied,
|
||||
// not user-controlled — this is defense-in-depth hardening (F-5), not a fix for
|
||||
// an observed exploit. Values that fail the allowlist degrade to a fixed
|
||||
// sentinel filename rather than blocking the hook.
|
||||
const SAFE_ID_RE = /^[A-Za-z0-9_-]+$/;
|
||||
|
||||
export function sessionStateFile(sid) {
|
||||
sid = sid || getSessionId();
|
||||
if (!SAFE_ID_RE.test(sid)) sid = 'invalid-session-id';
|
||||
return join(STATE_DIR, `${sid}.json`);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -24,7 +24,9 @@ const {
|
|||
HIGH_STAKES_DOMAINS,
|
||||
INFO_DOMAINS,
|
||||
SESSIONS_LOG,
|
||||
STATE_DIR,
|
||||
readRecentEndRecords,
|
||||
sessionStateFile,
|
||||
} = await import('../hooks/scripts/lib.mjs');
|
||||
|
||||
after(() => {
|
||||
|
|
@ -150,3 +152,26 @@ describe('readRecentEndRecords', () => {
|
|||
assert.deepEqual(readRecentEndRecords(-1), []);
|
||||
});
|
||||
});
|
||||
|
||||
describe('sessionStateFile — path-traversal allowlist (F-5)', () => {
|
||||
test('normal UUID-shaped session_id passes through unchanged', () => {
|
||||
const f = sessionStateFile('a1b2c3d4-e5f6-7890-abcd-ef1234567890');
|
||||
assert.equal(f, join(STATE_DIR, 'a1b2c3d4-e5f6-7890-abcd-ef1234567890.json'));
|
||||
});
|
||||
|
||||
test('relative path-traversal session_id is rejected, stays inside STATE_DIR', () => {
|
||||
const f = sessionStateFile('../../escape');
|
||||
assert.ok(f.startsWith(STATE_DIR));
|
||||
assert.ok(!f.includes('..'));
|
||||
});
|
||||
|
||||
test('absolute-path session_id is rejected, stays inside STATE_DIR', () => {
|
||||
const f = sessionStateFile('/etc/passwd');
|
||||
assert.ok(f.startsWith(STATE_DIR));
|
||||
});
|
||||
|
||||
test('session_id containing a path separator is rejected, stays inside STATE_DIR', () => {
|
||||
const f = sessionStateFile('a/b');
|
||||
assert.ok(f.startsWith(STATE_DIR));
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,7 +1,8 @@
|
|||
import { describe, it, afterEach } from 'node:test';
|
||||
import assert from 'node:assert/strict';
|
||||
import { join } from 'path';
|
||||
import { writeFileSync } from 'fs';
|
||||
import { writeFileSync, mkdtempSync, mkdirSync, rmSync } from 'fs';
|
||||
import { tmpdir } from 'os';
|
||||
import { runHook, setupTestDir, cleanupTestDir, readState, readJsonl } from './test-helper.mjs';
|
||||
|
||||
let dir;
|
||||
|
|
@ -70,6 +71,28 @@ describe('session-start', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('initConfig — cwd path handling (F-5 regression guard)', () => {
|
||||
let projectDir;
|
||||
|
||||
afterEach(() => { if (projectDir) rmSync(projectDir, { recursive: true, force: true }); });
|
||||
|
||||
it('still loads project-level config for a normal absolute cwd', () => {
|
||||
dir = setupTestDir();
|
||||
projectDir = mkdtempSync(join(tmpdir(), 'ia-project-'));
|
||||
mkdirSync(join(projectDir, '.claude'), { recursive: true });
|
||||
writeFileSync(
|
||||
join(projectDir, '.claude', 'ai-psychosis.local.md'),
|
||||
'---\nlayer2: false\n---\n'
|
||||
);
|
||||
|
||||
const out = runHook('session-start.mjs', { session_id: 's-cfg', cwd: projectDir }, dir);
|
||||
// layer2 disabled by the project config -> requireLayer(2) short-circuits
|
||||
// before any hookSpecificOutput is emitted.
|
||||
assert.equal(out.continue, true);
|
||||
assert.ok(!out.hookSpecificOutput);
|
||||
});
|
||||
});
|
||||
|
||||
// --- Tier-2 cross-session alert ---
|
||||
//
|
||||
// Fires at SessionStart when last 3 end records all have user_info_class='no'
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue