in reply to A Little review for a little DBI and CGI?
coolmichael, if this is your first useful piece of code, as you say, you are doing quite well. I can see that the advice from more experienced monks is influencing your coding style. With that said, I have a few comments:
You should place a T beside the -w on your first line. This will turn on taint mode, which should be on inside a CGI, and for that matter, any perl script that accepts user supplied data. Read perlsec to see why this switch is so important.
On line 38, you use something called indirect object syntax. This is personal preference, but I try to always use direct object syntax, like:
my $q = CGI->new;For a good explanation of why this could cause problems, check out this warning in perlobj.
You are doing alot of checking against the DBI calls, die'ing if there is a problem. You should look into using the RaiseError attribute when creating your database handle. In DBI::connect it is the 4th argument, but you can also embed it into your DSN definition on line 56, like so:
my $connectstr="DBI:CSV(RaiseError=>1):f_dir=/home/httpd/data;" .It's your choice how to use this, but the net effect is a reduction of debugging code.
Have you thought of embedding the column names inside the CSV file? DBD::CSV will read the first line of the file, and figure out the column names for you.
I noticed that you had the column names in two places, once in the regex, and once in the @names initialization. If you wanted to, you could abstract this out and keep the names in a single place. For example, you could do something like this:
use constant COLUMNS => [qw(Consign ISBN Price Title Author Subject)]; my $regex = join '|', @{COLUMNS()}; my ($search) = $q->param('search') =~ /^${regex}$/; die 'Bad search criteria' unless defined $search;
The only thing about this technique, is it will open up your database to be searchable by the Consign column. This one is totally your preference, it's just that when I see the same data in two places red flags are raised, as there is the chance for that information to diverge.
I think some of the things in the DSN are unecessary. I believe with regular quote-comma format, like the one you are using, the only necessary attribute to define is csv_eol. The others you are defining are that module's documented defaults.
Your SQL query would then become:
SELECT * FROM onshelf WHERE Title LIKE "%%" AND something LIKE "sensitive data%"Obviously, this isn't a real world example, but it illustrates my point, which is to always validate the user input AND try to use placeholders in your SQL query:
my $statement = qq{ SELECT * FROM onshelf WHERE $search LIKE ? }; my $sth = $dbh->prepare($statement); $sth->execute("%$criteria%");
The difference with this method is that the information passed to $sth->execute will be quoted. Combine this with checking the criteria parameter for validity, will make your code more, but not absolutely, secure. Never trust information you are getting from the user.
This one is more of a neat trick. One thing that always bugged me with bind_columns was that I'd need to define my lexically scoped variables, then bind them in two steps. That was until I figured out I could do this:
$sth->bind_columns(\my($consign, $isbn, $price, $title, $author, $subject));You may want to reconsider using a SELECT * in your SQL query. There was an excellent thread a few months ago regarding this: Topics in Perl Programming: Table-Mutation Tolerant Database Fetches with DBI. It's a node I would definately recommend reading, it was very educational for me.
That's all the suggestions I have for now. All in all your code is quite good, please don't take the length of the review as an insult. I wanted to explain each point so that you, and others, understood the significance of each point I was trying to make.
|
|---|
| Replies are listed 'Best First'. | |
|---|---|
|
Re: (dkubb) Re: (2) A Little review for a little DBI and CGI?
by coolmichael (Deacon) on Mar 28, 2001 at 13:38 UTC | |
by tye (Sage) on Mar 28, 2001 at 23:01 UTC | |
by coolmichael (Deacon) on Mar 29, 2001 at 00:45 UTC | |
by marius (Hermit) on Mar 28, 2001 at 22:00 UTC |