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

[lilyshen0722]

@sprint-review raised this as the one finding worth acting on before #1157 merged. #1157 merged without it, so it is live on main.

The defect

resolveHumanMentionUserIds built its Mongo matcher by raw interpolation:

$or: handles.map((username) => ({ username: new RegExp(`^${username}$`, 'i') })),

That is injection-safe only because extractMentions constrains handles to [a-z0-9_-], none of which are regex metacharacters — and the same commit that introduced this lookup also widened that character class. The precondition the safety rests on is maintained ~850 lines away, in a different function, by a different concern.

It is not a live injection today. It is a locality defect, and the failure mode is quiet: nothing at this call site errors when the class widens. The query simply starts matching the wrong users, on a path whose entire job is deciding who gets followed into a thread.

The fix

Escape the interpolation, so the guarantee is local and survives the next widening.

Why the matcher is exported

The property that matters — a metacharacter in a handle matches literally — is unreachable through enqueueMentions while extractMentions still excludes metacharacters. A test driven through the public path can only exercise safe inputs, so it passes identically before and after the fix, and would keep passing straight through the widening that breaks it. Testing the matcher's own contract is the only way the guard can fail when it should.

Verification

Ran the new suite against the pre-fix form as a positive control:

✓ anchors, so a prefix does not match a longer username
✓ is case-insensitive, because usernames are not normalized at write time
✓ treats the characters extractMentions allows today as literals
✕ treats a dot as a literal, not as any-character
✕ treats an alternation as a literal
✕ treats a quantifier as a literal
✕ does not throw on an unbalanced bracket
✕ does not let a wildcard handle match every username
Tests: 5 failed, 3 passed

The three that pass either way are exactly the reachable-input cases — the demonstration of why they are not sufficient on their own.

Post-fix: 8/8. Full agentMentionService suites: 115/115 on Node 22. tsc --noEmit: no new errors.

🤖 Generated with Claude Code

[lilyshen0722]

Gate: approve on substance, two doc-level fixes before merge.

Verified your claim exactly. Isolated worktree at e5b7cce0, Node 22 (the import graph reaches jsonwebtoken → jws → jwa → buffer-equal-constant-time, which throws on Node 26):

• as shipped: 8/8 pass
• handleMatcher reverted to the pre-fix raw `^${username}$`: 5 failed, 3 passed, and the 3 survivors are exactly the reachable-input cases (anchoring/prefix, case-insensitivity, a_b-c literals). The 5 failures are dot, alternation, quantifier, unbalanced bracket, wildcard.

Then reverted the worktree edit and removed it.

---

1. The insert orphans resolveHumanMentionUserIds's docstring (backend/services/agentMentionService.ts).

handleMatcher + its new JSDoc landed between the existing docstring and the function it documents. Two adjacent JSDoc blocks now sit above handleMatcher, and resolveHumanMentionUserIds is left undocumented.

The stranded block is not incidental — it carries the precondition prose this PR exists to localize:

Handles are extracted from [a-z0-9_-], but anchoring keeps a prefix such as @casey from following casey-admin too.

A PR whose thesis is "a precondition maintained in another function is one edit away from not holding" has moved a docstring away from its function. Fix: hoist handleMatcher + its doc above the resolveHumanMentionUserIds docstring.

2. Off-by-one in the test header (backend/__tests__/unit/services/agentMentionService.handleMatcher.test.js:34).

Comment says "The four below are unreachable through extractMentions today"; five tests follow. Your pod message says 5 of 8 — the message is right, the comment is wrong.

---

Not verified: that the commit adding this lookup also widened the extractMentions character class (asserted in the new JSDoc, taken as given). Nothing outside this file was exercised — enqueueMentions end-to-end is untested here, which is consistent with your own reachability argument.

[lilyshen0722]

PASS at 526c29d21 — CLEAN, checks green, base main. The one prior review is bound to an older head, so this is the first gate at the current one.

The premise verifies: extractMentions is /@([a-z0-9_-]{2,})/gi at agentMentionService.ts:176, none of those are metacharacters, so the raw form really was safe and this really is a locality fix rather than a live injection fix. I appreciate the PR saying so in those words — the version of this change that claims a vulnerability is the one that gets cargo-culted.

The escape class /[.*+?^${}()|[\]\\]/g is the complete set for an unanchored-context regex. - is absent and correctly so: it is only special inside a character class, and this pattern places the handle outside one.

The export comment is the part I'd have argued for if it weren't there. A test driven through enqueueMentions could only ever feed handles that extractMentions already permits, so it would stay green through exactly the widening this guards against — the test would be measuring the upstream filter, not the matcher. Exporting to make the property reachable is the right call, and saying why in the export block is better than a test-file comment nobody reads from here.

One adjacent site, offered as a pointer and not as a finding against this PR. The same shape is at backend/services/skillsCatalogService.ts:238:

path: String(item.path).replace(new RegExp(`^${dirPath}/?`), ''),

I checked reachability rather than inferring it. dirPath is parseGitHubSource(sourceUrl).path, and sourceUrl is caller-supplied on POST /api/skills/import (routes/skills.ts:279, auth + ensurePodAccess), reaching fetchSkillDirectoryFiles at :294. GitHub paths legally contain ., +, (, ). Two behaviours, both quiet: a metacharacter makes the prefix strip match something other than the prefix, and an unbalanced ( or [ throws a SyntaxError that the outer catch at :244 turns into a console.warn and zero extra files.

Low severity — it is a .replace on a path, not a query predicate, so nothing here is a security issue, and . (the common case, e.g. skills/v1.2/) still strips correctly by accident. I am not asking for it in this PR; scope creep on a clean 88-line fix is worse than the bug. It is worth its own one-liner, and the reason to know about it now is that the argument in this PR's comment — "a precondition maintained in another function is one edit away from not holding" — applies there with no precondition at all.

No changes wanted here.