104521 - remove old attachment interface in favor of attachment tracker

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

The so-called "attachment tracker" interface is really a generalized system for
handling attachments, especially once bug 98602 gets checked in.  We should
remove the old interface to attachments (createattachment.cgi,
showattachment.cgi) and replace it with the attachment tracker.

Comment 1

[Jacob Steenhagen]

I think the "attachment tracker" will be considered stable enough by the time
2.16 ships that we can remove the Param() for it and "cvs remove" the
appropriate files...

Comment 2

[timeless]

don't do that.

bugzilla installs have absolute urls to showattachment.cgi

Comment 3

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 4

[Dave Miller [:justdave]]

we got this started, need to finish the job before we ship...

showattachment.cgi can't go away for the reasons stated.  It should just be a
pass-through CGI that modifies $::ENV{QUERY_STRING} appropriately and calls
attachment.cgi.

createattachment.cgi, however, can go away, because the user should never be
accessing it without clicking a link, and we can fix all the links (see bug 109480)

Comment 5

[Bradley Baetz (:bbaetz)]

The plan (on one of the other related bugs) was to send a permenant redirect.
Theres a patch on that bug, too.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: removes old interface

This patch:

* Removes the useattachmenttracker parameter from defparams.pl.
* Modifies defparams.pl to remove obsolete parameters from the data/params file
upon modification of parameters.
* Removes conditional logic based on the useattachmenttracker parameter from
CGI.pl, bug_form.pl, and processmail.
* Removes comments about the attachment tracker from checksetup.pl.
* Modifies showattachment.cgi using some of the code from 109354 so that it
provides a permanent redirect to attachment.cgi.

Ready for review.

Comment 7

[Jacob Steenhagen]

Comment on attachment 63953 [details] [diff] [review]
patch v1: removes old interface

After applying the patch:
> [jake@jake bugzilla]$ grep showattachment.cgi *
> createattachment.cgi:      "\" HREF=\"showattachment.cgi?attach_id=$attachid\">$attachid</A> to bug $id created</H2>\n";
> globals.pl:        $item = qq{<A HREF="showattachment.cgi?attach_id=$num">$item</A>};
> globals.pl:            $item = qq{<A HREF="showattachment.cgi?attach_id=$num">$item</A>};
> processmail:            Param('urlbase') . "showattachment.cgi?attach_id=";


createattachment.cgi will be going away, so that's no big deal, but the links
everywhere else to showattachment.cgi really should be changed to
attachment.cgi (one less server hit, amoung other reasons).

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: fixes Jake's issues

Jake: good point.  here's v2 with fixes for those references to
showattachment.cgi.

Comment 9

[Bradley Baetz (:bbaetz)]

Comment on attachment 65427 [details] [diff] [review]
patch v2: fixes Jake's issues


>@@ -321,31 +324,12 @@
> <TD ALIGN=right><B><A HREF="describekeywords.cgi">Keywords:</A></B>
> <TD COLSPAN=7><INPUT NAME="keywords" VALUE="$value" SIZE=60></TD>
> </TR>
>+</TABLE>
> };
> }

The </table> should be outside this block, so that its still printed if
keywords are disabled.

>--- showattachment.cgi	20 Dec 2001 23:42:28 -0000	1.8
>+++ showattachment.cgi	17 Jan 2002 11:33:26 -0000

>+# Redirect to the new interface for displaying attachments.
>+my $attach_id = $::FORM{'attach_id'};
>+print "Status: 301 Permanent Redirect\n";
>+print "Location: attachment.cgi?id=$attach_id&action=view\n\n";
>+exit;

Won't this cause a warning if a user goes here w/o an attachment id?

>+my $suppress_warnings = $::FORM{'attach_id'};

use vars, please (although you'll be referencing it more than once by fixing
the above)

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: resolves bbaetz's issues and closes potential security hole

This version moves the closing TABLE tag to the right place, uses "use vars",
and detaints the "attach_id" form/url parameter in showattachment.cgi before
forwarding the user to attachment.cgi.	I don't know of any way to take
advantage of non-validated form data being used in HTTP response headers, but
just in case there are such beasts (or will be in the future), this taint check
solves the problem.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: forgot bbaetz' warning issue

This one properly prevents warnings from being issued if attach_id isn't
defined.

Comment 12

[Bradley Baetz (:bbaetz)]

Comment on attachment 65526 [details] [diff] [review]
patch v4: forgot bbaetz' warning issue

I played with it a bit and it worked.

r=bbaetz

Comment 13

[timeless]

Comment on attachment 65526 [details] [diff] [review]
patch v4: forgot bbaetz' warning issue

Hunk #1 FAILED at 26.
1 out of 1 hunk FAILED -- saving rejects to file showattachment.cgi.rej

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: applies cleanly

I didn't make any code changes, I just redid the patch to showattachment.cgi so
patch doesn't choke on it.

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 66900 [details] [diff] [review]
patch v5: applies cleanly

patch is stupid. r=bbaetz

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 66900 [details] [diff] [review]
patch v5: applies cleanly

<kiko> myk_: r=kiko on 104521, applied cleanly

Comment 17

[Christian Reis]

Yes, and the defparams.pl feature to remove the tracker param works according to
spec. Congrats, clean patch.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

It's checked in, and the tree is green.  Resolving fixed.

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.129; previous revision: 1.128
done
Checking in bug_form.pl;
/cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v  <--  bug_form.pl
new revision: 1.86; previous revision: 1.85
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.115; previous revision: 1.114
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.64; previous revision: 1.63
done
Checking in processmail;
/cvsroot/mozilla/webtools/bugzilla/processmail,v  <--  processmail
new revision: 1.76; previous revision: 1.75
done
Checking in showattachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/showattachment.cgi,v  <--  showattachment.cgi
new revision: 1.10; previous revision: 1.9
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.134; previous revision: 1.133
done

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Resolving for real this time.