diff --git a/backend/__tests__/unit/services/agentMentionService.handleMatcher.test.js b/backend/__tests__/unit/services/agentMentionService.handleMatcher.test.js new file mode 100644 index 000000000..9dc2f866a --- /dev/null +++ b/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); + }); +}); diff --git a/backend/services/agentMentionService.ts b/backend/services/agentMentionService.ts index 1f9081cbf..ee2b4ea33 100644 --- a/backend/services/agentMentionService.ts +++ b/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,