Bug 523495: Re-work attachment.cgi and the general attachment_base-checking code to prevent an infinite redirect loop when ssl_redirect is on and Bugzilla has an attachment_base set.

Patch by Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=LpSolit

3 files changed, 43 insertions, 21 deletions

diff --git a/Bugzilla.pm b/Bugzilla.pm
index 62b1af659..67ec611a9 100644
--- a/Bugzilla.pm
+++ b/Bugzilla.pm
@@ -113,7 +113,10 @@ sub init_page {
         };
     }
 
-    do_ssl_redirect_if_required();
+    # Because of attachment_base, attachment.cgi handles this itself.
+    if (basename($0) ne 'attachment.cgi') {
+        do_ssl_redirect_if_required();
+    }
 
     # If Bugzilla is shut down, do not allow anything to run, just display a
     # message to the user about the downtime and log out.  Scripts listed in 
diff --git a/Bugzilla/CGI.pm b/Bugzilla/CGI.pm
index c30e13618..8c68f996c 100644
--- a/Bugzilla/CGI.pm
+++ b/Bugzilla/CGI.pm
@@ -28,6 +28,8 @@ use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::Util;
 
+use File::Basename;
+
 BEGIN {
     if (ON_WINDOWS) {
         # Help CGI find the correct temp directory as the default list
@@ -71,15 +73,9 @@ sub new {
     $self->charset(Bugzilla->params->{'utf8'} ? 'UTF-8' : '');
 
     # Redirect to urlbase/sslbase if we are not viewing an attachment.
-    if (use_attachbase() && i_am_cgi()) {
-        my $cgi_file = $self->url('-path_info' => 0, '-query' => 0, '-relative' => 1);
-        $cgi_file =~ s/\?$//;
-        my $urlbase = Bugzilla->params->{'urlbase'};
-        my $sslbase = Bugzilla->params->{'sslbase'};
-        my $path_regexp = $sslbase ? qr/^(\Q$urlbase\E|\Q$sslbase\E)/ : qr/^\Q$urlbase\E/;
-        if ($cgi_file ne 'attachment.cgi' && $self->self_url !~ /$path_regexp/) {
-            $self->redirect_to_urlbase;
-        }
+    my $script = basename($0);
+    if ($self->url_is_attachment_base and $script ne 'attachment.cgi') {
+        $self->redirect_to_urlbase();
     }
 
     # Check for errors
@@ -398,6 +394,28 @@ sub redirect_to_urlbase {
     exit;
 }
 
+sub url_is_attachment_base {
+    my ($self, $id) = @_;
+    return 0 if !use_attachbase() or !i_am_cgi();
+    my $attach_base = Bugzilla->params->{'attachment_base'};
+    # If we're passed an id, we only want one specific attachment base
+    # for a particular bug. If we're not passed an ID, we just want to
+    # know if our current URL matches the attachment_base *pattern*.
+    my $regex;
+    if ($id) {
+        $attach_base =~ s/\%bugid\%/$id/;
+        $regex = quotemeta($attach_base);
+    }
+    else {
+        # In this circumstance we run quotemeta first because we need to
+        # insert an active regex meta-character afterward.
+        $regex = quotemeta($attach_base);
+        $regex =~ s/\\\%bugid\\\%/\\d+/;
+    }
+    $regex = "^$regex";
+    return ($self->self_url =~ $regex) ? 1 : 0;
+}
+
 1;
 
 __END__
diff --git a/attachment.cgi b/attachment.cgi
index be82294d7..a10d9f970 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -77,10 +77,8 @@ my $action = $cgi->param('action') || 'view';
 # You must use the appropriate urlbase/sslbase param when doing anything
 # but viewing an attachment.
 if ($action ne 'view') {
-    my $urlbase = Bugzilla->params->{'urlbase'};
-    my $sslbase = Bugzilla->params->{'sslbase'};
-    my $path_regexp = $sslbase ? qr/^(\Q$urlbase\E|\Q$sslbase\E)/ : qr/^\Q$urlbase\E/;
-    if (use_attachbase() && $cgi->self_url !~ /$path_regexp/) {
+    do_ssl_redirect_if_required();
+    if ($cgi->url_is_attachment_base) {
         $cgi->redirect_to_urlbase;
     }
     Bugzilla->login();
@@ -243,10 +241,6 @@ sub view {
 
     if (use_attachbase()) {
         $attachment = validateID(undef, 1);
-        # Replace %bugid% by the ID of the bug the attachment belongs to, if present.
-        my $attachbase = Bugzilla->params->{'attachment_base'};
-        my $bug_id = $attachment->bug_id;
-        $attachbase =~ s/%bugid%/$bug_id/;
         my $path = 'attachment.cgi?id=' . $attachment->id;
         # The user is allowed to override the content type of the attachment.
         if (defined $cgi->param('content_type')) {
@@ -254,10 +248,16 @@ sub view {
         }
 
         # Make sure the attachment is served from the correct server.
-        if ($cgi->self_url !~ /^\Q$attachbase\E/) {
-            # We couldn't call Bugzilla->login earlier as we first had to make sure
-            # we were not going to request credentials on the alternate host.
+        my $bug_id = $attachment->bug_id;
+        if (!$cgi->url_is_attachment_base($bug_id)) {
+            # We couldn't call Bugzilla->login earlier as we first had to 
+            # make sure we were not going to request credentials on the
+            # alternate host.
             Bugzilla->login();
+            my $attachbase = Bugzilla->params->{'attachment_base'};
+            # Replace %bugid% by the ID of the bug the attachment 
+            # belongs to, if present.
+            $attachbase =~ s/\%bugid\%/$bug_id/;
             if (attachmentIsPublic($attachment)) {
                 # No need for a token; redirect to attachment base.
                 print $cgi->redirect(-location => $attachbase . $path);
@@ -291,6 +291,7 @@ sub view {
             }
         }
     } else {
+        do_ssl_redirect_if_required();
         # No alternate host is used. Request credentials if required.
         Bugzilla->login();
         $attachment = validateID();