fix(sessions): validate persisted allowedTools shape before hydration (lr-8b2e)#305
Conversation
…ta (lr-8b2e)
session.allowedTools (Allow-for-session tool grants) previously lived only
in the in-memory session object. saveSessionFile() now writes it into the
per-session meta record when non-empty, and loadSessions()/resumeSession()
hydrate it back on rebuild instead of always defaulting to {}. Grants that
were never given still round-trip to {}, not a stored empty object.
…me (lr-8b2e) - allow_always decision handler now calls saveSessionFile() right after setting session.allowedTools so the grant survives daemon restart / resume-by-cliSessionId rehydration even without a later save trigger. - resume_session WS handler reads persisted allowedTools from the meta record (same pattern already used for vendor) and threads it through to resumeSession()'s opts.
…istence
Covers: grant survives loadSessions() rehydration and the auto-approve
early-return at sdk-bridge.js:691 hits; an ungranted tool still prompts;
saveSessionFile omits allowedTools when nothing was ever granted;
resumeSession() hydrates allowedTools from opts and defaults to {} when
none is supplied.
![clagentic-reviewer[bot]](https://avatars.githubusercontent.com/in/3944489?s=60&v=4){kind=small}
There was a problem hiding this comment.
PEACHES — clean
Reviewed PR #305: fix(sessions) persist Allow-for-session permission grants across rehydration (lr-8b2e).
Summary: The fix correctly serializes previously-granted tool permissions into the durable session meta record, restores them during rehydration (loadSessions and resumeSession), and flushes immediately after grant assignment. Three-part repair with full regression test coverage.
Validation:
- Persist (lib/sessions.js:71): conditional write only when allowedTools is non-empty; prevents empty object round-trip pollution.
- Restore (lib/sessions.js:298, 842): both paths default to
{}when persisted state absent; no invented permissions. - Thread (lib/project-sessions.js:385, 410, 413): allowedTools read from session file and passed through resumeSession opts in both success and catch branches.
- Flush (lib/project-sessions.js:1253): saveSessionFile called immediately after grant write; survives daemon restart.
- Auto-approve gate (lib/sdk-bridge.js:691): unchanged; still checks only persisted grants, never invents.
- Test suite: 5 regression tests exercise all three fix points against production code paths, including idempotency (empty grants omitted from schema) and breadth (unganted tools still prompt).
No blocking findings.
![clagentic-security[bot]](https://avatars.githubusercontent.com/in/4134208?s=60&v=4){kind=small}
There was a problem hiding this comment.
BOBBIE — clean
Audit scope: PR #305 (fix/lr-8b2e-persist-permission-grant), files lib/project-sessions.js, lib/sessions.js, test/permission-grant-persist-lr-8b2e.test.js.
Focused review of permission-grant persistence semantics per the four audit questions:
-
No widening of auto-approve scope. The grant write (
session.allowedTools[pending.toolName] = truein lib/project-sessions.js:1248) and the auto-approve read (exact-matchsession.allowedTools[toolName]at lib/sdk-bridge.js:691, unmodified by this diff) are unchanged in shape — no wildcarding, no tool-name coercion. The PR only adds an immediatesm.saveSessionFile(session)flush after the existing grant write (lib/project-sessions.js:1253). -
lib/sessions.js:298 —
nit—allowedTools: m.allowedTools || {}and lib/project-sessions.js:385/410/413 readmetaObj.allowedToolsstraight from the persisted.jsonlmeta record with no shape/type validation before assigning it into the livesession.allowedToolsauto-approve table. A crafted meta line ({"type":"meta","cliSessionId":"x","allowedTools":{"Bash":true}}) would silently auto-approve that tool with no operator click. This extends the existing (pre-PR) trust boundary of the session.jsonlstore — an attacker able to write that file already controls history/vendor/ownerId — so it is not a new privilege boundary, but it is a new lever on that boundary. Recommend validatingallowedToolsis a plain object with boolean-true values only before hydration, but not blocking since it does not cross a new trust boundary introduced by this PR. -
No cross-session/owner grant bleed found.
resumeSession()'s existing-session branch (lib/sessions.js:817-826) returns the live in-memory session unchanged and does not re-applyopts.allowedTools; grants are always keyed 1:1 to a singlecliSessionId's own meta record. -
Scanners: gitleaks hit is a private key in stray
.claude/worktrees/*/lib/certs/privkey.pemfiles, unrelated to this diff's changed files. semgrep hit (unsafe-formatstring) is at lib/project-sessions.js:664, outside this PR's diff hunks (371-417, 1246-1253). osv-scanner findings are pre-existing package-lock.json vulnerabilities; this PR does not touch package-lock.json. None attributable to lr-8b2e.
scanners_run: gitleaks (ok, no in-scope findings), semgrep (ok, no in-scope findings), osv-scanner (ok, no in-scope findings — lockfile unchanged by this PR).
review.status: clean
… (lr-8b2e) BOBBIE nit on PR #305: metaObj.allowedTools was read from the persisted .jsonl meta record and hydrated directly into the live auto-approve table with no shape/type validation. A crafted record (e.g. {"allowedTools":{"Bash":true-like-non-boolean}}) could silently auto-approve a tool with no operator click. Adds a shared sanitizeAllowedTools() helper in lib/utils.js that keeps only string-keyed entries whose value is strictly boolean true, dropping malformed entries and defaulting non-object input to {}. Applied at every hydration site BOBBIE named: lib/sessions.js (loadSessions, resumeSession) and lib/project-sessions.js (persistedAllowedTools assignment feeding resume_session's opts). Persist path (saveSessionFile) and the auto-approve read at lib/sdk-bridge.js:691 are unchanged — a legitimately-granted tool still round-trips and auto-approves exactly as before.
clagentic-builder
Bot
changed the title
There was a problem hiding this comment.
PEACHES — clean (0 blocking)
Hardening delta review: sanitizeAllowedTools() placement, correctness, test coverage
The re-review scope validates three aspects of the injection hardening:
-
Helper placement — sanitizeAllowedTools() is correctly homed in lib/utils.js alongside encodeCwd() and resolveEncodedFile(). No duplication across hydration sites.
-
Correctness & defense-in-depth — The sanitizer is applied at all three critical entry points before the auto-approve check (lib/sdk-bridge.js:691):
- lib/sessions.js:300 (loadSessions)
- lib/sessions.js:846 (resumeSession)
- lib/project-sessions.js:390 (resume_session handler, parsing persisted meta)
Logic correctly drops non-string keys and non-true values; gracefully handles null/undefined/arrays by returning {} rather than throwing.
-
Test coverage — New test file (permission-grant-persist-lr-8b2e.test.js, 379 lines) covers:
- Persistence round-trip: granted tool survives rehydration and auto-approves (lines 87–96)
- Ungrant isolation: tools never granted still prompt after rehydration (lines 102–125)
- Empty-grant omission: sessions with no grants do not round-trip {} (lines 131–163)
- Injection hardening regressions:
- Non-boolean values rejected at loadSessions (line 217–257, asserts Edit remains ungrant)
- Non-object allowedTools (string) hydrates safely as {} (line 263–289)
- Direct sanitizer unit tests (line 295–304)
- resumeSession defense-in-depth sanitizes opts.allowedTools (line 310–319)
Code-craft: minimal change, no test modifications, lr-8b2e task cited, no new deps.
No findings. Hardening is surgical and well-tested.
There was a problem hiding this comment.
BOBBIE — clean
Re-audit of PR #305 at head b58a434, following hardening commit b58a434 (fix(sessions): validate persisted allowedTools shape before hydration, lr-8b2e).
Prior nit (unsanitized allowedTools hydration) is closed. Verified against the live head, not just the diff:
- Sanitizer implementation (lib/utils.js:72-80, sanitizeAllowedTools): rejects non-object input, arrays (Array.isArray guard), and null/undefined by returning {} before iterating. Keeps only string keys whose value is strictly === true (no coercion of "yes", 1, {}, [] etc). Uses for...in + Object.prototype.hasOwnProperty.call guard.
- Applied at every hydration site — confirmed no unsanitized path remains:
- lib/sessions.js:300 (loadSessions, meta.allowedTools -> session.allowedTools)
- lib/sessions.js:846 (resumeSession, opts.allowedTools -> session.allowedTools)
- lib/project-sessions.js:390 (resume_session WS handler reads persisted meta.allowedTools before passing into resumeSession's opts)
All three call utils.sanitizeAllowedTools() before the value reaches session.allowedTools. The only sink that reads session.allowedTools for auto-approve is lib/sdk-bridge.js:691 (session.allowedTools[toolName]) — unchanged this PR, and only reachable through sanitized state.
- Bypass check on the sanitizer: array input -> {} (Array.isArray check). Non-boolean truthy values ("yes", 1, {}, []) rejected by strict === true comparison — no coercion path. proto-keyed entries: even if present as an own enumerable key post-JSON.parse, the value must still be === true to survive the filter; a nested-object proto value (the actual pollution shape) is dropped as non-boolean. Assignment out[key]=true for key==="proto" only affects out's own prototype pointer (a spec no-op when the RHS is a primitive) and never touches Object.prototype — no cross-session pollution path found.
- Legit grant round-trip verified: saveSessionFile persists session.allowedTools only when non-empty (lib/sessions.js:71-73); loadSessions/resumeSession sanitize-and-restore it; the allow_always grant handler in project-sessions.js flushes immediately via sm.saveSessionFile(session). No regression to the persist/restore/auto-approve flow for a real {"Write": true}-shaped grant.
Scanner delta: semgrep flagged a pre-existing unsafe-formatstring pattern in project-sessions.js:670 (console.error concatenation) outside this PR's changed line ranges — not introduced by this diff, dropped. gitleaks flagged pre-existing test-fixture private keys under unrelated .claude/worktrees/*/lib/certs/privkey.pem paths, untouched by this PR — dropped as out of scope (test fixtures, no production import).
No findings.
scanners_run: semgrep (ok, 1 hit, out of scope/dropped), gitleaks (ok, 6 hits, all pre-existing test fixtures/dropped), osv-scanner (not run — no dependency changes in this diff)
review.status: clean
clagentic gate-note — authorized
Authorize rationale: PEACHES clean (initial+hardening-delta), BOBBIE clean (initial+re-audit, nit closed via sanitizeAllowedTools), tests 641/641 pass. lr-8b2e persists allowedTools grant to durable session metadata. |
lr-8b2e follow-up: closes the injection nit BOBBIE raised on this PR.
What changed: Added a shared sanitizeAllowedTools() helper in lib/utils.js that keeps only entries whose key is a string and whose value is strictly boolean true, dropping malformed entries (non-boolean values, non-object input, arrays, null/undefined all yield {} and never throw). Applied at every hydration site BOBBIE named: lib/sessions.js:298 (loadSessions), lib/sessions.js resumeSession (defense-in-depth on opts.allowedTools), and lib/project-sessions.js (persistedAllowedTools assignment feeding resume_session opts, covering the former lines 385/410/413).
Why: a persisted .jsonl meta record shares its trust boundary with other session-file fields, but allowedTools was hydrated straight into the live auto-approve table with no shape/type validation. A crafted record could silently auto-approve a tool with no operator click at the handleCanUseTool check (lib/sdk-bridge.js:691).
Out of scope (unchanged): persist path (saveSessionFile), grant write, and the auto-approve read at lib/sdk-bridge.js:691. A legitimately-granted tool still round-trips and auto-approves exactly as before.
Tests: extended test/permission-grant-persist-lr-8b2e.test.js with a legit-grant regression case, a non-boolean-value rejection case, a non-object hydrates-as-empty case, direct sanitizeAllowedTools unit coverage, and a resumeSession opts-sanitize case.
npm test: 641 of 641 passing.
Task: lr-8b2e