4
\$\begingroup\$

I've just written this Twisted resource base class for serving files. It's part of a larger application, this is just for serving its images, JavaScript files, etc.

Is it okay? I wonder if checking for .. in the path is enough -- all the files that exist in self.directory are intended to be served, to anyone, so this should be all that's needed, I think. Should these it be a Unicode string? Should they all be Unicode strings?

Will return open(file).read() be enough? I would think the resource would be closed when the function goes out of scope, but I'm not completely sure.

"""
Base class for resources serving files to the browser.

This doesn't use the `Page` class because it doesn't need to set up templates
or anything, just serve the file.
"""
from twisted.web.resource import Resource
import os.path

class File(Resource):
    """
    Serves a certain type of file from the server.

    It could be images, javascript, etc. It serves files from a single
    directory which have a single extension.

    members:
    content_type -- the HTTP content type of the files to be served
    directory -- the local directory the files are stored in
    extension -- the extension which the files are saved with.
    """
    content_type = None
    directory = None
    extension = None

    def render_GET(self, request):
        """
        Serve the requested file.

        Makes sure the file exists and the request doesn't contain '..'
        to escape the file directory.
        """
        file = request.path.strip("/")
        file = file.split("/", 1)[1]
        file = "{directory}/{filepath}.{extension}".format(
            directory=self.directory, filepath=file, extension=self.extension)

        request.responseHeaders.setRawHeaders("content-type", [self.content_type])

        if ".." in file:
            request.setResponseCode(400)
            return ""

        if not os.path.isfile(file):
            request.setResponseCode(404)
            return ""

        return open(file).read()
\$\endgroup\$
2
\$\begingroup\$

Firstly, don't try to think of everything that could go wrong. Instead, check for what you know to be valid.

import re
directory, filename = re.match('/([A-Za-z0-9]+)/([A-Za-z0-9]+)/?', alpha).groups()

By using this method to extract the directory and filename, we can be sure that directory and filename contain nothing except letters and numbers which are perfectly safe.

Its also best to use os.path.join to produce folder paths rather then string formatting to put them together.

    return open(file).read()

This works, but not if you try and run on IronPython/Jython/PyPy. CPython depends on reference counting which will cause the file to be closed as soon as it returns. A better version is probably:

with open(file) as source:
     return source.read()

This is guaranteed to close the file even without reference counting.

\$\endgroup\$
3
  • \$\begingroup\$ The regex for directory and filename isn't very good. Many, many valid directory and filenames would be treated as invalid. \$\endgroup\$ – jwg Sep 18 '17 at 17:51
  • \$\begingroup\$ @jwg, yes, but that's my point. Its better to reject good filenames than to fail to reject bad ones. If all you need are alphanumeric names, then you don't need to go the extra mile to handle other characters. \$\endgroup\$ – Winston Ewert Sep 18 '17 at 23:31
  • \$\begingroup\$ Rejecting anything with a . or _ is clearly broken. \$\endgroup\$ – jwg Sep 19 '17 at 7:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.