Skip to content

fix(security): honor shell hook blocks even when message/reason is absent#27231

Closed
flamiinngo wants to merge 3 commits into
NousResearch:mainfrom
flamiinngo:fix/shell-hook-block-dropped-without-message
Closed

fix(security): honor shell hook blocks even when message/reason is absent#27231
flamiinngo wants to merge 3 commits into
NousResearch:mainfrom
flamiinngo:fix/shell-hook-block-dropped-without-message

Conversation

@flamiinngo

@flamiinngo flamiinngo commented May 17, 2026

Copy link
Copy Markdown
Contributor

Bug

_parse_response in agent/shell_hooks.py only forwarded a
pre_tool_call block directive if the hook also provided a non-empty
message or reason. When either field was missing the function
returned None, causing Hermes to treat the block as a no-op and
execute the tool unconditionally.

A hook that outputs {"action": "block"} or {"decision": "block"}
without a reason string is silently ignored. The security boundary
fails open — tools the user intended to gate are executed anyway.

Reproduction

from agent.shell_hooks import _parse_response

print(_parse_response("pre_tool_call", '{"action": "block"}'))
# None  ← tool executes, block ignored

print(_parse_response("pre_tool_call", '{"action": "block", "message": "x"}'))
# {'action': 'block', 'message': 'x'}  ← works

Fix

Remove the message-presence guard. Honor the block unconditionally and
fall back to "Blocked by shell hook." when no message is provided.
Existing hooks that already include a message or reason are unaffected.

After fix

{"action": "block"}            → {'action': 'block', 'message': 'Blocked by shell hook.'}
{"decision": "block"}          → {'action': 'block', 'message': 'Blocked by shell hook.'}
{"action": "block", "message": "Forbidden"} → {'action': 'block', 'message': 'Forbidden'}
{"decision": "block", "reason": "Forbidden"} → {'action': 'block', 'message': 'Forbidden'}

@cardtest15-coder

This comment was marked as spam.

Sign in to view
@github-actions github-actions Bot mentioned this pull request May 17, 2026
Open
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/agent Core agent loop, run_agent.py, prompt builder P0 Critical — data loss, security, crash loop labels May 17, 2026
@flamiinngo

flamiinngo commented May 17, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — addressed all points:

  1. isinstance check restored — raw is now validated as a non-empty
    string before being used as the message. Non-string values (integers,
    lists, etc.) fall back to the default instead of being forwarded as-is.

  2. Default message extracted — "Blocked by shell hook." is now a
    module-level constant _DEFAULT_BLOCK_MESSAGE used in both branches.

  3. Tests added — 4 new cases in tests/agent/test_shell_hooks.py:

    • action: block with no message → uses default
    • decision: block with no reason → uses default
    • Empty string message → uses default
    • Non-string message (e.g. integer) → uses default

@cardtest15-coder

This comment was marked as spam.

Sign in to view
@flamiinngo

flamiinngo commented May 17, 2026

Copy link
Copy Markdown
Contributor Author

Updated — isinstance guard restored, _DEFAULT_BLOCK_MESSAGE constant extracted, _block_message() helper added to unify the two branches, and 4 unit tests added covering missing message, missing reason, empty string, and non-string type. All 14 TestParseResponse tests pass.

@flamiinngo flamiinngo force-pushed the fix/shell-hook-block-dropped-without-message branch from 4f17e21 to a2d5193 Compare May 17, 2026 03:21
@cardtest15-coder

This comment was marked as spam.

Sign in to view
flamiinngo added 3 commits May 17, 2026 09:37
@flamiinngo
fix(security): honor shell hook blocks even when message/reason is ab…
9e5c11f 
…sent

_parse_response in agent/shell_hooks.py only forwarded a pre_tool_call
block directive if the hook also provided a non-empty message or reason.
When either field was missing the function returned None, causing Hermes
to treat the response as a no-op and execute the tool unconditionally.

This means a hook that outputs {"action": "block"} or {"decision": "block"}
without a reason string is silently ignored. The security boundary fails
open: tools the user intended to gate are executed anyway.

Fix: remove the message-presence guard. Honor the block unconditionally
and fall back to a default message when none is provided. Existing hooks
that already include a message or reason are unaffected.
@flamiinngo
fix(security): restore type safety and extract constant in shell hook…
32464e9 
… block handler

Address code review feedback on _parse_response:

1. Restore isinstance(raw, str) guard so non-string message/reason values
   (e.g. integers, lists) from a malformed hook response fall back to the
   default rather than being forwarded as-is. This keeps the contract that
   message in the returned dict is always a string.

2. Extract the repeated literal 'Blocked by shell hook.' into a module-level
   constant _DEFAULT_BLOCK_MESSAGE to avoid duplication and make it easy to
   change in one place.

Four new unit tests added to tests/agent/test_shell_hooks.py covering:
- action block with no message (uses default)
- decision block with no reason (uses default)
- action block with empty string message (uses default)
- action block with non-string message, e.g. integer (uses default)
@flamiinngo
refactor(security): extract _block_message helper to unify block logi…
12ec5d5 
…c in _parse_response

Both the `action=block` and `decision=block` branches in _parse_response
shared identical field-priority and type-validation logic. Extract it into
a single _block_message(primary, secondary) helper so the two branches are
one line each and the type guard lives in exactly one place.

No functional change: existing tests (TestParseResponse, 14 tests) all
pass unchanged, confirming identical behaviour.
@flamiinngo flamiinngo force-pushed the fix/shell-hook-block-dropped-without-message branch from a2d5193 to 12ec5d5 Compare May 17, 2026 08:37
@teknium1 teknium1 mentioned this pull request May 17, 2026
Merged
teknium1 added a commit that referenced this pull request May 17, 2026
@teknium1
chore(release): AUTHOR_MAP entries for batch salvage group 5 contribu…
150b577 
…tors

Adds release-note attribution mappings for the contributors from group 5:
- @haran2001 (PR #27070, #27068)
- @ms-alan (PR #26443)
- @godlin-gh (PR #26118)
- @wesleysimplicio (PR #25777, ext-email form)
- @Carry00 (PR #26851)
- @alaamohanad169-ship-it (PR #26036)
- @hawknewton (PR #26294)

(YanzhongSu PR #25879 and flamiinngo PR #27231 already mapped.)
@teknium1

teknium1 commented May 17, 2026

Copy link
Copy Markdown
Contributor

Merged via PR #27382 — your commit was cherry-picked onto current main as part of a batch salvage of low-risk new-contributor PRs. Authorship preserved (fix(security): honor shell hook blocks even when message/reason is absent (+2 follow-ups)). Thanks for the contribution.

@teknium1 teknium1 closed this May 17, 2026
gweeteve pushed a commit to gweeteve/hermes-agent that referenced this pull request Jun 2, 2026
@teknium1
chore(release): AUTHOR_MAP entries for batch salvage group 5 contribu…
80f8ecb 
…tors

Adds release-note attribution mappings for the contributors from group 5:
- @haran2001 (PR NousResearch#27070, NousResearch#27068)
- @ms-alan (PR NousResearch#26443)
- @godlin-gh (PR NousResearch#26118)
- @wesleysimplicio (PR NousResearch#25777, ext-email form)
- @Carry00 (PR NousResearch#26851)
- @alaamohanad169-ship-it (PR NousResearch#26036)
- @hawknewton (PR NousResearch#26294)

(YanzhongSu PR NousResearch#25879 and flamiinngo PR NousResearch#27231 already mapped.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/agent Core agent loop, run_agent.py, prompt builder P0 Critical — data loss, security, crash loop type/security Security vulnerability or hardening

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@flamiinngo @cardtest15-coder @teknium1 @alt-glitch