StaticFiles middleware doesn't follow symlinks

[abe-winter]

Checklist

• The bug is reproducible against the latest release and/or master.
• There are no similar issues or pull requests to fix it yet.

Describe the bug

The StaticFiles middleware is checking the os.realpath of a file and returning a 404 for symlinks that lead outside the static directory.

To reproduce

1. create a minimal app with a staticfiles middleware
2. put a symlink in your static directory. the link's target must be above the static directory.
3. you'll get a 404

Expected behavior

Support symlinks in static directory.

The use case for symlinks in static is to target frontend assets that are being generated in file-watch mode.

Actual behavior

Debugging material

It's happening here:

starlette/starlette/staticfiles.py

Lines 147 to 149 in b95acea

	full_path = os.path.realpath(os.path.join(directory, path))
	directory = os.path.realpath(directory)
	if os.path.commonprefix([full_path, directory]) != directory:

Environment

• OS: linux
• Python version: 3.7.5
• Starlette version: 0.13.8

Additional context

I'm happy to post a PR for this if useful, ideally adding a bool param to the StaticFiles middleware that allows symlinks.

[leondmello]

I can confirm that we are facing this issue as well

[onumossn]

post the PR!!!

[abe-winter]

here's a subclass that solves the problem

you install it the same way you install the stock StaticFiles. I used it with starlette==0.13.8, not sure if it works on newer versions.

class StaticFilesSym(StaticFiles):
  "subclass StaticFiles middleware to allow symlinks"
  async def lookup_path(self, path):
    for directory in self.all_directories:
      full_path = os.path.realpath(os.path.join(directory, path))
      try:
        stat_result = await aio_stat(full_path)
        return (full_path, stat_result)
      except FileNotFoundError:
        pass
    return ("", None)

---

update jan 2022: this is broken with starlette 0.17. You can patch by:

• async def -> def
• aio_stat -> os.stat

---

update jun 2022: this patch seems to not be vulnerable to the same security issue as #1681 -- a request for /static/../../../etc/passwd seems to be getting rewritten to / (tested with starlette 0.17.1, fastapi 0.73.0). I'm guessing the .. are being processed before the request hits the middleware.

This is just luck, I wasn't thinking defensively either. I would recommend disabling my patch in production. (in my case I only need it in local dev mode for hitting incremental webpack builds).

[leondmello]

not sure if it works on newer versions.

That's usually the problem with such fixes. They might break on newer versions of the library.
It's better to fix the issue in the library itself.

[kikohs]

Any news on this issue? @abe-winter code works with the modifications.
Thank you very much

[Kludex]

A fix is already merged ,and it will be available on the next release (we are at 0.20.1 by the time I wrote this). 🙌

[Kludex]

We reverted the PR that fixed this issue on Starlette 0.20.3.

[bodograumann]

I would be interested to know why the change had to be reverted.

[Kludex]

Yeah, I'm going to clarify things in a few hours. 👍

[Kludex]

Report about the incident: #1681 (comment)

[abe-winter]

sorry for not thinking through security issues when posting the feature request

great catch, thanks to kind stranger on gitter, and thanks for being quick + transparent about the fix

[Kludex]

It's a valid feature request. We'll come back to this issue later on. :)

[Kludex]

I've been reading around, and this feature can potentially be harmful if users are allowed to update files to the StaticFiles directory.

Is there any known web framework that implements this? How they make sure the above paragraph will not be a problem? If there's none, let's not open this possibility.

[aminalaee]

I know Flask/Werkzeug does this and I guess Django does it too. I don't think it's a real must-have anyway. But:

I've been reading around, and this feature can potentially be harmful if users are allowed to update files to the StaticFiles directory.

Well this is obviously wrong, we can't prevent this.

Let's say even if this feature is not available in Starlette, there's an Nginx reverse proxy that serves the static files which by default will follow symlinks.
Even in that case if we allow uploads to the statics folder, Nginx will serve it and that's an issue.

[Kludex]

Well this is obviously wrong, we can't prevent this.

But we shouldn't make it easier for this to happen.

Should we make this behavior opt-in? Something like follow_symlink, and default it to False to not change the current behavior. 🤔

It would be cool to have here a snippet on how this is done on Flask and Django, and see if there were some previous discussions about it.

[abe-winter]

flask

In flask, static files are served with:

send_static_file -> send_from_directory -> werkzeug.safe_join

safe_join checks that the provided path isn't above the current directory, but it will happily follow a symlink that points above the process's working directory (I tested this).

django

In django, the staticfiles finders call FileSystemStorage.path which uses a similar safe_join

from testing, it seems like this will also follow a symlink above the working directory

edit note django by default only serves static files when DEBUG=1, see here for example

symlink upload

Re someone's point about what if users can write symlinks to the StaticFiles dir -- would be interested to know if this is possible via normal HTTP file uploads.

In ext4, for example, I think symlink status is stored in the inode table along with the uga/rwx mode bits. From skimming the nginx upload module, it doesn't look like those get transferred. It sets permission here from store_access, which is a config, not from the upload. (I could be wrong about this).

Also the RFC for HTTP uploads doesn't talk about symlinks or mode bits https://www.rfc-editor.org/rfc/rfc1867.

I'm assuming starlette also doesn't receive or set mode bits for file uploads?

opt-in

I like the idea of an opt-in follow_symlink setting -- in my case I would enable it in DEBUG and disable in prod.

[rob-pomelo]

Just an additional piece of info here, this causes issues when using bazel, which relies heavily on symlinks.

In particular a dep of ours uses starlette underneath and because the underlying static file is a symlink (managed by bazel), starlette throws on lookup, making the package not usable.

[mvgijssel]

Just an additional piece of info here, this causes issues when using bazel, which relies heavily on symlinks.

In particular a dep of ours uses starlette underneath and because the underlying static file is a symlink (managed by bazel), starlette throws on lookup, making the package not usable.

Wow this is a crazy coincidence! Using Bazel trying to setup Dagster which relies on Starlette and was only getting empty pages with assets not being served. I can now stop pulling my hair out. +1 to having this fixed!

[rob-pomelo]

Just an additional piece of info here, this causes issues when using bazel, which relies heavily on symlinks.

In particular a dep of ours uses starlette underneath and because the underlying static file is a symlink (managed by bazel), starlette throws on lookup, making the package not usable.

Wow this is a crazy coincidence! Using Bazel trying to setup Dagster which relies on Starlette and was only getting empty pages with assets not being served. I can now stop pulling my hair out. +1 to having this fixed!

Same re dagster :)

[mvgijssel]

Just an additional piece of info here, this causes issues when using bazel, which relies heavily on symlinks.

In particular a dep of ours uses starlette underneath and because the underlying static file is a symlink (managed by bazel), starlette throws on lookup, making the package not usable.

Wow this is a crazy coincidence! Using Bazel trying to setup Dagster which relies on Starlette and was only getting empty pages with assets not being served. I can now stop pulling my hair out. +1 to having this fixed!

Same re dagster :)

Currently using this workaround until a new version of Starlette is released. It's really hacky but it gets the job done 🤣

dagit_wrapper.py

from dagit.cli import main
import os

from dagit.webserver import DagitWebserver
from starlette.routing import Mount
from starlette.staticfiles import StaticFiles

# Necessary because of https://github.com/encode/starlette/issues/1083
def custom_build_static_routes(self):
    manifest_path = self.relative_path("webapp/build/manifest.json")
    real_manifest_path = os.path.realpath(manifest_path)
    real_build_dir = os.path.dirname(real_manifest_path)

    return [
        Mount(
            "/static",
            StaticFiles(
                directory=os.path.join(real_build_dir, "static"),
                check_dir=False,
            ),
            name="static",
        ),
        Mount(
            "/vendor",
            StaticFiles(
                directory=os.path.join(real_build_dir, "vendor"),
                check_dir=False,
            ),
            name="vendor",
        ),
        *self.root_static_file_routes(),
    ]


DagitWebserver.build_static_routes = custom_build_static_routes

if __name__ == "__main__":
    working_directory = os.environ["BUILD_WORKING_DIRECTORY"]
    os.chdir(working_directory)
    main()

[Kludex]

This will be available on Starlette 0.24.0.

You'll be able to use StaticFiles(follow_symlink=True) for this purpose.