From a62bcad27f87bdadd56faf174552e490fe4024b9 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 8 Oct 2026 15:13:12 -0400 Subject: [PATCH 1/8] feat: Bug 2079840 - Support GitHub-style @nickname mentions to needinfo and CC users Each @nickname in a new comment on an existing bug CCs the matching user and requests needinfo from them. Mentions in quotes, code and email addresses are ignored, ambiguous nicknames are skipped, and a mention never blocks the comment from being saved. --- extensions/Needinfo/Extension.pm | 89 +++++++++++++++++++++++++++++++- extensions/Needinfo/t/mentions.t | 45 ++++++++++++++++ 2 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 extensions/Needinfo/t/mentions.t diff --git a/extensions/Needinfo/Extension.pm b/extensions/Needinfo/Extension.pm index 79331e13eb..a7c6fa10ff 100644 --- a/extensions/Needinfo/Extension.pm +++ b/extensions/Needinfo/Extension.pm @@ -97,10 +97,19 @@ sub bug_end_of_create { $self->bug_start_of_update($args); } -# Clear the needinfo? flag if comment is being given by -# requestee or someone used the override flag. sub bug_start_of_update { my ($self, $args) = @_; + _process_needinfo_params($args); + + # Runs after the explicit needinfo handling so that the duplicate check + # sees any flags that were just requested through the form. + _process_mentions($args->{bug}) if $args->{old_bug}; +} + +# Clear the needinfo? flag if comment is being given by +# requestee or someone used the override flag. +sub _process_needinfo_params { + my ($args) = @_; my $bug = $args->{bug}; my $old_bug = $args->{old_bug}; @@ -248,6 +257,82 @@ sub bug_start_of_update { } } +# Returns the unique nicknames @mentioned in a comment, in order of first +# appearance. Mentions inside quoted lines (> ...) and code are ignored, as +# are email addresses (the @ must not follow a word character). +sub _extract_mentions { + my ($text) = @_; + return () unless defined $text; + + $text =~ s/^[ \t]*(```|~~~).*?(?:^[ \t]*\1[^\n]*$|\z)//msg; + $text =~ s/`[^`\n]*`//g; + $text =~ s/^[ \t]*>.*$//mg; + + my (@nicks, %seen); + while ($text =~ /(?dbh; + my $rows = $dbh->selectall_arrayref( + 'SELECT userid, nickname FROM profiles WHERE is_enabled = 1 AND ' + . $dbh->sql_in('nickname', [map { $dbh->quote($_) } @nicks])); + + my %ids_by_nick; + push @{$ids_by_nick{lc $_->[1]}}, $_->[0] foreach @$rows; + my @ids = map { $_->[0] } grep { @$_ == 1 } values %ids_by_nick; + return @{Bugzilla::User->new_from_list(\@ids)}; +} + +# GitHub-style mentions: each @nickname in a new comment is CC'd and gets a +# needinfo request. Mentions never block the comment from being saved; anyone +# who can't be needinfo'd (blocked, already asked, no permission) is only CC'd. +sub _process_mentions { + my ($bug) = @_; + my $user = Bugzilla->user; + + # Lowercased nick => true if mentioned in at least one public comment. + my %public; + foreach my $comment (@{$bug->{added_comments} || []}) { + $public{lc $_} ||= !$comment->{isprivate} + foreach _extract_mentions($comment->{thetext}); + } + + my @mentioned + = grep { $_->id != $user->id && ($public{lc $_->nick} || $_->is_insider) } + _users_for_mentions(keys %public); + return unless @mentioned; + + my ($type) = grep { $_->name eq 'needinfo' } @{$bug->flag_types}; + my $can_flag + = $type && $bug->check_can_change_field('flagtypes.name', 0, 1)->{allowed}; + + my @new_flags; + foreach my $mentioned (@mentioned) { + $bug->add_cc($mentioned); + + next if !$can_flag || $mentioned->needinfo_blocked; + next if grep { + $_->status eq '?' && ($_->requestee_id // 0) == $mentioned->id + } @{$type->{flags}}; + + push @new_flags, + {type_id => $type->id, status => '?', requestee => $mentioned->login}; + } + + $bug->set_flags([], \@new_flags) if @new_flags; +} + sub _check_requestee { my ($requestee) = @_; my $user diff --git a/extensions/Needinfo/t/mentions.t b/extensions/Needinfo/t/mentions.t new file mode 100644 index 0000000000..bc937e52ef --- /dev/null +++ b/extensions/Needinfo/t/mentions.t @@ -0,0 +1,45 @@ +#!/usr/bin/env perl +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# +# This Source Code Form is "Incompatible With Secondary Licenses", as +# defined by the Mozilla Public License, v. 2.0. +use strict; +use warnings; +use lib qw( . lib local/lib/perl5 ); + +BEGIN { + $ENV{LOG4PERL_CONFIG_FILE} = 'log4perl-t.conf'; + $ENV{BUGZILLA_DISABLE_HOSTAGE} = 1; +} + +use Bugzilla::Test::MockDB; +use Bugzilla::Test::MockParams; + +use Test2::V0; + +use Bugzilla; +BEGIN { Bugzilla->extensions } + +my $extract = \&Bugzilla::Extension::Needinfo::_extract_mentions; + +is([$extract->(undef)], [], 'undef text'); +is([$extract->('no mentions here')], [], 'no mentions'); +is([$extract->('@alice can you look?')], ['alice'], 'leading mention'); +is([$extract->('thanks @alice, @bob and @carol.')], + ['alice', 'bob', 'carol'], 'multiple mentions with punctuation'); +is([$extract->('@alice and @Alice again @alice')], ['alice'], + 'duplicates collapsed case-insensitively'); +is([$extract->('(@dk.l_x-y)')], ['dk.l_x-y'], 'nick symbols, parens'); +is([$extract->('mail foo@example.com or a/@b or x.@y')], [], + 'emails and embedded @ ignored'); +is([$extract->("> \@quoted said\n\@alice")], ['alice'], 'quoted lines ignored'); +is([$extract->('run `@decorator` @alice')], ['alice'], 'inline code ignored'); +is( + [$extract->("```perl\n\@inside\n```\n\@after")], + ['after'], 'fenced code block ignored' +); +is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); + +done_testing; From d8b9968d72641555bc11b89db7450f47deeabeda Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 8 Oct 2026 16:28:00 -0400 Subject: [PATCH 2/8] feat: Bug 2079840 - Add misuse safeguards to @nickname mentions Only editbugs users can trigger mentions, at most 10 distinct nicknames are processed per update, and mentioned users who cannot already see the bug are skipped so a mention, typo or squatted nickname never grants access to a restricted bug. --- extensions/Needinfo/Extension.pm | 33 ++++++++++--- extensions/Needinfo/t/mentions.t | 80 ++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 7 deletions(-) diff --git a/extensions/Needinfo/Extension.pm b/extensions/Needinfo/Extension.pm index a7c6fa10ff..d867a51d67 100644 --- a/extensions/Needinfo/Extension.pm +++ b/extensions/Needinfo/Extension.pm @@ -20,6 +20,8 @@ use Bugzilla::User::Setting; our $VERSION = '0.01'; +use constant MAX_MENTIONS => 10; + BEGIN { *Bugzilla::User::needinfo_blocked = \&_user_needinfo_blocked; } @@ -297,20 +299,37 @@ sub _users_for_mentions { # GitHub-style mentions: each @nickname in a new comment is CC'd and gets a # needinfo request. Mentions never block the comment from being saved; anyone # who can't be needinfo'd (blocked, already asked, no permission) is only CC'd. +# +# Safeguards against misuse: +# - only editbugs users can trigger mentions; for everyone else they are text +# - only the first MAX_MENTIONS distinct nicknames per update are processed +# - users who can't already see the bug are skipped, so a mention (or a typo, +# or a squatted nickname) can never grant access to a restricted bug sub _process_mentions { my ($bug) = @_; - my $user = Bugzilla->user; - # Lowercased nick => true if mentioned in at least one public comment. my %public; + + # The first MAX_MENTIONS distinct lowercased nicks, in order of appearance. + my @nicks; + foreach my $comment (@{$bug->{added_comments} || []}) { - $public{lc $_} ||= !$comment->{isprivate} - foreach _extract_mentions($comment->{thetext}); + foreach my $nick (map {lc} _extract_mentions($comment->{thetext})) { + push @nicks, $nick if !exists $public{$nick} && @nicks < MAX_MENTIONS; + $public{$nick} ||= !$comment->{isprivate}; + } } + return unless @nicks; + + # Checked after parsing so updates without mentions skip the group lookup. + my $user = Bugzilla->user; + return unless $user->in_group('editbugs', $bug->product_id); - my @mentioned - = grep { $_->id != $user->id && ($public{lc $_->nick} || $_->is_insider) } - _users_for_mentions(keys %public); + my @mentioned = grep { + $_->id != $user->id + && ($public{lc $_->nick} || $_->is_insider) + && $_->can_see_bug($bug->id) + } _users_for_mentions(@nicks); return unless @mentioned; my ($type) = grep { $_->name eq 'needinfo' } @{$bug->flag_types}; diff --git a/extensions/Needinfo/t/mentions.t b/extensions/Needinfo/t/mentions.t index bc937e52ef..00716261fa 100644 --- a/extensions/Needinfo/t/mentions.t +++ b/extensions/Needinfo/t/mentions.t @@ -42,4 +42,84 @@ is( ); is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); +# _process_mentions safeguards, using minimal stand-ins for users and bugs. +{ + + package FakeUser; + sub new { my ($class, %args) = @_; bless {%args}, $class } + sub id { $_[0]{id} } + sub nick { $_[0]{nick} } + sub login { "$_[0]{nick}\@example.com" } + sub in_group { $_[0]{editbugs} } + sub is_insider { $_[0]{insider} } + sub can_see_bug { $_[0]{sees_bug} // 1 } + sub needinfo_blocked { 0 } + + package FakeBug; + sub new { + my ($class, $text) = @_; + bless { + added_comments => [{thetext => $text}], + cc => [], + needinfo => [], + type => bless({flags => []}, 'FakeType'), + }, $class; + } + sub id {1} + sub product_id {1} + sub flag_types { [$_[0]{type}] } + sub check_can_change_field { {allowed => 1} } + sub add_cc { push @{$_[0]{cc}}, $_[1]->nick } + sub set_flags { push @{$_[0]{needinfo}}, map { $_->{requestee} } @{$_[2]} } + + package FakeType; + sub name {'needinfo'} + sub id {1} +} + +my %users = map { $_->nick => $_ } ( + FakeUser->new(id => 2, nick => 'alice'), + FakeUser->new(id => 3, nick => 'bob'), + FakeUser->new(id => 4, nick => 'hidden', sees_bug => 0), + map { FakeUser->new(id => 10 + $_, nick => "u$_") } 1 .. 12, +); +my $commenter = FakeUser->new(id => 1, nick => 'me', editbugs => 1); +my @looked_up; + +my $mock_bugzilla = mock 'Bugzilla' => (override => [user => sub {$commenter}]); +my $mock_ext = mock 'Bugzilla::Extension::Needinfo' => ( + override => [ + _users_for_mentions => sub { + @looked_up = @_; + return grep {defined} map { $users{$_} } @_; + }, + ], +); + +my $process = \&Bugzilla::Extension::Needinfo::_process_mentions; + +my $bug = FakeBug->new('@alice and @bob please look, cc @me'); +$process->($bug); +is($bug->{cc}, ['alice', 'bob'], 'mentioned users CCd, self skipped'); +is($bug->{needinfo}, ['alice@example.com', 'bob@example.com'], + 'mentioned users needinfod'); + +$bug = FakeBug->new('@hidden @alice'); +$process->($bug); +is($bug->{cc}, ['alice'], 'user who cannot see the bug is not CCd'); +is($bug->{needinfo}, ['alice@example.com'], '... nor needinfod'); + +$bug = FakeBug->new(join ' ', map {"\@u$_"} 1 .. 12); +$process->($bug); +is(\@looked_up, [map {"u$_"} 1 .. 10], 'only the first 10 nicknames are looked up'); +is($bug->{cc}, [map {"u$_"} 1 .. 10], '... and only they are CCd'); + +$commenter->{editbugs} = 0; +@looked_up = (); +$bug = FakeBug->new('@alice'); +$process->($bug); +is(\@looked_up, [], 'no lookup for commenters without editbugs'); +is($bug->{cc}, [], 'no CC for commenters without editbugs'); +is($bug->{needinfo}, [], 'no needinfo for commenters without editbugs'); + done_testing; From 57c588d1cb1633474ef8ba10d89822205c67de84 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 8 Oct 2026 17:13:19 -0400 Subject: [PATCH 3/8] fix: Bug 2079840 - Never let @mentions block a comment from saving - Create the needinfo flag type as multiplicable, matching production. - With a non-multiplicable needinfo type, request at most one needinfo instead of throwing flag_type_not_multiplicable. - Process each mentioned user in an eval under ERROR_MODE_DIE so a failure (e.g. add_cc's strict_isolation check) skips that user instead of rolling back the update. - Ignore mentions in multi-backtick code spans and allow | in nicknames, matching extract_nicks. --- extensions/Needinfo/Extension.pm | 56 ++++++++++++++++++++---------- extensions/Needinfo/t/mentions.t | 58 ++++++++++++++++++++++++++++---- 2 files changed, 90 insertions(+), 24 deletions(-) diff --git a/extensions/Needinfo/Extension.pm b/extensions/Needinfo/Extension.pm index d867a51d67..36b510c980 100644 --- a/extensions/Needinfo/Extension.pm +++ b/extensions/Needinfo/Extension.pm @@ -12,9 +12,11 @@ use warnings; use base qw(Bugzilla::Extension); +use Bugzilla::Constants; use Bugzilla::Error; use Bugzilla::Flag; use Bugzilla::FlagType; +use Bugzilla::Logging; use Bugzilla::User; use Bugzilla::User::Setting; @@ -62,7 +64,7 @@ sub install_update_db { is_active => 1, is_requestable => 1, is_requesteeble => 1, - is_multiplicable => 0, + is_multiplicable => 1, request_group => '', grant_group => '', inclusions => ['0:0'], @@ -267,12 +269,15 @@ sub _extract_mentions { return () unless defined $text; $text =~ s/^[ \t]*(```|~~~).*?(?:^[ \t]*\1[^\n]*$|\z)//msg; - $text =~ s/`[^`\n]*`//g; + # A code span closes on a backtick run of the same length, within a paragraph. + $text = join "\n\n", + map { s/(?.*$//mg; my (@nicks, %seen); - while ($text =~ /(?name eq 'needinfo' } @{$bug->flag_types}; - my $can_flag - = $type && $bug->check_can_change_field('flagtypes.name', 0, 1)->{allowed}; - - my @new_flags; + undef $type + unless $type && $bug->check_can_change_field('flagtypes.name', 0, 1)->{allowed}; + + # A failure for one user (e.g. add_cc's strict_isolation check) skips that + # user rather than rolling back the whole update. Throw*Error only dies + # (instead of printing an error page and exiting) in ERROR_MODE_DIE, so the + # eval can catch it. + local Bugzilla->request_cache->{error_mode} = ERROR_MODE_DIE; + local $@; foreach my $mentioned (@mentioned) { - $bug->add_cc($mentioned); + eval { _cc_and_needinfo($bug, $mentioned, $type); 1 } + or WARN('Skipped mention of ' . $mentioned->login . " on bug " + . $bug->id . ": $@"); + } +} - next if !$can_flag || $mentioned->needinfo_blocked; - next if grep { - $_->status eq '?' && ($_->requestee_id // 0) == $mentioned->id - } @{$type->{flags}}; +sub _cc_and_needinfo { + my ($bug, $mentioned, $type) = @_; + $bug->add_cc($mentioned); - push @new_flags, - {type_id => $type->id, status => '?', requestee => $mentioned->login}; - } + return if !$type || $mentioned->needinfo_blocked; + + # A non-multiplicable type allows only one needinfo flag per bug. + return if !$type->is_multiplicable && @{$type->{flags}}; + return if grep { + $_->status eq '?' && ($_->requestee_id // 0) == $mentioned->id + } @{$type->{flags}}; - $bug->set_flags([], \@new_flags) if @new_flags; + # One call per user so $type->{flags} reflects each flag as it is added. + $bug->set_flags([], + [{type_id => $type->id, status => '?', requestee => $mentioned->login}]); } sub _check_requestee { diff --git a/extensions/Needinfo/t/mentions.t b/extensions/Needinfo/t/mentions.t index 00716261fa..dabcd7bcad 100644 --- a/extensions/Needinfo/t/mentions.t +++ b/extensions/Needinfo/t/mentions.t @@ -36,6 +36,11 @@ is([$extract->('mail foo@example.com or a/@b or x.@y')], [], 'emails and embedded @ ignored'); is([$extract->("> \@quoted said\n\@alice")], ['alice'], 'quoted lines ignored'); is([$extract->('run `@decorator` @alice')], ['alice'], 'inline code ignored'); +is([$extract->('``@alice`` and `` a`@b ``')], [], 'double backtick code ignored'); +is([$extract->("it's a ` stray\n\n\@alice `x`")], + ['alice'], 'unclosed backtick does not span paragraphs'); +is([$extract->('@foo|bar and |@alice|')], ['foo|bar', 'alice'], + 'pipe allowed in nick, trailing pipe stripped'); is( [$extract->("```perl\n\@inside\n```\n\@after")], ['after'], 'fenced code block ignored' @@ -57,30 +62,49 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); package FakeBug; sub new { - my ($class, $text) = @_; + my ($class, $text, %type) = @_; bless { added_comments => [{thetext => $text}], cc => [], needinfo => [], - type => bless({flags => []}, 'FakeType'), + type => bless({flags => [], multiplicable => 1, %type}, 'FakeType'), }, $class; } sub id {1} sub product_id {1} sub flag_types { [$_[0]{type}] } sub check_can_change_field { {allowed => 1} } - sub add_cc { push @{$_[0]{cc}}, $_[1]->nick } - sub set_flags { push @{$_[0]{needinfo}}, map { $_->{requestee} } @{$_[2]} } + sub add_cc { + my ($self, $user) = @_; + die "add_cc failed\n" if $user->{cc_fails}; + push @{$self->{cc}}, $user->nick; + } + + # Like Bugzilla::Flag->set_flag, new flags are added to the type's list. + sub set_flags { + my ($self, undef, $new_flags) = @_; + foreach my $flag (@$new_flags) { + push @{$self->{needinfo}}, $flag->{requestee}; + push @{$self->{type}{flags}}, + bless({status => '?', requestee_id => 0}, 'FakeFlag'); + } + } package FakeType; - sub name {'needinfo'} - sub id {1} + sub name {'needinfo'} + sub id {1} + sub is_multiplicable { $_[0]{multiplicable} } + + package FakeFlag; + sub status { $_[0]{status} } + sub requestee_id { $_[0]{requestee_id} } } my %users = map { $_->nick => $_ } ( FakeUser->new(id => 2, nick => 'alice'), FakeUser->new(id => 3, nick => 'bob'), FakeUser->new(id => 4, nick => 'hidden', sees_bug => 0), + FakeUser->new(id => 6, nick => 'boom', cc_fails => 1), map { FakeUser->new(id => 10 + $_, nick => "u$_") } 1 .. 12, ); my $commenter = FakeUser->new(id => 1, nick => 'me', editbugs => 1); @@ -114,6 +138,28 @@ $process->($bug); is(\@looked_up, [map {"u$_"} 1 .. 10], 'only the first 10 nicknames are looked up'); is($bug->{cc}, [map {"u$_"} 1 .. 10], '... and only they are CCd'); +$bug = FakeBug->new('@alice @bob', multiplicable => 0); +$process->($bug); +is($bug->{cc}, ['alice', 'bob'], 'non-multiplicable type: everyone CCd'); +is($bug->{needinfo}, ['alice@example.com'], '... but only one needinfo'); + +$bug = FakeBug->new('@alice', multiplicable => 0); +push @{$bug->{type}{flags}}, bless({status => '?', requestee_id => 9}, 'FakeFlag'); +$process->($bug); +is($bug->{cc}, ['alice'], 'non-multiplicable with existing flag: CCd'); +is($bug->{needinfo}, [], '... and no needinfo, instead of an error'); + +$bug = FakeBug->new('@alice'); +push @{$bug->{type}{flags}}, bless({status => '?', requestee_id => 2}, 'FakeFlag'); +$process->($bug); +is($bug->{needinfo}, [], 'no duplicate needinfo for a pending request'); + +$bug = FakeBug->new('@boom @alice'); +ok(lives { $process->($bug) }, + 'a failing CC (e.g. strict_isolation) does not throw'); +is($bug->{cc}, ['alice'], '... that user is skipped'); +is($bug->{needinfo}, ['alice@example.com'], '... others still processed'); + $commenter->{editbugs} = 0; @looked_up = (); $bug = FakeBug->new('@alice'); From fae49e47b987f54b18a534a36776d84cc342815d Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 8 Oct 2026 17:35:39 -0400 Subject: [PATCH 4/8] Potential fix for pull request finding 'Capture full fence length to prevent premature code block closure' Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- extensions/Needinfo/Extension.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/Needinfo/Extension.pm b/extensions/Needinfo/Extension.pm index 36b510c980..4b6051d390 100644 --- a/extensions/Needinfo/Extension.pm +++ b/extensions/Needinfo/Extension.pm @@ -268,7 +268,7 @@ sub _extract_mentions { my ($text) = @_; return () unless defined $text; - $text =~ s/^[ \t]*(```|~~~).*?(?:^[ \t]*\1[^\n]*$|\z)//msg; + $text =~ s/^[ \t]*(`{3,}|~{3,}).*?(?:^[ \t]*\1[^\n]*$|\z)//msg; # A code span closes on a backtick run of the same length, within a paragraph. $text = join "\n\n", map { s/(? Date: Mon, 12 Oct 2026 16:52:56 -0400 Subject: [PATCH 5/8] Review fixes --- extensions/Needinfo/Extension.pm | 27 ++++++++++++---- extensions/Needinfo/t/mentions.t | 55 +++++++++++++++++++++++++++++--- 2 files changed, 72 insertions(+), 10 deletions(-) diff --git a/extensions/Needinfo/Extension.pm b/extensions/Needinfo/Extension.pm index 4b6051d390..9a416f80b8 100644 --- a/extensions/Needinfo/Extension.pm +++ b/extensions/Needinfo/Extension.pm @@ -20,6 +20,8 @@ use Bugzilla::Logging; use Bugzilla::User; use Bugzilla::User::Setting; +use List::Util qw(all); + our $VERSION = '0.01'; use constant MAX_MENTIONS => 10; @@ -107,7 +109,7 @@ sub bug_start_of_update { # Runs after the explicit needinfo handling so that the duplicate check # sees any flags that were just requested through the form. - _process_mentions($args->{bug}) if $args->{old_bug}; + _process_mentions($args->{bug}, $args->{old_bug}) if $args->{old_bug}; } # Clear the needinfo? flag if comment is being given by @@ -307,12 +309,17 @@ sub _users_for_mentions { # needinfo type isn't multiplicable and a flag exists) is only CC'd. # # Safeguards against misuse: +# - only comments made in the web UI count, so bots and API clients posting +# commit messages, code or logs can't CC people by accident # - only editbugs users can trigger mentions; for everyone else they are text # - only the first MAX_MENTIONS distinct nicknames per update are processed # - users who can't already see the bug are skipped, so a mention (or a typo, -# or a squatted nickname) can never grant access to a restricted bug +# or a squatted nickname) can never grant access to a restricted bug; that +# includes groups being added to the bug in this same update sub _process_mentions { - my ($bug) = @_; + my ($bug, $old_bug) = @_; + return unless Bugzilla->usage_mode == USAGE_MODE_BROWSER; + # Lowercased nick => true if mentioned in at least one public comment. my %public; @@ -331,10 +338,18 @@ sub _process_mentions { my $user = Bugzilla->user; return unless $user->in_group('editbugs', $bug->product_id); + # can_see_bug reads the groups stored in the database, which don't yet + # include groups added by this update, so check those separately. + my %old_groups = map { $_->id => 1 } @{$old_bug->groups_in}; + my @added_groups + = map { $_->name } grep { !$old_groups{$_->id} } @{$bug->groups_in}; + my @mentioned = grep { - $_->id != $user->id - && ($public{lc $_->nick} || $_->is_insider) - && $_->can_see_bug($bug->id) + my $mentioned = $_; + $mentioned->id != $user->id + && ($public{lc $mentioned->nick} || $mentioned->is_insider) + && $mentioned->can_see_bug($bug->id) + && all { $mentioned->in_group($_) } @added_groups } _users_for_mentions(@nicks); return unless @mentioned; diff --git a/extensions/Needinfo/t/mentions.t b/extensions/Needinfo/t/mentions.t index dabcd7bcad..801815df75 100644 --- a/extensions/Needinfo/t/mentions.t +++ b/extensions/Needinfo/t/mentions.t @@ -55,7 +55,11 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); sub id { $_[0]{id} } sub nick { $_[0]{nick} } sub login { "$_[0]{nick}\@example.com" } - sub in_group { $_[0]{editbugs} } + sub in_group { + my ($self, $group) = @_; + return $self->{editbugs} if $group eq 'editbugs'; + return scalar grep { $_ eq $group } @{$self->{groups} || []}; + } sub is_insider { $_[0]{insider} } sub can_see_bug { $_[0]{sees_bug} // 1 } sub needinfo_blocked { 0 } @@ -65,6 +69,7 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); my ($class, $text, %type) = @_; bless { added_comments => [{thetext => $text}], + groups => [], cc => [], needinfo => [], type => bless({flags => [], multiplicable => 1, %type}, 'FakeType'), @@ -73,6 +78,7 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); sub id {1} sub product_id {1} sub flag_types { [$_[0]{type}] } + sub groups_in { $_[0]{groups} } sub check_can_change_field { {allowed => 1} } sub add_cc { my ($self, $user) = @_; @@ -98,19 +104,26 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); package FakeFlag; sub status { $_[0]{status} } sub requestee_id { $_[0]{requestee_id} } + + package FakeGroup; + sub new { my ($class, $id, $name) = @_; bless {id => $id, name => $name}, $class } + sub id { $_[0]{id} } + sub name { $_[0]{name} } } my %users = map { $_->nick => $_ } ( FakeUser->new(id => 2, nick => 'alice'), - FakeUser->new(id => 3, nick => 'bob'), + FakeUser->new(id => 3, nick => 'bob', groups => ['core-security']), FakeUser->new(id => 4, nick => 'hidden', sees_bug => 0), FakeUser->new(id => 6, nick => 'boom', cc_fails => 1), map { FakeUser->new(id => 10 + $_, nick => "u$_") } 1 .. 12, ); my $commenter = FakeUser->new(id => 1, nick => 'me', editbugs => 1); my @looked_up; +my $usage_mode = Bugzilla::Constants::USAGE_MODE_BROWSER; -my $mock_bugzilla = mock 'Bugzilla' => (override => [user => sub {$commenter}]); +my $mock_bugzilla = mock 'Bugzilla' => ( + override => [user => sub {$commenter}, usage_mode => sub {$usage_mode}]); my $mock_ext = mock 'Bugzilla::Extension::Needinfo' => ( override => [ _users_for_mentions => sub { @@ -120,7 +133,11 @@ my $mock_ext = mock 'Bugzilla::Extension::Needinfo' => ( ], ); -my $process = \&Bugzilla::Extension::Needinfo::_process_mentions; +# The bug as it was before the update; it has no groups unless a test adds some. +my $old_bug = FakeBug->new(''); +my $process = sub { + Bugzilla::Extension::Needinfo::_process_mentions($_[0], $old_bug); +}; my $bug = FakeBug->new('@alice and @bob please look, cc @me'); $process->($bug); @@ -160,6 +177,36 @@ ok(lives { $process->($bug) }, is($bug->{cc}, ['alice'], '... that user is skipped'); is($bug->{needinfo}, ['alice@example.com'], '... others still processed'); +$bug = FakeBug->new('@alice @bob'); +$bug->{added_comments}[0]{isprivate} = 1; +$users{bob}{insider} = 1; +$process->($bug); +is($bug->{cc}, ['bob'], 'private comment: only insiders are CCd'); +is($bug->{needinfo}, ['bob@example.com'], '... and needinfod'); +$users{bob}{insider} = 0; + +my $security = FakeGroup->new(1, 'core-security'); +$bug = FakeBug->new('@alice @bob'); +$bug->{groups} = [$security]; +$process->($bug); +is($bug->{cc}, ['bob'], 'group added in this update: only its members are CCd'); +is($bug->{needinfo}, ['bob@example.com'], '... and needinfod'); + +$old_bug->{groups} = [$security]; +$bug = FakeBug->new('@alice'); +$bug->{groups} = [$security]; +$process->($bug); +is($bug->{cc}, ['alice'], 'existing group is left to can_see_bug'); +$old_bug->{groups} = []; + +$usage_mode = Bugzilla::Constants::USAGE_MODE_REST; +@looked_up = (); +$bug = FakeBug->new('@alice'); +$process->($bug); +is(\@looked_up, [], 'no lookup for comments made through the API'); +is($bug->{cc}, [], 'no CC for comments made through the API'); +$usage_mode = Bugzilla::Constants::USAGE_MODE_BROWSER; + $commenter->{editbugs} = 0; @looked_up = (); $bug = FakeBug->new('@alice'); From a8f9f9b7d54e91dc787e662ba9f67805c1b34029 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 15 Oct 2026 17:33:53 -0400 Subject: [PATCH 6/8] Review fixes --- Bugzilla/WebService/Bug.pm | 12 ++- extensions/BugModal/web/comments.js | 9 ++ extensions/Needinfo/Extension.pm | 88 ++++++++++++++----- extensions/Needinfo/lib/Config.pm | 23 +++++ extensions/Needinfo/t/mentions.t | 53 ++++++++++- .../default/admin/params/needinfo.html.tmpl | 22 +++++ js/text-editor.js | 53 +++++++++-- skins/standard/text-editor.css | 7 ++ .../global/text-editor-include.html.tmpl | 1 + 9 files changed, 238 insertions(+), 30 deletions(-) create mode 100644 extensions/Needinfo/lib/Config.pm create mode 100644 extensions/Needinfo/template/en/default/admin/params/needinfo.html.tmpl diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm index 264d2bfed3..46dce54215 100644 --- a/Bugzilla/WebService/Bug.pm +++ b/Bugzilla/WebService/Bug.pm @@ -423,7 +423,11 @@ sub render_comment { ? Bugzilla->markdown->render_html($params->{text}, $bug) : Bugzilla::Template::quoteUrls($params->{text}, $bug); - return {html => $html}; + my $result = {html => $html}; + Bugzilla::Hook::process('webservice_bug_render_comment', + {webservice => $self, params => $params, result => $result}); + + return $result; } # Helper for every method that returns comments. Fetches the revision metadata @@ -5424,6 +5428,12 @@ C The ID of the bug to render the comment against. C containing the HTML rendering. +Extensions can add params and return values through the +C hook. The Needinfo extension accepts +C and C and returns C, an array +of C/C hashes for the users that saving the comment on that bug +would CC and needinfo. + =item B This method can throw all of the errors that L throws. diff --git a/extensions/BugModal/web/comments.js b/extensions/BugModal/web/comments.js index bc1133b35b..dc7f66d6b7 100644 --- a/extensions/BugModal/web/comments.js +++ b/extensions/BugModal/web/comments.js @@ -988,6 +988,15 @@ Bugzilla.BugModal.InstantUpdater = class InstantUpdater { }); } + // `@nickname` mentions are only processed for comments submitted through the form, not the + // API, so let the form handle them. This loosely mirrors `_extract_mentions` in the Needinfo + // extension. A false positive only costs a page reload. + if (/(?new_from_list(\@ids)}; } -# GitHub-style mentions: each @nickname in a new comment is CC'd and gets a -# needinfo request. Mentions never block the comment from being saved; anyone -# who can't be needinfo'd (blocked, already asked, no permission, or the -# needinfo type isn't multiplicable and a flag exists) is only CC'd. -# -# Safeguards against misuse: -# - only comments made in the web UI count, so bots and API clients posting -# commit messages, code or logs can't CC people by accident -# - only editbugs users can trigger mentions; for everyone else they are text -# - only the first MAX_MENTIONS distinct nicknames per update are processed -# - users who can't already see the bug are skipped, so a mention (or a typo, -# or a squatted nickname) can never grant access to a restricted bug; that -# includes groups being added to the bug in this same update -sub _process_mentions { - my ($bug, $old_bug) = @_; - return unless Bugzilla->usage_mode == USAGE_MODE_BROWSER; +# Returns the users a set of new comments ({thetext, isprivate} hashes) on +# $bug would CC and needinfo, applying the safeguards described above +# _process_mentions. Shared by saving and the comment preview so the preview +# can't disagree with what actually happens. $old_bug is only passed when +# saving, to account for groups added in the same update. +sub _mentioned_users { + my ($bug, $comments, $old_bug) = @_; + return () unless Bugzilla->params->{needinfo_mentions_enabled}; # Lowercased nick => true if mentioned in at least one public comment. my %public; @@ -326,31 +319,57 @@ sub _process_mentions { # The first MAX_MENTIONS distinct lowercased nicks, in order of appearance. my @nicks; - foreach my $comment (@{$bug->{added_comments} || []}) { + foreach my $comment (@$comments) { foreach my $nick (map {lc} _extract_mentions($comment->{thetext})) { push @nicks, $nick if !exists $public{$nick} && @nicks < MAX_MENTIONS; $public{$nick} ||= !$comment->{isprivate}; } } - return unless @nicks; + return () unless @nicks; # Checked after parsing so updates without mentions skip the group lookup. my $user = Bugzilla->user; - return unless $user->in_group('editbugs', $bug->product_id); + return () unless $user->in_group('editbugs', $bug->product_id); # can_see_bug reads the groups stored in the database, which don't yet # include groups added by this update, so check those separately. - my %old_groups = map { $_->id => 1 } @{$old_bug->groups_in}; - my @added_groups - = map { $_->name } grep { !$old_groups{$_->id} } @{$bug->groups_in}; + my @added_groups; + if ($old_bug) { + my %old_groups = map { $_->id => 1 } @{$old_bug->groups_in}; + @added_groups + = map { $_->name } grep { !$old_groups{$_->id} } @{$bug->groups_in}; + } - my @mentioned = grep { + return grep { my $mentioned = $_; $mentioned->id != $user->id && ($public{lc $mentioned->nick} || $mentioned->is_insider) && $mentioned->can_see_bug($bug->id) && all { $mentioned->in_group($_) } @added_groups } _users_for_mentions(@nicks); +} + +# GitHub-style mentions: each @nickname in a new comment is CC'd and gets a +# needinfo request. Mentions never block the comment from being saved; anyone +# who can't be needinfo'd (blocked, already asked, no permission, or the +# needinfo type isn't multiplicable and a flag exists) is only CC'd. +# +# Safeguards against misuse: +# - only comments made in the web UI count, so bots and API clients posting +# commit messages, code or logs can't CC people by accident +# - only editbugs users can trigger mentions; for everyone else they are text +# - only the first MAX_MENTIONS distinct nicknames per update are processed +# - users who can't already see the bug are skipped, so a mention (or a typo, +# or a squatted nickname) can never grant access to a restricted bug; that +# includes groups being added to the bug in this same update +# - admins can switch the whole feature off with the needinfo_mentions_enabled +# param +sub _process_mentions { + my ($bug, $old_bug) = @_; + return unless Bugzilla->usage_mode == USAGE_MODE_BROWSER; + + my @mentioned + = _mentioned_users($bug, $bug->{added_comments} || [], $old_bug); return unless @mentioned; my ($type) = grep { $_->name eq 'needinfo' } @{$bug->flag_types}; @@ -387,6 +406,23 @@ sub _cc_and_needinfo { [{type_id => $type->id, status => '?', requestee => $mentioned->login}]); } +# Lets the comment preview list who saving the comment would CC and needinfo, +# so a stray @word is caught before the comment is submitted. The comment +# editor sends mention_bug_id (and mention_is_private) only for new comments +# on an existing bug, the only case _process_mentions handles. +sub webservice_bug_render_comment { + my ($self, $args) = @_; + my $params = $args->{params}; + return unless $params->{mention_bug_id}; + + my $bug = Bugzilla::Bug->check($params->{mention_bug_id}); + my @mentioned = _mentioned_users($bug, + [{thetext => $params->{text}, isprivate => $params->{mention_is_private}}]); + + $args->{result}->{mentions} + = [map { +{nick => $_->nick, name => $_->name} } @mentioned]; +} + sub _check_requestee { my ($requestee) = @_; my $user @@ -402,6 +438,12 @@ sub _check_requestee { {requestee => $user}); } +sub config_add_panels { + my ($self, $args) = @_; + my $modules = $args->{panel_modules}; + $modules->{Needinfo} = "Bugzilla::Extension::Needinfo::Config"; +} + sub object_end_of_create { my ($self, $args) = @_; my $object = $args->{object}; diff --git a/extensions/Needinfo/lib/Config.pm b/extensions/Needinfo/lib/Config.pm new file mode 100644 index 0000000000..7868713d48 --- /dev/null +++ b/extensions/Needinfo/lib/Config.pm @@ -0,0 +1,23 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# +# This Source Code Form is "Incompatible With Secondary Licenses", as +# defined by the Mozilla Public License, v. 2.0. + +package Bugzilla::Extension::Needinfo::Config; + +use 5.10.1; +use strict; +use warnings; + +use Bugzilla::Config::Common; + +sub get_param_list { + my ($class) = @_; + + my @param_list = ({name => 'needinfo_mentions_enabled', type => 'b', default => '1',},); + return @param_list; +} + +1; diff --git a/extensions/Needinfo/t/mentions.t b/extensions/Needinfo/t/mentions.t index 801815df75..f70afd40d7 100644 --- a/extensions/Needinfo/t/mentions.t +++ b/extensions/Needinfo/t/mentions.t @@ -54,6 +54,7 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); sub new { my ($class, %args) = @_; bless {%args}, $class } sub id { $_[0]{id} } sub nick { $_[0]{nick} } + sub name { ucfirst $_[0]{nick} } sub login { "$_[0]{nick}\@example.com" } sub in_group { my ($self, $group) = @_; @@ -121,9 +122,15 @@ my %users = map { $_->nick => $_ } ( my $commenter = FakeUser->new(id => 1, nick => 'me', editbugs => 1); my @looked_up; my $usage_mode = Bugzilla::Constants::USAGE_MODE_BROWSER; +my %params = (needinfo_mentions_enabled => 1); my $mock_bugzilla = mock 'Bugzilla' => ( - override => [user => sub {$commenter}, usage_mode => sub {$usage_mode}]); + override => [ + user => sub {$commenter}, + usage_mode => sub {$usage_mode}, + params => sub { \%params }, + ] +); my $mock_ext = mock 'Bugzilla::Extension::Needinfo' => ( override => [ _users_for_mentions => sub { @@ -207,6 +214,50 @@ is(\@looked_up, [], 'no lookup for comments made through the API'); is($bug->{cc}, [], 'no CC for comments made through the API'); $usage_mode = Bugzilla::Constants::USAGE_MODE_BROWSER; +$params{needinfo_mentions_enabled} = 0; +@looked_up = (); +$bug = FakeBug->new('@alice'); +$process->($bug); +is(\@looked_up, [], 'no lookup when needinfo_mentions_enabled is off'); +is($bug->{cc}, [], 'no CC when needinfo_mentions_enabled is off'); +$params{needinfo_mentions_enabled} = 1; + +# The comment preview lists the users that saving would CC and needinfo. +my $preview_bug = FakeBug->new(''); +my $mock_bug = mock 'Bugzilla::Bug' => + (override => [check => sub {$preview_bug}]); +my $render = sub { + my ($params) = @_; + my $result = {html => ''}; + Bugzilla::Extension::Needinfo->webservice_bug_render_comment( + {params => $params, result => $result}); + return $result; +}; + +is( + $render->({text => '@alice @hidden @me', mention_bug_id => 1})->{mentions}, + [{nick => 'alice', name => 'Alice'}], + 'preview lists the users a save would notify' +); +is($preview_bug->{cc}, [], '... without changing the bug'); + +$users{bob}{insider} = 1; +is( + $render->({text => '@alice @bob', mention_bug_id => 1, mention_is_private => 1}) + ->{mentions}, + [{nick => 'bob', name => 'Bob'}], + 'preview of a private comment lists only insiders' +); +$users{bob}{insider} = 0; + +ok(!exists $render->({text => '@alice'})->{mentions}, + 'no mentions without mention_bug_id (e.g. a new bug or an edited comment)'); + +$params{needinfo_mentions_enabled} = 0; +is($render->({text => '@alice', mention_bug_id => 1})->{mentions}, + [], 'preview lists nobody when needinfo_mentions_enabled is off'); +$params{needinfo_mentions_enabled} = 1; + $commenter->{editbugs} = 0; @looked_up = (); $bug = FakeBug->new('@alice'); diff --git a/extensions/Needinfo/template/en/default/admin/params/needinfo.html.tmpl b/extensions/Needinfo/template/en/default/admin/params/needinfo.html.tmpl new file mode 100644 index 0000000000..02513a527d --- /dev/null +++ b/extensions/Needinfo/template/en/default/admin/params/needinfo.html.tmpl @@ -0,0 +1,22 @@ +[%# This Source Code Form is subject to the terms of the Mozilla Public + # License, v. 2.0. If a copy of the MPL was not distributed with this + # file, You can obtain one at http://mozilla.org/MPL/2.0/. + # + # This Source Code Form is "Incompatible With Secondary Licenses", as + # defined by the Mozilla Public License, v. 2.0. + #%] + +[% + title = "Needinfo" + desc = "Needinfo configuration" +%] + +[% + param_descs = { + needinfo_mentions_enabled => + "When on, an @nickname mention in a new comment made in the web UI by " _ + "a member of editbugs CCs that user and requests needinfo from them. " _ + "The comment preview lists who will be notified. When off, mentions " _ + "are plain text.", + } +%] diff --git a/js/text-editor.js b/js/text-editor.js index e6e783ab6f..c9745bfcda 100644 --- a/js/text-editor.js +++ b/js/text-editor.js @@ -654,12 +654,23 @@ Bugzilla.TextEditor = class TextEditor { this.toggleToolbarButtons(); } + /** + * Get the params sent to the comment render API when previewing. + * @returns {Record} Params. + */ + getPreviewParams() { + return { text: this.$textarea.value }; + } + /** * Called whenever the Preview tab is selected. Fetch and display the rendered text if it's * edited. */ async preview() { - if (this.$textarea.value === this.lastPreviewedText) { + const params = this.getPreviewParams(); + const paramsKey = JSON.stringify(params); + + if (paramsKey === this.lastPreviewedParams) { return; } @@ -673,21 +684,32 @@ Bugzilla.TextEditor = class TextEditor { this.$preview.innerHTML = `

${this.str.loading.htmlEncode()}

`; try { - const { html } = await Bugzilla.API.post('bug/comment/render', { - text: this.$textarea.value, - }); + const { html, mentions } = await Bugzilla.API.post('bug/comment/render', params); // Use the HTML without `htmlEncode()` because the API-provided rendered text should be safe // to be injected as is; using `htmlEncode()` rather breaks the text this.$preview.innerHTML = html; + // List who saving the comment will CC and needinfo, so a stray `@word` is noticed before + // the comment is submitted + if (mentions?.length) { + const users = mentions + .map(({ nick, name }) => (name ? `${name} (@${nick})` : `@${nick}`).htmlEncode()) + .join(', '); + + this.$preview.insertAdjacentHTML( + 'afterbegin', + `

${this.str.mention_notice.htmlEncode()} ${users}

`, + ); + } + // Highlight code if possible if (Prism) { Prism.highlightAllUnder(this.$preview); } this.$preview.setAttribute('aria-busy', 'false'); - this.lastPreviewedText = this.$textarea.value; + this.lastPreviewedParams = paramsKey; } catch (ex) { this.$preview.innerHTML = ` @@ -749,6 +771,27 @@ Bugzilla.CommentEditor = class CommentEditor extends Bugzilla.TextEditor { this.$editTabPanel.appendChild(this.$textarea); } + /** + * Get the params sent to the comment render API when previewing. A new comment on an existing + * bug also asks for the users its `@nickname` mentions will CC and needinfo. + * @returns {Record} Params. + */ + getPreviewParams() { + const params = super.getPreviewParams(); + const form = this.$textarea.form; + const bugId = BUGZILLA.bug_id ?? form?.elements.namedItem('bugid')?.value; + + if (bugId) { + params.mention_bug_id = bugId; + params.mention_is_private = !!( + form?.elements.namedItem('comment_is_private')?.checked || + form?.elements.namedItem('isprivate')?.checked + ); + } + + return params; + } + /** * Submit the bug form if the comment is entered. Trigger a `click` event instead of submitting * the form directly to make sure any event handler associated with the button will be called. diff --git a/skins/standard/text-editor.css b/skins/standard/text-editor.css index e09c40ab82..7cc543f9ce 100644 --- a/skins/standard/text-editor.css +++ b/skins/standard/text-editor.css @@ -93,6 +93,13 @@ font-style: italic; } +.text-editor .comment-text .mention-notice { + border-bottom: 1px solid var(--control-border-color); + padding-bottom: 8px; + color: var(--secondary-label-color); + font-size: var(--font-size-small); +} + .text-editor footer:not([hidden]) { padding: 8px 4px; font-size: var(--font-size-small); diff --git a/template/en/default/global/text-editor-include.html.tmpl b/template/en/default/global/text-editor-include.html.tmpl index a87097eea5..b8387edc5a 100644 --- a/template/en/default/global/text-editor-include.html.tmpl +++ b/template/en/default/global/text-editor-include.html.tmpl @@ -29,6 +29,7 @@ js_BUGZILLA.string.TextEditor = { command_bulleted_list => 'Bulleted list', loading => 'Loading…', preview_error => 'Preview could not be loaded. Please try again later.', + mention_notice => 'Saving will CC these users and request needinfo where allowed:', markdown_link => { text => 'Markdown supported', href => 'https://guides.github.com/features/mastering-markdown/', From 99a2413048c32aa220725b70657da8a83e87b8d3 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 15 Oct 2026 17:46:23 -0400 Subject: [PATCH 7/8] Potential fix for pull request finding 'Restrict notification parameters to new-comment editors' Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- js/text-editor.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/text-editor.js b/js/text-editor.js index c9745bfcda..328b6a03c8 100644 --- a/js/text-editor.js +++ b/js/text-editor.js @@ -781,7 +781,7 @@ Bugzilla.CommentEditor = class CommentEditor extends Bugzilla.TextEditor { const form = this.$textarea.form; const bugId = BUGZILLA.bug_id ?? form?.elements.namedItem('bugid')?.value; - if (bugId) { + if (bugId && this.$textarea.name === 'comment') { params.mention_bug_id = bugId; params.mention_is_private = !!( form?.elements.namedItem('comment_is_private')?.checked || From 37c4583003e07a5d0cd96863321e81f86f83bee0 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sun, 18 Oct 2026 23:08:40 -0400 Subject: [PATCH 8/8] Review fixes --- Bugzilla/WebService/Bug.pm | 10 ++++---- extensions/Needinfo/Extension.pm | 19 +++++++++++---- extensions/Needinfo/t/mentions.t | 41 ++++++++++++++++++++++++-------- js/text-editor.js | 6 +++-- 4 files changed, 54 insertions(+), 22 deletions(-) diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm index 46dce54215..0c95f3277b 100644 --- a/Bugzilla/WebService/Bug.pm +++ b/Bugzilla/WebService/Bug.pm @@ -425,7 +425,7 @@ sub render_comment { my $result = {html => $html}; Bugzilla::Hook::process('webservice_bug_render_comment', - {webservice => $self, params => $params, result => $result}); + {webservice => $self, bug => $bug, params => $params, result => $result}); return $result; } @@ -5429,10 +5429,10 @@ C The ID of the bug to render the comment against. C containing the HTML rendering. Extensions can add params and return values through the -C hook. The Needinfo extension accepts -C and C and returns C, an array -of C/C hashes for the users that saving the comment on that bug -would CC and needinfo. +C hook. When C is given, the Needinfo +extension accepts C and C and returns +C, an array of C/C hashes for the users that saving the +comment on that bug would CC and needinfo. =item B diff --git a/extensions/Needinfo/Extension.pm b/extensions/Needinfo/Extension.pm index 78e350e6ed..16dbd38e6c 100644 --- a/extensions/Needinfo/Extension.pm +++ b/extensions/Needinfo/Extension.pm @@ -359,6 +359,8 @@ sub _mentioned_users { # commit messages, code or logs can't CC people by accident # - only editbugs users can trigger mentions; for everyone else they are text # - only the first MAX_MENTIONS distinct nicknames per update are processed +# - mass edits are skipped, since the same comment lands on every bug and +# would CC and needinfo each mentioned user once per bug # - users who can't already see the bug are skipped, so a mention (or a typo, # or a squatted nickname) can never grant access to a restricted bug; that # includes groups being added to the bug in this same update @@ -367,6 +369,7 @@ sub _mentioned_users { sub _process_mentions { my ($bug, $old_bug) = @_; return unless Bugzilla->usage_mode == USAGE_MODE_BROWSER; + return if _is_mass_edit(); my @mentioned = _mentioned_users($bug, $bug->{added_comments} || [], $old_bug); @@ -389,6 +392,12 @@ sub _process_mentions { } } +# process_bug.cgi takes a single bug as `id` and a mass edit as `id_` +# params, one per bug. +sub _is_mass_edit { + return grep {/^id_[1-9]/} Bugzilla->cgi->param; +} + sub _cc_and_needinfo { my ($bug, $mentioned, $type) = @_; $bug->add_cc($mentioned); @@ -408,14 +417,14 @@ sub _cc_and_needinfo { # Lets the comment preview list who saving the comment would CC and needinfo, # so a stray @word is caught before the comment is submitted. The comment -# editor sends mention_bug_id (and mention_is_private) only for new comments -# on an existing bug, the only case _process_mentions handles. +# editor sends include_mentions (and mention_is_private) along with the bug id +# only for new comments on an existing bug, the only case _process_mentions +# handles. sub webservice_bug_render_comment { my ($self, $args) = @_; - my $params = $args->{params}; - return unless $params->{mention_bug_id}; + my ($bug, $params) = @$args{qw(bug params)}; + return unless $bug && $params->{include_mentions}; - my $bug = Bugzilla::Bug->check($params->{mention_bug_id}); my @mentioned = _mentioned_users($bug, [{thetext => $params->{text}, isprivate => $params->{mention_is_private}}]); diff --git a/extensions/Needinfo/t/mentions.t b/extensions/Needinfo/t/mentions.t index f70afd40d7..0d37d3702d 100644 --- a/extensions/Needinfo/t/mentions.t +++ b/extensions/Needinfo/t/mentions.t @@ -106,6 +106,9 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); sub status { $_[0]{status} } sub requestee_id { $_[0]{requestee_id} } + package FakeCGI; + sub param { @{$_[0]{names}} } + package FakeGroup; sub new { my ($class, $id, $name) = @_; bless {id => $id, name => $name}, $class } sub id { $_[0]{id} } @@ -123,12 +126,14 @@ my $commenter = FakeUser->new(id => 1, nick => 'me', editbugs => 1); my @looked_up; my $usage_mode = Bugzilla::Constants::USAGE_MODE_BROWSER; my %params = (needinfo_mentions_enabled => 1); +my $cgi = bless {names => ['id', 'comment']}, 'FakeCGI'; my $mock_bugzilla = mock 'Bugzilla' => ( override => [ user => sub {$commenter}, usage_mode => sub {$usage_mode}, params => sub { \%params }, + cgi => sub {$cgi}, ] ); my $mock_ext = mock 'Bugzilla::Extension::Needinfo' => ( @@ -214,6 +219,14 @@ is(\@looked_up, [], 'no lookup for comments made through the API'); is($bug->{cc}, [], 'no CC for comments made through the API'); $usage_mode = Bugzilla::Constants::USAGE_MODE_BROWSER; +$cgi->{names} = ['id_12', 'id_13', 'comment', 'dontchange']; +@looked_up = (); +$bug = FakeBug->new('@alice'); +$process->($bug); +is(\@looked_up, [], 'no lookup in a mass edit'); +is($bug->{cc}, [], 'no CC in a mass edit'); +$cgi->{names} = ['id', 'comment']; + $params{needinfo_mentions_enabled} = 0; @looked_up = (); $bug = FakeBug->new('@alice'); @@ -224,18 +237,20 @@ $params{needinfo_mentions_enabled} = 1; # The comment preview lists the users that saving would CC and needinfo. my $preview_bug = FakeBug->new(''); -my $mock_bug = mock 'Bugzilla::Bug' => - (override => [check => sub {$preview_bug}]); my $render = sub { my ($params) = @_; my $result = {html => ''}; - Bugzilla::Extension::Needinfo->webservice_bug_render_comment( - {params => $params, result => $result}); + Bugzilla::Extension::Needinfo->webservice_bug_render_comment({ + bug => $params->{id} ? $preview_bug : undef, + params => $params, + result => $result + }); return $result; }; is( - $render->({text => '@alice @hidden @me', mention_bug_id => 1})->{mentions}, + $render->({text => '@alice @hidden @me', id => 1, include_mentions => 1}) + ->{mentions}, [{nick => 'alice', name => 'Alice'}], 'preview lists the users a save would notify' ); @@ -243,18 +258,24 @@ is($preview_bug->{cc}, [], '... without changing the bug'); $users{bob}{insider} = 1; is( - $render->({text => '@alice @bob', mention_bug_id => 1, mention_is_private => 1}) - ->{mentions}, + $render->({ + text => '@alice @bob', + id => 1, + include_mentions => 1, + mention_is_private => 1 + })->{mentions}, [{nick => 'bob', name => 'Bob'}], 'preview of a private comment lists only insiders' ); $users{bob}{insider} = 0; -ok(!exists $render->({text => '@alice'})->{mentions}, - 'no mentions without mention_bug_id (e.g. a new bug or an edited comment)'); +ok(!exists $render->({text => '@alice', include_mentions => 1})->{mentions}, + 'no mentions without a bug (e.g. a new bug)'); +ok(!exists $render->({text => '@alice', id => 1})->{mentions}, + 'no mentions unless asked for (e.g. an edited comment)'); $params{needinfo_mentions_enabled} = 0; -is($render->({text => '@alice', mention_bug_id => 1})->{mentions}, +is($render->({text => '@alice', id => 1, include_mentions => 1})->{mentions}, [], 'preview lists nobody when needinfo_mentions_enabled is off'); $params{needinfo_mentions_enabled} = 1; diff --git a/js/text-editor.js b/js/text-editor.js index 328b6a03c8..04ff6ad1ea 100644 --- a/js/text-editor.js +++ b/js/text-editor.js @@ -773,7 +773,8 @@ Bugzilla.CommentEditor = class CommentEditor extends Bugzilla.TextEditor { /** * Get the params sent to the comment render API when previewing. A new comment on an existing - * bug also asks for the users its `@nickname` mentions will CC and needinfo. + * bug is rendered against that bug, and also asks for the users its `@nickname` mentions will + * CC and needinfo. * @returns {Record} Params. */ getPreviewParams() { @@ -782,7 +783,8 @@ Bugzilla.CommentEditor = class CommentEditor extends Bugzilla.TextEditor { const bugId = BUGZILLA.bug_id ?? form?.elements.namedItem('bugid')?.value; if (bugId && this.$textarea.name === 'comment') { - params.mention_bug_id = bugId; + params.id = bugId; + params.include_mentions = true; params.mention_is_private = !!( form?.elements.namedItem('comment_is_private')?.checked || form?.elements.namedItem('isprivate')?.checked