Bug 2079840 - allow @username in the textarea like github

[dklawren]

Bug 2079840 - Support GitHub-style @Nickname mentions to needinfo and CC users

Summary

When a user with editbugs adds a comment to an existing bug that contains @nickname, each mentioned user is CC'd and gets a needinfo request. Several mentions in one comment (@alice, @bob) each get a needinfo. Safeguards limit who can trigger mentions, how many are processed, and make sure a mention never grants access to a restricted bug.

Behavior

• Matching: a mention matches the profile nickname field of an enabled account. If a nickname matches several accounts, or none, it is skipped silently. It is never guessed.
• Ignored: email addresses (foo@example.com), quoted lines (> ...), inline code (any number of backticks), fenced code blocks, and mentioning yourself.
• Needinfo vs CC: mentioned users are always CC'd. The needinfo is skipped if the user blocks needinfo or already has a needinfo pending. If a site's needinfo type is not multiplicable, at most one needinfo is requested.
• Never blocks saving: each mentioned user is processed in an eval under ERROR_MODE_DIE. A failure, such as add_cc's strict_isolation check, skips that user and logs a WARN instead of rolling back the comment.
• Private comments: a mention that appears only in private comments counts only if that user is an insider.
• Scope: only comments added to existing bugs. The description of a new bug is not parsed.
• Mass edit: mentions are processed on every bug in a mass edit.

Safeguards

• Only editbugs users trigger mentions (product-specific, the same check core uses). For everyone else, @nickname is plain text, so there is no lookup, CC or needinfo.
• At most 10 mentions per update: only the first 10 distinct nicknames are processed (MAX_MENTIONS), and the rest are ignored. This prevents notification spam from one comment.
• No access by mention: mentioned users who cannot already see the bug are skipped entirely, with no CC and no needinfo. A typo, or a nickname someone else has claimed (squatting), can never expose a security bug.

Implementation

• extensions/Needinfo/Extension.pm
  • The needinfo flag type is now created with is_multiplicable => 1, matching production. This only affects new installs.
  • bug_start_of_update now calls the existing form handling (moved unchanged into _process_needinfo_params), then _process_mentions. Mentions run second, so a needinfo just requested through the form is detected and not duplicated.
  • _extract_mentions parses the comment text. It is a pure function covered by unit tests.
  • _users_for_mentions resolves nicknames with one query and drops nicknames shared by more than one account.
  • _process_mentions applies the safeguards. Updates with no mentions return before any database work.
  • _cc_and_needinfo adds the CC and needinfo for one user.
• extensions/Needinfo/t/mentions.t
  • Parser tests.
  • Behavior tests for CC plus needinfo, skipping yourself, skipping users who can't see the bug, the cap of 10, the editbugs gate, non-multiplicable types, no duplicate needinfo, and a failing CC not throwing.

Known limitations

• Mentions are parsed with regexes, not the Markdown renderer, so indented code blocks and HTML <code> are not ignored.
• _users_for_mentions (one SQL query) has no unit test.
• Unknown or ambiguous nicknames count toward the cap of 10.

Testing

• test_sanity: extensions/Needinfo/t/mentions.t, t/001compile.t, t/002goodperl.t, t/005whitespace.t, t/critic.t all pass.
• Not yet covered: webservices or Selenium tests, and a manual check in the dev stack.

[Copilot]

🟡 Changes recommended

Multiple valid scenarios can incorrectly trigger mentions or roll back comment submission.

7 open findings

• Allow multiple Needinfo flags for multiple mentions · New
• Remove multi-backtick inline code spans from mention parsing · New
• Support pipe characters in extracted mention nicknames · New
• Prevent ineligible mention users from rolling back comment updates · New
• Implement claimed @​nickname mention behavior or correct PR scope
• Add stateful tests for mention processing and failure handling · New
• Remove redundant animation wait and duplicate timeout diagnostics

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Longer fenced code blocks can close early, causing mentions inside code to be processed.

1 open finding

• Capture full fence length to prevent premature code block closure · New

7 resolved since last review

• Allow multiple Needinfo flags for multiple mentions
• Prevent ineligible mention users from rolling back comment updates
• Support pipe characters in extracted mention nicknames
• Remove multi-backtick inline code spans from mention parsing
• Implement claimed @​nickname mention behavior or correct PR scope
• Add stateful tests for mention processing and failure handling
• Remove redundant animation wait and duplicate timeout diagnostics

🧠 Review effort: Balanced

[Xzzz]

Thanks Dave! Just one issue with the "no access by mention" safeguard when groups change in the same update, one question about API/bot comments, and a small test addition. The rest looks good to me.

[Xzzz]

Thanks! lgtm

[Copilot]

🟡 Changes recommended

Parsing and nickname matching can notify unintended users, and existing-comment previews incorrectly promise notifications.

1 open finding

• Restrict notification parameters to new-comment editors · New

1 resolved since last review

• Capture full fence length to prevent premature code block closure

Previously missed (4)

In code that hasn't changed since last review

Require valid closing fences during code extraction

extensions/​Needinfo/​Extension.pm:274

The closing-fence matcher accepts arbitrary trailing text. For ~~~text\n@before\n~~~not-a-closing-fence\n@inside\n~~~\n\n@after, extraction returns inside instead of after, potentially notifying someone mentioned only inside code. Require a closing run of the same fence character, at least as long as the opener, followed only by whitespace. Add a regression test for a fence-like line with trailing text.

Preserve mention boundaries when removing inline code

extensions/​Needinfo/​Extension.pm:277

Removing an inline code span joins the surrounding text: @alice`code`bob becomes @alicebob, potentially notifying the wrong account. Likewise, foo`code`@alice loses the mention. Replace removed spans with whitespace to preserve token boundaries, and add regression tests for both cases.

Use case-insensitive nickname matching consistently

extensions/​Needinfo/​Extension.pm:299

_mentioned_users lowercases every nickname, but this query compares against the original column. On case-sensitive backends such as PostgreSQL, nickname Alice cannot be mentioned even as @Alice. If both Alice and alice exist, only the latter is returned, bypassing the ambiguity safeguard. Use sql_istring for the column and lowercase the requested values so lookup and ambiguity detection agree.

Reject accent-insensitive nickname mismatches

extensions/​Needinfo/​Extension.pm:302

The MySQL collation (Bugzilla/DB/Mysql.pm:818–820) is accent-insensitive, so querying @jose can return an account named josé. Grouping by lowercase does not reject that unmentioned nickname, and _mentioned_users accepts it if the account is an insider and passes the visibility checks. This can CC and request needinfo from the wrong user. Filter returned rows against the requested lowercase nicknames before resolving accounts.

🧠 Review effort: Balanced

[cgsheeh]

Do you have a screenshot showing what this looks like?

LGTM with a few questions.

[dklawren]