RFC: New GNATS web (query-pr.cgi) interface

Sun Sep 17 12:04:23 PDT 2006

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 at 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
-------------- next part --------------

--- 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);
 
