Critique my First Script for me?

jerrygarciuh has asked for the wisdom of the Perl Monks concerning the following question:

The following is my first script created using the Llama and the help of the Monks hereabouts to whom I am very grateful. I have added vars for the bits most likely to change or which are repeated and made modifications based on all responses to my posts. I would like to ask for a critique of my programming so that I may know where I have used bad form, insufficient commenting, insecure methods, or shown insuffucient laziness etc.
The script is meant to check a password and, if correct, write form input to a text file which is used as an SSI. I want to learn to make dummy-friendly interfaces for clients to update the most frequently changed parts of their websites. It will become more elaborate as I try to make it edit a table calendar. That said here is the script.
TIA
jg

#!/usr/bin/perl -w
use strict;

use CGI qw/:standard/;

use CGI::Carp qw/fatalsToBrowser /;

my $q = CGI->new();

#declare some vars
my $mail ='menu@uranus.com';
my $path_to_text="/home/mysite/www/myfolder/my.txt"

# Parse Form Search Information
# if it's a single form value, map the value as a scalar, otherwise, t
+he value is
# an arrayref
my %FORM = map { $_, @{[ param($_) ]} > 1 ? [ param($_) ] : param($_) 
} param();

my $words = $q->param('words');   
          if ($words eq ""){
        $words = "no specials today";
          }
my $user = $ENV{'REMOTE_ADDR'};

my $pass = $q->param('pass');
   if ($pass ne "whatever"){
        open (TEXTFILE,"$path_to_text") || die "where's the damn file?
+ : $!";
      $words = <TEXTFILE>;
      close (TEXTFILE) || die "close damn you : $!";
        print "Content-type: text/html\n\n";
        print "<HTML> \n";
        print "<body bgcolor=000000 text=99CCCC> \n";
        print "<CENTER><H1><font face=Arial>!Unauthorized Access!\n";
        print "<BR>";
        print "The menu for tonight remains \n";
        print "<font color = blue> $words </font>";
        print ". <BR> \n";
        print "<font color = red>Hey bub, stop messin' with my menu!\n
+";
        print "<BR>You are at IP address $user !";
        print "</H1></CENTER></font></body>";
        open  MAIL,"|mail $mail" or die "mail problem : $!";
        print MAIL "$user tried to get in using $pass for a password.\
+n";
        close MAIL;
        exit 0;
   }else{
    open (TEXTFILE,"$path_to_text") || die "where's the damn file? : $
+!";
    print TEXTFILE  $words;
    close (TEXTFILE) || die "close damn you : $!";
    print "Content-type: text/html\n\n";
    print "<HTML><body bgcolor=000000 text=99CCCC><CENTER><H1> <font f
+ace=Arial> \n";
    print "The menu has been updated for you. <BR> \n";
    print "The menu for tonight is \n";
    print "<font color = blue> $words </font>.</CENTER></H1></font></b
+ody>";
        open  MAIL,"|mail $mail" or die "mail problem : $!";
        print MAIL "$user changed the menu $words.\n";
        close MAIL;
   }
[download]

Comment on Critique my First Script for me? Download Code

Replies are listed 'Best First'.
(crazyinsomniac) Re: Critique my First Script for me? by crazyinsomniac (Prior) on Sep 22, 2001 at 01:25 UTC
You're using the CGI OO interface, so why `use CGI qw/:standard/;`? //////* Open up your copy of CGI.pm, and take a look at :standard ':html2'=>['h1'..'h6',qw/p br hr ol ul li dl dt dd menu code v +ar strong em tt u i b blockquote pre img a address cite samp dfn htm +l head base body Link nextid title meta kbd start_html end_htm +l input Select option comment charset escapeHTML/], ':html3'=>[qw/div table caption th td TR Tr sup Sub strike app +let Param embed basefont style span layer ilayer font frameset fr +ame script small big/], ':form'=>[qw/textfield textarea filefield password_field hidde +n checkbox checkbox_group submit reset defaults radio_group popup_menu button auto +Escape scrolling_list image_button start_form end_form startfor +m endform start_multipart_form end_multipart_form isindex tmpFileN +ame uploadInfo URL_ENCODED MULTIPART/], ':cgi'=>[qw/param upload path_info path_translated url self_ur +l script_name cookie Dump raw_cookie request_method query_string Accept user_agent +remote_host content_type remote_addr referer server_name server_software server_po +rt server_protocol virtual_host remote_ident auth_type http save_parameters restore_parameters param_fetch remote_user user_name header redirect import_names put Delete Delete_all url_param cgi_error/], ':standard' => [qw/:html2 :html3 :form :cgi/], [download] That's a lot isn't it, and you're only using param (wasteful). You should either `use CGI qw/param/;` (not reccommended), or change your map hash builder to use `$q->param(...` instead of just `param(...` (also not reccommended). Or, and this is reccomended, just call Vars like i mention below (ala `my %FORM = $q->Vars();`. //////// You should also turn on taint ( #!perl -wT...) look for the cgi security stuff in perlfaq or search the site for taint checking (or visit Ovids homenode) for more info What do you think `$words = <TEXTFILE>;` accomplishes? That will get a single line out the handle, is that what you want? What's this? `print "Content-type: text/html\n\n"; print "<HTML> \n"; print "<body bgcolor=000000 text=99CCCC> \n";` [download] You got CGI, use it (`print $q->header(),$q->start_html()....`). `open (TEXTFILE,"$path_to_text") \|\| die "where's the damn file? : $!"` is pretty good, but `\|\| die "where's the hell is $path_to_text? : $!"` is better. It is also a good idea, to use CGI 2.73; #version, cause you never know. Also, the way you build you %FORM is clever, but CGI (modern versions, not ancient), have a Vars method that will do that for you, and considerably (%50+) quicker (cause it doesn't call param a million times). * - updates made ___crazyinsomniac_______________________________________ `Disclaimer: Don't blame. It came from inside the void` `perl -e "$q=$_;map({chr unpack qq;H;,$_}split(q;;,qH*));print;$q/$q;"`	[reply] [d/l] [select]
Re: (crazyinsomniac) Re: Critique my First Script for me? by jerrygarciuh (Curate) on Sep 22, 2001 at 02:05 UTC
The %FORM was contributed by Ovid during my search engine research, the `use CGI qw/:standard/;` is left over from that, I thought it was invoking CGI. I will make a point of rereading some of my CGI books now that my code knowledge is growing, I was reading them first when I found I needed to learn Perl to make any sense of them. I don't understand several of your questions and this will make them excellent points on which I may build knowledge. Thanks again crazyinsomniac!	[reply] [d/l]
Re: Re: (crazyinsomniac) Re: Critique my First Script for me? by runrig (Abbot) on Sep 22, 2001 at 02:58 UTC
the use CGI qw/:standard/; is left over from that, I thought it was invoking CGI. `use CGI;` is all you need if you are using the OO interface (that's when you do `my $q = CGI->new;`). The 'qw/:standard/' part imports function names so that you don't need the $q variable and lets you, e.g., say 'param($field)' instead of '$q->param($field)', which makes it a little more of a hassle, but I don't like to import anything I don't have to, and sometimes you might need to create more than one CGI object, and so in that case the OO way is the only way to go.	[reply] [d/l] [select]
Re: Re: Re: (crazyinsomniac) Re: Critique my First Script for me? by blakem (Monsignor) on Sep 22, 2001 at 03:35 UTC
Re: Re: Re: Re: (crazyinsomniac) Re: Critique my First Script for me? by runrig (Abbot) on Sep 22, 2001 at 03:46 UTC
Re: (crazyinsomniac) Re: Critique my First Script for me? by jerrygarciuh (Curate) on Sep 23, 2001 at 22:00 UTC
CI, About using `use CGI qw/:standard/;` while using CGI OO interface: When I don't use it the bit Ovid gave me won't work: `my %FORM = map { $_, @{[ param($_) ]} > 1 ? [ param($_) ] : param($_) } param();` [download] presumably because it is written in the function-oriented style (?correct?). I have been reading perlman:lib:CGI and I am attempting to get my head around how the line does its thing. Maybe I will attempt an OO rewrite of it soon. Peace. jg	[reply] [d/l] [select]
Re: Critique my First Script for me? by clintp (Curate) on Sep 22, 2001 at 02:50 UTC
Nothing tragically wrong, but the sequence: `print ... print ... print ... print ... print ...` [download] Is a little hard on the eyes. All those keywords, punctucation, backslashes, etc.. Instead consider using a here-document (sometimes called a here-script): `print<<EOHTML; Content-type: text/html <html> <body> <font color="blue">$words</font> </body> </html> EOHTML` [download] They're documented in the "perldata" manual page. For now, just remember to put the opening token right after the <<, use a semicolon at the end of that line, and put the closing token ("EOHTML") against the left edge on a line by itself and you'll be okay.	[reply] [d/l] [select]
Re: Critique my First Script for me? by grinder (Bishop) on Sep 22, 2001 at 17:09 UTC
For a first script, this is a mighty fine effort. A few more points worth pointing out, but they are minor niggles: At `open (TEXTFILE,"$path_to_text")`, when you have a variable interpolated into a string, and nothing else, just use the variable (e.g. `$path_to_text` instead of `"$path_to_text"`). Although I realise this could well just be an artifact of the development process. The initialisation of `$words` is more idiomatically performed using the `\|\|` short-circuit operator thusly: `my $words = $q->param(words) \|\| 'no specials today'` Sending mail can be performed by perl, rather than spawning an external program. Options include Mail::Mailer, Net::SMTP or my favourite Mail::Sendmail (that, despite its misleading name, does not require sendmail (nor Unix, for that matter)). Point of order on your HTML, `<font color = blue>` is better written as `<font color="blue">`. You can use a heredoc or the `qq{}` operator to avoid having to escape out the embedded "s in your string. -- `g r i n d e r`	[reply] [d/l] [select]
Re: Critique my First Script for me? by blakem (Monsignor) on Sep 22, 2001 at 04:04 UTC
I got called into a meeting before I could respond to this, and people have already covered everything I was going to suggest. However, I just wanted to let you know that this script was far, far better than I expected from the title "Critique my First Script for me?." I think you deserve a pat on the back for taking the time to read the Llama and hang out here. You are off to a great start. -Blake	[reply]
Re: Critique my First Script for me? by Amoe (Friar) on Sep 22, 2001 at 14:59 UTC
Thatsh shome nische shcriptin', fella. The only issue I can see is that it's probably not a good idea to hard-code a plaintext password into the script, as if any security holes arise with the CGI and the cracker in question manages to dump the source, they've got the password right in front of their eyes. You might want to keep it in a separate file, preferable encrypted. -- `my @words = grep({ref} @clever);` `sub version { print "I cuss your capitalist pig tendencies bad!\n" }`	[reply] [d/l] [select]

g r i n d e r

`g r i n d e r`