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

Allow StaticFiles follow symlinks #1683

Merged
merged 12 commits into from
Feb 4, 2023
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ trio==0.19.0

# Documentation
mkdocs==1.3.0
mkdocs-material==8.2.8
mkdocs-material==8.3.3
aminalaee marked this conversation as resolved.
Show resolved Hide resolved
mkautodoc==0.1.0

# Packaging
Expand Down
2 changes: 1 addition & 1 deletion starlette/staticfiles.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ def lookup_path(
self, path: str
) -> typing.Tuple[str, typing.Optional[os.stat_result]]:
for directory in self.all_directories:
full_path = os.path.realpath(os.path.join(directory, path))
full_path = os.path.abspath(os.path.join(directory, path))
directory = os.path.realpath(directory)
if os.path.commonprefix([full_path, directory]) != directory:
# Don't allow misbehaving clients to break out of the static files
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand os.path.realpath will follow symbolic links and here we will stop it from going outside of statics directory.
Instead we can build the path with abspath and not follow symlinks (yet) and prevent breaking outside of directory and if file is a symlink it will do os.stat and by default it will follow symlinks.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not change line 165 realpath to abspath? If the directory is a symbolic link, line 166 will fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test with the scenario @scientificRat mentioned?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I'll take a look.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I got the point correctly, but I added the tests for this, but I think this change is not needed.

Expand Down
49 changes: 49 additions & 0 deletions tests/test_staticfiles.py
Original file line number Diff line number Diff line change
Expand Up @@ -441,3 +441,52 @@ def mock_timeout(*args, **kwargs):
response = client.get("/example.txt")
assert response.status_code == 500
assert response.text == "Internal Server Error"


def test_staticfiles_follows_symlinks(tmpdir, test_client_factory):
statics_path = os.path.join(tmpdir, "statics")
os.mkdir(statics_path)

symlink_path = os.path.join(tmpdir, "symlink")
os.mkdir(symlink_path)

symlink_file_path = os.path.join(symlink_path, "index.html")
with open(symlink_file_path, "w") as file:
file.write("<h1>Hello</h1>")

statics_file_path = os.path.join(statics_path, "index.html")
os.symlink(symlink_file_path, statics_file_path)

app = StaticFiles(directory=statics_path)
client = test_client_factory(app)

response = client.get("/index.html")
assert response.url == "http://testserver/index.html"
assert response.status_code == 200
assert response.text == "<h1>Hello</h1>"


def test_staticfiles_disallows_path_traversal_with_symlinks(tmpdir):
statics_path = os.path.join(tmpdir, "statics")
os.mkdir(statics_path)

symlink_path = os.path.join(tmpdir, "symlink")
os.mkdir(symlink_path)

symlink_file_path = os.path.join(symlink_path, "index.html")
with open(symlink_file_path, "w") as file:
file.write("<h1>Hello</h1>")

temp_path = os.path.join(tmpdir, "index.html")
os.symlink(symlink_file_path, temp_path)
aminalaee marked this conversation as resolved.
Show resolved Hide resolved

app = StaticFiles(directory=statics_path)
# We can't test this with 'requests', so we test the app directly here.
aminalaee marked this conversation as resolved.
Show resolved Hide resolved
Kludex marked this conversation as resolved.
Show resolved Hide resolved
path = app.get_path({"path": "/../index.html"})
scope = {"method": "GET"}

with pytest.raises(HTTPException) as exc_info:
anyio.run(app.get_response, path, scope)

assert exc_info.value.status_code == 404
assert exc_info.value.detail == "Not Found"