comment on

First of all, I just want to say thank you for the excellent comments you offered. I've coded Perl for about a year now, but within Text::Forge I'm shielded from most of the issues involved in traditional CGI programming, hence my immature code. I ignored some of your suggestions because I'm lazy.

This line does nothing useful, it does not untaint $mode
tr/// can't be used to untaint? I didn't know that.

This code is a very obtuse way to do something quite simple, namely go to one of two subs depending on the user input.
Quite true. This is just my "cookie cutter" dispatch hash... it makes more sense in a CGI that has many functions; although a CGI with many functions probably doesn't make much sense...

The main thing I've changed below is file locking. Could you take a look at this and tell me what you think? I'm attempting to use semaphores as advised by KM in a few of his/her posts.

I like the following and will definitely use them in my next CGI:
1. waiting for lock sleeper 2. your DieNice, WarnAdmin, and Unindent routines

#!/usr/local/bin/perl -Tw

# Pre-live checklist:
# [X] add some file locking for Godsakes!!!
# [X] link to this thing and do auth ONLY through SSL!!
# [X] protect /admin dir with .htaccess
# [X] use $ENV{ REMOTE_USER } once .htaccess is in place
# [X] make sure to untaint $ENV{ REMOTE_USER }
# [X] write a file-storing routine ... just did it inline instead
# [X] bonus points: use XML :)
# [X] app tempfiles?
# [X] move to "live" data dir automatically?
# [X] add CSV validation to _validate_upload
# [X] test CSV validation to _validate_upload
# [X] count fields in a cool way for $num_fields

use lib '/dw02/d37/dawgpolo/myperl';
use lib '/dw02/d37/dawgpolo/myperl/lib';

use CGI;
use CGI::Carp qw( fatalsToBrowser );  # I think this slows us down
use Data::Dumper;
use Date::Format;
use Dawg qw( %CONFIG %CSV );
use Dawg::DB;
use Dawg::Draw;
use Fcntl qw( :flock );
use strict;

$ENV{ HTTPS } or die "Insecure! Change URL to https://$ENV{ HTTP_HOST 
+}".
                     "$ENV{ SCRIPT_NAME }\n";
$ENV{ HTTPS } =~ tr/A-Za-z//cd;
$ENV{ SERVER_ADMIN } = $CONFIG{ EMAIL };
$ENV{ PATH }         = '/usr/local/bin:/usr/bin:/bin';

my $cgi    = CGI->new;
my $draw   = Dawg::Draw->new;
my $title  = 'CSV Update Tool';
my $self   = $ENV{ SCRIPT_NAME };
my $user   = $ENV{ REMOTE_USER };
$user =~ tr/A-Za-z//cd;
my $userIP = $ENV{ REMOTE_ADDR };
sub main {
  my %dispatch = (
    upload  => \&upload,
    debug   => \&debug,
    DEFAULT => \&upload,
  );

  $draw->headers();
  $draw->html_start( { title => $title } );

  my $action = $cgi->param('action') || 'DEFAULT';
  my $do_sub = $dispatch{ $action };

  if ( $do_sub ) {
    &$do_sub();
  } else {
    warn "[warn] Invalid action: [$action]";
    die "[die] You can't make me do that!\n";
  }

  $draw->html_end();
}

sub upload {
  my @errors = ();  # array of errors found during execution
  my @lines  = ();  # input lines of uploaded file
  my $page   = '';  # page that uploaded file corresponds to
  my $ucpage = '';  # uppercased version of the same thang

  my $mode    = $cgi->param(    'mode'        );
  $mode       =~ tr#a-z_##cd;                        # untaint that pa
+ram!
  my $in      = $cgi->upload( 'datafile_in' );       # returns a fileh
+andle
  my $logfile = "$CONFIG{ ADMIN }/transactions.log"; # log entire tran
+saction
  my $qwiklog = "$CONFIG{ ADMIN }/quicklog.csv";     # abbreviated log

  open( LOG, ">> $logfile" ) or die "Couldn't open $logfile";
  open( QWK, ">> $qwiklog" ) or die "Couldn't open $qwiklog";

  if ( $mode ) {
    # They're trying to upload a file

    push @errors, "Invalid filename" unless ( ref $in eq 'Fh' );

    # retrieve the uploaded file and check for data integrity
    unless ( @errors ) {

      my $rv = Dawg::DB->slurp_csv_safe( $in );
      if ( $rv ) {
        @lines = @$rv;
      } else {
        die "No lines returned while slurping CSV";
      }

      if ( $mode =~ /^([a-z_]+)_go$/ ) {
        $page   = $1;
        $ucpage = uc($page);
        die "unknown page [$ucpage]" unless ( exists $CSV{$ucpage} );
        my $result = Dawg::DB->validate_csv( $ucpage, \@lines );
        @errors = @$result unless $result == 1;
      }

    }

    if ( @errors ) {
      print qq[ <FONT COLOR="RED">ERRORS OCCURRED DURING FILE UPLOAD:\
+n<BR> ];
      print qq[ <UL>\n ];
  
      foreach my $error ( @errors ) {
        print "<LI>$error\n";
      }
  
      print qq[ </UL></FONT>\n ];

    } else {
      # upload was good.
      my $time    = time2str( "%C", time() );
      my $outfile = "$CONFIG{DATA}/$page.csv"
        or die "cannot draft datafile filename for [$page]";

      open( CSV, "> $outfile" ) or die "Couldn't open $outfile";

      print qq[ <FONT COLOR="GREEN">$ucpage UPLOAD SUCCESSFUL</FONT><B
+R>\n ];

      # - 1 - open and lock semaphores
      my $csv_sem = "$CONFIG{ADMIN}/.csv.lock";
      my $log_sem = "$CONFIG{ADMIN}/.transactions.lock";
      my $qwk_sem = "$CONFIG{ADMIN}/.quicklog.lock";
      open( CSV_SEM, "> $csv_sem" ) or die "$csv_sem: $!";
      open( LOG_SEM, "> $log_sem" ) or die "$log_sem: $!";
      open( QWK_SEM, "> $qwk_sem" ) or die "$qwk_sem: $!";
      flock( CSV_SEM, LOCK_EX ) or die "Can't lock CSV";
      flock( LOG_SEM, LOCK_EX ) or die "Can't lock LOG";
      flock( QWK_SEM, LOCK_EX ) or die "Can't lock QWK";

      # - 2 - write the datafile itself
      for (@lines) { print CSV "$_\n" }  # write csv to datafile

      # - 3 - write the entire transaction
      print LOG "[ $time ] $ucpage UPDATED by $user [$userIP]\n";
      for (@lines) { print LOG "$_\n" }  # log the transaction

      # - 4 - write a "qwiklog" entry
      print QWK "[ $time ] $ucpage UPDATED by $user [$userIP]\n";

      # - 5 - close all filehandles, drop locks
      close( CSV ) or die "Couldn't close file $outfile";  # file lock
+s
      close( LOG ) or die "Couldn't close file $logfile";  # are remov
+ed
      close( QWK ) or die "Couldn't close file $qwiklog";  # automatic
+ally
      close( CSV_SEM ) or warn "close error on $csv_sem: $!";
      close( LOG_SEM ) or warn "close error on $log_sem: $!";
      close( QWK_SEM ) or warn "close error on $qwk_sem: $!";
    }
  } else {
    # They're not trying to upload a file YET, so give them tips

  print <<End_HTML;
  
Hints:<BR>
<UL>
  <LI>Make sure the uploaded file is TRUE CSV, eg. blah,1,2,"blow, joe
+"
  <LI>Click the respective Upload button once, then <EM>wait</EM>
  <LI>You can only upload one file at a time
</UL>

End_HTML
  }

  foreach my $table ( sort keys %CSV ) {
    my $name = lc($table);

  print <<UploadForms;

  
<BR>
Download <A HREF="$CONFIG{ WEBDATA }/$name.csv">$name</A> datafile
<FORM ACTION="$self" METHOD="POST" ENCTYPE="multipart/form-data">
  <INPUT TYPE="FILE" NAME="datafile_in" SIZE="50" MAXLENGTH="80">
  <BR>
  <INPUT TYPE="SUBMIT" VALUE="Upload $name">
  <INPUT TYPE="HIDDEN" NAME="mode" VALUE="${name}_go">
</FORM>
<BR>

UploadForms
  }
}

sub debug {
  print "<PRE>\n";
  print Data::Dumper->Dump( [\%ENV], ['ENV'] );
  print "</PRE>\n";
}

main();
[download]

In reply to Re: Re: Code review, good 'ol CGI. by meonkeys
in thread Code review, good 'ol CGI. by meonkeys

Are you posting in the right place? Check out Where do I post X? to know for sure.
Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:
<code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>
Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
Want more info? How to link or How to display code and escape characters are good places to start.


Syntactic Confectionery Delight
	PerlMonks