It isn't only the security. Your line $req = 'index' if -e $req; will make all requests invoke "index". Your code means " assign to 'index' if a file named $req exists". I am not sure what you wanted to achieve that way, but here is how I would do it.

use strict;
use warnings;

my $req = $ENV{QUERY_STRING};

# limits the applicability. Only lowercase file names
$req = lc $req;

# remove all unwanted characters from the beginning of the 
string. # In this example, everything except alphanumerics
# and underscore is removed.
$req =~ s/^[^a-z_0-9]+//;

# remove an extension, if any
$req =~ s/\.html$//;

# default value is the index
my $page = "pages/index.html";

# if the page exists, then we use it
$page = "pages/$req.html" if  -e "pages/$req.html" ;
[download]

Also, consider using CGI param instead of reading the environment.

HTH

Your line $req = 'index' if -e $req; will make all requests invoke "index"

no it won't.

The QUERY_STRING should not match anything, since the filename would be composed as pages/QUERY_STRING.html That's why if it matches any file, it should roll back to a default.

Yeah. As written, the user could pass, e.g., ../topsecretpages/index.html and start looking at the topsecretpages directory that exists at the same level as pages. (Of course, the user would have to guess or learn the name of the directory, and it is to be hoped you don't really have top secret pages lying around under your web server's document root without any protection.)

Updated: Like the followups say, the regexp dealt with that. Teach me to answer SoPWs in the middle of the night...

that's why there is $req =~ s/^.*\///; which should take care of that.

I believe your code should look for literal periods:

  $req =~ s/^\.\.\///;
[download]

But that's still poor, because what about:

   blah/../../topsecretpages/page.html
[download]

or

   ../../topsecretpages/page.html
[download]

Update: Chady is right. I retract.


pbeckingham - typist, perishable vertebrate.