#4668
Pull Requests/HKUDS/nanobot/#4668

fix: enforce message outbound policy

Openhamb1ywants to merge intofrom
3 commits6 files changed+173-34

Summary

  • add an authorization hook before message tool outbound dispatch
  • wire AgentLoop to enforce channel allow_from / group_allow_from for cross-target sends
  • confine local media attachments to workspace/media roots even when workspace restriction is disabled

Fixes #4076

Verification

  • uv run pytest tests/tools/test_message_tool.py::test_message_tool_blocks_unauthorized_cross_target_send tests/tools/test_message_tool.py::test_message_tool_rejects_outside_absolute_media_paths_when_unrestricted tests/tools/test_message_tool.py::test_message_tool_resolves_mixed_media_paths tests/tools/test_message_tool.py::test_message_tool_records_media_deliveries tests/tools/test_message_tool.py::test_message_tool_tracks_turn_media_for_same_target tests/tools/test_message_tool.py::test_message_tool_allows_ws_explicit_when_matches_context tests/agent/test_loop_tool_context.py::test_message_tool_authorizer_uses_channel_allowlists -q
  • uv run ruff check nanobot/agent/tools/message.py nanobot/agent/loop.py tests/tools/test_message_tool.py tests/agent/test_loop_tool_context.py

Activity

8
H
fix: enforce message outbound policy
32a53c1
H
test: update message outbound policy expectations
d23031f
hamb1y-bot-hkuds-nanobot4mo ago

Review — fix: enforce message outbound policy

What this PR does: Two changes:

  1. Outbound authorization: Adds outbound_authorizer callback to MessageTool. Called before dispatch with (channel, chat_id, default_channel, default_chat_id). If it returns a string, send is blocked with that error. AgentLoop._authorize_message_tool_target implements the policy: same-target allowed; otherwise must be in channel's allow_from or group_allow_from, or be "*" wildcard, or be is_approved (pairing system).
  2. Media path confinement: _resolve_media no longer has an "absolute paths allowed when unrestricted" branch. All non-URL paths go through resolve_workspace_path with access.allowed_root or workspace and extra_allowed_dirs=[get_media_dir()]. Outside-workspace absolute paths are rejected even when restrict_to_workspace=False.

What's good

  • Authorizer is a callback, not hardcoded. MessageTool is agnostic to the policy; AgentLoop wires in the specific check. Testable in isolation (the test passes a custom _authorize function).
  • Same-target fast-path. if channel == default_channel and chat_id == default_chat_id: return None — normal replies never hit the allowlist check. No perf concern for the common case.
  • **"*"`` wildcard support.** Channels with "allow_from": ["*"]` allow any target. Backwards-compatible with open-policy deployments.
  • is_approved (pairing) is respected. Explicitly paired chats can be messaged cross-target without being in allow_from. Reasonable design — pairing IS authorization.
  • Media path change is correctly breaking. The old test_message_tool_passes_through_absolute_media_paths test is renamed and inverted to test_message_tool_rejects_outside_absolute_media_paths_when_unrestricted. Existing media tests are updated to use workspace-relative or media_dir paths.
  • test_message_tool_authorizer_uses_channel_allowlists tests all four branches: same-target, allow_from match, group_allow_from match, unconfigured channel, unauthorized target. Thorough.

Concerns / questions

  • Breaking change for users who relied on unrestricted absolute media paths. A user with restrict_to_workspace: false who legitimately attaches files from /var/data/reports/ would now be blocked. The fix is correct from a security standpoint (the issue explicitly calls this out), but should be called out in release notes with a migration path (e.g., set media_dir to /var/data/reports/ or use symlinks).
  • _resolve_media change affects ALL MessageTool instances, not just Dream or agent-driven ones. If any external code constructs a MessageTool directly (e.g., a plugin), it would now have stricter media path enforcement. Probably fine, but worth flagging.
  • get_media_dir() is called at _resolve_media time, not at construction. If the media dir is configured dynamically (e.g., per-tenant), this is correct. If it's static, it could be cached. Minor perf.
  • OutboundAuthorizer type alias is defined but the type isn't exported. External plugins that want to provide their own authorizer would need to import Callable directly. Consider exporting OutboundAuthorizer from the module.
  • test_not_suppress_when_sent_to_different_target in test_message_tool_suppress.py is updated to call mt.set_outbound_authorizer(lambda *_args: None) — a permissive authorizer. This is necessary because the test sends to a different target, which would now be blocked by the default authorizer. Correct fix.
  • No test for the "*" wildcard case. A test with "allow_from": ["*"] and a target not otherwise in the list would lock in the wildcard behavior.

Verdict

Approve. Correct security fix with good test coverage. The breaking change for unrestricted media paths is intentional and justified — but please add a release note / migration guide for affected users.

hamb1y-bot-hkuds-nanobot4mo ago

Review — fix: enforce message outbound policy

What this PR does: Two changes:

  1. Outbound authorization: Adds outbound_authorizer callback to MessageTool. Called before dispatch with (channel, chat_id, default_channel, default_chat_id). If it returns a string, send is blocked with that error. AgentLoop._authorize_message_tool_target implements the policy: same-target allowed; otherwise must be in channel's allow_from or group_allow_from, or be "*" wildcard, or be is_approved (pairing system).
  2. Media path confinement: _resolve_media no longer has an "absolute paths allowed when unrestricted" branch. All non-URL paths go through resolve_workspace_path with access.allowed_root or workspace and extra_allowed_dirs=[get_media_dir()]. Outside-workspace absolute paths are rejected even when restrict_to_workspace=False.

What's good

  • Authorizer is a callback, not hardcoded. MessageTool is agnostic to the policy; AgentLoop wires in the specific check. Testable in isolation (the test passes a custom _authorize function).
  • Same-target fast-path. if channel == default_channel and chat_id == default_chat_id: return None — normal replies never hit the allowlist check. No perf concern for the common case.
  • **"*"`` wildcard support.** Channels with "allow_from": ["*"]` allow any target. Backwards-compatible with open-policy deployments.
  • is_approved (pairing) is respected. Explicitly paired chats can be messaged cross-target without being in allow_from. Reasonable design — pairing IS authorization.
  • Media path change is correctly breaking. The old test_message_tool_passes_through_absolute_media_paths test is renamed and inverted to test_message_tool_rejects_outside_absolute_media_paths_when_unrestricted. Existing media tests are updated to use workspace-relative or media_dir paths.
  • test_message_tool_authorizer_uses_channel_allowlists tests all four branches: same-target, allow_from match, group_allow_from match, unconfigured channel, unauthorized target. Thorough.

Concerns / questions

  • Breaking change for users who relied on unrestricted absolute media paths. A user with restrict_to_workspace: false who legitimately attaches files from /var/data/reports/ would now be blocked. The fix is correct from a security standpoint (the issue explicitly calls this out), but should be called out in release notes with a migration path (e.g., set media_dir to /var/data/reports/ or use symlinks).
  • _resolve_media change affects ALL MessageTool instances, not just Dream or agent-driven ones. If any external code constructs a MessageTool directly (e.g., a plugin), it would now have stricter media path enforcement. Probably fine, but worth flagging.
  • get_media_dir() is called at _resolve_media time, not at construction. If the media dir is configured dynamically (e.g., per-tenant), this is correct. If it's static, it could be cached. Minor perf.
  • OutboundAuthorizer type alias is defined but the type isn't exported. External plugins that want to provide their own authorizer would need to import Callable directly. Consider exporting OutboundAuthorizer from the module.
  • test_not_suppress_when_sent_to_different_target in test_message_tool_suppress.py is updated to call mt.set_outbound_authorizer(lambda *_args: None) — a permissive authorizer. This is necessary because the test sends to a different target, which would now be blocked by the default authorizer. Correct fix.
  • No test for the "*" wildcard case. A test with "allow_from": ["*"] and a target not otherwise in the list would lock in the wildcard behavior.

Verdict

Approve. Correct security fix with good test coverage. The breaking change for unrestricted media paths is intentional and justified — but please add a release note / migration guide for affected users.

chengyongrureviewed4mo ago
chengyongru4mo ago

Automated PR review by nanobot. This is not a human maintainer review or approval.

I found one authorization mismatch that leaves a common configured target blocked.

hamb1yreviewed4mo ago
H
fix: honor nested slack dm allowlist
a8c2622
hamb1yreviewed4mo ago