Skip to content

fix(context-source-guard): fail-closed on exception inside gated section (security finding #4) - #1985

Merged
liususan091219 merged 2 commits into
sonichi:mainfrom
john-the-dev:fix/context-source-guard-fail-closed
Jul 15, 2026
Merged

liususan091219 merged 2 commits into
sonichi:mainfrom
john-the-dev:fix/context-source-guard-fail-closed

Conversation

@john-the-dev

Copy link
Copy Markdown
Collaborator

Summary

  • Finding README: inline demo video #4 from workspace/notes/security-audit-2026-07-06.md: the outer except Exception in __main__ of hooks/context-source-guard.py would fail-open on any unhandled exception, including one that occurs after a non-empty contextNotFrom blacklist has been confirmed.
  • Any runtime error inside the channel-check loop (guild resolution, token lookup, membership test) could silently allow a blacklisted …/channels/<id>/messages read through.
  • Blast radius is narrow (channel message reads only), but the bypass is deterministic if an exception can be induced after the blacklist check.

Fix in hooks/context-source-guard.py:

  • Wrap the critical section (entered once blacklist is confirmed non-empty) in a scoped try/except that calls _deny() on any non-SystemExit exception.
  • Pre-check errors (stdin JSON parsing, tool-type detection, serving-channel lookup, blacklist load) still flow to the outer fail-open handler — correct behaviour because no guard is active yet.
  • except SystemExit: raise ensures _deny()'s sys.exit(0) propagates normally through the inner block.
  • Updated module docstring and comment to reflect the tightened exception policy.

New test 9 in tests/context-source-guard.test.py:

  • Imports the hook as a module, seeds serving state to a channel with a non-empty blacklist, monkey-patches _resolve_guild to raise RuntimeError, feeds a matching channel-read payload, and asserts the hook denies (fails-closed) rather than allowing.

Test plan

🤖 Generated with Claude Code

https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh

…ion (security audit finding sonichi#4)

Previously the outer `except Exception` in `__main__` would fail-open on
any exception — including those raised after a non-empty contextNotFrom
blacklist was confirmed. An unexpected runtime error inside the channel-check
loop (guild resolution, token lookup, blacklist membership test) could
silently allow a blacklisted read through.

Fix: wrap the critical section (non-empty blacklist confirmed) in its own
try/except that calls `_deny()` on any non-SystemExit exception.
Pre-check errors (JSON parsing, tool detection, serving/blacklist lookup)
still fail-open via the outer handler — correct, because no guard is active.

Add test 9: monkey-patch `_resolve_guild` to throw RuntimeError while
serving a channel with a non-empty blacklist; assert the hook denies
(fail-closed) rather than allowing.

Security audit: workspace/notes/security-audit-2026-07-06.md finding sonichi#4.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh
@github-actions

github-actions Bot commented Jul 7, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 48%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Cold-reviewed — fix is correct; one non-blocking test-precision note.

Fix (hooks/context-source-guard.py) — correct and important. Wrapping the post-blacklist-confirmed critical section in its own try/except closes the fail-open bypass: previously any exception from _token()/_resolve_guild() after a non-empty blacklist was confirmed unwound to the top-level except → sys.exit(0) (fail-open = allow a potentially-blacklisted read). Now it fails closed via _deny(). ✅

  • except SystemExit: raise correctly lets _deny()s exit propagate — and since SystemExit derives from BaseException (not Exception), the except Exception arm couldnt swallow a deny even without the explicit re-raise. Nicely double-safe.
  • Splitting fail-open (pre-check: parsing/serving lookup) from fail-closed (guard-active) is the right boundary, and the docstring update matches.

Test note (non-blocking): test #9 asserts SystemExit.code == 0 was raised. Sound for this setup — _resolve_guild throws on the first target, so exit(0) can only come from _deny(), and the old code would escape main() as a RuntimeError (not SystemExit) → assertion fails → regression caught. But exit(0) is also what a fail-open allow emits, so the assertion is indirect. Consider redirecting stdout and asserting the deny payload is actually emitted — makes it assert deny-content rather than an exit happened. Nice-to-have.

No blockers — recommend merge. Merge stays your call.

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Automated mergeability review (2026-07-09)

Signal: NOT MERGEABLE

Current blocker(s):

  • Branch is behind current main; update/merge main and rerun checks before merging.

This is a conservative backlog triage signal. After the blockers are cleared, please rerun checks and request a focused review.

liususan091219
liususan091219 previously approved these changes Jul 13, 2026

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Codex full-review sweep: checked against the 7 triage CR classes (source-grep tests, incomplete fix, band-aid, naming, false claim, live-test policy, hardcoded paths) - none fire. Approving.

review-reason: codex-clean per full-review sweep

@liususan091219
liususan091219 dismissed their stale review July 13, 2026 02:02

Approval withheld pending owner go (resequenced: CRs first). Will re-approve.

@liususan091219 liususan091219 added bug Something isn't working and removed bug Something isn't working labels Jul 13, 2026
@qingyun-wu

Copy link
Copy Markdown
Collaborator

LGTM. I diffed this against main and ran python3 tests/context-source-guard.test.py in the PR worktree; it passed, including the new injected-exception case inside the guarded section. The inner try/except now closes the fail-open hole without changing the intended pre-check fail-open behavior at process entry.

Qingyun's Personal Codex Agent

@john-the-dev john-the-dev added the ag2product AG2 product related (packages, PyPI, task-relay) label Jul 14, 2026
@liususan091219 liususan091219 added security Security vulnerabilities and fixes and removed ag2product AG2 product related (packages, PyPI, task-relay) labels Jul 14, 2026
@liususan091219

Copy link
Copy Markdown
Collaborator

Independent before/after verification (maintainer): imported the hook as a module, seeded a serving channel with a non-empty contextNotFrom blacklist, monkey-patched _resolve_guild to raise inside the gated section, and fed a matching …/channels/999/messages Bash read (999 not directly in the blacklist, so it reaches guild resolution). Ran each version through its own __main__ wrapper.

BEFORE (origin/main):

[main] result -> ALLOW (fail-open) ⚠  [outer except allowed: True]

The exception escapes the (unwrapped) gated block → hits the outer except Exception → prints "allowing" and exits 0. The blacklisted channel read is permitted despite an active guard — exactly finding #4.

AFTER (this PR):

[1985] result -> DENY (fail-closed) ✅

The scoped try/except catches it and emits permissionDecision: deny. The read is blocked (fail-closed). except SystemExit: raise confirmed intact (a normal _deny still propagates).

Confirms the security claim: an exception after a non-empty blacklist is confirmed now fails closed instead of open. Verified working.

@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@liususan091219
liususan091219 merged commit 8c5cd28 into sonichi:main Jul 15, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

security Security vulnerabilities and fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants