downloading cgi advice

[Kragen Sitaker]

Jeff Shipman <shippy at nmt.edu> writes:

My goodness!  Are you related to John Shipman?  Do you know my cousin
Ben Sittler?  He's in town this week, on vacation.

>     # Determine size of file
>     try:
>        s = os.stat(BASEDIR+'/'+loc)

You're computing the filename in a couple of places.  Why don't you
open it first and then fstat the opened filehandle?  It'll eliminate a
race condition, which will make the code easier to test (and possibly
more secure, if you use the stat data to e.g. check ownership).  Also,
you can probably put them inside a single try-except.

You should probably use os.path.join if there's a possibility you
might ever want to run this on a non-UNIX system (as your 'rb' in open
suggests you think you might.)

And, of course, be careful about what 'loc' contains.  Very careful.
Things to keep in mind on Unix are symlinks, .., double slashes, etc.

>        size = int(s[6])

I'd import stat and use stat.ST_SIZE instead of 6.

>     except OSError:
>        errmsg('Couldn\'t stat() file!')

I'd except OSError, err and include err in the error message; and I'd
try to distinguish 404 from other errors for user convenience.

>     print 'Content-disposition: attachment; filename=%s\n'\

Cute!  Do browsers actually support the filename=%s parameter?  In the
past, I'd used a redirect to the same script with different parameters
(including some PATH_INFO, e.g. /foo/download.cgi/item3.tar.gz) to
hint to the browser about what the file should be.

> This seems to work fine, but I'm just wanting
> to make sure that I'm not doing anything that
> could be wrong here.

Not really, no.