support absolute root path for serveStatic

[miyamonz]

Currently, serveStatic's options.root doesn't support absolute paths.
It appears that the getFilePath utility function isn't designed for this case.
I've created a small wrapper to address this issue.

[yusukebe]

It would be better to place this function in src/utils/filepath.ts instead of src/getFilePathforAbsRoot.ts. Additionally, the test should be located in src/utils/filepath.test.ts.

[yusukebe]

I think that getFilePathForAbsRoot would be a better name.

[miyamonz]

I agree.

I am considering a better name but have not yet been able to come up with one.

---

The current getFilePath function has the following features:

1. the relative path to the parent should be undefind (../a => undefined)
2. it normalizes the path for KV (considering the relative path to KV root ./hoge/fuga => hoge/fuga, /hoo => hoo)
3. it appends defaultDocument (/hoo => /hoo/index.html)

ref: https://github.com/honojs/hono/blob/fc73d5d105b65ae823184d33e67c8228aa1c2e02/src/utils/filepath.test.ts

I think that getFilePath has two roles:
features 1 and 2 are for KV, and the rest are for general use to serve something. Is that correct?

Relative to the parent or absolute path doesn't work because of the features 1 and 2.
I've tested that serveStatic in Bun and Deno also doesn't work.

I believe it would be better to prepare another function specifically for KV that utilizes getFilePath and implements features for KV as follows:

function getFilePathForKV(options :FilePathOptions) { // or more better function name
  if (/(?:^|\/)\.\.(?:$|\/)/.test(options.filename)) return
  const path = getFilePath(options)
  // ./assets/foo.html => assets/foo.html
  return path.replace(/^\.?\//, '')
}

What are your thoughts on this? If you agree with this opinion, I will create a pull request for it.

[yusukebe]

These are for both KV and other file system-based runtimes.

'1' is necessary to avoid issues such as directory traversal. In older versions of Bun, the URL path was not normalized, so there was a possibility that the URL path included .., which could lead to unexpected behavior. I think we can handle it in a different place, such as serve-static for Bun, but currently, we are handling it in getFilePath().

'2' is to normalize the path to find the file. The file path emitted from getFilePath() will be used as shown in the link:

https://github.com/honojs/hono/blob/fc73d5d105b65ae823184d33e67c8228aa1c2e02/src/adapter/bun/serve-static.ts#L38

In KV as well, /foo will fall back to /foo/index.html. So, this is necessary for KV.

[yusukebe]

HI @miyamonz !

Thank you for the PR. This is a good feature. I've leaved some comments. Please check them!

[lilnasy]

@miyamonz @yusukebe Spent a solid 30 minutes on this bug. Do you mind if I continue this PR? Seems like only the only comment is for a function name. Would love to see this land soon.

[yusukebe]

I've reconsidered this feature. Allowing absolute paths could potentially lead to a traversal attack vulnerability.

If we decide to proceed, we need to address this risk seriously. I would like to hear opinions about this potential security issue.

[lilnasy]

I'm not sure I see the concern. It's common enough to offer absolute path as root, and it's done by disallowing "/../" in the path.

For example, this is what send uses:

https://github.com/pillarjs/send/blob/b69cbb3dc4c09c37917d08a4c13fcd1bac97ade5/index.js#L63

Could you elaborate where a threat could come from?

[yusukebe]

In Hono, we also do not allow /../ in this line. So, as you mentioned, we might not need to consider it as a risk.

https://github.com/honojs/hono/blob/main/src/utils/filepath.ts#L9

[lilnasy]

That's reassuring. What do you think is next to progress this PR?

[yuyinws]

Any progress on this PR? It's very useful in some cases.

[schettn]

Any progress on this PR? It's very useful in some cases.

Workaround: honojs/hono#3108 (comment)

[fumieval]

I've just stumbled upon this issue. I think one way to move this forward is to add a field like allowAbsoluteRoot: true to ServeStaticOptions, so that existing users of serveStatic are not affected by potential security issues.

[schettn]

@yusukebe

Why not just introduce another param?

serveStatic({ root: './' }))
serveStatic({ absoluteRoot: '/' }))

• Add some checks to prevent both using both.
  (Typescript unions / runtime checks)

And then merge them at some point when hono has a major.

[yusukebe]

@fumieval @schettn

I like allowAbsoluteRoot: true option. Letting users choose the option is a good idea.

[schettn]

Should this be implemented in import { getFilePath } from 'hono/utils/filepath'?

Then just pass allowAbsoluteRoot to getFilePath instead of implementing getFilePathforAbsRoot here.

[yusukebe]

Hey! I'll work on this matter from now.