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

It has been a while since i last made something with Perl so I decided for a brief refresher to make a simple guestbook. I was just wondering if any one had any comments or if any one could see any huge security holes.

Your script has a race condition. That is, two copies
of your script can be running at the same time
and they both can try to write a new entry to the
guestbook, resulting in it being curropted.
To fix this problem you can use flock.
(Just use Fcntl qw(:flock); ... flock HTML, LOCK_EX; would probably be enough, see the
docs for details.)

A more minor issue with your script is
that print start_html() will print out
<html> and a <head> section
for your document. You probably shouldn't use it, unless
you plan to use CGI to generate all your
HTML. (Especially considering that CGI will output
XHTML in newer versions, making it so browsers
would be welcome to reject your apparently well-formed
HTML.)

I notice that you're testing variables for a pattern and then
preforming subtitutions for that pattern. It would be clearer, and probably just as fast, to just try the subtitution -- if the pattern's not found, nothing will happen.
Personally, I would write your repetitive substitutions
of <, etc. for the appropriate entities with
a foreach loop:

Ok, thank you very much. and about the typo, can you tell I'm running windows at the moment? lol
( and if any one is interested (which they aren't) I have a duel boot, I'm running windows at the moment because i can't get any multi-messengers to work and told somebody I'd talk to them on msn)

The escapeHTML function is of course provided by CGI.pm.
It doesn't come with the :standard set
use CGI qw/:standard/; so if you want to use
it that way you have to include it
use CGI qw/:standard escapeHTML/;. This of course
is not an issue if you're using CGI.pm with its object
interface (as I have assumed above).

Remember that that $in in the foreach loop
is an alias to the elements in the list we've created. So for
the first iteration of the loop, $in is the same as
$message and everything you do to it is done to
$message.

You also might be interested in looking at Damian Conway's
Regexp::Common module as it has a very elaborate smut
filter that would save you having to write one of your own.

(1) I'd use a module for stripping those tags, especially since you want to allow some and disallow others. I've not looked at this before, but there's bound to be one somewhere... searches CPAN for HTML stuff... well, just from glancing at the docs, looks like HTM::TagFilter would do just what you want. Looks as though it wd let you be pretty flexible abt what html you allowed your users, in an easy to maintain kind of way.

(2) Wog alluded to this one in a different context: you cd do your censor in one regex:
$message =~ s/badword2|badword3|badword/bleep/ig;Although n.b. you have to be a little careful with the order of your search words, if some of them "contain" others. You notice I have

My preference in a short script (or in any script, because I have only a limited understanding of OO programming) is the former. Then you could do all your printing right at the end, after munging around your $message etc variables, with these lines which (and I agree beauty is in the eye of the beholder) I think are elegant and easy to read:

I realize you run regexes on everything currently, so it won't hurt you, and you never know when somebody will come in and edit your code sometime and take out one of the censoring checks without realizing the problem.

I agree with the other wise comments in this thread, but have one more point to add:

You can let your users use 'real' HTML tags (only those you allow, of course) instead of having them use other brackets or parentheses in an HTML-like language. Just update your regexps accordingly, it's really quite simple. Make sure you do filter out all non-allowed HTML tags, of course.

(When I saw the 'headline' on a Slashdot nodelet box, I just knew it was one of your threads :-) .)

This is useless since we have already replaced all occurrences of (red) and (/red). Perhaps we could count successful matches or place this if block before the foreach block.: print "Name: $name <br> Email: $mail <br> Message: $message";Let's move this down and combine it with the ending.

I still have this lying around, a subroutine to convert 'AAAtml' (in use at the boards at The Alien Adoption Agency (which is powered by Perl (though the db seems pretty flaky))). It's not 100% bug-free (it hiccups when you incorrectly write links), but otherwise, it's pretty good.