Fix for files with % in filename
#452
Conversation
| @@ -221,7 +221,8 @@ async function fastifyStatic (fastify, opts) { | |||
| } | |||
| } | |||
|
|
|||
| const stream = send(request.raw, pathnameForSend, options) | |||
| // `send(..., path, ...)` will URI-decode path so we pass an encoded path here | |||
| const stream = send(request.raw, encodeURI(pathnameForSend), options) | |||
There was a problem hiding this comment.
your comment: we uri decode path in send? How about apply a change in @fastify/send , like pass decodeUri option in options, by default true for backwards compat and we set it here to false and then we dont need to encodeURI?
There was a problem hiding this comment.
From the docs of @fastify/send is seems the path passed is intended to be URL-encoded:
pathis a urlencoded path to send (urlencoded, not the actual file-system path)
https://github.com/fastify/send?tab=readme-ov-file#api
While I agree that this seems like a little extra overhead it looks like the @fastify/send API was not being used correctly.
There was a problem hiding this comment.
Maybe send could be refactored to do what Uzlopak suggests
| @@ -149,7 +149,7 @@ const dirList = { | |||
| route = path.normalize(path.join(route, '..')) | |||
| } | |||
| return { | |||
| href: path.join(prefix, route, entry.name).replace(/\\/gu, '/'), | |||
| href: encodeURI(path.join(prefix, route, entry.name).replace(/\\/gu, '/')), | |||
There was a problem hiding this comment.
I wonder, can we avoid this regex?
There was a problem hiding this comment.
This regex was already there. I tried switching to path.posix.join but that failed the tests on Windows.
| @@ -149,7 +149,7 @@ const dirList = { | |||
| route = path.normalize(path.join(route, '..')) | |||
There was a problem hiding this comment.
What if we change it to?
Maybe on top of the file we do:
const pathPosixNormalize = path.posix.normalize
and then do:
| route = path.normalize(path.join(route, '..')) | |
| route = pathPosixNormalize(path.join(route, '..')) |
There was a problem hiding this comment.
This might work but it's not part of the fix for this PR. See #452 (comment)
|
I have the feeling, that we would reduce the performance with this change, and with a little bit more effort we can improve the overall performance. wdyt? |
In my opinion the slight performance hit is worth it for the correctness. It's worth noting that |
|
@Uzlopak the performance of this module is already pretty bad. We can't even try to match NGINX and similar in hosting static files. I don't think adding this is a problem. |
|
@mcollina would fastify/send#15 improve performance? |
|
To be honest, I think performance is not much of a consideration here and is a little off-topic. This change is about fixing a behaviour that is clearly a bug. |
|
Thanks all! 🙏🏼 When do you expect this to be in a release? |
|
Just released v7.0.4 |
Resolves #451
400 Bad Requesterror when serving files with a%in the filenamehrefparameter for directory listings (otherwise files with a%in the filename generate an invalidhref)Checklist
npm run testandnpm run benchmarkand the Code of conduct