Please ignore my not using CGI.PMs HTML shortcuts, I'm still reading up on those.

It's fine to not use CGI.pm's HTML shortcuts, but seriously, consider using it to at least parse your input for you. It's pretty easy to generate HTML output on your own, but it's difficult to do a good job parsing CGI input.

Also, just for kicks and giggles, turn on -T (taint checking) by putting the -T on the shebang line just like you've done with -w. You'll find that you are actually doing something quite dangerous by accepting a filename and sending user input through the open command. You may think it doesn't matter much if the script is just for personal use. But if you put that script on your ISP or webhosting company's server, you are exposing the ISP/webhost to a serious security breech, and yourself, to a possible legal liability if they get attacked through your script.

Please read: use CGI or die;, and escaping filenames taken in via user input.

One alternative to taking user input for filenames is this:

Generate your own list of filenames by reading the target directory. Give the user a list of filenames, and let him select from that list. And don't accept the actual file name as an HTML parameter. Use a hash lookup table: A => file1.dat, B => file2.dat, etc. That way the input you get from your HTML form never directly finds its way into your open statement.

Yeah, that is exactly why taint protection isn't on.. I know it's dangerous to take the input that way. While the regex keeps all traditional '../' and such attacks from using the script, I decided to keep this one under a passworded folder on the server so it's harder to access.

Turning off -T taint checking because your CGI script is too unsafe to run under taint mode reminds me of part two of the definition of Ostrich from the American Heritage Dictionary:

"One who tries to avoid disagreeable situations by refusing to face them."

If you insist on letting users give you filenames, at very least, use the three-argument version of open.

I seem to remember reading somewhere that .htaccess is not infallable as a security measure. I can't seem to find the link now though.

I still think you should give the user a filename list, and read which item they selected from the list, by some index value. That way you only pass index values as input from the CGI script, and then you look up what file that index pertains to, and open the file yourself. Such a setup eliminates any possibility of the user specifying a dirty filename.



Dave

"If I had my life to live over again, I'd be a plumber." -- Albert Einstein

It would probably be a good idea to make sure that your file actually exists before you display info from it. It's not drop-dead if you don't, but it's a usually a good idea to do a check before I/O. You can just branch w/ a friendly "file not found!" message if the file is not there.

And keep an eye out for unbalanced header tags. ;)

Yeah, so far when no file is there or it doesn't match the window just comes up blank, I'm working on a file not found thing... ::laugh:: But I keep screwing it up.

::Pekkhum looks closely at the headers...::

I decided to spend a little time in sprucing this up a small bit, just for fun. I took out the nasty javascript, changed the copyright (not that this snippet is copyrighten or anything), threw in a templating system, and some very simple error checking. Enjoy.

#!c:/perl/bin/perl -w
$|++;

use strict;
use CGI::Simple;
use HTML::Template;

my $q = CGI::Simple->new;
my $t = HTML::Template->new( filehandle => *DATA );

my @ext = qw( pl css htm html shtm shtml );
my $file = $q->param('file');
my $fh;

if (
  ($file =~ /[^a-zA-Z0-9_\-\.]/) or
  ($file =~ /\.\./) or ($file eq '.') or
  !(grep { $file =~ /\.$_\z/i } @ext)
  !(-e $file) or
  !(open $fh, '<', $file) or
) {
  $t->param(
    title => 'My File Viewer',
    file  => 'Script Error',
    code  => 'An error occured while preparing the file for display.'
  );
}

else {
  $t->param(
    title => 'My File Viewer',
    file  => $file,
    code  => do { local $/; <$fh> }
  );
}

print $q->header, $t->output;
exit;

__DATA__

<html>

  <head>
    <title>Pekkhum's Script Veiwer</title>
  </head>

  <body>

    <p style="text-align: center; font-weight: bold">
      <!-- TMPL_VAR NAME="title" ESCAPE="HTML" -->
    </p>

    <p><hr /></p>

    <p style="text-decoration: underline; font-weight: bold">
      <!-- TMPL_VAR NAME="file" ESCAPE="HTML" -->
    </p>

    <form>
      <textarea name="code" rows="30" cols="90" readonly>
<!-- TMPL_VAR NAME="code" ESCAPE="HTML" -->
      </textarea>
    </form>

    <p>
      <small>&copy; Copyright 2003 Nathan Bessette</small>
    </p>

  </body>

</html>
[download]

Heh, thanks for all this new information Coruscate... Now this aught to take me a while to decode... Sadly such programming goes beyond the scope of The Visual Quickstart Guide I'm learning from. ^_^

Not sure what's up, but there when I try running your version of the script I get:
Can't locate HTML/Template.pm in @INC (@INC contains: e:/ActivePerl/Perl/lib e:/ActivePerl/Perl/site/lib .) at c:\webweaver\docs\sv.pl line 6. BEGIN failed--compilation aborted at c:\webweaver\docs\sv.pl line 6.

I found the HTML directory, and sure enough there is no module named Template. On my way to download it now.

pekkhum, if you are using the Elizabeth Castro book, like I did, you will find that you will have to unlearn some stuff to conform with methods in the monastery. I recommend you switch to the O'Reilly books, Learning Perl (Llama book), Programming Perl (Camel book), and CGI Programming in Perl. Invaluable.

Speaking of invaluable: HTML::Template! BTW, the module is not called Template, but HTML::Template.

Appreciate your bravery in posting your code, I've learned a lot by following the thread. You don't get better teachers than the likes of davido and Coruscate.

I was asked by pekkhum if I could explain a bit, so here's the best job I am capable of doing. Hopefully it's enough to help out a bit.

Read more... (6 kB)

  my $request=$ENV{'QUERY_STRING'};
  $request=~/([-\w_]+\.($ext))/g;
  my $file=$1;
[download]

  my ($file) = $ENV{'QUERY_STRING'} =~ /([-\w_]+\.($ext))/g;
[download]

  foreach(@code){
    $_=~s/</&lt;/g;
    $_=~s/>/&gt;/g;
  }
[download]

  foreach(@code){
    s/</&lt;/g;
    s/>/&gt;/g;
  }
[download]

Thanks for those suggestions. I hadn't thought to store the search direct to the variable. I also hadn't realized the statement of the default variable was somewhat redundant.