🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🟠 Mention-required Matrix room messages processed before mention drop (DoS via poll/media/thread resolution)

Description

In createMatrixRoomMessageHandler, the mention requirement drop for room messages is now deferred when there is non-empty message text (mentionPrecheckText). This allows unmentioned room messages (in rooms configured with requireMention) to still execute potentially expensive processing before being dropped.

Impact:

• An attacker in an open/public room can spam unmentioned messages with text to trigger work that previously short-circuited.
• Before the deferred no-mention return, the handler may perform:
  • Poll snapshot resolution (fetchMatrixPollSnapshot) which calls Matrix APIs like getEvent and paginated getRelations.
  • Media download/decryption (downloadMatrixMedia) up to mediaMaxBytes.
  • Thread context resolution (resolveThreadContext → client.getEvent).
  • Route/binding resolution that can also have side effects (e.g., sessionBindingService.touch(...)).

This creates a denial-of-service vector (CPU/network/IO amplification) precisely in the pathway meant to reduce processing for unmentioned room chatter.

Vulnerable logic (defer drop + drop only after route resolution):

​// ...
if (isRoom && shouldRequireMention && !wasMentioned && !shouldBypassMention && !mentionPrecheckText) {
  logger.info("skipping room message", { roomId, reason: "no-mention" });
  return;
}
const mentionDropDeferred =
  isRoom && shouldRequireMention && !wasMentioned && !shouldBypassMention;

​// ... expensive work occurs here (poll/media/thread resolution) ...

const { route } = resolveMatrixInboundRoute({ /* ... */ });

if (mentionDropDeferred) {
  ​// ... re-resolve mentions using agent-level patterns ...
  if (!wasMentioned) {
    logger.info("skipping room message", { roomId, reason: "no-mention" });
    return;
  }
}

Recommendation

Perform route resolution + agent-level mention re-check before any expensive processing, and return early if the message is still unmentioned.

One safe restructuring pattern:

​// after initial resolveMentions + shouldRequireMention/shouldBypassMention computation
const needsAgentMentionCheck =
  isRoom && shouldRequireMention && !wasMentioned && !shouldBypassMention;

if (needsAgentMentionCheck) {
  ​// Resolve route early (cheap) so we can build agent-specific mention patterns
  const { route: preRoute } = resolveMatrixInboundRoute({
    cfg,
    accountId,
    roomId,
    senderId,
    isDirectMessage,
    messageId,
    threadRootId,
    eventTs: eventTs ?? undefined,
    resolveAgentRoute: core.channel.routing.resolveAgentRoute,
  });

  const agentMentionRegexes = core.channel.mentions.buildMentionRegexes(cfg, preRoute.agentId);
  const agentMentionResult = resolveMentions({
    content,
    userId: selfUserId,
    text: mentionPrecheckText,
    mentionRegexes: agentMentionRegexes,
  });

  if (!agentMentionResult.wasMentioned) {
    logger.info("skipping room message", { roomId, reason: "no-mention" });
    return; // BEFORE poll snapshot, media download, thread lookups, etc.
  }

  ​// carry forward route/agentId from preRoute to avoid resolving twice
}

​// only now do fetchMatrixPollSnapshot / downloadMatrixMedia / resolveThreadContext

Additionally consider:

• Skipping fetchMatrixPollSnapshot until after mention acceptance.
• Adding rate limiting / concurrency limits for media downloads and poll relation pagination per room/sender.

Analyzed PR: #51106 at commit a46e328

_{Last updated on: 2026-03-20T16:34:03Z}

Greptile Summary

This PR fixes a bug (#51082) where the Matrix plugin's mention-required filter was dropping all messages in rooms with requireMention: true when mention patterns were configured only at the agent level. The root cause was that buildMentionRegexes(cfg) was called without agentId, so agent-scoped patterns were never applied.

The fix defers the mention-required drop to after resolveMatrixInboundRoute, at which point route.agentId is available and agent-level regexes can be resolved and checked. Media-only and no-body events are still dropped early via the new !mentionPrecheckText guard, preserving the existing optimization.

Key changes:

• handler.ts: Early drop now also requires !mentionPrecheckText; a mentionDropDeferred flag triggers a second resolveMentions pass with buildMentionRegexes(cfg, route.agentId) after route resolution.
• handler.test-helpers.ts: buildMentionRegexes added to the test harness, defaulting to () => [].
• handler.test.ts: New test verifying agent-level patterns are checked post-route-resolution; two existing tests updated to reflect that route resolution now occurs before the final mention drop for text messages.
• docs/automation/standing-orders.md: Formatting-only whitespace and table alignment changes.

Notable concern: canDetectMention (line 556 of handler.ts) is computed from channel-level mentionRegexes and hasExplicitMention before the deferred agent-pattern check. When agent-level patterns are the sole reason wasMentioned becomes true, canDetectMention remains false, which is passed to shouldAckReaction — potentially suppressing ack reactions in this scenario.

Confidence Score: 3/5

• Safe to merge with minor caveats — the core fix is correct, but a stale canDetectMention value and removed test assertions reduce confidence slightly.
• The fix correctly addresses the stated bug and matches the pattern used by Zalo and Discord. The deferred-drop approach is sound. Two concerns bring the score down from a 4: (1) canDetectMention is not updated after agent-level pattern resolution, which could suppress ack reactions for the exact scenario this PR enables; (2) three meaningful assertions were removed from the media-download test even though the underlying behavior did not change for media-only messages, reducing coverage unnecessarily.
• extensions/matrix/src/matrix/monitor/handler.ts — review the canDetectMention variable usage at line 556 and line 804 in light of the new deferred agent-pattern path.

This is a comment left during a code review.
Path: extensions/matrix/src/matrix/monitor/handler.ts
Line: 556

Comment:
**`canDetectMention` not updated after agent-level pattern resolution**

`canDetectMention` is computed here using only channel-level `mentionRegexes` and `hasExplicitMention`. When the deferred agent-level check later sets `wasMentioned = true` (via `agentMentionRegexes`), `canDetectMention` remains `false` if the channel-level regexes were empty and no explicit Matrix `m.mentions` was present.

This value is then passed to `shouldAckReaction` at line 804:
```ts
canDetectMention,
effectiveWasMentioned: wasMentioned || shouldBypassMention,  // may now be true
```

This creates an inconsistency: the bot detected a mention (`wasMentioned = true`) but `canDetectMention = false`. Depending on `shouldAckReaction`'s internal logic, ack reactions could be silently suppressed for messages that match only agent-level patterns.

Consider updating `canDetectMention` after the deferred check resolves:
```ts
// after the deferred block
const canDetectMention =
  mentionRegexes.length > 0 || hasExplicitMention || wasMentioned;
```

Or, re-evaluate it before it is consumed by `shouldAckReaction`.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: extensions/matrix/src/matrix/monitor/handler.test.ts
Line: 476-479

Comment:
**Removed assertions were still valid for media-only messages**

The assertions `expect(getMemberDisplayName).not.toHaveBeenCalled()`, `expect(getRoomInfo).not.toHaveBeenCalled()`, and `expect(resolveAgentRoute).not.toHaveBeenCalled()` were removed from the "skips media downloads for unmentioned group media messages" test.

For media-only events (`body: ""`), `mentionPrecheckText` is empty. The new early-drop guard:
```ts
if (isRoom && shouldRequireMention && !wasMentioned && !shouldBypassMention && !mentionPrecheckText) {
  return; // fires for empty body
}
```
still fires before `getSenderName()`, `getRoomInfo()`, and `resolveMatrixInboundRoute()` are reached (lines 647, 648, 664). The three removed assertions should still pass and serve as useful guardrails confirming the early-exit optimization is preserved for media-only messages.

Compare this with the poll-response test at line 481 which still retains all three equivalent assertions — it would be consistent to keep them here too.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "fix(matrix): pass ag..."}

Thanks for chasing the same Matrix root cause here. I ran a de-dupe pass on the active cluster around #51082, and this one overlaps directly with #51272 on the same buildMentionRegexes(..., agentId) failure mode.

I'm keeping #51272 as the canonical path because it resolves route selection before the mention check, stays narrower in the handler flow, and is currently green across CI. This branch also avoids carrying the deferred-drop shape that triggered extra review and red checks here.

Credit to you for surfacing the same fix direction and for the additional review context around mention detection. If there's still behavior left uncovered after #51272 lands, reopen or spin a focused follow-up and link it back here.

Totally fair - #51272's approach of resolving route before the mention check is cleaner. Glad the fix direction landed. I'll keep an eye on #51272 and follow up if anything's left uncovered.

@mvanhorn , @vincentkoc - appreciate it!