fix(hooks): harden CLI transcript loading and gate conversation hooks#70786
Conversation
openclaw-barnacle
Bot
added
agents
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🟠 Conversation access policy bypass via before_reset/before_compaction hooks still exposing session messages
DescriptionThe new This creates a policy-bypass/data-exfiltration risk:
Vulnerable behavior: // registry.ts only gates llm_input/llm_output/agent_end via isConversationHookName
if (isConversationHookName(hookName)) {
...block unless allowConversationAccess...
}
// session-reset-service.ts passes full messages to before_reset regardless of allowConversationAccess
messages = readSessionMessages(sessionId, params.storePath, sessionFile);
void hookRunner.runBeforeReset({ sessionFile, messages, reason }, ctx);Impact:
RecommendationApply the conversation-access policy to all hooks that can expose conversation transcripts, not only Suggested approach:
Example: // hook-types.ts
export const CONVERSATION_HOOK_NAMES = [
"llm_input",
"llm_output",
"agent_end",
"before_reset",
"before_compaction",
// add others that can carry full transcripts
] as const;
// registry.ts uses isConversationHookName(hookName) unchangedAlternatively (more robust): tag hook types with metadata like Also consider adding tests proving that a non-bundled plugin without 2. 🟡 TOCTOU race allows symlink swap to bypass session transcript path checks
Description
An attacker who can write to the sessions directory can:
Vulnerable code: const entryStat = fs.lstatSync(sessionFile);
...
const realSessionFile = safeRealpathSync(sessionFile);
...
const stat = fs.statSync(realSessionFile);
...
const entries = SessionManager.open(realSessionFile).getEntries();RecommendationAvoid path-based TOCTOU by opening and validating the file once, using a file descriptor, and then reading from that descriptor. Options:
import { constants as c } from "node:fs";
const fd = fs.openSync(sessionFile, c.O_RDONLY | c.O_NOFOLLOW);
try {
const st = fs.fstatSync(fd);
if (!st.isFile() || st.size > MAX_CLI_SESSION_HISTORY_FILE_BYTES) return [];
// Derive the real path from the fd (or re-check with realpath + inode comparison)
const real = fs.realpathSync.native(sessionFile);
if (!isPathWithinBase(realSessionsDir, real)) return [];
// Ensure the path didn't change since open: compare `fs.statSync(sessionFile).ino/dev` with `st.ino/dev`.
const entries = SessionManager.openFromFd?.(fd) /* preferred */
?? SessionManager.open(real).getEntries();
} finally {
fs.closeSync(fd);
}
This removes the race window where the checked file can be swapped for a symlink or different file between validation and use. Analyzed PR: #70786 at commit Last updated on: 2026-04-23T21:27:47Z |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f96cac1f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { | ||
| limitAgentHookHistoryMessages, | ||
| MAX_AGENT_HOOK_HISTORY_MESSAGES, | ||
| } from "../harness/hook-history.js"; |
There was a problem hiding this comment.
src/agents/cli-runner/session-history.ts now imports ../harness/hook-history.js, but that file is not present in this commit (and the same missing module is imported from src/agents/cli-runner.ts). This makes CLI runner modules fail at import time with Cannot find module '../harness/hook-history.js', so the reliability/session-history suites and runtime CLI path cannot execute at all. Please either include the new module in this change or point these imports at an existing helper.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR hardens CLI transcript loading against symlink traversal and custom session-store omissions, gates conversation hooks (
Confidence Score: 4/5Not safe to merge: the PR imports from a source file (src/agents/harness/hook-history.ts) that does not exist anywhere in the repository, which will break the build. All security hardening logic and policy gating are well-reasoned and test-covered, but the missing hook-history.ts module is a P0 build-breaking omission that must be resolved before this can land. src/agents/harness/hook-history.ts (must be created), src/agents/cli-runner.ts and src/agents/cli-runner/session-history.ts (both import the missing module) Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/cli-runner/session-history.ts
Line: 10-13
Comment:
**Missing source file for imported module**
`hook-history.js` is imported for `limitAgentHookHistoryMessages` and `MAX_AGENT_HOOK_HISTORY_MESSAGES`, but `src/agents/harness/hook-history.ts` does not exist anywhere in the repository (confirmed via `git log`, `git ls-files`, and a full filesystem search). The same missing module is also referenced in `src/agents/cli-runner.ts` (`buildAgentHookConversationMessages`). Without this file the project will fail to compile and all tests that exercise the changed paths will error at import time.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/cli-runner.ts
Line: 6
Comment:
**Same missing module referenced here**
`buildAgentHookConversationMessages` is imported from `./harness/hook-history.js`, which resolves to the same non-existent `src/agents/harness/hook-history.ts`. This import will cause a compilation failure.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/cli-runner/session-history.ts
Line: 26-29
Comment:
**`isPathWithinBase` silently rejects when paths are identical**
`path.relative(basePath, targetPath)` returns `""` when the two paths are equal, so `Boolean(relative)` is `false` and the function returns `false`. In the current call site a session *file* can never equal the sessions *directory*, so this is harmless today, but the function's contract is misleading and could cause subtle regressions if it's reused in other contexts. Consider using `relative !== null && !relative.startsWith("..") && !path.isAbsolute(relative)` to express the true intent, or add a comment explaining the deliberate exclusion of equal paths.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(hooks): harden cli transcript loadin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { | ||
| limitAgentHookHistoryMessages, | ||
| MAX_AGENT_HOOK_HISTORY_MESSAGES, | ||
| } from "../harness/hook-history.js"; |
There was a problem hiding this comment.
hook-history.js is imported for limitAgentHookHistoryMessages and MAX_AGENT_HOOK_HISTORY_MESSAGES, but src/agents/harness/hook-history.ts does not exist anywhere in the repository (confirmed via git log, git ls-files, and a full filesystem search). The same missing module is also referenced in src/agents/cli-runner.ts (buildAgentHookConversationMessages). Without this file the project will fail to compile and all tests that exercise the changed paths will error at import time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/cli-runner/session-history.ts
Line: 10-13
Comment:
**Missing source file for imported module**
`hook-history.js` is imported for `limitAgentHookHistoryMessages` and `MAX_AGENT_HOOK_HISTORY_MESSAGES`, but `src/agents/harness/hook-history.ts` does not exist anywhere in the repository (confirmed via `git log`, `git ls-files`, and a full filesystem search). The same missing module is also referenced in `src/agents/cli-runner.ts` (`buildAgentHookConversationMessages`). Without this file the project will fail to compile and all tests that exercise the changed paths will error at import time.
How can I resolve this? If you propose a fix, please make it concise.| import { loadCliSessionHistoryMessages } from "./cli-runner/session-history.js"; | ||
| import type { PreparedCliRunContext, RunCliAgentParams } from "./cli-runner/types.js"; | ||
| import { FailoverError, isFailoverError, resolveFailoverStatus } from "./failover-error.js"; | ||
| import { buildAgentHookConversationMessages } from "./harness/hook-history.js"; |
There was a problem hiding this comment.
buildAgentHookConversationMessages is imported from ./harness/hook-history.js, which resolves to the same non-existent src/agents/harness/hook-history.ts. This import will cause a compilation failure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/cli-runner.ts
Line: 6
Comment:
**Same missing module referenced here**
`buildAgentHookConversationMessages` is imported from `./harness/hook-history.js`, which resolves to the same non-existent `src/agents/harness/hook-history.ts`. This import will cause a compilation failure.
How can I resolve this? If you propose a fix, please make it concise.| function isPathWithinBase(basePath: string, targetPath: string): boolean { | ||
| const relative = path.relative(basePath, targetPath); | ||
| return Boolean(relative) && !relative.startsWith("..") && !path.isAbsolute(relative); | ||
| } |
There was a problem hiding this comment.
isPathWithinBase silently rejects when paths are identical
path.relative(basePath, targetPath) returns "" when the two paths are equal, so Boolean(relative) is false and the function returns false. In the current call site a session file can never equal the sessions directory, so this is harmless today, but the function's contract is misleading and could cause subtle regressions if it's reused in other contexts. Consider using relative !== null && !relative.startsWith("..") && !path.isAbsolute(relative) to express the true intent, or add a comment explaining the deliberate exclusion of equal paths.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/cli-runner/session-history.ts
Line: 26-29
Comment:
**`isPathWithinBase` silently rejects when paths are identical**
`path.relative(basePath, targetPath)` returns `""` when the two paths are equal, so `Boolean(relative)` is `false` and the function returns `false`. In the current call site a session *file* can never equal the sessions *directory*, so this is harmless today, but the function's contract is misleading and could cause subtle regressions if it's reused in other contexts. Consider using `relative !== null && !relative.startsWith("..") && !path.isAbsolute(relative)` to express the true intent, or add a comment explaining the deliberate exclusion of equal paths.
How can I resolve this? If you propose a fix, please make it concise.
…guards (#2764) (#2767) Re-port two medium-severity upstream security fixes missed in the v2026.4.24 content-only sync — both apply to kept fork paths (gateway, plugins) and were advertised/expected controls that were absent live. - gateway: block webchat clients from sessions.compact. Extend rejectWebchatSessionMutation to the "compact" action and guard the handler (client + isWebchatConnect), mirroring sessions.patch/delete. Upstream 873dce1 (PR openclaw#70716). The fork has no sessions.restore handler, so only "compact" is ported (not upstream's "restore"). - plugins: enforce plugins.entries.*.hooks.allowConversationAccess. Add isConversationHookName and gate conversation hooks (llm_input, llm_output, agent_end) in registerTypedHook so non-bundled plugins must opt in explicitly; bundled plugins stay allowed unless they set it false. The config field was fully plumbed but inert. Upstream 51f9f94 (PR openclaw#70786) — registry/hooks portion only. Tests: extend the webchat-mutation gateway test to assert compact is rejected; add a conversation-access gate suite to plugins dead-hooks.test.ts (non-bundled block, opt-in allow, bundled default, bundled opt-out, non-conversation-hook not gated). Co-authored-by: Claude Opus 4.8 <[email protected]>
1 participant
Summary
session.storeroots, and eagerly parsed transcript history even when no lifecycle hook needed it.loadCliSessionHistoryMessages, skipped transcript parsing unlessllm_inputoragent_endis active, stopped emitting emptyllm_output, and addedplugins.entries.<id>.hooks.allowConversationAccessas the explicit opt-in for non-bundled conversation hooks.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
statSync/SessionManager.open()without rejecting symlinks or confirming the real path stayed inside the allowed sessions root. At the same time, typed conversation hooks had no equivalent of the existing prompt-injection policy, so non-bundled plugins could subscribe to raw prompt/history/output payloads by default.Regression Test Plan (if applicable)
src/agents/cli-runner/session-history.test.ts,src/agents/cli-runner.reliability.test.ts,src/plugins/loader.test.ts,src/plugins/config-state.test.ts,src/plugins/status.test.tssession.storeroots still resolve hook history, transcript parsing is skipped when onlyllm_outputis active, empty CLI output does not emitllm_output, and non-bundled plugins cannot register conversation hooks without explicit opt-in.src/agents/cli-runner.retry.live.test.tscontinues to cover the live session-expired retry path.User-visible / Behavior Changes
plugins.entries.<plugin-id>.hooks.allowConversationAccess: truebefore they can registerllm_input,llm_output, oragent_endtyped hooks.llm_outputevents for empty assistant output.session.storenow keep prior CLI transcript history in hook payloads again.Diagram (if applicable)
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) YesYes, explain risk + mitigation: non-bundled plugins now get less data by default because conversation hook registration is blocked unless explicitly trusted viaallowConversationAccess: true. Transcript loading also now rejects symlinked/special-file tricks and enforces real-path containment under the allowed sessions root.Repro + Verification
Environment
OPENCLAW_LIVE_CLI_BACKEND_RESUME_PROBE=1; plugin hook policy defaults plus explicithooks.allowConversationAccessSteps
session.storeroot.runPreparedCliAgent(...)with lifecycle hooks enabled or load a non-bundled plugin that registersllm_input/llm_output/agent_end.Expected
session.storetranscript roots still load bounded history.llm_outputis skipped when assistant text is empty.allowConversationAccess: truefor conversation hooks.Actual
Evidence
Human Verification (required)
pnpm build, and the live CLI resume retry probe all passed after the fix.session.store, empty assistant output, and non-bundled conversation-hook registration without opt-in.pnpm check && pnpm test, and I did not add a content-redaction layer for trusted bundled hooks.Review Conversations
Compatibility / Migration
Yes/No) MostlyYes/No) YesYes/No) Only for non-bundled plugins that rely on conversation hooksplugins.entries.<plugin-id>.hooks.allowConversationAccess: truefor any trusted non-bundled plugin that intentionally consumesllm_input,llm_output, oragent_endpayloads.Risks and Mitigations
session.storeregression coverage and validated the live retry probe after the resolver change.AI-assisted: yes
Testing: fully tested