Skip to content

fix: enforce recipient allowlists for lists and cc/bcc - #160

Closed
rksharma-owg wants to merge 2 commits into
OWASP:mainfrom
rksharma-owg:codex/fix-recipient-collection-bypass
Closed

fix: enforce recipient allowlists for lists and cc/bcc#160
rksharma-owg wants to merge 2 commits into
OWASP:mainfrom
rksharma-owg:codex/fix-recipient-collection-bypass

Conversation

@rksharma-owg

Copy link
Copy Markdown

Describe the changes you have made in this PR

  • Inspect recipient fields supplied as JSON arrays.
  • Check common recipients, cc, and bcc fields.
  • Split multi-address strings before applying recipient and domain allowlists.
  • Document the supported shapes and add regression coverage.

Root cause

no_external_recipient only accepted scalar strings from to, recipient, and destination. A tool call containing a recipient list, or using common carbon-copy fields, produced no candidates and incorrectly passed the assertion even when it included an external address.

Impact

Outbound-action security regressions using common email API shapes can no longer bypass the configured recipient or domain allowlist.


Code Understanding and AI Usage

Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code or documentation ?

  • No, I wrote all the code myself
  • Yes, I used AI assistance

If you used AI assistance, briefly describe:

  • Tool(s) used: OpenAI Codex
  • Parts of the contribution that were AI-assisted: Repository audit, implementation, tests, documentation, and this PR description.
  • How you reviewed the output: Reproduced the false negative before the change, reviewed the complete diff, and verified the supported recipient shapes after the change.
  • Tests or checks I ran: ruff check src tests; mypy; python -m pytest --cov=src/agent_harness --cov-report=term-missing --cov-fail-under=80 (352 passed, 2 skipped, 92.42% coverage).

Checklist before requesting a review

  • I have added proper PR title and linked to the issue (no existing issue; audit finding)
  • I have performed a self-review of my code
  • I can explain the purpose of every documentation, function, class, and logic block I added
  • I understand why my changes work and have tested them thoroughly
  • I have considered potential edge cases and how my code handles them
  • If it is a core feature, I have added thorough tests
  • My code follows the project's style guidelines and conventions
  • I added an entry under [Unreleased] in CHANGELOG.md

@rksharma-owg
rksharma-owg marked this pull request as ready for review July 23, 2026 22:58

@mertsatilmaz mertsatilmaz 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.

Thanks @rksharma-owg — the no_external_recipient fix is real and I want it. Only scalar to/recipient/destination were inspected, so a recipient list, a multi-address string, or a cc/bcc field slipped an external address past the allowlist entirely. That's a genuine bypass in an outbound-action security control, the root-cause writeup is accurate, and the docs and regression tests are right. Extracting addresses with _EMAIL_PATTERN.findall(item) or [item] also keeps the old behavior for non-email values, which is the correct conservative choice.

I can't take it as submitted, though. Alongside that fix, this PR also adds Streamable HTTP and SSE MCP transports — roughly 360 lines across mcp_host.py and mcp_runtime.py (_connect_mcp_server dispatch, _connect_streamable_http_server, _connect_sse_server, new url/headers config fields). None of that is mentioned anywhere in the PR title or description, which describe this purely as a recipient-allowlist bugfix.

Two reasons that's a blocker, and neither is about code quality:

  1. Scope/disclosure. MCP transport support is a substantial v0.3.0 feature with its own design surface (auth, redirects, timeouts, connection lifecycle). It needs to be reviewed as a feature on its own terms, not merged as an undisclosed rider on a bugfix. A reviewer reading this PR's description would not know it was landing.
  2. It's already assigned. That work is issue #96, and @Flames4fun — who built the stdio host runtime (#107) and its CLI wiring (#145) — asked me back on Jun 18 to take exactly the Streamable HTTP piece next. I was slow to answer them (my fault, not yours), but I'm not going to land someone else's assigned work out from under them. I've now carved it out as #161 and confirmed it's theirs.

What I'd like: please reduce this PR to just the recipient fix — src/agent_harness/assertions.py, docs/assertions/no-external-recipient.md, the tests/test_assertions.py cases, and the CHANGELOG Fixed entry — dropping the mcp_host.py/mcp_runtime.py/test_mcp_host.py/test_mcp_runtime.py changes. I'll review and merge it promptly; it's a security fix and I don't want it stuck behind anything.

If you'd like to contribute transport work, you're very welcome to — say so on #161 and we'll coordinate with @Flames4fun rather than duplicating effort. And going forward, please check in on an issue thread before building something large so I can tell you if it's already claimed; two contributors lost work to collisions this cycle and I'd rather that not happen again.

CI is green on the current head, for what it's worth — this is purely about scope.

@rksharma-owg

Copy link
Copy Markdown
Author

I split the scope as requested in review. I created a dedicated follow-up PR that contains only the recipient allowlist/security fix from #160: #165. It intentionally excludes MCP transport changes (now tracked by #164). Please feel free to review/merge that instead.

@rksharma-owg

Copy link
Copy Markdown
Author

Closing this superseded mixed-scope PR in favor of #165, which contains only the recipient allowlist fix, documentation, changelog entry, and focused regression tests requested in review. The MCP transport changes are excluded from #165.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants