fix(mentions): the handle regex was safe because of a character class 850 lines away

backend/__tests__/unit/services/agentMentionService.handleMatcher.test.js

@@ -0,0 +1,63 @@
+/**
+ * @handle → Mongo matcher: escaped, not interpolated raw.
+ *
+ * @sprint-review's finding on #1157: `new RegExp(`^${username}$`, 'i')` was
+ * injection-safe only because `extractMentions` constrains handles to
+ * `[a-z0-9_-]` — and that same commit widened the character class the safety
+ * rested on. The precondition lives ~850 lines from the call site that relies
+ * on it.
+ *
+ * These cases deliberately feed metacharacters that `extractMentions` cannot
+ * currently produce. That is the point: a test restricted to today's reachable
+ * inputs passes identically before and after the fix, and would keep passing
+ * through the widening that breaks it. The property under test is the matcher's
+ * own contract, not the composition.
+ */
+
+const { handleMatcher } = require('../../../services/agentMentionService');
+
+describe('handleMatcher escapes the handle', () => {
+  it('anchors, so a prefix does not match a longer username', () => {
+    expect(handleMatcher('casey').test('casey-admin')).toBe(false);
+    expect(handleMatcher('casey').test('casey')).toBe(true);
+  });
+
+  it('is case-insensitive, because usernames are not normalized at write time', () => {
+    expect(handleMatcher('casey_dev').test('Casey_Dev')).toBe(true);
+  });
+
+  it('treats the characters extractMentions allows today as literals', () => {
+    expect(handleMatcher('a_b-c').test('a_b-c')).toBe(true);
+    expect(handleMatcher('a_b-c').test('aXbYc')).toBe(false);
+  });
+
+  // The five below are unreachable through extractMentions today. Each one
+  // fails against the raw-interpolation form and passes against the escaped
+  // one — they are the regression guard for the next time the class widens.
+  // (Counted, not eyeballed: reverting handleMatcher to the raw form fails
+  // exactly these five and leaves the three reachable-input cases above green.)
+  it('treats a dot as a literal, not as any-character', () => {
+    expect(handleMatcher('a.c').test('abc')).toBe(false);
+    expect(handleMatcher('a.c').test('a.c')).toBe(true);
+  });
+
+  it('treats an alternation as a literal', () => {
+    expect(handleMatcher('a|b').test('a')).toBe(false);
+    expect(handleMatcher('a|b').test('a|b')).toBe(true);
+  });
+
+  it('treats a quantifier as a literal', () => {
+    expect(handleMatcher('ab+').test('abbb')).toBe(false);
+    expect(handleMatcher('ab+').test('ab+')).toBe(true);
+  });
+
+  it('does not throw on an unbalanced bracket', () => {
+    expect(() => handleMatcher('a[b')).not.toThrow();
+    expect(handleMatcher('a[b').test('a[b')).toBe(true);
+  });
+
+  it('does not let a wildcard handle match every username', () => {
+    expect(handleMatcher('.*').test('someone-else')).toBe(false);
+    expect(handleMatcher('.*').test('.*')).toBe(true);
+  });
+});

backend/services/agentMentionService.ts

@@ -1016,6 +1016,24 @@ const resolveBotUserIds = async (
   return out;
 };
 
+/**
+ * Anchored, case-insensitive matcher for one @handle — escaped, not
+ * interpolated raw.
+ *
+ * This is not a live injection fix. `extractMentions` constrains handles to
+ * `[a-z0-9_-]`, and none of those are regex metacharacters, so the raw form
+ * was safe. It is a LOCALITY fix (@sprint-review on #1157): the safety rested
+ * on a character class defined ~850 lines away, and the very commit that added
+ * this lookup also widened that class. A precondition maintained in another
+ * function is one edit away from not holding, and nothing at this call site
+ * would fail when it stops — the query would just silently match the wrong
+ * users. Escaping makes the guarantee local and survives the next widening.
+ */
+const handleMatcher = (username: string): RegExp => new RegExp(
+  `^${String(username).replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}$`,
+  'i',
+);
+
 /**
  * Resolve explicit @handles that belong to humans, not installed agents.
  *
@@ -1039,7 +1057,7 @@ const resolveHumanMentionUserIds = async (
   try {
     const rows = await User.find({
       isBot: false,
-      $or: handles.map((username) => ({ username: new RegExp(`^${username}$`, 'i') })),
+      $or: handles.map((username) => ({ username: handleMatcher(username) })),
     }).select('_id username').lean() as Array<{ _id?: unknown }>;
     return new Set(
       rows
@@ -2003,6 +2021,12 @@ const enqueueDmEvent = async ({
 
 export {
   extractMentions,
+  // Exported for the escaping test: the property that matters — a metacharacter
+  // in a handle matches literally — is unreachable through `enqueueMentions`
+  // while `extractMentions` still excludes metacharacters. A test that can only
+  // observe the safe inputs cannot fail when the class widens, which is the
+  // whole defect this guards.
+  handleMatcher,
   enqueueMentions,
   enqueueDmEvent,
   MENTION_ALIASES,