Skip to content

bugzilla
bugzilla
Commit 5fbf48df authored by myk%mozilla.org's avatar myk%mozilla.org
Browse files
Options

Fix for bug 305773: fixes regression in flag validation code that let through…

Fix for bug 305773: fixes regression in flag validation code that let through untrusted requestees when multiple requestees were entered into a requestee field (per the new feature that lets multiple requestees be entered into those fields); r=lpsolit
parent a9d5c487
Hide whitespace changes
Inline Side-by-side
Showing with 110 additions and 85 deletions
Bugzilla/Flag.pm
View file @ 5fbf48df
...@@ -278,6 +278,7 @@ sub validate { ...@@ -278,6 +278,7 @@ sub validate {
foreach my $id (@ids) { foreach my $id (@ids) {
my $status = $cgi->param("flag-$id"); my $status = $cgi->param("flag-$id");
my @requestees = $cgi->param("requestee-$id");
# Make sure the flag exists. # Make sure the flag exists.
my $flag = get($id); my $flag = get($id);
...@@ -294,66 +295,79 @@ sub validate { ...@@ -294,66 +295,79 @@ sub validate {
{ id => $id, status => $status }); { id => $id, status => $status });
# Make sure the user didn't request the flag unless it's requestable. # Make sure the user didn't request the flag unless it's requestable.
# If the flag was requested before it became unrequestable, leave it as is. # If the flag was requested before it became unrequestable, leave it
if ($status eq '?' && $flag->{status} ne '?' && # as is.
!$flag->{type}->{is_requestable}) { if ($status eq '?'
&& $flag->{status} ne '?'
&& !$flag->{type}->{is_requestable})
{
ThrowCodeError("flag_status_invalid", ThrowCodeError("flag_status_invalid",
{ id => $id, status => $status }); { id => $id, status => $status });
} }
# Make sure the user didn't specify a requestee unless the flag # Make sure the user didn't specify a requestee unless the flag
# is specifically requestable. If the requestee was set before # is specifically requestable. If the requestee was set before
# the flag became specifically unrequestable, leave it as is. # the flag became specifically unrequestable, don't let the user
my $old_requestee = # change the requestee, but let the user remove it by entering
$flag->{'requestee'} ? $flag->{'requestee'}->login : ''; # an empty string for the requestee.
my $new_requestee = trim($cgi->param("requestee-$id") || ''); if ($status eq '?' && !$flag->{type}->{is_requesteeble}) {
my $old_requestee =
$flag->{'requestee'} ? $flag->{'requestee'}->login : '';
my $new_requestee = join('', @requestees);
if ($new_requestee && $new_requestee ne $old_requestee) {
ThrowCodeError("flag_requestee_disabled",
{ type => $flag->{type} });
}
}
# Make sure the user didn't enter multiple requestees for a flag
# that can't be requested from more than one person at a time.
if ($status eq '?' if ($status eq '?'
&& !$flag->{type}->{is_requesteeble} && !$flag->{type}->{is_multiplicable}
&& $new_requestee && scalar(@requestees) > 1)
&& ($new_requestee ne $old_requestee))
{ {
ThrowCodeError("flag_requestee_disabled", ThrowUserError("flag_not_multiplicable", { type => $flag->{type} });
{ name => $flag->{type}->{name} });
} }
# Make sure the requestee is authorized to access the bug. # Make sure the requestees are authorized to access the bug.
# (and attachment, if this installation is using the "insider group" # (and attachment, if this installation is using the "insider group"
# feature and the attachment is marked private). # feature and the attachment is marked private).
if ($status eq '?' if ($status eq '?' && $flag->{type}->{is_requesteeble}) {
&& $flag->{type}->{is_requesteeble} my $old_requestee =
&& $new_requestee $flag->{'requestee'} ? $flag->{'requestee'}->login : '';
&& ($old_requestee ne $new_requestee)) foreach my $login (@requestees) {
{ next if $login eq $old_requestee;
# We know the requestee exists because we ran
# Bugzilla::User::match_field before getting here.
my $requestee = Bugzilla::User->new_from_login($new_requestee);
# Throw an error if the user can't see the bug.
# Note that if permissions on this bug are changed,
# can_see_bug() will refer to old settings.
if (!$requestee->can_see_bug($bug_id)) {
ThrowUserError("flag_requestee_unauthorized",
{ flag_type => $flag->{'type'},
requestee => $requestee,
bug_id => $bug_id,
attach_id =>
$flag->{target}->{attachment}->{id} });
}
# Throw an error if the target is a private attachment and # We know the requestee exists because we ran
# the requestee isn't in the group of insiders who can see it. # Bugzilla::User::match_field before getting here.
if ($flag->{target}->{attachment}->{exists} my $requestee = Bugzilla::User->new_from_login($login);
&& $cgi->param('isprivate')
&& Param("insidergroup") # Throw an error if the user can't see the bug.
&& !$requestee->in_group(Param("insidergroup"))) # Note that if permissions on this bug are changed,
{ # can_see_bug() will refer to old settings.
ThrowUserError("flag_requestee_unauthorized_attachment", if (!$requestee->can_see_bug($bug_id)) {
{ flag_type => $flag->{'type'}, ThrowUserError("flag_requestee_unauthorized",
requestee => $requestee, { flag_type => $flag->{'type'},
bug_id => $bug_id, requestee => $requestee,
attach_id => bug_id => $bug_id,
$flag->{target}->{attachment}->{id} }); attach_id =>
$flag->{target}->{attachment}->{id} });
}
# Throw an error if the target is a private attachment and
# the requestee isn't in the group of insiders who can see it.
if ($flag->{target}->{attachment}->{exists}
&& $cgi->param('isprivate')
&& Param("insidergroup")
&& !$requestee->in_group(Param("insidergroup")))
{
ThrowUserError("flag_requestee_unauthorized_attachment",
{ flag_type => $flag->{'type'},
requestee => $requestee,
bug_id => $bug_id,
attach_id =>
$flag->{target}->{attachment}->{id} });
}
} }
} }
......
Bugzilla/FlagType.pm
View file @ 5fbf48df
...@@ -352,6 +352,7 @@ sub validate { ...@@ -352,6 +352,7 @@ sub validate {
foreach my $id (@ids) { foreach my $id (@ids) {
my $status = $cgi->param("flag_type-$id"); my $status = $cgi->param("flag_type-$id");
my @requestees = $cgi->param("requestee_type-$id");
# Don't bother validating types the user didn't touch. # Don't bother validating types the user didn't touch.
next if $status eq "X"; next if $status eq "X";
...@@ -374,48 +375,53 @@ sub validate { ...@@ -374,48 +375,53 @@ sub validate {
# Make sure the user didn't specify a requestee unless the flag # Make sure the user didn't specify a requestee unless the flag
# is specifically requestable. # is specifically requestable.
my $new_requestee = trim($cgi->param("requestee_type-$id") || '');
if ($status eq '?' if ($status eq '?'
&& !$flag_type->{is_requesteeble} && !$flag_type->{is_requesteeble}
&& $new_requestee) && scalar(@requestees) > 0)
{ {
ThrowCodeError("flag_requestee_disabled", ThrowCodeError("flag_requestee_disabled", { type => $flag_type });
{ name => $flag_type->{name} });
} }
# Make sure the requestee is authorized to access the bug # Make sure the user didn't enter multiple requestees for a flag
# (and attachment, if this installation is using the "insider group" # that can't be requested from more than one person at a time.
# feature and the attachment is marked private).
if ($status eq '?' if ($status eq '?'
&& $flag_type->{is_requesteeble} && !$flag_type->{is_multiplicable}
&& $new_requestee) && scalar(@requestees) > 1)
{ {
# We know the requestee exists because we ran ThrowUserError("flag_not_multiplicable", { type => $flag_type });
# Bugzilla::User::match_field before getting here. }
my $requestee = Bugzilla::User->new_from_login($new_requestee);
# Make sure the requestees are authorized to access the bug
# Throw an error if the user can't see the bug. # (and attachment, if this installation is using the "insider group"
if (!$requestee->can_see_bug($bug_id)) { # feature and the attachment is marked private).
ThrowUserError("flag_requestee_unauthorized", if ($status eq '?' && $flag_type->{is_requesteeble}) {
{ flag_type => $flag_type, foreach my $login (@requestees) {
requestee => $requestee, # We know the requestee exists because we ran
bug_id => $bug_id, # Bugzilla::User::match_field before getting here.
attach_id => $attach_id }); my $requestee = Bugzilla::User->new_from_login($login);
}
# Throw an error if the user can't see the bug.
# Throw an error if the target is a private attachment and if (!$requestee->can_see_bug($bug_id)) {
# the requestee isn't in the group of insiders who can see it. ThrowUserError("flag_requestee_unauthorized",
if ($attach_id { flag_type => $flag_type,
&& Param("insidergroup") requestee => $requestee,
&& $cgi->param('isprivate') bug_id => $bug_id,
&& !$requestee->in_group(Param("insidergroup"))) attach_id => $attach_id });
{ }
ThrowUserError("flag_requestee_unauthorized_attachment",
{ flag_type => $flag_type, # Throw an error if the target is a private attachment and
requestee => $requestee, # the requestee isn't in the group of insiders who can see it.
bug_id => $bug_id, if ($attach_id
attach_id => $attach_id }); && Param("insidergroup")
&& $cgi->param('isprivate')
&& !$requestee->in_group(Param("insidergroup")))
{
ThrowUserError("flag_requestee_unauthorized_attachment",
{ flag_type => $flag_type,
requestee => $requestee,
bug_id => $bug_id,
attach_id => $attach_id });
}
} }
} }
......
template/en/default/global/code-error.html.tmpl
View file @ 5fbf48df
...@@ -179,8 +179,9 @@ ...@@ -179,8 +179,9 @@
[% END %] [% END %]
[% ELSIF error == "flag_requestee_disabled" %] [% ELSIF error == "flag_requestee_disabled" %]
[% title = "Flag not Specifically Requestable" %] [% title = "Flag not Requestable from Specific Person" %]
The flag <em>[% name FILTER html %]</em> is not specifically requestable. You can't ask a specific person for
<em>[% type.name FILTER html %]</em>.
[% ELSIF error == "flag_status_invalid" %] [% ELSIF error == "flag_status_invalid" %]
The flag status <em>[% status FILTER html %]</em> The flag status <em>[% status FILTER html %]</em>
......
template/en/default/global/user-error.html.tmpl
View file @ 5fbf48df
...@@ -420,6 +420,10 @@ ...@@ -420,6 +420,10 @@
you could convert it to a compressible format like JPG or PNG and try you could convert it to a compressible format like JPG or PNG and try
again. again.
[% ELSIF error == "flag_not_multiplicable" %]
You can't ask more than one person at a time for
<em>[% type.name FILTER html %]</em>.
[% ELSIF error == "flag_requestee_unauthorized" %] [% ELSIF error == "flag_requestee_unauthorized" %]
[% title = "Flag Requestee Not Authorized" %] [% title = "Flag Requestee Not Authorized" %]
...@@ -430,8 +434,8 @@ ...@@ -430,8 +434,8 @@
but that [% terms.bug %] has been restricted to users in certain groups, but that [% terms.bug %] has been restricted to users in certain groups,
and the user you asked isn't in all the groups to which and the user you asked isn't in all the groups to which
the [% terms.bug %] has been restricted. the [% terms.bug %] has been restricted.
Please choose someone else to ask, or make the [% terms.bug %] accessible to users Please choose someone else to ask, or make the [% terms.bug %] accessible
on its CC: list and add that user to the list. to users on its CC: list and add that user to the list.
[% ELSIF error == "flag_requestee_unauthorized_attachment" %] [% ELSIF error == "flag_requestee_unauthorized_attachment" %]
[% title = "Flag Requestee Not Authorized" %] [% title = "Flag Requestee Not Authorized" %]
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment