Perl CGI script to serve a PDF file

Question

In a different post I showed the following Perl CGI script. Someone in the comments said that

"You're using a lot of very unperlish syntax, and I'd like to give you some feedback".

So, here it is, and tell me how to make it more Perlish? I came from a C background so I'm all ears (or eyes)!

#!/usr/bin/perl -T

use strict;
use warnings;
use CGI qw/:all/;

my $basepath = '/some/path/to';
my $f = param("f") || '';

($f eq '') && die;

my $path = "$basepath/$f.pdf";

open(PATH, "<$path") || die;
binmode PATH;

print header(-type => "application/pdf",
         -target => "$f");
print;

while (<PATH>) {
    print($_);
}
close(PATH);

Answer 1

I will go through your code line by line and give feedback. We will skip the general advice on don't use CGI as it's actually suited for what you are trying to do here.

I wrote this answer in two stages, so some of the things here might have been said by other people. I'm attempting to provide full feedback, and I'm not copying from others. If several of us give the same feedback, it must probably make some sense.

#!/usr/bin/perl -T

The -T flag turns on taint mode. I am not sure you intended to do this, or just copied it from somewhere else. Taint mode essentially makes sure that data coming into your program from the outside must be validated before it's being used. But you are not untainting the variable $f. That could bite you in the back.

---

use strict;
use warnings;

This is good! Well done. Your code should always start with strict and warnings.

---

use CGI qw/:all/;

You are importing a lot of stuff into your namespace, but all you use is param. Instead, consider importing only :cgi, which will give you less stuff, or only import param and header. That also makes it easier to remember where that function came from.

use CGI qw(param header);

---

my $f = param("f") || '';

$f is not the greatest variable name. Use descriptive variable names, like $filename.

If the file is called zero 0, or 0000, this will fail. Read more about that at the end of this post.

---

($f eq '') && die;

While some people like to use logical operators for flow control, this is really weird to read. If you want concise syntax without blocks, use a post-fix if or unless.

Also, provide an error message as to why this failed. It's CGI, so this will show up in your server log. You don't want it to say Died at line 10, do you?

I prefer to use the q{} quoting operator for empty strings, as it shows the intent of giving an empty string more clearly.

die "No filename given" if $f eq q{}.

---

open(PATH, "<$path") || die;

Don't use a GLOB as a file handle. It's global to all of your program, including other modules and namespaces. If another module was to use the same PATH name for a handle, this would conflict. Instead, use a lexical file handle. The convention is to name file handles $fh.

This has the added benefit that Perl will take care of closing the handle for you as soon as the variable goes out of scope. In this case, that's when the program ends. So you don't have to use close explicitly.

Use the three-argument form of open, where the second argument is the mode. In your case, that's < for reading. This makes file operations more secure, as your $path could for example start with a pipe |, which would change the meaning. Especially if part of your file name comes from the user (and you didn't even untaint it), this is crucial advice.

Don't use binary or || for flow control as it has tight binding. Use or instead, which binds very loosely, so you don't need to put the parentheses after open. It will also read more like actual English.

Include a meaningful error message with your die, especially the error $! that was returned from the open call.

open my $fh, '<', $path or die "Error opening '$path': $!";
binmode $fh;

---

print header(-type => "application/pdf",
         -target => "$f");

Indent your code properly. This looks weird. You have indented the second line 9 spaces. Looks like one full tab in vim, and one space. Instead, break it around properly, and align everything. On average, code is read ten times more than written. Prepare for that.

Don't quote variables if there is no interpolation needed. If it's only one var, leave out the quotes.

print header(
    -type   => "application/pdf",
    -target => $filename,
);

---

print;

This line will print the current value of $_. I am not sure what that is at this time in the execution of the program. It's probably not intended. Get rid of it.

---

while (<PATH>) {
    print($_);
}

If you intend to use the loop variable, name it. The topic $_ is really useful for short one-liners, map and such, but a while loop is rarely such a case.

while ( my $buffer = <$fh> ) {
    print $buffer;
}

You can also use the aforementioned post-fix notation for that. Again, you don't need to use the topic, because it's implicit.

print while <$fh>;

But you can also just get rid of that completely, and simply use the fact that print is list context, and the diamond operator <> returns the full file as an array in list context.

print <$fh>;

---

I said earlier that your check for $filename with || will fail if the name is a false value. Remember that Perl has no actual true and false, just values that evaluate truthy, and values that don't. There are very few things that are false in Perl. 0, undef, the empty string q{} and the empty list () are such cases.

my $filename = param('f');
die unless $filename;

This would be similar, but again, 0 would be cast out.

my $filename = param('f');
die unless defined $filename;

If there was no param f, the param function returns undef. So we can avoid that by saying unless defined. That lets 0.pdf through. But what about the empty string?

Essentially you need to decide whether a file named $basepath/.pdf is valid. That's up to you. If you don't want that, you have to do additional checks.

You could also check if that file exists in general, with -e.

die "No such file" unless -e $path;

However, open will check that anyway, so it's not needed.

Answer 2

#!/usr/bin/perl -T
use strict;
use warnings;

use CGI qw/ :all /;

my $basepath = '/some/path/to';
my $f = param('f') || '';                  # No interpolation, no double quotes.

die if $f eq '';                           # Don't use && for flow control.

my $path = "$basepath/$f.pdf";

open my $PATH, '<', $path or die;          # Lexical filehandles, 3-argument open.
binmode $PATH;

print header(-type   => 'application/pdf', # Single quotes.
             -target => $f);               # No quotes needed at all.
# print;                                   # This prints $_, are you sure?

print while <$PATH>;
close $PATH;

I'd also provide an argument to die so you can search the logs easier.

Answer 3

Your param('f') is user-provided and may therefore open up security concerns. In particular, it may be used to navigate to $basepath/../../some-secret-file.pdf. The impact is reduced because you can only read files ending in .pdf, but it fundamentally still exists.

As a mitigation, you should limit the allowed file names and match for those. I.e. explicitly validate that you have a good filename, don't just try to exclude a few known-bad patterns. For example, you might want to allow any filenames that only consist of ASCII letters or digits, in that case:

unless ($filename =~ /\A[a-zA-Z0-9]+\z/) {
  die "Illegal file name was provided: $filename";
}

(The assertions \A and \z anchor at the beginning/end of the string, and therefore ensure that the whole string matches this pattern and doesn't just contain this pattern somewhere.)

Instead of die which will be visible to users as a 500 internal server error, explicitly returning some 4xx error might be more appropriate.

Answer 4

Use a raw header, to combine "inline" with a filename:

print $q->header(
  -type   => "application/pdf",
  -target => $file,
  '-Content-Disposition' => "inline; filename=\"$file\"",
);