http://qs321.pair.com?node_id=100755


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

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