From e5b7cce09f67e23d0f39ae6d0fa895442e96ac73 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:41:45 -0700 Subject: [PATCH 1/2] fix(mentions): the handle regex was safe because of a class 850 lines away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review flagged this on #1157 before it merged; the press landed without it. `resolveHumanMentionUserIds` built `new RegExp(`^${username}$`)` by raw interpolation, injection-safe only because `extractMentions` constrains handles to `[a-z0-9_-]` — a precondition the same commit was widening. Not a live injection: none of those characters are metacharacters. It is a locality defect. Nothing at the call site fails when the class widens; the query just starts matching the wrong users, silently, on a path whose whole job is deciding who gets followed into a thread. Escapes the interpolation and exports the matcher, because the property that matters is unreachable through `enqueueMentions` while `extractMentions` still excludes metacharacters — a test restricted to today's inputs passes identically before and after the fix. Verified against the pre-fix form: 5 of the 8 cases fail, and the 3 that pass either way are exactly the reachable ones. Co-Authored-By: Claude Opus 5 --- .../agentMentionService.handleMatcher.test.js | 61 +++++++++++++++++++ backend/services/agentMentionService.ts | 26 +++++++- 2 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 backend/__tests__/unit/services/agentMentionService.handleMatcher.test.js 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..bcd9b370c --- /dev/null +++ b/backend/__tests__/unit/services/agentMentionService.handleMatcher.test.js @@ -0,0 +1,61 @@ +/** + * @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 four 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. + 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..f9c40be9b 100644 --- a/backend/services/agentMentionService.ts +++ b/backend/services/agentMentionService.ts @@ -1030,6 +1030,24 @@ const resolveBotUserIds = async ( * the message is already durable. A Mongo failure must not turn a successful * send into a 500; it only leaves this one implicit follow unmaterialized. */ +/** + * 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', +); + const resolveHumanMentionUserIds = async ( mentions: Iterable, podMemberIds: Set, @@ -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, From 526c29d214aa3830796e5a9f73e1afd5b641571c Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 02:52:48 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(mentions):=20both=20gate=20fixes=20?= =?UTF-8?q?=E2=80=94=20the=20hoist,=20and=20the=20count=20that=20was=20off?= =?UTF-8?q?=20by=20one?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review's gate on #1206: approve on substance, two doc-level fixes. 1. The insert had orphaned `resolveHumanMentionUserIds`'s docstring. `handleMatcher` plus its new JSDoc landed BETWEEN that docstring and the function it documents, leaving two adjacent JSDoc blocks above `handleMatcher` and `resolveHumanMentionUserIds` undocumented — and the stranded block is the one carrying the precondition prose this PR exists to localize. A PR arguing that a precondition maintained elsewhere is one edit away from not holding should not move a docstring away from its function. `handleMatcher` and its doc are now hoisted above it; both functions are adjacent to their own prose again. 2. The test header said "The four below"; five tests follow. Corrected, and the count is now stated with how it was obtained — reverting the matcher to the raw form fails exactly those five and leaves the three reachable-input cases green — so the next reader can re-derive it instead of recounting by eye. No behaviour change: pure reorder plus a comment. 70/70 green across the handleMatcher suite and agentMentionService (Node 22), 0 tsc errors in the file. Co-Authored-By: Claude Opus 5 --- .../agentMentionService.handleMatcher.test.js | 4 ++- backend/services/agentMentionService.ts | 28 +++++++++---------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/backend/__tests__/unit/services/agentMentionService.handleMatcher.test.js b/backend/__tests__/unit/services/agentMentionService.handleMatcher.test.js index bcd9b370c..9dc2f866a 100644 --- a/backend/__tests__/unit/services/agentMentionService.handleMatcher.test.js +++ b/backend/__tests__/unit/services/agentMentionService.handleMatcher.test.js @@ -31,9 +31,11 @@ describe('handleMatcher escapes the handle', () => { expect(handleMatcher('a_b-c').test('aXbYc')).toBe(false); }); - // The four below are unreachable through extractMentions today. Each one + // 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); diff --git a/backend/services/agentMentionService.ts b/backend/services/agentMentionService.ts index f9c40be9b..ee2b4ea33 100644 --- a/backend/services/agentMentionService.ts +++ b/backend/services/agentMentionService.ts @@ -1016,20 +1016,6 @@ const resolveBotUserIds = async ( return out; }; -/** - * Resolve explicit @handles that belong to humans, not installed agents. - * - * The composer inserts a member's real username, while `extractMentions` - * normalizes that handle to lowercase for the agent resolver. Usernames are - * not themselves case-normalized at write time, so the lookup is anchored and - * case-insensitive rather than assuming a lowercased stored value. Handles - * are extracted from `[a-z0-9_-]`, but anchoring keeps a prefix such as - * `@casey` from following `casey-admin` too. - * - * This is deliberately best-effort for the same reason as bot resolution: - * the message is already durable. A Mongo failure must not turn a successful - * send into a 500; it only leaves this one implicit follow unmaterialized. - */ /** * Anchored, case-insensitive matcher for one @handle — escaped, not * interpolated raw. @@ -1048,6 +1034,20 @@ const handleMatcher = (username: string): RegExp => new RegExp( 'i', ); +/** + * Resolve explicit @handles that belong to humans, not installed agents. + * + * The composer inserts a member's real username, while `extractMentions` + * normalizes that handle to lowercase for the agent resolver. Usernames are + * not themselves case-normalized at write time, so the lookup is anchored and + * case-insensitive rather than assuming a lowercased stored value. Handles + * are extracted from `[a-z0-9_-]`, but anchoring keeps a prefix such as + * `@casey` from following `casey-admin` too. + * + * This is deliberately best-effort for the same reason as bot resolution: + * the message is already durable. A Mongo failure must not turn a successful + * send into a 500; it only leaves this one implicit follow unmaterialized. + */ const resolveHumanMentionUserIds = async ( mentions: Iterable, podMemberIds: Set,