I have a number of comments:
You should Use strict and warnings (also, since you're currently using -w, see What's wrong with -w and $^W).
For debugging, you can run CGI.pm scripts from the command line, e.g. perl -T comment.cgi 'event=foo&newCOMMENT=bar' and you can inspect the contents of variables (such as the filename, if you were to store it in a variable) with modules like Data::Dumper - see also the Basic debugging checklist.
When you are getting 500 server errors from CGI scripts, you should check the server's error logs for the exact error message. See also CGI Help Guide and Troubleshooting Perl CGI scripts (I've linked you to these before). There is also CGI::Carp's use CGI::Carp qw/fatalsToBrowser/;, although you should not use this in a production environment as it may reveal too many details of the server to an attacker.
You should use the three-argument form of open, lexical filehandles, and always check the return value for errors, for example: open my $fh, '>>', $filename or die "$filename: $!";
Since you can't always be sure what environment CGI scripts will be run in, you should use absolute pathnames.
But the biggest issue I see with this script is security. You are taking form input, completely unchecked, and using it directly in a filename. An attacker could inject arbitrary values here. At least you are using Taint mode (-T) - which is currently where your errors are coming from (and I think it's a good thing you're getting errors in this case...). But even so I would be very strict with the values that are allowed for those variables. (Also, what if $newCOMMENT contains newlines?)
Using flat files is not necessarily the most efficient or safe way to do this. I see several race conditions here, such as $commentUID (what if there are multiple comments submitted within the same second?), opening two files separately (what if two processes are running concurrently? you may get out-of-order lines), and also no file locking going on. This would be a good place to use a database instead.
Now, having said all that, it's still possible to do something like what you want. Although as I said I wouldn't recommend using flat files, this is just for the sake of showing one way to do it. Note that I am only using a single file - but a warning about this - I'm using flock, which is not supported on all filesystems!
#!/usr/bin/perl -T use warnings; use strict; use CGI qw/param header/; use Fcntl qw/:flock :seek/; my $COMMENTFILE = '/path/to/comments.txt'; # absolute pathname if ( length param('newCOMMENT') ) { # untaint form input with strict regexes my ($event) = param('event') =~ /\A(\w+)\z/ or die "bad event"; my ($comment) = param('newCOMMENT') =~ /\A([\w\h]+)\z/ or die "bad comment"; # we use "---" as a marker below, so strip that out $comment=~s/^---//gm; open my $fh, '>>', $COMMENTFILE or die "$COMMENTFILE: $!"; # lock the file flock($fh, LOCK_EX) or die "flock $COMMENTFILE: $!"; # in case someone else has written to the file in the meantime seek($fh, 0, SEEK_END) or die "seek $COMMENTFILE: $!"; # I've just picked a format for the flat file print $fh "--- ",time," ",$event,"\n"; # we know $comment doesn't contain "---" because of the regex print $fh $comment,"\n"; flock($fh, LOCK_UN) or die "un-flock $COMMENTFILE: $!"; close $fh; } print header('text/plain'); print "Hello, World\n";
In reply to Re: open file using variable passed by form
by haukex
in thread open file using variable passed by form
by michael.kitchen
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |