523495 - Infinite redirect loop when viewing an attachment

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Max Kanat-Alexander]

When viewing an attachment with ssl_redirect turned on, I get an infinite redirect loop.

Comment 1

[Frédéric Buclin]

So this is a regression due to bug 514913 as it was working fine before.

Comment 2

[Max Kanat-Alexander]

Attachment: v1

Here we go, this fixes it. I tested it with and without an attachment_base, and with the attachment_base having both http and https.

I also took the opportunity to do some refactoring--we now only redirect to urlbase/sslbase on every page load away from attachment_base if the url matches attachment_base--this once again gets us closer to allowing people to use arbitrary URLs for their Bugzillas (often requested on the support list).

Comment 3

[Frédéric Buclin]

Comment on attachment 407447 [details] [diff] [review]
v1

>Index: attachment.cgi

>-        $attachbase =~ s/%bugid%/$bug_id/;

The comment associated with it must move too.


>-        if ($cgi->self_url !~ /^\Q$attachbase\E/) {
>+        if (!$cgi->url_is_attachment_base) {

At this point, we know the bug ID and so we compare $cgi->self_url with the attachment base updated with this ID in place of %bugid%. In your code, you only compare it to \d+, which would reintroduce XSS again.


>+            # 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.

I'm not a fan of this kind of "cleanup". Shortening comment lines because you like tiny ones hides which bug really implemented them in Bonsai, forcing us to do extra work to find the original patch. There is no reason to alter these comments. Please revert that.


>-                print $cgi->redirect(-location => $attachbase . $path);
>+                print $cgi->redirect(-location => $base . $path);

Here too, unless you have a very good reason to, please don't change the variable name. This again makes it harder to track real code changes.



>Index: Bugzilla/CGI.pm

>+    if ($self->url_is_attachment_base and $script ne 'attachment.cgi') {

It would be slightly cheaper to first compare $script to 'attachment.cgi' and then call ->url_is_... if necessary.


>+sub url_is_attachment_base {
>+    my ($self, $id) = @_;

You never pass any argument to this method.

>+    if ($id) {

So either this IF block goes away, or you have to use it somewhere (attachment.cgi?action=view comes to mind).


>+        $regex = quotemeta($attach_base);
>+        $regex =~ s/\\\%bugid\\\%/\\d+/;
>+    }
>+    $regex = "^$regex";

I don't see how this syntax is more readable/cleaner than the one I used, using \Q \E. I don't like all these escaping characters (it's prone to errors).

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> >+            # 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.
> 
> I'm not a fan of this kind of "cleanup". Shortening comment lines because you
> like tiny ones hides which bug really implemented them in Bonsai, forcing us to
> do extra work to find the original patch. There is no reason to alter these
> comments. Please revert that.

  The comment was longer than 80 characters. :-) I made it be only 80 characters.

> >+sub url_is_attachment_base {
> >+    my ($self, $id) = @_;
> 
> You never pass any argument to this method.

  Ah, yeah, that's left over from the fact that I originally *was* checking the $bug_id in this method, which I'll put back. :-)

> I don't see how this syntax is more readable/cleaner than the one I used, using
> \Q \E. I don't like all these escaping characters (it's prone to errors).

  It's not an issue of readability, it's that if I need to insert a valid regex metacharacter in there, that's the *only* way it works. (I'd tell you to try it yourself, but believe me, it was a huge waste of time to figure it out.)

Comment 5

[Max Kanat-Alexander]

Attachment: v2

Okay, I've addressed all the comments I didn't respond to.

Comment 6

[Frédéric Buclin]

Comment on attachment 408109 [details] [diff] [review]
v2

>Index: Bugzilla/CGI.pm

>+    return 0 if !use_attachbase() or !i_am_cgi();

Nit: I think || would be clearer. Here, I tend to read this as:
  (return 0 if !use_attachbase()) or !i_am_cgi();


Also, having POD for this method would be useful. Seems to work fine. r=LpSolit

Comment 7

[Max Kanat-Alexander]

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.79; previous revision: 1.78
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.163; previous revision: 1.162
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.50; previous revision: 1.49
done