Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

support absolute root path for serveStatic #78

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+55 −17
Open

support absolute root path for serveStatic #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b21cc76
support absolute root path
miyamonz Aug 1, 2023
89b8104
fix comment
miyamonz Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/getFilePathforAbsRoot.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { getFilePathforAbsRoot } from './getFilePathforAbsRoot'

describe('getFilePathforAbsRoot', () => {
it('Should return file path correctly', async () => {
expect(getFilePathforAbsRoot({ filename: 'foo', root: '/bar' })).toBe('/bar/foo/index.html')
expect(getFilePathforAbsRoot({ filename: 'foo.txt', root: '/bar' })).toBe('/bar/foo.txt')
})
})
23 changes: 23 additions & 0 deletions src/getFilePathforAbsRoot.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { getFilePath } from 'hono/utils/filepath'

/**
* wrapper for getFilePath, with absolute root path
*/
export function getFilePathforAbsRoot(options: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that getFilePathForAbsRoot would be a better name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

root: string
filename: string
defaultDocument?: string
}) {
if (!options.root.startsWith('/')) {
throw new Error('root must be absolute path')
}

const path = getFilePath({
filename: options.filename,
defaultDocument: options.defaultDocument,
})
if (!path) return undefined

const root = options.root + (options.root?.endsWith('/') ? '' : '/')
return root + path
}
41 changes: 24 additions & 17 deletions src/serve-static.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import type { MiddlewareHandler } from 'hono'
import { ReadStream, createReadStream, existsSync, lstatSync } from 'fs'
import { getFilePath } from 'hono/utils/filepath'
import { getFilePathforAbsRoot } from './getFilePathforAbsRoot'
import { getMimeType } from 'hono/utils/mime'

export type ServeStaticOptions = {
/**
* Root path, relative to current working directory. (absolute paths are not supported)
* Root path, relative to current working directory or absolte path.
*/
root?: string
path?: string
Expand Down Expand Up @@ -38,27 +39,33 @@ export const serveStatic = (options: ServeStaticOptions = { root: '' }): Middlew

const url = new URL(c.req.url)

const filename = options.path ?? decodeURIComponent(url.pathname)
let path = getFilePath({
filename: options.rewriteRequestPath ? options.rewriteRequestPath(filename) : filename,
root: options.root,
defaultDocument: options.index ?? 'index.html',
})

if (!path) return next()

path = `./${path}`

if (!existsSync(path)) {
const path = options.path ?? decodeURIComponent(url.pathname)
const filename = options.rewriteRequestPath?.(path) ?? path
const defaultDocument = options.index ?? 'index.html'

const filePath = options.root?.startsWith('/')
? getFilePathforAbsRoot({
filename,
root: options.root,
defaultDocument,
})
: './' +
getFilePath({
filename,
root: options.root,
defaultDocument,
})

if (!filePath || !existsSync(filePath)) {
return next()
}

const mimeType = getMimeType(path)
const mimeType = getMimeType(filePath)
if (mimeType) {
c.header('Content-Type', mimeType)
}

const stat = lstatSync(path)
const stat = lstatSync(filePath)
const size = stat.size

if (c.req.method == 'HEAD' || c.req.method == 'OPTIONS') {
Expand All @@ -71,7 +78,7 @@ export const serveStatic = (options: ServeStaticOptions = { root: '' }): Middlew

if (!range) {
c.header('Content-Length', size.toString())
return c.body(createStreamBody(createReadStream(path)), 200)
return c.body(createStreamBody(createReadStream(filePath)), 200)
}

c.header('Accept-Ranges', 'bytes')
Expand All @@ -88,7 +95,7 @@ export const serveStatic = (options: ServeStaticOptions = { root: '' }): Middlew
}

const chunksize = end - start + 1
const stream = createReadStream(path, { start, end })
const stream = createReadStream(filePath, { start, end })

c.header('Content-Length', chunksize.toString())
c.header('Content-Range', `bytes ${start}-${end}/${stat.size}`)
Expand Down