Date: Sun, 17 Sep 2006 21:04:20 +0200 From: "Simon L. Nielsen" <simon@FreeBSD.org> To: Shaun Amott <shaun@FreeBSD.org> Cc: freebsd-www@FreeBSD.org Subject: Re: RFC: New GNATS web (query-pr.cgi) interface Message-ID: <20060917190419.GC33937@zaphod.nitro.dk> In-Reply-To: <20060912201245.GA1915@picobyte.net> References: <20060912201245.GA1915@picobyte.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--sHrvAb52M6C8blB9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On 2006.09.12 21:12:45 +0100, Shaun Amott wrote: > Some of you will already be aware of this, but for those that don't: I > have been working on a replacement web interface for GNATS. My main > motivation has been the desire to add a mechanism for downloading > patches from within PRs. > > Enhancements over the interface we have in place now: > > - Improved layout. > - Clearer, easier-to-read Audit-Trail. > - Coloured cdiff(1)-style patches. > - Patches are easily downloadable. > - Patches are uudecoded where applicable. > > The script is available here: > http://people.freebsd.org/~shaun/stuff/gnatsweb/ > > Since CGI is disabled on the GNATS machine, I have saved the output for > a few PRs in 'samples' - obviously the download links won't work. > > Comments and feedback welcome. :) I'm mainly commenting on the code, since I think most people who have already commented on the visual/usability part elsewhere generally agree, as do I, that it's an improvement over the current script. The code is certainly in the very good end of what perl can be, it was generally readable which is certainly not always the case with perl :-). Other than the minor cross-site-scripting issue, which I mentioned on IRC and which you have already fixed, I don't see any problems wrt. security in the script. One thing which did bug me a lot when reading it though was the use of the reversed conditionals like this: $category = undef if ($category && $category !~ /^$valid_category$/); I think it really breaks the flow of the code, but since some people like it I'm not going to object if this is presereved, just bitch a bit here :-). The main thing which I think should be done before commit is use of cgi-style.pl. While it might seem unnecessary now, it really is much nicer for the people maintaing the website if there are as few places as possible where we need to change if modifications are made to the layout. Also, the script currently doesn't totally match the style of the rest of the FreeBSD.org web site, which I suspect is related to it not using cgi-style.pl. I have attached a patch which fixes some minor issues. Most should be obvious, but: - I moved the URL variable to the start of the script since that makes it simpler to change when needed, e.g. when switching to the new www.freebsd.org hopefully soon... - I removed $FreeBSD$ from the resulting HTML, since I don't really see the pointing having it there. I noticed that you put Convert/UU.pm in the subdir of gnatsweb. I assume this is just /usr/ports/converters/p5-Convert-UU which we can just install on systems which need to run the CGI? I installed the port at freefall and it seems to make the script run "stand-alone". One other issue which also need to be fixed is that the script don't work with perl 5.0, which is still requires for a bit. Can't locate warnings.pm in @INC (@INC contains: /usr/local/lib/perl5/site_perl/5.005/i386-freebsd /usr/local/lib/perl5/site_perl/5.005 . /usr/libdata/perl/5.00503/mach /usr/libdata/perl/5.00503) at /usr/local/www/www.freebsd.org/data/cgi/shauns-new-query-pr.cgi line 33. The last thing I can think of now is that the script should use taint mode like shown below, just in case. [simon@freefall:tmp] head -n 1 query-pr.cgi #!/usr/bin/perl -T I think that was all I could think if saying now :-). -- Simon L. Nielsen --sHrvAb52M6C8blB9 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="shaun-query-pr-minor.diff" --- query-pr.txt.orig Sun Sep 17 18:49:39 2006 +++ query-pr.txt Sun Sep 17 21:04:04 2006 @@ -86,6 +86,8 @@ my $scriptname = htmlclean($ENV{'SCRIPT_NAME'}); my $querystring = htmlclean($ENV{'QUERY_STRING'}); +my $self_url_base = "http://www.FreeBSD.org/cgi/query-pr.cgi?pr="; +my $cvsweb_url = "http://www.FreeBSD.org/cgi/cvsweb.cgi/"; #----------------------------------------------------------------------- @@ -181,9 +183,6 @@ <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> <html> <head> - <!-- - \$FreeBSD\$ - --> <title>%%(1)</title> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <link href="query-pr.css" rel="stylesheet" type="text/css"> @@ -339,7 +338,7 @@ $i++; } - + if (!$f or $i == $#fields_single) { $section++; next; @@ -438,7 +437,7 @@ my %block; my $blockwhy; my ($inblock, $inresponse, $mbreak) = (0, 0, 0); - my $url = "http://www.freebsd.org/cgi/query-pr.cgi?pr=${PR}"; + my $url = "${self_url_base}${PR}"; # Hack for older PRs with no usable delimiter push @{$mfields{'Audit-Trail'}}, $url; @@ -602,8 +601,7 @@ $inresponse = 0; } } elsif ($field eq "Fix") { - foreach (@{$mfields{$field}}) - { + foreach (@{$mfields{$field}}) { s/\s+$//; $cfound = ($_ ? 1 : 0) if (!$cfound); @@ -733,8 +731,6 @@ sub htmlparse { - my $cvsweb_url = "http://www.freebsd.org/cgi/cvsweb.cgi/"; - my $v = shift; return "" if (!$v); --sHrvAb52M6C8blB9--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060917190419.GC33937>