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 to follow symlinks outside directory #1377

Merged
merged 17 commits into from
May 28, 2022

Conversation

aminalaee
Copy link
Member

Fixes #1083.

Right now we don't allow StaticFiles to break outside of statics directory:

if os.path.commonprefix([full_path, directory]) != directory:
# Don't allow misbehaving clients to break out of the static files
# directory.
continue

Since we already follow symlinks for directory and path, I think it makes sense to allow StaticFiles to break out of statics directory if it's following a symlink.

@aminalaee aminalaee added the staticfiles Static file serving label Dec 17, 2021
@aminalaee aminalaee requested a review from a team January 16, 2022 14:25
@adriangb adriangb added the bug Something isn't working label Feb 2, 2022
@aminalaee aminalaee force-pushed the fix-staticfiles-follow-symlinks branch from 3dee9ab to 6b6fe29 Compare April 19, 2022 07:53
@euri10
Copy link
Member

euri10 commented Apr 19, 2022

isn't that behavior a security concern and would allow path traversal ?

@aminalaee
Copy link
Member Author

@euri10 For example in Nginx docs this is the default behaviour it makes sense to break outside of the directory, only if following symlinks.
if I'm not mistaken that's also the case in Django, but I need to double check.

Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

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

added a few comments mostly about os.path vs pathlib stuff, so not blocking on my side but I would see it as probably more readable

thoughts ?

tests/test_staticfiles.py Outdated Show resolved Hide resolved
starlette/staticfiles.py Outdated Show resolved Hide resolved
@Kludex Kludex added the hold Don't merge it label Apr 20, 2022
starlette/staticfiles.py Outdated Show resolved Hide resolved
@aminalaee aminalaee requested review from euri10 and a team April 20, 2022 08:29
Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

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

looks good to me

starlette/staticfiles.py Show resolved Hide resolved
starlette/staticfiles.py Outdated Show resolved Hide resolved
@Kludex
Copy link
Member

Kludex commented Apr 20, 2022

Please do not merge until 0.19.1 is released. This should be up on 0.20.0.

@aminalaee aminalaee changed the title Fix staticfiles follow symlinks outside directory Allow staticfiles to follow symlinks outside directory Apr 20, 2022
@Kludex Kludex removed the hold Don't merge it label Apr 21, 2022
@Kludex Kludex added this to the Version 0.20.0 milestone Apr 21, 2022
@aminalaee aminalaee force-pushed the fix-staticfiles-follow-symlinks branch from e201817 to 0291dd4 Compare April 22, 2022 05:49
@Kludex Kludex mentioned this pull request Apr 22, 2022
2 tasks
@aminalaee aminalaee force-pushed the fix-staticfiles-follow-symlinks branch from 0291dd4 to 2c86a96 Compare April 24, 2022 09:55
@aminalaee
Copy link
Member Author

@Kludex Can we merge this now?

@Kludex
Copy link
Member

Kludex commented Apr 24, 2022

correct me if I'm wrong, but this PR changes the API from str to Path, and allows staticfiles to follow symlinks outside directory. Right? - just for the release notes

starlette/staticfiles.py Outdated Show resolved Hide resolved
@aminalaee
Copy link
Member Author

correct me if I'm wrong, but this PR changes the API from str to Path, and allows staticfiles to follow symlinks outside directory. Right? - just for the release notes

@Kludex

  • About "following symlinks" the answer is yes.
  • About "changing API from str to Path" it's a bit tricky. we are changing some methods like get_response and lookup_path but I'm not sure if they are the API here. The main entry to Staticfiles is not changed, but in some cases people might depend on the changed methods, which is not common, and it might break for them. So a note might be useful.

@Kludex Kludex modified the milestones: Version 0.20.0, Version 0.21.0 Apr 30, 2022
@aminalaee aminalaee force-pushed the fix-staticfiles-follow-symlinks branch from 2c86a96 to 422983c Compare May 3, 2022 15:53
@Kludex Kludex mentioned this pull request May 22, 2022
5 tasks
starlette/staticfiles.py Outdated Show resolved Hide resolved
tests/test_staticfiles.py Show resolved Hide resolved
starlette/staticfiles.py Outdated Show resolved Hide resolved
@Kludex Kludex modified the milestones: Version 0.20.1, Version 0.21.0 May 28, 2022
Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

I've added a comment about an alternative solution, but use whatever you prefer.

starlette/staticfiles.py Show resolved Hide resolved
@aminalaee aminalaee merged commit d3dccdc into master May 28, 2022
@aminalaee aminalaee deleted the fix-staticfiles-follow-symlinks branch May 28, 2022 14:17
@aminalaee aminalaee mentioned this pull request May 31, 2022
Kludex added a commit that referenced this pull request Jun 10, 2022
Kludex added a commit that referenced this pull request Jun 10, 2022
@Kludex Kludex restored the fix-staticfiles-follow-symlinks branch December 12, 2022 07:16
@Kludex Kludex deleted the fix-staticfiles-follow-symlinks branch December 12, 2022 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working staticfiles Static file serving
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StaticFiles middleware doesn't follow symlinks
5 participants