Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More robust path-traversal check in StaticFiles app #985

Merged
merged 5 commits into from Jul 16, 2020

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Jun 25, 2020

Closes #981

@tomchristie tomchristie merged commit e611476 into master Jul 16, 2020
@tomchristie tomchristie deleted the staticfiles-traversal branch July 16, 2020 14:18
@char
Copy link

char commented Jul 17, 2020

Looks like this causes an issue where you can get 404s for every static file except if you create your StaticFiles object with StaticFiles(directory=os.path.realpath(...)).

Also (and potentially should be its own issue), could we consider allowing following symbolic links with a flag that we pass in?

@norefle
Copy link

norefle commented Jul 19, 2020

The latest changes break previous behaviour from 0.13.4.
Assume we have a following layout locally:

/ project-root
    - app.py
    / static
        - example.txt

Based on the documentation we can register static files as following: Mount('/static', app=StaticFiles(directory='static'), name="static")
Then requests like GET /static/example.txt should be served. And they used to be served, until these changes.

If we use the layout from above and the recommended way from documentation then the code from this PR will produce the following:

>>> import os
>>> full_path = os.path.join("static", "/static/example.txt")
>>> full_path
'/static/example.txt'
>>> os.path.commonprefix([os.path.realpath(full_path), "static"])
''
>>> os.path.commonprefix([os.path.realpath(full_path), "static"]) != "static"
True

@tomchristie what is the recommended way to register StaticFiles now?

Especially in case of different layouts:

  • Local dev: /home/whatev/whatev/projects/static/ -> static
  • In a docker container: /app/static -> static

@i-Ching
Copy link
Contributor

i-Ching commented Jul 19, 2020

At a local layout, I convert realpath+static to string in order to fix the error 404 for the version 0.13.5 as below:

from pathlib import Path
...
Mount("/static", app=StaticFiles( directory=str(Path(Path.cwd()).joinpath('static')) ), name="static")

@pk400 pk400 mentioned this pull request Jul 19, 2020
@indeets-vasily
Copy link

indeets-vasily commented Jul 20, 2020

I have tested a little bit on my Windows 10 Notebook and it seems that StaticFiles return 404 if directory parameter is pathlib class. When you pass the string of correct format (Windows for Windows system) it works:

  • StaticFiles(directory=Path("C:\\path\\to\\directory")) -- 404
  • StaticFiles(directory=Path("C:/path/to/directory")) -- 404
  • StaticFiles(directory=str(Path("C:\\path\\to\\directory"))) -- OK
  • StaticFiles(directory=str(Path("C:/path/to/directory"))) -- OK, Path automatically converts Linux path to Windows format
  • StaticFiles(directory=StaticFiles(directory="C:\\path\\to\\directory") -- OK
  • StaticFiles(directory=StaticFiles(directory="C:/path/to/directory") -- 404, because this string is in Linux format

os.path.* functions work because they return string values, even if their argument is Path:

  • StaticFiles(directory=os.path.realpath("C:\\path\\to\\directory")) -- OK
  • StaticFiles(directory=os.path.realpath("C:/path/to/directory")) -- OK
  • StaticFiles(directory=os.path.realpath(Path("C:\\path\\to\\directory"))) -- OK

Most interestingly that there are different results depending on whether directory passed as Path exists or not:
Both StaticFiles(directory=Path("C:\\non\\existent\\directory")) and StaticFiles(directory="C:\\non\\existent\\directory") return same:
RuntimeError: Directory 'C:/non/existent/directory' does not exist

@tomchristie
Copy link
Member Author

Resolved in 0.13.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arbitrary path traversal possible with StaticFiles
5 participants