[FileResponse] allow pre-opening of file #3041
Draft
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Hello! I am using Starlette (under FastAPI) to serve some untrusted directories from disk, and as such I have intentional file opening logic that ensures files are not served from outside the untrusted directory. It's similar to the common path check in
StaticFiles
, but checking the path then opening the file is at risk of TOCTOU attack if a symlink is injected into the path between validation and opening (e.g. eitherpathsend
or the file open withinFileResponse
). It's a tight squeeze, but definitely possible when the filesystem is writeable, especially by untrusted entities (aka users 😁).As such, I currently have my own internal adapted version of
FileResponse
that accepts an openIO[bytes]
which was securely opened without any symlink following.I just wanted to open this draft to see if you would be amenable to a change like this. (happy to add tests if we can arrive at an acceptable approach through discussion!)
Alternative I considered were:
path
gains a thirdIO[bytes]
type option -- this muddles the implementation IMOopener: Callable[[str | os.PathLike[str]], Awaitable[anyio.AsyncFile]]
-- more confusing, muddles exception handling IMOI am fine to keep rolling with my own internal version, but wondered if this might be generally useful to others. It could also be used in combination with changes to
StaticFiles
to close that TOCTOU symlink gap (though sinceStaticFiles
is read-only, may not be useful).Checklist