Skip to content

feat(hooks): add session memory policy controls (blockSessionSave, redirectPath) [claude, human developer oversight]#35567

Closed
zeroaltitude wants to merge 35 commits intoopenclaw:mainfrom
zeroaltitude:feat/hook-session-memory-policy
Closed

feat(hooks): add session memory policy controls (blockSessionSave, redirectPath) [claude, human developer oversight]#35567
zeroaltitude wants to merge 35 commits intoopenclaw:mainfrom
zeroaltitude:feat/hook-session-memory-policy

Conversation

@zeroaltitude
Copy link
Copy Markdown
Contributor

@zeroaltitude zeroaltitude commented Mar 5, 2026

Summary

Allows upstream hooks (e.g. security/provenance plugins) to control session memory persistence behavior via three new context fields:

  • blockSessionSave — prevent the session from being persisted to memory (e.g. for sensitive/classified conversations)
  • sessionSaveRedirectPath — redirect session save to an alternate file path (e.g. quarantine directory)
  • sessionSaveContent — override the saved content

These fields are read by the bundled session-memory hook handler. Upstream hooks (registered by plugins, which load before bundled hooks in FIFO order) can set these fields to control save behavior.

Changes

  • src/hooks/bundled/session-memory/handler.ts — check blockSessionSave, use sessionSaveRedirectPath for file path, support sessionSaveContent override

Testing

  • Degree of testing: Well-tested — 3 unit tests covering blockSessionSave, sessionSaveRedirectPath (with mkdir), and sessionSaveContent override. pnpm check clean. Deployed locally and in daily use for ~4 weeks.
  • AI-assisted development with human developer oversight

…directPath)

Allow upstream hooks (e.g. security/provenance plugins) to control
session memory behavior:
- blockSessionSave: prevent session from being persisted to memory
- sessionSaveRedirectPath: redirect session save to alternate path
- sessionSaveContent: override the saved content

[claude, human developer oversight]
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 5, 2026

Greptile Summary

This PR introduces three new context fields (blockSessionSave, sessionSaveRedirectPath, sessionSaveContent) to control session memory persistence behavior in the bundled session-memory hook. The implementation is sound:

  • Security model is correct: Path validation is enforced by writeFileWithinRoot (symlink-safe, path-traversal-resistant, atomic writes), providing defense-in-depth
  • Fail-closed semantics: Redirect validation errors prevent policy bypass; non-redirect writes are constrained to memory/ subdirectory
  • Comprehensive tests: 7 new unit tests cover all three fields including an outside-workspace escape rejection test
  • Symlink handling: The canonicalizeViaAncestor helper properly handles macOS /tmp/private/tmp symlink aliasing for not-yet-created paths

The code is safe to merge and ready for use by upstream hooks and security plugins.

Confidence Score: 5/5

  • No security vulnerabilities or correctness issues identified. The containment and fail-closed guarantees are sound.
  • The PR implements a well-designed security feature with correct threat modeling. The pre-existing writeFileWithinRoot infrastructure provides all necessary containment checks (symlink resolution, path-traversal prevention, atomic writes). Upstream hooks can control session persistence safely — redirect failures fail closed, and non-redirects are scoped to memory/. The comprehensive test suite validates all three new fields including adversarial cases (outside-workspace escapes).
  • No files require special attention

Last reviewed commit: ae873ad

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6f6d612032

ℹ️ 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".

Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
When sessionSaveRedirectPath is set, write to the full redirect path
(mkdir -p + appendFile) instead of path.basename() under memoryDir.
Fixes the quarantine use case where redirected saves silently landed
in the default memory directory.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 029a706af1

ℹ️ 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".

Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
Use fs.open with mode 0o600 (owner-only) and mkdir with 0o700 for
redirect paths, matching writeFileWithinRoot's confidentiality
guarantees. Prevents session data exposure on shared hosts.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee5cf1307c

ℹ️ 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".

Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
Instead of bypassing writeFileWithinRoot for redirected session saves,
shift the rootDir to the redirect target's parent directory. This
preserves path traversal protection while honoring the redirect.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 17cca2bb37

ℹ️ 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".

Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
writeFileWithinRoot requires rootDir to be realpath-resolvable.
When redirecting to a fresh quarantine path, the parent directory
may not exist yet. Create it before calling writeFileWithinRoot.
3 tests for blockSessionSave, sessionSaveRedirectPath, and
sessionSaveContent. Uses existing test infrastructure.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d899f4a8d6

ℹ️ 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".

Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
- Resolve relative redirect paths against workspaceDir
- Reject paths outside workspace with log warning, fall back to normal memory dir
- 2 new tests: relative path resolution + workspace escape rejection
- Total: 21 tests (16 existing + 5 new)
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3adfbe5486

ℹ️ 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".

Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
…t check

path.normalize() doesn't resolve symlinks — a symlink inside workspace
could point outside it. Now uses fs.realpath() on the parent directory
to get the true filesystem path before checking containment.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c2f8660304

ℹ️ 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".

Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
… custom content

1. Two-pass validation: textual containment check BEFORE mkdir,
   then symlink resolution after. Prevents creating directories
   outside workspace on invalid redirect paths.

2. Skip transcript loading and LLM slug generation when
   sessionSaveContent is set. Prevents leaking raw session
   text to the model provider in redaction workflows.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ab62f9b470

ℹ️ 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".

Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
Stop doing manual path validation (normalize, realpath, startsWith).
Instead, use writeFileWithinRoot with workspaceDir as root for ALL
writes. It handles path traversal, symlink resolution, containment,
and mkdir internally. Invalid redirect paths fall back to memory dir.

This is the canonical pattern used throughout the codebase (fs-safe.ts)
and handles all edge cases: symlinks, relative paths, path alias
escapes, and workspace-as-symlink.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eb6c0182b5

ℹ️ 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".

Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
…rors only

Operational I/O failures (ENOSPC, EACCES on a valid quarantine path) now
fail closed instead of silently falling back to the default memory dir,
which could bypass security policy intent in deployments using
sessionSaveRedirectPath.
When a redirect path is rejected, log the fallback writePath instead of
the rejected writeRelativePath to avoid misleading security audits.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f444fc8d47

ℹ️ 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".

Comment thread src/hooks/bundled/session-memory/handler.ts Outdated
…verride

An empty string is a valid redaction signal — hooks may intentionally set
sessionSaveContent to '' to persist a blank marker while avoiding transcript
retention. Previously this was treated as unset due to the length > 0 check.
…orkflows

writeFileWithinRoot handles directory creation internally, so the pre-existing
unconditional fs.mkdir(memoryDir) was creating an empty memory/ directory
even in redirect-only/quarantine workflows. Removed to avoid unintended
artifacts.
…prove redirect fallback warning

- blockSessionSave now reads from the safe 'context' local variable
  (with || {} fallback) instead of accessing event.context directly
  outside the try block.
- Redirect fallback log.warn now explicitly warns that quarantine
  workflows should pair sessionSaveRedirectPath with blockSessionSave.
@zeroaltitude zeroaltitude deleted the feat/hook-session-memory-policy branch March 7, 2026 21:59
@darfaz
Copy link
Copy Markdown

darfaz commented Mar 10, 2026

Makes sense — centralizing path security in writeFileWithinRoot is cleaner than per-policy guards. Good approach.

zeroaltitude added a commit to zeroaltitude/openclaw that referenced this pull request Mar 22, 2026
…emory handler

Two new context fields for upstream hooks (e.g. security plugins) to
control session memory persistence:

- blockSessionSave (boolean): prevent session from being saved to memory
- sessionSaveContent (string): override saved content with custom text
  (empty string is valid — persists a blank marker without transcript)

When sessionSaveContent is set, LLM slug generation and session content
loading are skipped (unnecessary when content is overridden).

Split from openclaw#35567 — sessionSaveRedirectPath follows separately as it
requires path canonicalization, symlink resolution, and filesystem
write policy review.
zeroaltitude added a commit to zeroaltitude/openclaw that referenced this pull request Mar 22, 2026
…emory handler

Two new context fields for upstream hooks (e.g. security plugins) to
control session memory persistence:

- blockSessionSave (boolean): prevent session from being saved to memory
- sessionSaveContent (string): override saved content with custom text
  (empty string is valid — persists a blank marker without transcript)

When sessionSaveContent is set, LLM slug generation and session content
loading are skipped (unnecessary when content is overridden).

Split from openclaw#35567 — sessionSaveRedirectPath follows separately as it
requires path canonicalization, symlink resolution, and filesystem
write policy review.
zeroaltitude added a commit to zeroaltitude/openclaw that referenced this pull request Mar 22, 2026
Allows upstream hooks to redirect session memory writes to an alternate
path (e.g. quarantine directory for tainted sessions).

Security:
- canonicalizeViaAncestor() resolves symlinks via nearest existing ancestor
- writeFileWithinRoot validates containment within workspace
- Fails closed for outside-workspace paths (no fallback to default memory/)
- Both workspace and redirect paths canonicalized before comparison

Depends on openclaw#38161 (blockSessionSave + sessionSaveContent).
Split from openclaw#35567 per review feedback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants