You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is a bit interesting for me, I wonder if we should check the path in send_file like we do in StaticFileHandler and see if it is a file or not. Right now it treats every path as a file and this is a bit error prone.
WDYT @straight-shoota?
I don't think this is a big problem. send_file is a helper method to write contents of a file from disk to an HTTP response. It does not handle user input directly. That's in application code (path = env.params.query["path"].to_s in the OP).
It should be the responsibility of the caller of send_file to ensure that the path is safe to read.
I don't think Kemal could even take this responsibility because what's considered legitimate access depends very much on the specific application. It's impossible to make a default assessment.
For example, we could restrict the path to within the current directory (which might actually be the intention of file_path = File.expand_path(path, Dir.current) in send_file, but doesn't work). But that would be a bit arbitrary - I might want to explicitly send a file at a specific path outside the current directory. And - most importantly - it could still involuntarily expose private data within the current directory.
So I don't think it would help much, might even insinuate a false sense of safety.
But I suppose Kemal could help users to be aware of this potential issue and avoid vulnerabilities.
For example, mentioning this potential problem explicitly in the documentation of send_file.
Reacted by Sijawusz Pur Rahnama, Anton Maminov and M. Caner Cakici
When we consider Express’s sendFile, it requires either an absolute path or a root dir, blocking path traversal by design. Helpers like send_file should ideally add similar checks to ease devs’ burden and improve security—validation can’t be left solely to app code. https://github.com/expressjs/express/blob/master/lib/response.js#L397
Thanks a lot for the fruitful conversation everyone. As there's no action to take directly in Kemal itself I'm closing this issue.
I've created an issue for the Kemal Website to add a security notice kemalcr/kemalcr.com#81
Activity
sdogruyol commentedon Jun 3, 2025
This is a bit interesting for me, I wonder if we should check the
pathinsend_filelike we do inStaticFileHandlerand see if it is a file or not. Right now it treats everypathas a file and this is a bit error prone.WDYT @straight-shoota?
straight-shoota commentedon Jun 3, 2025
I don't think this is a big problem.
send_fileis a helper method to write contents of a file from disk to an HTTP response. It does not handle user input directly. That's in application code (path = env.params.query["path"].to_sin the OP).It should be the responsibility of the caller of
send_fileto ensure that the path is safe to read.I don't think Kemal could even take this responsibility because what's considered legitimate access depends very much on the specific application. It's impossible to make a default assessment.
For example, we could restrict the path to within the current directory (which might actually be the intention of
file_path = File.expand_path(path, Dir.current)insend_file, but doesn't work). But that would be a bit arbitrary - I might want to explicitly send a file at a specific path outside the current directory. And - most importantly - it could still involuntarily expose private data within the current directory.So I don't think it would help much, might even insinuate a false sense of safety.
But I suppose Kemal could help users to be aware of this potential issue and avoid vulnerabilities.
For example, mentioning this potential problem explicitly in the documentation of
send_file.huseyingulsin commentedon Jun 3, 2025
When we consider Express’s sendFile, it requires either an absolute path or a root dir, blocking path traversal by design. Helpers like send_file should ideally add similar checks to ease devs’ burden and improve security—validation can’t be left solely to app code.
https://github.com/expressjs/express/blob/master/lib/response.js#L397
sdogruyol commentedon Jun 3, 2025
Thanks a lot for the fruitful conversation everyone. As there's no action to take directly in Kemal itself I'm closing this issue.
I've created an issue for the Kemal Website to add a security notice kemalcr/kemalcr.com#81