Repository navigation
2.0.1: scan a journal after a shell command changes it - #2
Conversation
A shell write such as F=<journal>; cat notes > "$F" hides both the target and the content from PreToolUse, so in deny mode a credential could land in a journal unchecked. After any shell command that changed a journal, PostToolUse now scans the journal file itself and tells the agent to remove what matched; in deny mode Stop blocks until the journal is clean. Values are never echoed. Bumps to 2.0.1.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: ec84673aa5
ℹ️ 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".
| if cmd is not None and ctx.secrets != 'off': | ||
| # A shell write can carry a credential PreToolUse never saw (`cat secret > "$F"`): scan the journal itself. | ||
| for jn in names: | ||
| hits = scan_secrets(read(ctx.jpath(jn))) |
There was a problem hiding this comment.
With the installed Bash hooks, a command such as ./update-journal.sh or . /tmp/vars; cat /tmp/notes > "$F" can modify a journal without containing the literal journal-directory name or matching the landing regex. run_hook() then exits through tool_may_matter() for both PreToolUse and PostToolUse, so no pending timestamp is recorded and this new scan never runs; the credential remains and Stop has no leaked entry. Route opaque Bash calls through the before/after detection path as well.
Useful? React with 👍 / 👎.
| if leaks and not inp.get('stop_hook_active'): | ||
| ctx.save_lease(lease) | ||
| ctx.trace(ev='stop', sid=sid, result='secret', journal=leaks[-1][0]) | ||
| return {'decision': 'block', 'reason': 'SESSION-JOURNAL-SECRETS: ' + '; '.join( |
There was a problem hiding this comment.
After the first Stop block, the runtime's next Stop invocation has stop_hook_active=true; if the agent failed, refused, or only partially completed the requested cleanup, this condition skips the secret block and the session can finish with the credential still present. The newly added scenario reproduces this by leaving the file untouched after the first block: the active Stop returns no decision while lease['leaked'] remains populated, contradicting deny mode's promise to block until clean.
Useful? React with 👍 / 👎.
| for jn in names: | ||
| hits = scan_secrets(read(ctx.jpath(jn))) | ||
| if hits: | ||
| leaks.append((jn, hits)) |
There was a problem hiding this comment.
When an opaque shell command modifies three or more journals, journals_changed_since() returns more than two extras and the existing bulk guard discards the entire set before this loop. Thus names is empty, none of the modified journals is scanned, and credentials written by the bulk command produce neither a warning nor a Stop block. Keep the candidates used for security scanning separate from the smaller set allowed to drive session binding.
Useful? React with 👍 / 👎.
Addresses the Codex review finding on #1 (P1, hooks/session-journal.py:1010): a credential written to a journal by an opaque shell command (
F=<journal>; cat notes > "$F") escaped the deny-mode check.denymode the journal is remembered on the lease and Stop blocks until it is clean.