During an IRC converstation today, it was mentioned that the dependency changed messages may be leaking bug summaries. I don't believe that the permissions are checked before sending the message that begins "This bug depends on bug $bug, which changed state." This wasn't a big deal before bug 28736 was filed as a bug's status is not conidered to be confidential, but its summary is.

My feeling is that status should be confidential, but if it isn't, I would expect resolution to be. And in any case, there's no point sending it out if they can't access it.

This is going to be a blocker for the 2.16 release

How about this? Don't send changedmail if the depending bug is invisible to the relevant user. This way the status, etc. all stays private. Gerv

Taking. Gerv

Accepting. Gerv

Comment on attachment 57547 [details] [diff] [review] Patch v.1 >@@ -187,17 +187,19 @@ > my $thisdiff = ""; > my $lastbug = ""; > my $interestingchange = 0; >+ my $depbug = 0; > while (MoreSQLData()) { >- my ($bug, $summary, $what, $old, $new) = (FetchSQLData()); >- if ($bug ne $lastbug) { >+ my ($summary, $what, $old, $new); >+ ($depbug, $summary, $what, $old, $new) = (FetchSQLData()); [...] So basically this first part is just a namechage for depbug to avoid a nameclash? >@@ -665,6 +667,12 @@ > # > return unless CanSeeBug($id, $userid, $groupset); > >+ # We shouldn't send changedmail if this is a dependency mail, and the >+ # depending bug is not visible to the user. >+ if ($depbug) { >+ return unless CanSeeBug($depbug, $userid, $groupset); >+ } >+ I'm not very familiar with NewProcessOnePerson, but I guess you've picked to check for this here instead of ProcessOneBug to avoid an extra SQL hit, and because there is a very similar check close to it. Sounds okay; this API doesn't look very usable as it is, so it's no big deal making it worse. Still, this part of the code is scary: 12 parameters? Steve McConnell would have cried :) If I've guessed properly, then r=kiko.

> So basically this first part is just a namechage for depbug to avoid a > nameclash? To make it more clear, yes - it's not to avoid a clash. > because there is a very similar check close to it. This was my logic. Yes, the API sucks - we'll have to wait until we spank that file to fix it. Gerv

In reviewing attachment 57547 [details] [diff] [review] ("Patch v.1"), I don't believe it will properly handle the case where more than one bug depends on the bug being changed. Only the "last" bug id returned from the database will be set to $depbug, and only that last bug id will be checked. My recommendation is to change $depbug to @depbug or %depbug and store *all* dependent bug ids, pass \@depbug or \%depbug to NewProcessOnePerson(), then loop over @$depbug or keys %$depbug to check all dependent bugs.

Comment on attachment 57547 [details] [diff] [review] Patch v.1 See my previous comments.

OK, then I don't understand how this works. A single mail is about a single change to a single bug, and a single dependency - isn't that right? I can see why you think it should be a list, but I can't see how it could make any sense as one. Unless someone else can figure this out, this patch will have to wait, and probably won't make 2.16. Gerv

This is a release blocker.

ddk: can you explain more what you're looking at? As far as I know dependency mail only goes out on one bug at a time. Where is there a list of bugs being processed that would be stale by the time the mail is sent?

When reviewing this patch to add more detailed comments (as per the request by justdave), I ran into the code fragment below on line 172 (give or take a few lines). Can anyone explain this??? my $resid = Okay, back to more detailed comments about my previous comment. This is the SendSQL statement that is run just before the while() loop SendSQL("SELECT " . join(',', @::log_columns) . ", lastdiffed, now() " . "FROM bugs WHERE bug_id = $id"); my @row = FetchSQLData(); foreach my $i (@::log_columns) { $values{$i} = shift(@row); } my ($start, $end) = (@row); that appears in the patch. SendSQL("SELECT bugs_activity.bug_id, bugs.short_desc, fielddefs.name, " . " removed, added " . "FROM bugs_activity, bugs, dependencies, fielddefs ". "WHERE bugs_activity.bug_id = dependencies.dependson " . " AND bugs.bug_id = bugs_activity.bug_id ". " AND dependencies.blocked = $id " . " AND fielddefs.fieldid = bugs_activity.fieldid" . " AND (fielddefs.name = 'bug_status' " . " OR fielddefs.name = 'resolution') " . " AND bug_when > '$start' " . " AND bug_when <= '$end' " . "ORDER BY bug_when, bug_id"); This is the patched while() loop (with variable declarations) that appears immediately after it: my $thisdiff = ""; my $lastbug = ""; my $interestingchange = 0; my $depbug = 0; while (MoreSQLData()) { my ($summary, $what, $old, $new); ($depbug, $summary, $what, $old, $new) = (FetchSQLData()); if ($depbug ne $lastbug) { if ($interestingchange) { $deptext .= $thisdiff; } $lastbug = $depbug; $thisdiff = "\nThis bug depends on bug $depbug, which changed state.\n"; $thisdiff .= "Bug $depbug Summary: $summary\n\n"; $thisdiff .= FormatTriple("What ", "Old Value", "New Value"); $thisdiff .= ('-' x 76) . "\n"; $interestingchange = 0; } $thisdiff .= FormatTriple($fielddescription{$what}, $old, $new); if ($what eq 'bug_status' && IsOpenedState($old) ne IsOpenedState($new)) { $interestingchange = 1; } } My concern is that if the SQL statement returns more than one row with different bug ids, then not every $depbug value will be checked. In other words, I presume that if the current bug (represented by $id) depends on more than one bug, the while() loop would go through all such bugs, setting $depbug to each one in turn, and that if a "confidential" bug id is assigned to $depbug before a non-confidential bug id, then the safety check will be invalidated (since the check will only be done on the non-confidential bug id). Does that make sense? Note that it is very possible that I don't fully understand the limits on what rows may be returned by the SQL statement at the time the code runs. You may also want to review this in light of bug 106377. If the lastdiffed column hasn't been updated, then more than one change will be written out to the mail message, and I *believe* that the situation I'm concerned about here would be much more likely. See this code in ProcessOneBug() (prior to the quoted code above) where $start and $end are set. SendSQL("SELECT " . join(',', @::log_columns) . ", lastdiffed, now() " . "FROM bugs WHERE bug_id = $id"); my @row = FetchSQLData(); foreach my $i (@::log_columns) { $values{$i} = shift(@row); } my ($start, $end) = (@row); If it's impossible to have such a condition, then I'll give the code an r=. Whew!

Ack! Something's amiss with the Mozilla 0.9.5 textarea editor. It has some VERY strange behavior under RH 6.2. Anyway, the second paragraph in my previous comment should read like this (without all the code in the middle of it): "Okay, back to more detailed comments about my previous comment. This is the SendSQL statement that is run just before the while() loop that appears in the patch."

Hmm, it seems the bulk change thinks I'm not changing anything if all I do is add names to the CC list, so I guess I have to make a comment. Anyhow, adding the representatives from the organizations we know of that support Bugzilla distributions so they're aware of our upcoming security release

ddk is right. We need to create a list of all the values $depbug has been set to, pass that around and then check them all. Here's another patch. Gerv

Comment on attachment 63024 [details] [diff] [review] Patch v.2 Looks great, except there are some typos: - "push($depbug, @depbugs)" should be "push(@depbugs, $depbug)" - Call to NewProcessOnePerson() uses "\@depbug" instead of "\@depbugs" as last argument. - Would like to see "$debpugRef" be called "$depbugsRef" for consistency, but is not necessary.

Try this for size. All comments addressed. Gerv

Comment on attachment 63179 [details] [diff] [review] Patch v.3 r=ddk Looks good!

Comment on attachment 63179 [details] [diff] [review] Patch v.3 With this patch applied, I get an error when trying to resolve or reopen a secure bug: Insecure dependency in parameter 1 of DBI::db=HASH(0x83ac2d4)->prepare method call while running with -T switch at globals.pl line 216. I tracked this down to line 680 of the patched version of processmail. If you add the line: detaint_natural($_) || die 'Unexpected Error: @depbugs contains a non-numeric value'; right before the return line in the foreach (@depbugs) loop it solves the problem.

This patch adds the detaint_natural() line I mentioned in my review (comment #20)

Using die() here seems a bit drastic in this case in my opinion. detaint_natural($_) || die 'Unexpected Error: @depbugs contains a non-numeric value'; Granted, we got an invalid bug number from the database, but a simple 'return' would prevent any email from being sent for the current person/bug_id. detaint_natural($_) || return; Another option would be to print a warning and then return: if (! detaint_natural($_)) { warn '@depbugs contains a non-numeric value'; return; } It might be useful to save the original value in order to complain about it: my $current_id = $_; if (! detaint_natural($_)) { warn "\@depbugs contains a non-numeric value: '$current_id'"; return; }

OK, this patch does warn && return instead of die. Or course, if it has to do anything, we've got real problems :)

Comment on attachment 64283 [details] [diff] [review] Patch v.5 [Jake] r=gerv. Gerv

Comment on attachment 64283 [details] [diff] [review] Patch v.5 [Jake] Oh the pain! 1. Doesn't apply cleanly due to fix for bug 113383. 2. Arguments to warn() need to be quoted otherwise precedence rules are equivalent to: warn("Unexpected" && return); at least in Perl v5.6.0 on my machine here. 3. If $dep_id is not a number, detaint_natural() makes it equivalent to 'undef', which means the warn() generates yet another warning about using an undefined value. Another patch to follow.

Fixes from comment #25.

Example script for item two in comment #25. #!/usr/bonsai/bin/perl -wT use strict; sub foo { warn "D'OH!" && return; } sub bar { warn("D'OH!") && return; } foo(); print "We should have seen D'OH! warning.\n"; bar(); print "There it is!\n"; exit;

Example script for item three in comment #25. Must be run from your Bugzilla directory, or have a local copy of globals.pl. #!/usr/bonsai/bin/perl -wT use lib '.'; use strict; require 'globals.pl'; my $id = 'foobar'; detaint_natural($id) || warn "Unexpected Error: \$id contains a non-numeric value: '$id'"; print "D'OH! Two warnings!\n"; exit; Output: [Thu Jan 10 18:44:14 2002] bar.pl: Use of uninitialized value in concatenation (.) at ./bar.pl line 10. [Thu Jan 10 18:44:14 2002] bar.pl: Unexpected Error: $id contains a non-numeric value: '' at ./bar.pl line 10. D'OH! Two warnings!

Duh. Changed: my $save_id; to: my $save_id = $dep_id;

Comment on attachment 64505 [details] [diff] [review] Patch v.7 [ddk] r=gerv. Gerv

Comment on attachment 64505 [details] [diff] [review] Patch v.7 [ddk] r= justdave

Checked in. /cvsroot/mozilla/webtools/bugzilla/processmail,v <-- processmail new revision: 1.75; previous revision: 1.74

*** Bug 146141 has been marked as a duplicate of this bug. ***

Do we want this for 2.14.2?

Bug summaries weren't included in these emails in 2.14.

ah, ok

*** Bug 149938 has been marked as a duplicate of this bug. ***