Conversation
Add test to prevent path traversal Update requirements.txt Update tests
9e4d72a to
4c6c59e
Compare
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why not change line 165 realpath to abspath? If the directory is a symbolic link, line 166 will fail.
There was a problem hiding this comment.
Can we add a test with the scenario @scientificRat mentioned?
There was a problem hiding this comment.
sure, I'll take a look.
There was a problem hiding this comment.
Not sure if I got the point correctly, but I added the tests for this, but I think this change is not needed.
Add test to prevent path traversal Update requirements.txt Update tests
4c6c59e to
a799da7
Compare
| 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 |
There was a problem hiding this comment.
Can we add a test with the scenario @scientificRat mentioned?
… into statics-follow-symlinks
a54b472 to
9c7eab5
Compare
9c7eab5 to
140edb6
Compare
Kludex
left a comment
There was a problem hiding this comment.
I'm still not 100% sure about this. 😞
I think I need to write a small summary about this, and what are the alternatives if we don't do it. 👀
|
My previous concern on this PR was:
Should we be concerned about it? Is this feature wanted that bad? 🤔 |
|
Can we make this feature opt-in i.e. add Let me know what you think @aminalaee |
Yeah that works, I will update the PR. |
|
Thanks |
|
I guess only docs are missing, and we are ready here 👀 |
|
@Kludex I just updated the docs. Let me know what you think. |
f689dc4 to
e3e85db
Compare
|
Thanks @aminalaee :) Check your Twitter please. |
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
After the previous attempt #1377 , this PR will allow StaticFiles to follow symlinks outside the directory.
Fixes #1083
Thanks to @Kludex and @m1ckey