Newbie request for script review
A. Sinan Unur
I am trying to familiarize myself with Perl. To that end, I decided to
concoct a simple cgi script to browse images in a directory on web site.
The script is intended to be called in the following manner:
http://somehost.com/cgi-bin/photobrowser.pl?album/spring
As I have virtually no experience, and no feel for Perl, I would
appreciate your comments about this script before I "improve" it.
BEGIN: photobrowser.pl
#!C:/Perl/bin/perl.exe -w
# photobrowser
# Display links (optionally with thumbnails) to
# images in a directory
use strict;
use HTML::Template;
# Clean up the location specified in the request by
# removing all relative paths
my $location = shift;
if($location =~ s/(\.\.\/)|(\.\.)//g) {
print "Content: text/plain\n\nInvalid location specified in
request.";
} else {
# Configuration Variables
my $wwwroot = '';
my $script_location = '/cgi-bin/photobrowser.pl';
my $htdocs_path = 'C:/Program Files/Apache Group/Apache2/htdocs';
my $template_path =
'C:/Program Files/Apache Group/Apache2/templates';
my $template_file = 'photobrowser.tmpl.html';
my $photo_path = "$htdocs_path/$location";
my $thumb_dir_name = 'thumb';
my $template =
HTML::Template->new(filename => "$template_path/$template_file");
# Provide some information during development
$template->param(HTDOCS => $htdocs_path);
$template->param(LOCATION => $location);
$template->param(PHOTO_PATH => $photo_path);
opendir(PHOTO_DIR, $photo_path);
# For the line below, I get the warning
# [Sun Feb 23 15:03:18 2003] [error] [client 127.0.0.1]
# Use of uninitialized value in concatenation (.) or string at
# C:/Program Files/Apache Group/Apache2/cgi-bin/photobrowser.pl
my @files = grep { !/^\.+/ && /$\.jpg/ } readdir(PHOTO_DIR);
closedir PHOTO_DIR;
my @loop_data = ();
foreach my $file (@files) {
my %iter_data;
$iter_data{IMGFILE} = $file;
$iter_data{IMGURL} = "$wwwroot/$location/$file";
if(-e "$photo_path/$thumb_dir_name/$file") {
$iter_data{THUMBURL} = "$wwwroot/$location/thumb/$file";
} else {
$iter_data{THUMBURL} = "";
}
push(@loop_data, \%iter_data);
}
$template->param(IMGLIST => \@loop_data);
print "Content-Type: text/html\n\n", $template->output;
}
### END
BEGIN: Template file to use with HTML::Template
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html lang="en">
<head>
<title>Photo Browser - <TMPL_VAR LOCATION></title>
</head>
<body>
<table border="0" bgcolor="#dadada" cellspacing="2" cellpadding="4"
>
<tr>
<td bgcolor="white" align="right">HTDOCS: </td>
<td bgcolor="white"><TMPL_VAR HTDOCS></td>
</tr>
<tr>
<td bgcolor="white" align="right">LOCATION: </td>
<td bgcolor="white"><TMPL_VAR LOCATION></td>
</tr>
<tr>
<td bgcolor="white" align="right">PHOTO_PATH: </td>
<td bgcolor="white"><TMPL_VAR PHOTO_PATH></td>
</tr>
</table>
<TMPL_LOOP IMGLIST>
<a href="<TMPL_VAR IMGURL>"><TMPL_IF THUMBURL><img
src="<TMPL_VAR THUMBURL>"
title="<TMPL_VAR IMGFILE>"><TMPL_ELSE><TMPL_VAR IMGFILE></TMPL_IF>
</a>
</TMPL_LOOP>
</body>
</html>
--
A. Sinan Unur
as...@c-o-r-n-e-l-l.edu
Remove dashes for address
Spam bait: mailto:u...@ftc.gov
Bob Walton
...
> I am trying to familiarize myself with Perl. To that end, I decided to
Wow, a CGI script using HTML::Template is a whale of a way to
familiarize yourself with Perl -- quite a tall order.
> concoct a simple cgi script to browse images in a directory on web site.
> The script is intended to be called in the following manner:
>
> http://somehost.com/cgi-bin/photobrowser.pl?album/spring
>
> As I have virtually no experience, and no feel for Perl, I would
> appreciate your comments about this script before I "improve" it.
>
> BEGIN: photobrowser.pl
>
> #!C:/Perl/bin/perl.exe -w
Well, on Windoze, the #! line doesn't do much, so one could just replace
this with #!perl -w . The -w is good, though, as it will be picked up
and will turn warnings on.
> # photobrowser
> # Display links (optionally with thumbnails) to
> # images in a directory
>
> use strict;
Excellent!
> use HTML::Template;
>
For a CGI script, you should definitely:
use CGI;
even though it would only be used to print headers in this script.
> # Clean up the location specified in the request by
> # removing all relative paths
> my $location = shift;
Very interesting -- I never knew the URL arguments could be obtained in
this fashion. But it does seem to work. Where is this documented?
> if($location =~ s/(\.\.\/)|(\.\.)//g) {
OK, I guess? You might want to prohibit some specific directories under
your htdocs, though.
> print "Content: text/plain\n\nInvalid location specified in
> request.";
Hmmm...not sure why, but this is doing a file download, with the
filename being the name of the Perl script and the contents being the
"Invalid location ..." string. Maybe I don't have something configured
correctly for text/plain . Nope, that's not it -- I checked, a test
program works fine. I have no idea why it is doing that. The filetype
coming down is wwwserver/shellcgi . Ah, I see why now. It should be
"Content-type:", not "Content:". If you had used:
use CGI qw(:standard);
and done
print header,"Invalid location...";
that wouldn't have happened.
> } else {
> # Configuration Variables
> my $wwwroot = '';
> my $script_location = '/cgi-bin/photobrowser.pl';
> my $htdocs_path = 'C:/Program Files/Apache Group/Apache2/htdocs';
> my $template_path =
> 'C:/Program Files/Apache Group/Apache2/templates';
> my $template_file = 'photobrowser.tmpl.html';
> my $photo_path = "$htdocs_path/$location";
> my $thumb_dir_name = 'thumb';
>
> my $template =
> HTML::Template->new(filename => "$template_path/$template_file");
>
> # Provide some information during development
> $template->param(HTDOCS => $htdocs_path);
> $template->param(LOCATION => $location);
> $template->param(PHOTO_PATH => $photo_path);
>
> opendir(PHOTO_DIR, $photo_path);
You should check for failure here, especially since it is user-specified
>
> # For the line below, I get the warning
> # [Sun Feb 23 15:03:18 2003] [error] [client 127.0.0.1]
> # Use of uninitialized value in concatenation (.) or string at
> # C:/Program Files/Apache Group/Apache2/cgi-bin/photobrowser.pl
That's probably because your opendir failed -- if the opendir doesn't
fail, you don't get that -- or at least I don't.
> my @files = grep { !/^\.+/ && /$\.jpg/ } readdir(PHOTO_DIR);
Not sure what you are trying to do with the regex /$\.jpg/ -- that will
interpolate $\ and insert whatever that was (it wasn't modified, so it
will be undef, which will interpolate to the empty string) into the
regex. Then it will match any single character (the unescaped . ),
followed by jpg, lowercase only. You probably meant something more like
/\.jpg$/i or maybe even /\.jpe?g/i .
When there are no thumbnails, this is sort of ugly -- just a bunch of
filenames strung together. Works OK when there are thumbnails present,
though.
> </a>
> </TMPL_LOOP>
>
> </body>
> </html>
>
>
--
Bob Walton
A. Sinan Unur
news:3E597FAC...@rochester.rr.com:
> A. Sinan Unur wrote:
...
>> As I have virtually no experience, and no feel for Perl, I would
>> appreciate your comments about this script before I "improve" it.
>>
>> BEGIN: photobrowser.pl
>>
>> #!C:/Perl/bin/perl.exe -w
>
>
> Well, on Windoze, the #! line doesn't do much, so one could just
> replace this with #!perl -w . The -w is good, though, as it will be
> picked up and will turn warnings on.
I saw it in the printenv.pl script that came with Apache, and decided to
emulate it.
> For a CGI script, you should definitely:
>
> use CGI;
>
> even though it would only be used to print headers in this script.
OK.
>> # Clean up the location specified in the request by
>> # removing all relative paths
>> my $location = shift;
>
>
> Very interesting -- I never knew the URL arguments could be obtained
> in this fashion. But it does seem to work. Where is this documented?
Sorry, I don't know, but I should check. I guess I must have seen it in
someone else's code.
>> if($location =~ s/(\.\.\/)|(\.\.)//g) {
>
> OK, I guess? You might want to prohibit some specific directories
> under your htdocs, though.
My intent was to be able to use this script to browse images contained in
any directory. I don't know if that is wise. Probably limit it to a photo
album root.
>> print "Content: text/plain\n\nInvalid location specified in
>> request.";
>
...
> now. It should be "Content-type:", not "Content:".
arrrgh!!! Editing in the newsreader. I swear it it Content-type in the
file.
> If you had used:
>
> use CGI qw(:standard);
>
> and done
>
> print header,"Invalid location...";
>
> that wouldn't have happened.
Cool, thanks for pointing that out.
>>
>> opendir(PHOTO_DIR, $photo_path);
>
>
> You should check for failure here, especially since it is
> user-specified
OK.
>>
>> # For the line below, I get the warning
>> # [Sun Feb 23 15:03:18 2003] [error] [client 127.0.0.1]
>> # Use of uninitialized value in concatenation (.) or string at
>> # C:/Program Files/Apache Group/Apache2/cgi-bin/photobrowser.pl
>
> That's probably because your opendir failed -- if the opendir doesn't
> fail, you don't get that -- or at least I don't.
I rechecked. It looks like you are right.
>> my @files = grep { !/^\.+/ && /$\.jpg/ } readdir(PHOTO_DIR);
>
>
> Not sure what you are trying to do with the regex /$\.jpg/ -- that
> will interpolate $\ and insert whatever that was (it wasn't modified,
> so it will be undef, which will interpolate to the empty string) into
> the regex. Then it will match any single character (the unescaped .
> ), followed by jpg, lowercase only. You probably meant something more
> like /\.jpg$/i or maybe even /\.jpe?g/i .
Yes, I meant to place the $ at the end. I had assumed that $ modified the
pattern that came after it to be anchored at the end (does that sentence
make sense? I don't know how else to explain my motivation.) Thank you for
spotting it.
> When there are no thumbnails, this is sort of ugly -- just a bunch of
> filenames strung together. Works OK when there are thumbnails
> present, though.
Eventually, I would like to put the links/thumbnails in a table where the
width is specified as a configuration parameter. I am having difficulty
figuring out the nested <TMPL_LOOP>s and the associated data structure I
need to construct in Perl. I have read the relevant paragraph in the
HTML::Template pod several times, but it just hasn't sunk in yet.
Another planned extension is optionally reading image descriptions from
file stored in the image directory.
Thank you for your comments.
Sinan.
Tad McClellan
> I am trying to familiarize myself with Perl. To that end, I decided to
> concoct a simple cgi script
You should learn to crawl before running in a race.
Learn Perl from the command line first.
CGI applications are not "simple", there are several advanced
aspects to them, such as multitasking and security.
> I would
> appreciate your comments about this script before I "improve" it.
> #!C:/Perl/bin/perl.exe -w
> use strict;
Good. Very good.
> if($location =~ s/(\.\.\/)|(\.\.)//g) {
^^^^^^^^^
^^^^^^^^^
You don't need that part, the 2nd part will match that too.
No need for the s///g option either.
> # Configuration Variables
> my $wwwroot = '';
> my $script_location = '/cgi-bin/photobrowser.pl';
> my $htdocs_path = 'C:/Program Files/Apache Group/Apache2/htdocs';
> my $template_path =
> 'C:/Program Files/Apache Group/Apache2/templates';
> my $template_file = 'photobrowser.tmpl.html';
> my $photo_path = "$htdocs_path/$location";
> my $thumb_dir_name = 'thumb';
Consider putting all of those in a single hash instead:
$config{wwwroot} = '';
$config{script_location} = '/cgi-bin/photobrowser.pl';
...
> # Provide some information during development
> $template->param(HTDOCS => $htdocs_path);
> $template->param(LOCATION => $location);
> $template->param(PHOTO_PATH => $photo_path);
>
> opendir(PHOTO_DIR, $photo_path);
You may not get what you asked for.
You should check to see if you got it:
opendir(PHOTO_DIR, $photo_path) or die "could not open '$photo_path' dir $!";
> # For the line below, I get the warning
> # [Sun Feb 23 15:03:18 2003] [error] [client 127.0.0.1]
> # Use of uninitialized value in concatenation (.) or string at
> # C:/Program Files/Apache Group/Apache2/cgi-bin/photobrowser.pl
> my @files = grep { !/^\.+/ && /$\.jpg/ } readdir(PHOTO_DIR);
^^
^^
$\ is the "output record separator" special variable.
It defaults to undef (uninitialized).
Backslash the dollar sign if you really want to match a dollar sign char.
All of the messages that perl might issue are documented in:
perldoc perldiag
You should look there for any messages you get as a first step in debugging.
Or you can arrange to have perl look up messages for you, as
described in the Posting Guidelines that are posted here frequently.
> my @loop_data = ();
my() guarantees an empty array, there is no need to initialize
it to an empty list.
--
Tad McClellan SGML consulting
ta...@augustmail.com Perl programming
Fort Worth, Texas
A. Sinan Unur
news:slrnb5ir9m...@magna.augustmail.com:
> A. Sinan Unur <as...@c-o-r-n-e-l-l.edu> wrote:
>
>> I am trying to familiarize myself with Perl. To that end, I decided
>> to concoct a simple cgi script
>
> You should learn to crawl before running in a race.
>
> Learn Perl from the command line first.
>
> CGI applications are not "simple", there are several advanced
> aspects to them, such as multitasking and security.
All valid points. I just learn better in the course of building something
that is geared towards helping me solve an immediate problem. I have been
generating (using m4 and some shell utilities) my photo albums off-line
and uploading them, and I thought it would be a good idea to try
something that could replace that.
>> I would
>> appreciate your comments about this script before I "improve" it.
>
>> #!C:/Perl/bin/perl.exe -w
>> use strict;
>
> Good. Very good.
>
>
>> if($location =~ s/(\.\.\/)|(\.\.)//g) {
> ^^^^^^^^^
> ^^^^^^^^^
>
> You don't need that part, the 2nd part will match that too.
> No need for the s///g option either.
I noticed that a few hours after my post.
>> # Configuration Variables
>> my $wwwroot = '';
...
>
>
> Consider putting all of those in a single hash instead:
>
> $config{wwwroot} = '';
> $config{script_location} = '/cgi-bin/photobrowser.pl';
> ...
OK.
>> opendir(PHOTO_DIR, $photo_path);
>
> You may not get what you asked for.
>
> You should check to see if you got it:
>
> opendir(PHOTO_DIR, $photo_path) or die "could not open '$photo_path'
> dir $!";
I am a little confused about whether it is OK to use die in CGI scripts.
Admittedly, I have some reading to do but maybe someone can help clear it
up for me, especially if the script might run under mod_perl in Apache.
>> # For the line below, I get the warning
...
> All of the messages that perl might issue are documented in:
>
> perldoc perldiag
>
> You should look there for any messages you get as a first step in
> debugging.
>
> Or you can arrange to have perl look up messages for you, as
> described in the Posting Guidelines that are posted here frequently.
I did not know about that feature. Thanks.
>> my @loop_data = ();
> my() guarantees an empty array, there is no need to initialize
> it to an empty list.
Blind copying from the HTML::Template podwithout paying too much
attention to context. My bad.
Thanks for the help.
Sinan.
Tassilo v. Parseval
> ta...@augustmail.com (Tad McClellan) wrote in
> news:slrnb5ir9m...@magna.augustmail.com:
>> You should check to see if you got it:
>>
>> opendir(PHOTO_DIR, $photo_path) or die "could not open '$photo_path'
>> dir $!";
>
> I am a little confused about whether it is OK to use die in CGI scripts.
> Admittedly, I have some reading to do but maybe someone can help clear it
> up for me, especially if the script might run under mod_perl in Apache.
In this group the "or die" is often just a placeholder for
exception-handling. Your script is doing something with a directory and
obviously it can't do that if it fails to open it. In a CGI context you
probably want a more graceful exit to at least print out same valid
headers and a few lines of HTML:
opendir PHOTO_DIR, $photo_path or bail_out();
sub bail_out {
my $file = shift;
print "Content-type: text/html\n\n";
print <<EOERROR;
<html>
Error-message to visitor
</html>
EOERROR
die "$file: $!"; # goes to the webserver log
}
Whether you include the directory that could not be opened or the error
in $! in the error-message is up to you. Some people wouldn't want to
exhibit these information to the side-visitors. But it should at least
show up in the logs to make debugging easier for you.
Tassilo
--
$_=q#",}])!JAPH!qq(tsuJ[{@"tnirp}3..0}_$;//::niam/s~=)]3[))_$-3(rellac(=_$({
pam{rekcahbus})(rekcah{lrePbus})(lreP{rehtonabus})!JAPH!qq(rehtona{tsuJbus#;
$_=reverse,s+(?<=sub).+q#q!'"qq.\t$&."'!#+sexisexiixesixeseg;y~\n~~dddd;eval
A. Sinan Unur
news:b3cifb$7ts$1...@nets3.rz.RWTH-Aachen.DE:
> Also sprach A. Sinan Unur:
>
>> ta...@augustmail.com (Tad McClellan) wrote in
>> news:slrnb5ir9m...@magna.augustmail.com:
>
>>> You should check to see if you got it:
>>>
>>> opendir(PHOTO_DIR, $photo_path) or die "could not open
>>> '$photo_path' dir $!";
>>
>> I am a little confused about whether it is OK to use die in CGI
>> scripts.
...
> In a CGI context you probably want a more graceful exit to at least
> print out same valid headers and a few lines of HTML:
>
> opendir PHOTO_DIR, $photo_path or bail_out();
Thank you for the clarification.