fix(gateway): harden internal background events - #18099
Conversation
…ust-boundary # Conflicts: # gateway/run.py
…528200641 # Conflicts: # cli.py # gateway/run.py # tests/gateway/test_background_process_notifications.py
|
merge conflicts This PR does not merge cleanly with the base branch. Please rebase or merge current Signed: GPT-5.5-medium in Codex |
# Conflicts: # gateway/run.py # tests/test_tui_gateway_server.py # tests/tools/test_process_registry.py # tools/process_registry.py
egilewski
left a comment
There was a problem hiding this comment.
requesting changes
Blocking check:
git diff --check refs/remotes/origin/main...HEADreports trailing whitespace ingateway/run.py:12677on the PR head. This needs to be cleaned up before merge.
I otherwise validated the security direction: a forged MessageEvent(internal=True) reaches the agent path on current main, while the PR head rejects the same bare internal flag before agent handling, and the targeted gateway/process-registry tests pass. The remaining blocker is the repository whitespace check above.
Signed: GPT-5.5-xhigh in Codex
egilewski
left a comment
There was a problem hiding this comment.
looks mergeable
The previous blocking check is fixed: git diff --check refs/remotes/origin/main...refs/remotes/origin/pr/18099 is clean on head dd6b9d6055c4bfb4d1fd31d46ce5bf91c37526c3, including the earlier gateway/run.py whitespace area.
Security validation:
git merge-tree --write-tree refs/remotes/origin/main refs/remotes/origin/pr/18099succeeds against current main2944b3c394e3fe56cadbadd073a7fa54b24f3ba5.- A current-main replay commit with parents
2944b3c394e3fe56cadbadd073a7fa54b24f3ba5anddd6b9d6055c4bfb4d1fd31d46ce5bf91c37526c3passes the focused gateway/process-registry suite:tests/gateway/test_internal_event_bypass_pairing.py,tests/gateway/test_background_process_notifications.py,tests/gateway/test_pre_gateway_dispatch.py,tests/tools/test_process_registry.py, andtests/test_tui_gateway_server.py(401 passed). ruff checkpasses on the changed files in that current-main replay.
Worth improving: notification-mode normalization could be friendlier for unusual boolean/env aliases, but I do not see that as a merge blocker for this security fix.
Signed: GPT-5.5-xhigh in Codex
egilewski
left a comment
There was a problem hiding this comment.
requesting changes
Blocking check:
display.background_process_notifications: offnow also suppresses completeddelegate_task(background=true)results in the gateway._async_delegation_watcher()still routestype="async_delegation"completions through_inject_watch_notification(), but this PR added an unconditionaloffreturn at the top of that helper. That fixes watch-pattern/process wakeups by dropping them, but it also drops async delegation completions, which are not the process-watch notification path from #9290.
Reproduction:
# Current main with config.yaml setting display.background_process_notifications: off
# and an async_delegation event routed through _inject_watch_notification:
# adapter.handle_message await_count == 1
# PR head af21b9b8a2d2a772c2e29e9167b61bc944bcdd10 with the same setup:
# adapter.handle_message await_count == 0The fix should keep off from injecting background process/watch notifications while preserving async delegation completion delivery, or route async delegation through a helper that is not gated by the background-process notification setting.
Signed: GPT-5-medium in Codex
egilewski
left a comment
There was a problem hiding this comment.
looks mergeable
I reviewed the latest fix against current GitHub main 5378b941209d8f62a65455041658ce8ce8144cc9, PR base 81eaedd0f5c471c7ee748990066135a684f3c962, and PR head 01fba5887ce1fdf681594d2103b0252750eebc62.
Security evidence:
- trust boundary: gateway-internal synthetic process/delegation events must be gateway-originated and routed from trusted event metadata, not from untrusted user messages or ambient foreground session state.
- source/sink/invariant:
display.background_process_notifications=offmust suppress background process/watch notifications while still delivering completeddelegate_task(background=true)results as trusted gateway-owned completion events. - current-main reproduction: the original forged bare
internal=Trueevent bypass remains covered by the PR's internal-event tests; the prior review blocker was the old PR head dropping async delegation completions when notifications were off. - PR-head validation: this head routes async delegation through
_inject_async_delegation_completion(..., suppress_when_background_notifications_off=False)while leaving watch/process notification injection gated byoff. - positive/negative cases: focused tests cover both sides: watch notifications are suppressed in
off, and async delegation completions still reach the originating Telegram session withInternalEventKind.BACKGROUND_COMPLETION. - residual bypass search: checked the shared injection helper, async-delegation watcher, routing enrichment, and foreground-event rejection paths; no remaining same-class bypass or completion-drop path found in the reviewed scope.
- reviewer-tool status: CodeRabbit completed with no findings.
Validation passed:
git merge-tree --write-tree refs/remotes/origin/main refs/remotes/origin/pr/18099git diff --check refs/remotes/origin/main...refs/remotes/origin/pr/18099PYTHONPATH=<run-root>/deps /home/mac/hermes-agent/.venv/bin/python -B -m pytest -o addopts='' -p no:cacheprovider tests/gateway/test_internal_event_bypass_pairing.py tests/gateway/test_background_process_notifications.py tests/gateway/test_pre_gateway_dispatch.py tests/tools/test_process_registry.py tests/test_tui_gateway_server.py tests/tools/test_delegate.py tests/tools/test_notify_on_complete.py tests/cli/test_cli_background_tui_refresh.py -q(587 passed)/home/mac/hermes-agent/.venv/bin/python -B -m compileall -q cli.py gateway/run.py gateway/platforms/base.py gateway/platforms/msgraph_webhook.py gateway/platforms/yuanbao.py tools/process_registry.py tests/gateway/test_background_process_notifications.py tests/tools/test_delegate.py tests/tools/test_notify_on_complete.py/home/mac/hermes-agent/.venv/bin/python -B -m ruff check cli.py gateway/run.py gateway/platforms/base.py gateway/platforms/msgraph_webhook.py gateway/platforms/yuanbao.py tools/process_registry.py tests/gateway/test_background_process_notifications.py tests/tools/test_delegate.py tests/tools/test_notify_on_complete.py
The first focused test run failed one PTY stdin helper because the shared venv lacked the declared ptyprocess dependency; the same failure reproduced on current main. After installing declared ptyprocess==0.7.0 into the run-root dependency target only, the focused suite passed.
Signed: GPT-5.5-xhigh in Codex
|
Thanks for the thorough work here, @hanzckernel — the reframing of background-process output as non-authoritative observation text and the ANSI/control sanitization are nicely done. We're going to close this one, though, because the core security premise doesn't hold against how gateway events are actually built. The The one genuinely in-scope behavior — making Appreciate the effort, and please keep the contributions coming. |
What does this PR do?
Gateway/CLI background events now carry explicit trusted-internal metadata before they can bypass normal gateway policy. Process output is framed as observation text, not as an authoritative system instruction.
This keeps the previous legitimate synthetic-event flows working, but stops a bare mutable
MessageEvent.internal=Trueflag or user text that looks like a system/background message from becoming the trust boundary.Related Issue
Fixes #9290
Related / comparison:
background_process_notifications: off; this PR covers that behavior and additionally hardens the underlying gateway/CLI internal-event trust boundary.MessageEvent.internal; this PR narrows that bypass so it requires a trusted event kind and trusted producer source, not just the boolean flag.pre_gateway_dispatch; this PR preserves the intended internal-event guard while making untrusted or bare-internal events continue through hook/auth/pairing policy.notify_on_completesynthetic agent turns and feat(gateway): background process notification modes + fix spinner line spam #840 added background notification modes; this PR keeps those paths, but treats generated text as non-authoritative observations and makesbackground_process_notifications: offsuppress synthetic wakeups as well as visible notices.Type of Change
Changes Made
gateway/platforms/base.pyInternalEventKind, trusted internal event sources, andMessageEvent.is_trusted_internal().internalas a legacy marker, but stop treating it as sufficient for privileged gateway bypass.gateway/run.pyevent.is_trusted_internal().[Background process observation: ...]and strip ANSI/control sequences from process-supplied command/output text.offsuppress synthetic agent wakeups while still draining/consuming queued process notifications.cli.pyoffdoes not append synthetic pending input.gateway/platforms/yuanbao.pyinternal=Truerejection, pre-gateway hook behavior, notification-off no-wakeup/no-pending-input behavior, sanitized observation text, and gateway env/import isolation.How to Test
Using the project venv, run:
Local results on this branch:
51 passed in 3.70scompileall: passedruff --select F401: passedgit diff --cached --check: passed4288 passed, 10 skipped, 181 warnings in 269.97s590 passed, 3 warnings in 20.20sscripts/run_tests.sh tests/ -q: attempted locally but not green in this checkout; failures were outside this PR's touched gateway/CLI slice, so this is not claimed as a passing signal.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — N/A; behavior is covered by tests and existing config/docs for notification modes stay the samecli-config.yaml.exampleif I added/changed config keys — N/A; no new config keyCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A; no contributor workflow changeFor New Skills
N/A.
Screenshots / Logs
N/A. This is CLI/gateway control-flow and regression-test coverage.