http://qs1969.pair.com?node_id=167264

After blundering my first post to this site, I hope this Node redeems me to some degree. I have posted a complete revision of my horrible code at html/file security cgi [revisited]. (I tried to post it here, but neither pre nor code gave me the desired results).

Please take a look at the code now (if you are wondering how bad it was look for my post with a link to it). Is there anything else I should change to make this program more secure? Anything else I can do to make it good perl form? Or can I zip it up and call it a success? You be the Judge! :)

~UberDragon13

============================================================
....Sometimes life can be as bitter as dragon tears. But whether dragon tears are
bitter or sweet depends entirely on how each person perceives the taste....
============================================================

Replies are listed 'Best First'.
Re: Oh wise Brothers where will you guide me now?
by vladb (Vicar) on May 17, 2002 at 18:22 UTC
    I'm overjoyed to see you put so much effort in improving your code. That's an excellent spirit, brother! ;-).

    I have a couple suggestions regarding the code.
    • Stay away from using globals. Store them in a nice hash inside a configuration file. There's no need to keep it all in one file, less so as globals ;).

    • I know this may sound as a moot point, but I think the fewer curly brackets you have in your code the cleaner and more Perlish it looks. So, for example, start by changing all one-liner 'ifs' like so:

      From this:
      if ( cookie() ) { return 1; }
      to this...
      return 1 if cookie();


    • Use true Perl, brother. For example, replace this:
      if ($multi_in == 1) { ... }
      With ...
      if ($multi_in) { ... }
      Or, if you expect $multi_in to be equal to a set of specific values, I'd suggest using constancs for that.

    • Also, move all of your code inside convenient subroutines that do one thing at a time. Personally, I don't like a lot of clutter in the main package... try to move most of it into separate subs and it'll make the code look much cleaner and be easier to maintain.

    • In addition to CGI, consider using HTML::Template to separate all your program logic from display logic. I'm not sure how advanced the module is for a newcomer, but give it a try and ask questions if things don't work out the way they should. Challenges like this are by far the best way to learn new things and improve your programming skills. For one, I used this module in virtually every Perl application I wrote thus far and don't regret the experience! ;-)

    • Instead of using system mail commands to send your email messages, I suggest you use a well tested Mail::Mailer module. Here's an example:
      use Mail::Mailer; $mailer = Mail::Mailer->new(); $mailer->open({ From => $from_address, To => $to_address, Subject => $subject, }) or die "Can't open: $!\n"; print $mailer $body; $mailer->close();
      The beauty of this module is that it will allow your script to be a little less platform sensitive. For example, you may still choose to use 'mail' or 'sendmail' Unix tools via this module, or you may opt instead to send your email directly via Net::SMTP (simply use smtp() method of the module).

    • i'll update this node with more hits as I think of them..


    UPDATE 1: added comment on HTML::Template

    UPDATE 2: Ahh, thanks UD13, I didn't notice you were already using CGI.pm. I guess, a point in case would be to pull all 'use foobar' statements at the top of your script. No use in keeping 'use ...' scattered about the script ;-)

    UPDATE 3: fixed a few spelling 'bugs' thanks to c ;)

    _____________________
    $"=q;grep;;$,=q"grep";for(`find . -name ".saves*~"`){s;$/;;;/(.*-(\d+) +-.*)$/;$_=&#91"ps -e -o pid | "," $2 | "," -v "," "]`@$_`?{print" ++ $1"}:{print"- $1"}&&`rm $1`;print"\n";}