https://bugzilla.mozilla.org/show_bug.cgi?id=389014 reveals the alias of the two security-sensitive bugs it blocks :(

this is already deployed on b.m.o

Comment on attachment 412965 [details] [diff] [review] Patch v1 for 3.4 >- if ($options->{use_alias} && $link_text =~ /^\d+$/ && $bug_alias) { >+ if ($options->{use_alias} && $link_text =~ /^\d+$/ && $bug_alias && Bugzilla->user->can_see_bug($bug_num)) { This should go later in the code where we already call can_see_bug().

This is a regression due to bug 470262. Affects 3.4 and newer.

This has been assigned CVE-2009-3386.

(In reply to comment #2) > (From update of attachment 412965 [details] [diff] [review]) > >- if ($options->{use_alias} && $link_text =~ /^\d+$/ && $bug_alias) { > >+ if ($options->{use_alias} && $link_text =~ /^\d+$/ && $bug_alias && Bugzilla->user->can_see_bug($bug_num)) { > > This should go later in the code where we already call can_see_bug(). I was thinking that, but that section of code is dependent on having a status or something, and didn't know how that would affect it, that's why I didn't move it.

Comment on attachment 412965 [details] [diff] [review] Patch v1 for 3.4 No, actually, this is the right fix. The can_see_bug below is unrelated. I'd like to create an $user var and re-use it so that we don't have to manually do Bugzilla->user->can_see_bug in both places, but this works OK.

Comment on attachment 413030 [details] [diff] [review] Patch v2 for trunk >- if ($options->{use_alias} && $link_text =~ /^\d+$/ && $bug->alias) { >+ if ($options->{use_alias} && $link_text =~ /^\d+$/ >+ && $bug->alias && $user->can_see_bug($bug)) { Tiny stylistic nit: brace goes on the next line aligned with the "i" in "if". (It's perlstyle.) I'll test this, but this looks fine.

Comment on attachment 413031 [details] [diff] [review] Patch v2 for 3.4 branch Same stylistic nit. Also looks fine.

nit addressed (might as well get it right before we commit ;)

Comment on attachment 413034 [details] [diff] [review] Patch v3 for 3.4 branch >+ if ($options->{use_alias} && $link_text =~ /^\d+$/ >+ && $bug_alias && $user->can_see_bug($bug_num)) As I said earlier, this should be moved into the IF block below. All bugs have a bug status. We use it to check if the bug ID points to an existing bug. If the bug doesn't exist, it's undefined, else it exists. No need to call can_see_bug() twice. >+ if ($user->can_see_bug($bug_num)) {

Comment on attachment 413033 [details] [diff] [review] Patch v3 for trunk The same comment applies here too.

Actually, I think the patch as justdave wrote it is more readable, and there's no performance penalty for calling can_see_bug multiple times.

Comment on attachment 413034 [details] [diff] [review] Patch v3 for 3.4 branch LpSolit pointed out that in this version we're calling can_see_bug before verifying that the bug exists, so this one indeed should have a different patch.

Comment on attachment 413186 [details] [diff] [review] Patch v5 for 3.4 branch Now the logic makes sense. r=LpSolit

Comment on attachment 413185 [details] [diff] [review] Patch v5 for trunk Here too, I'm now fine with this patch. r=LpSolit

tip: Checking in Bugzilla/Template.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v <-- Template.pm new revision: 1.115; previous revision: 1.114 done 3.4: Checking in Bugzilla/Template.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v <-- Template.pm new revision: 1.99.2.4; previous revision: 1.99.2.3 done

Security advisory sent, unlocking bug.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.2/ modified t/test_security.t Committed revision 218. Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.0/ modified t/test_security.t Committed revision 205.