Cobo has asked for the wisdom of the Perl Monks concerning the following question:
Any comments would be appreciated, thanks.#!/use/bin/perl -w use strict; use CGI ':standard'; print header(); print start_html(); print "<html><head><title>Thanks for Signing!</title></head>"; print "<body bgcolor=black text=white>"; my $q=new CGI; my $name =$q->param('name'); my $mail =$q->param('mail'); my $message =$q->param('message'); print "<center><p>Thanks for signing my guestbook, your message +has been posted! $name!</center></p>"; # REMOVE THIS COMMENT TO ACTIVATE CENSOR if ($message =~/badword/i or +$message =~/badword2/i or $message =~/badword3/i) {$message =~ s/badw +ord/bleep/ig; $message =~ s/badword2/bleep/ig; $message =~ s/badword3 +/bleep/ig;}; if ($message =~/</ or $message =~/>/) {$message =~ s/</</g; $messag +e =~ s/>/>/g;}; if ($name =~/</ or $name =~/>/) {$name =~ s/</</g; $name =~ s/>/> +;/g;}; if ($mail =~ /</ or $mail =~/</) {$mail =~ s/</</g; $mail =~ s/</&g +t;/g;}; if ($message =~ /\(b\)/i or $message =~ /\(i\)/i or $message =~ /\(\/i +\)/ or $message =~ /\(\/b\)/) {$message =~ s/\(b\)/<b>/ig; $message = +~ s/\(i\)/<i>/ig; $message =~ s/\(\/b\)/<\/b>/ig; $message =~ s/\(\/i +\)/<\/i>/ig;}; if ($message =~ /\(red\)/i or $message =~ /\(\/red\)/i) {$message =~ s +/\(red\)/<font color=red>/ig; $message =~ s/\(\/red\)/<\/font>/ig;}; if ($message =~ /\(red\)/i and $message =! /\(\/red\)/i) {$message=$me +ssage."</font>"}; print "Name: $name <br> Email: $mail <br> Message: $message"; open HTML, ">>../gbook.html" or die $!; print HTML "<i>Name:</i> $name <br> <i>E-Mail: </i>$mail<br> <i>Messag +e: </i>$message <p>"; close HTML; print "</BODY></HTML>";
|
---|
Replies are listed 'Best First'. | |
---|---|
Re: Looking for feed back on a guestbook
by wog (Curate) on Nov 15, 2001 at 07:47 UTC | |
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:
A final potential problem in your script is that you might want to think of a better way of handling situtations where certain input fields are blank. In your use of CGI.pm you are mixing the object-oriented ($q->something) and function-based (something()) interfaces. It would be clearer if you stuck with only one. update: I notice you use #!/use/bin/perl -w. I suspect you meant #!/usr/bin/perl -w | [reply] [Watch: Dir/Any] [d/l] [select] |
by Cobo (Scribe) on Nov 15, 2001 at 08:02 UTC | |
| [reply] [Watch: Dir/Any] |
Re: Looking for feed back on a guestbook
by jarich (Curate) on Nov 15, 2001 at 09:42 UTC | |
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. | [reply] [Watch: Dir/Any] [d/l] [select] |
Re: Looking for feed back on a guestbook
by George_Sherston (Vicar) on Nov 15, 2001 at 12:16 UTC | |
(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 instead of because the latter wd turn "badword2" into "bleep2". (3) About printing out, I agree with the monks who say use CGI or die;, and also those who say that if you do You don't need then to do 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: Hope that's some use. Please do post your revised code when you're finished with it, as it makes the thread a more complete learning resource for future generations. § George Sherston | [reply] [Watch: Dir/Any] [d/l] [select] |
Re: Looking for feed back on a guestbook
by drinkd (Pilgrim) on Nov 15, 2001 at 18:49 UTC | |
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. drinkd | [reply] [Watch: Dir/Any] |
Censoring...
by joealba (Hermit) on Nov 15, 2001 at 18:35 UTC | |
Regexp::Common
| [reply] [Watch: Dir/Any] [d/l] |
Re: Looking for feed back on a guestbook
by orkysoft (Friar) on Nov 15, 2001 at 17:58 UTC | |
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 :-) .) | [reply] [Watch: Dir/Any] |
Re: Looking for feed back on a guestbook
by Cobo (Scribe) on Nov 15, 2001 at 20:42 UTC | |
I've used a couple of the suggestions so far, when I get home I'll mess around with it more. And thank you all for the comments, I found them very helpful. | [reply] [Watch: Dir/Any] [d/l] |
by CharlesClarkson (Curate) on Nov 16, 2001 at 02:01 UTC | |
For some reason, center is not included in :standard. If we lead an item with *, we can use start_ and end_ (*html is included in :standard.) No need to load this unless we're using it. (Moved down page.) This can be combined into: This uses CGI.pm as an object. We called CGI in the function oiented style. Most everyone agrees - don't mix the two styles. With center defined: These foreach blocks can be combined. I used function calls for the HTML and variables for (red) and (/red) to improve readability (TIMTOWTDI). 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. Or perhaps: Let's add the print from above and use it with CGI.pm.
HTH, Charles K. Clarkson | [reply] [Watch: Dir/Any] [d/l] [select] |
by tomhukins (Curate) on Nov 15, 2001 at 21:58 UTC | |
| [reply] [Watch: Dir/Any] [d/l] [select] |
Re: Looking for feed back on a guestbook
by orkysoft (Friar) on Nov 16, 2001 at 02:45 UTC | |
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.
| [reply] [Watch: Dir/Any] [d/l] |