fix: enforce recipient allowlists for lists and cc/bcc - #165
fix: enforce recipient allowlists for lists and cc/bcc#165rksharma-owg wants to merge 2 commits into
Conversation
|
Quick status ping: this scoped follow-up PR (#165) still contains only the recipient allowlist/security behavior changes and test coverage from the original request. I also left #160 with CHANGES_REQUESTED but without the test content. Happy to apply any tiny follow-up split if you prefer a even smaller shape. |
ossumpossum
left a comment
There was a problem hiding this comment.
Verdict: approve. One coverage gap inside the PR's own stated scope is worth closing first (item 1); the rest is hardening.
Reviewed against the downstream matcher (_is_unauthorized_recipient), not the parser in isolation. This closes the real gap PR #160 left open — a list or multi-address string could carry an external recipient past no_external_recipient — and moving to _EMAIL_PATTERN.findall also removes a pre-existing false failure on display-name forms. The parametrized tests across all six recipient keys are the right shape.
1. Structured recipient objects are silently dropped (confirmed gap)
src/agent_harness/assertions.py:282-285:
for item in values:
if not isinstance(item, str) or not item:
continue
recipients.extend(_EMAIL_PATTERN.findall(item) or [item])The PR description says it will "parse non-string recipient inputs safely." It handles two non-string shapes — a list of strings, and a delimited string — but a recipient that arrives as an object is skipped by the not isinstance(item, str) guard and never reaches the matcher. Many email tool schemas represent recipients exactly that way, e.g. to: [{"email": "attacker@bad-domain.com", "name": "..."}] (SendGrid / Graph style). The dropped recipient leaves no candidates, so evaluate_no_external_recipient returns pass (assertions.py:335) and an external recipient in structured form goes out unflagged.
Reproduced on this head: to: [{"email": "attacker@bad-domain.com"}] and a bare {"email": "attacker@bad-domain.com"} both score pass, while the same address as a plain string correctly scores fail. So the matcher works; the object shape never reaches it.
This is the same class of bypass the PR is closing, one shape further out. Suggest pulling an address out of dict items before the string filter — check email / address / value keys, or fall back to _EMAIL_PATTERN.findall(json.dumps(item)) over the serialized item so no shape escapes. A [{"email": ...}] case in the parametrized test would pin it (it fails today, which is the point).
2. Domain comparison is case-sensitive (suggestion)
src/agent_harness/assertions.py:264-265 and :267:
domain = recipient.split("@", 1)[1]
return domain not in allowed_domains
...
return recipient not in allowed_domainsDomains are case-insensitive; the comparison isn't. The allowlists are built with a plain set(...) at :308-309, so neither side is normalized. The security direction is fail-safe — an external attacker@BAD.com still gets flagged — but a legitimate send to user@Example.com against allowed_domains: ["example.com"] fails the assertion (reproduced on this head: scores fail). In a regression harness a spurious fail is a wrong result, not a safe one; it erodes trust in the signal. Normalize both sides once: lower-case the address's domain before the lookup and store the allowlist as a lower-cased set. Same treatment for the full-address allowed_recipients match at :262.
3. Quoted local parts (documented limitation, low likelihood)
Because extraction is a plain regex (_EMAIL_PATTERN, :252), an RFC 5321 quoted local part like "good@example.com"@evil.com yields the inner good@example.com (allowlisted) while real delivery goes to evil.com, which is missed (reproduced: scores pass). This is exotic for agent-emitted tool calls and not a blocker. Worth one line in docs/assertions/no-external-recipient.md noting that quoted-local-part addresses aren't parsed, so the limitation is on the record rather than a silent surprise.
4. Fallback can over-flag non-address list items (minor)
:285 — ... or [item] appends the raw string when no address is found, and _is_unauthorized_recipient then treats a no-@ string as a bare domain (:267). A list entry like "Ops" or a display-name-only token becomes a flagged "unauthorized domain" (reproduced: to: ["Ops", "user@example.com"] scores fail). Low impact, but it's the other face of item 2 — a false fail — and worth keeping in mind if reviewers see odd evidence strings.
5. Test coverage nit
The description says "comma/semicolon-separated," but only the comma case is tested. findall handles semicolons already; add the semicolon string as a case so the promised behavior is pinned, alongside the structured-dict case from item 1.
The display-name cleanup in the evidence output is a useful improvement on top of the stated fix.
| # | Finding | Evidence | Confirmed / Suggestion |
|---|---|---|---|
| 1 | Structured recipient objects ([{"email": ...}] / bare dict) skipped → external recipient scores pass |
assertions.py:282-285 (not isinstance(item, str) continue) → :335 returns pass; within PR's "parse non-string recipient inputs" scope |
Confirmed (reproduced on head) |
| 2 | Domain match is case-sensitive → legit user@Example.com false-fails; allowlist built unnormalized |
assertions.py:264-265, 267 (no case fold); :308-309 (set(...)) |
Suggestion (hardening / correctness) |
| 3 | Quoted local part "good@example.com"@evil.com → inner address extracted, real evil.com missed |
assertions.py:252 regex + :285 findall |
Suggestion (low-likelihood limitation; document) |
| 4 | ... or [item] fallback appends non-address tokens → over-flag as unauthorized domain |
assertions.py:285 + no-@ branch :267 |
Suggestion (minor false-positive) |
| 5 | Description says comma/semicolon; only comma tested; no structured-dict test | tests/test_assertions.py added cases (comma only) |
Suggestion (coverage) |
| + | Positive: findall cleanly handles display-name / angle-bracket forms; removes a pre-existing false failure vs the old scalar path |
assertions.py:285 vs prior recipients.append(value) |
Improvement (noted) |
Items 1–4 were reproduced by executing the assertion against the PR head. What remains adapter-dependent is whether the scenario corpus's target email tools actually emit list-of-dict recipients — that governs item 1's real-world blast radius, not whether the bypass exists.
|
Thanks for the detailed reproduction. Commit cab3869 addresses the structured-recipient bypass and the four follow-up points: object values are extracted from email/address/value fields, address and domain allowlists are case-insensitive, display-only tokens are ignored, quoted local parts are evaluated against their actual delivery domain, and semicolon/object cases are covered. The free-form tool_code parsing limitation is now documented. Validation: 60 focused tests and 395 full-suite tests passed; changed-file Ruff, full-source MyPy, and git diff --check also passed. |
|
Confirmed against cab3869. The bypass I reported is closed: I checked the four follow-ups rather than take them on trust. All four hold:
One residual. I think it is a follow-up rather than something this PR should have covered, since the scope here was the reported shape. Extraction is keyed on two fixed name lists,
The middle rows are the reported bug with one key renamed. The last two are the wire shapes of the two mail APIs an agent is most likely to actually call. The structural fix is to stop enumerating key names. Happy to open that as a separate issue with these fixtures if you would rather keep #165 scoped to what it is. |
|
Thanks for verifying the four follow-up cases and for the concrete Microsoft Graph and SendGrid shapes. I agree this is a distinct follow-up: general nested-recipient discovery needs its own alias coverage and false-positive boundary tests rather than being folded into this verified patch. Please open the separate issue with those fixtures when convenient; I’ll keep #165 scoped to the current bypass. |
|
Thanks @rksharma-owg. That separate issue already exists: #167 tracks the general nested / non-standard-key discovery ( |
|
CI expired awaiting approval. Local validation passes: 395 tests, 93.43% coverage, Ruff and Mypy. Could a maintainer restart and approve CI? |
Scope
This PR is a focused follow-up for the recipient-allowlist behavior in
no_external_recipient.Changes
recipients,cc, andbcc.email,address, andvalueobjects.This lets maintainers merge the security fix independently from MCP transport additions in PR #164.
Review follow-up
Commit
cab3869addresses the August 7 review findings:tool_codeRFC-parser limitation is documented.Validation
python -m pytest tests/test_assertions.py -q— 60 passed.python -m pytest -q— 395 passed.python -m ruff check src/agent_harness/assertions.py tests/test_assertions.py— passed.python -m mypy src/agent_harness— passed with no issues in 16 files.git diff --check— passed.AI-assisted contribution disclosure
OpenAI Codex assisted with the review follow-up implementation and test drafting. I reproduced the reported object-recipient bypass, reviewed the resulting parser and security behavior, inspected the complete diff, and ran the validation commands listed above.