Repository navigation
Conversation
Signed-off-by: Naveen Chatlapalli <[email protected]>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
github-actions
Bot
added
tests
agent-mesh
There was a problem hiding this comment.
Pull request overview
TL;DR: 0 blockers, 0 warnings. No issues found. Clean change.
| # | Sev | Issue | Where |
|---|---|---|---|
| — | — | No issues found | — |
This PR fixes a correctness gap in the AgentMesh relay’s in-memory inbox deduplication by scoping message identity to the recipient mailbox, aligning behavior with the wire spec’s mailbox semantics and preventing cross-recipient collisions.
Changes:
- Re-keys
InMemoryInboxStorestorage frommessage_idto(recipient_did, message_id)and updates fetch/ack/cleanup accordingly. - Makes unscoped acknowledgements refuse ambiguous IDs shared by multiple recipients.
- Adds a regression test covering same-ID delivery to multiple recipients plus scoped/unscoped acknowledgement behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| agent-governance-python/agent-mesh/src/agentmesh/relay/store.py | Scope inbox storage and lookups to (recipient_did, message_id); refuse ambiguous unscoped acknowledgements. |
| agent-governance-python/agent-mesh/tests/test_relay.py | Add regression test for per-recipient dedupe and acknowledgement isolation. |
|
message_id comes off the wire at relay/app.py:537, so any sender can pre-register an ID and permanently suppress delivery of that ID to a different recipient, and an unscoped acknowledge then drops it. That makes this a cross-recipient denial of service on the offline inbox rather than a correctness issue, which is worth stating in the description. The fix itself holds up. InMemoryInboxStore is the only implementation, and the unscoped acknowledge path becomes strictly more restrictive. |
|
Thanks — agreed, and that's a better framing than the one I opened with. I've rewritten the description with an "Impact" section stating it explicitly: because I also folded in your note that No code changes — the diff is unchanged from what you reviewed. |
|
@microsoft-github-policy-service agree |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Imran Siddique (imran-siddique)
left a comment
Imran Siddique (imran-siddique)
left a comment
There was a problem hiding this comment.
The impact section is what makes this worth prioritising, and it holds up.
message_id is taken straight off the wire at relay/app.py:537. from is stamped with the DID-PoP-verified sender identity, so it cannot be forged; the ID cannot be trusted at all. Once storage keys on the ID alone, the two mailboxes share a namespace the attacker controls. Any authenticated sender can pre-register an arbitrary ID and permanently suppress offline delivery of that ID to a different recipient, and the unscoped acknowledgement path then deletes the victim's message from whichever mailbox the global index happened to point at. So it is a cross-recipient denial of service plus a cross-recipient delete, from an authenticated but otherwise unprivileged position.
The distinction you drew is the general lesson: an identifier is only a safe key over the scope in which something guarantees its uniqueness. Message IDs are unique within a mailbox because a sender does not collide with itself, and nothing makes that true across mailboxes.
(recipient_did, message_id) is the right key. The detail I would call out is the legacy unscoped acknowledgement: rather than preserving the old behaviour, it now refuses when an ID is ambiguous across recipients. That path can delete someone else's message, so making the compatibility route strictly more restrictive than before is correct, and it is the opposite of what a compatibility shim usually does. Noting that InMemoryInboxStore is the only implementation and no caller loses relied-upon behaviour is the check that makes it safe to do.
Both of your red checks were one platform outage. docker-compose-test failed inside the image build on curl: (22) The requested URL returned error: 503 fetching the OPA binary from GitHub releases, through all its retries, and ci-complete failed only because docker-compose-test did. Nothing to do with your diff. I re-ran it and the run is green.
That 503 hit several PRs today across gitleaks, Validate PR title and this Docker build, so a maintainer triaging the queue right now will see healthy PRs looking broken.
No approve bit on this repo, so a comment, but I would merge it.
Signed-off-by: Naveen Chatlapalli <[email protected]>
Signed-off-by: Naveen Chatlapalli <[email protected]>
Signed-off-by: Naveen Chatlapalli <[email protected]> Signed-off-by: yuvrajsingh2428 <[email protected]>
Description
Fixes #3526.
InMemoryInboxStorecurrently indexes messages only bymessage_id, so a duplicate ID in a different recipient's mailbox is dropped. Message IDs are unique within a mailbox, not a safe global namespace.Impact
This is a cross-recipient denial of service on the offline inbox, not just a correctness wart.
message_idis taken straight off the wire atrelay/app.py:537(message_id = frame.get("id")). Unlikefrom, which is stamped with the DID-PoP-verified sender identity, the ID is entirely attacker-chosen. So any authenticated sender can pre-register an arbitrary ID and permanently suppress offline delivery of that ID to a different recipient. An unscoped acknowledgement then deletes the victim's message from whichever mailbox the global index happened to point at.Scoping storage to
(recipient_did, message_id)closes both halves: the mailboxes no longer share an ID namespace, and an acknowledgement can no longer reach across recipients.Changes
(recipient_did, message_id);InMemoryInboxStoreis the only implementation of this interface, and the unscoped acknowledgement path becomes strictly more restrictive, so no caller loses behaviour it previously relied on.Validation
PYTHONPATH=agent-governance-python/agent-mesh/src python -m pytest agent-governance-python/agent-mesh/tests/test_relay.py -q— 40 tests passed.python -m ruff check --select E,F --ignore E501on the changed files — passed.git diff --check.AI Assistance
Codex assisted with implementation and test drafting under user direction. This is a draft PR for human review; the contributor is responsible for reviewing and explaining every change before requesting merge.
Checklist