Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,15 @@ jobs:
- run: bun run lint
- run: bun run build
- run: bun run test

ci-windows:
runs-on: windows-latest

steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: 22.x
- uses: oven-sh/setup-bun@v2
- run: bun install
- run: bun run test
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ import { serveStatic } from '@hono/node-server/serve-static'
app.use('/static/*', serveStatic({ root: './' }))
```

Note that `root` must be _relative_ to the current working directory from which the app was started. Absolute paths are not supported.
If using a relative path, `root` will be relative to the current working directory from which the app was started.

This can cause confusion when running your application locally.

Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
}
},
"scripts": {
"test": "node --expose-gc ./node_modules/.bin/jest",
"test": "node --expose-gc node_modules/jest/bin/jest.js",
Copy link
Member Author

Choose a reason for hiding this comment

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

To support Windows on CI.

"build": "tsup --external hono",
"watch": "tsup --watch",
"postbuild": "publint",
Expand Down Expand Up @@ -99,4 +99,4 @@
"peerDependencies": {
"hono": "^4"
}
}
}
52 changes: 30 additions & 22 deletions src/serve-static.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import type { Context, Env, MiddlewareHandler } from 'hono'
import { getFilePath, getFilePathWithoutDefaultDocument } from 'hono/utils/filepath'
import { getMimeType } from 'hono/utils/mime'
import { createReadStream, lstatSync } from 'node:fs'
import type { ReadStream, Stats } from 'node:fs'
import { createReadStream, lstatSync } from 'node:fs'
import { join, resolve } from 'node:path'

export type ServeStaticOptions<E extends Env = Env> = {
/**
Expand Down Expand Up @@ -44,10 +44,6 @@ const createStreamBody = (stream: ReadStream) => {
return body
}

const addCurrentDirPrefix = (path: string) => {
return `./${path}`
}

const getStats = (path: string) => {
let stats: Stats | undefined
try {
Expand All @@ -60,6 +56,9 @@ const getStats = (path: string) => {
export const serveStatic = <E extends Env = any>(
options: ServeStaticOptions<E> = { root: '' }
): MiddlewareHandler<E> => {
const optionRoot = options.root || '.'
const optionPath = options.path

return async (c, next) => {
// Do nothing if Response is already set
if (c.finalized) {
Expand All @@ -69,35 +68,44 @@ export const serveStatic = <E extends Env = any>(
let filename: string

try {
filename = options.path ?? decodeURIComponent(c.req.path)
const rawPath = optionPath ?? c.req.path
// Prevent encoded path traversal attacks
if (!optionPath) {
const decodedPath = decodeURIComponent(rawPath)
if (decodedPath.includes('..')) {
await options.onNotFound?.(rawPath, c)
return next()
}
}
filename = optionPath ?? decodeURIComponent(c.req.path)
} catch {
await options.onNotFound?.(c.req.path, c)
return next()
}

let path = getFilePathWithoutDefaultDocument({
filename: options.rewriteRequestPath ? options.rewriteRequestPath(filename, c) : filename,
root: options.root,
})
const requestPath = options.rewriteRequestPath
? options.rewriteRequestPath(filename, c)
: filename
const rootResolved = resolve(optionRoot)
let path: string

if (path) {
path = addCurrentDirPrefix(path)
if (optionPath) {
// Use path option directly if specified
path = resolve(optionPath)
Copy link
Member

Choose a reason for hiding this comment

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

In the current main branch, I think "root" is also used here.

Suggested change
path = resolve(optionPath)
path = resolve(join(optionRoot, optionPath))

Copy link
Member Author

Choose a reason for hiding this comment

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

@usualoma Thanks.

I think join is not needed, so it should beresolve(optionRoot, optionPath).

I aslo added the tests and the change is here:

f6f5b90

What do you think of this?

Copy link
Member

Choose a reason for hiding this comment

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

@yusukebe Thank you!

The behavior when specifying an absolute path in path will change from before.

app.use('*', serveStatic({
    root: 'public',
    path: '/file.html',
}))

Previously, it behaved as if it joined to root. If this is an intentional change in behavior, I think it's OK.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@usualoma

Thank you for pointing that out! It's better to keep the current behavior. I updated the code: d26e6ee

Please review it!

} else {
return next()
// Build with root + requestPath
path = resolve(join(optionRoot, requestPath))
}

let stats = getStats(path)

if (stats && stats.isDirectory()) {
path = getFilePath({
filename: options.rewriteRequestPath ? options.rewriteRequestPath(filename, c) : filename,
root: options.root,
defaultDocument: options.index ?? 'index.html',
})
const indexFile = options.index ?? 'index.html'
path = resolve(join(path, indexFile))

if (path) {
path = addCurrentDirPrefix(path)
} else {
// Security check: prevent path traversal attacks
if (!optionPath && !path.startsWith(rootResolved)) {
Copy link
Member

Choose a reason for hiding this comment

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

👍

path.startsWith(rootResolved)

await options.onNotFound?.(path, c)
return next()
}

Expand Down
68 changes: 64 additions & 4 deletions test/serve-static.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Hono } from 'hono'

import request from 'supertest'
import path from 'node:path'
import { serveStatic } from './../src/serve-static'
import { createAdaptorServer } from './../src/server'

Expand Down Expand Up @@ -68,7 +68,7 @@ describe('Serve Static Middleware', () => {
expect(res.status).toBe(200)
expect(res.text).toBe('<h1>Hello Hono</h1>')
expect(res.headers['content-type']).toBe('text/html; charset=utf-8')
expect(res.headers['x-custom']).toBe('Found the file at ./test/assets/static/index.html')
expect(res.headers['x-custom']).toMatch(/Found the file at .*\/test\/assets\/static\/index\.html$/)
})

it('Should return hono.html', async () => {
Expand Down Expand Up @@ -167,8 +167,8 @@ describe('Serve Static Middleware', () => {
it('Should handle the `onNotFound` option', async () => {
const res = await request(server).get('/on-not-found/foo.txt')
expect(res.status).toBe(404)
expect(notFoundMessage).toBe(
'./not-found/on-not-found/foo.txt is not found, request to /on-not-found/foo.txt'
expect(notFoundMessage).toMatch(
/.*\/not-found\/on-not-found\/foo\.txt is not found, request to \/on-not-found\/foo\.txt$/
)
})

Expand Down Expand Up @@ -226,4 +226,64 @@ describe('Serve Static Middleware', () => {
expect(res.headers['vary']).toBeUndefined()
expect(res.text).toBe('Hello Not Compressed')
})

describe('Absolute path', () => {
const rootPaths = [
path.join(__dirname, 'assets'),
__dirname + path.sep + '..' + path.sep + 'test' + path.sep + 'assets',
]
rootPaths.forEach((root) => {
describe(root, () => {
const app = new Hono()
const server = createAdaptorServer(app)
app.use('/static/*', serveStatic({ root }))
app.use('/favicon.ico', serveStatic({ path: root + path.sep + 'favicon.ico' }))

it('Should return index.html', async () => {
const res = await request(server).get('/static')
expect(res.status).toBe(200)
expect(res.headers['content-type']).toBe('text/html; charset=utf-8')
expect(res.text).toBe('<h1>Hello Hono</h1>')
})

it('Should return correct headers and data for text', async () => {
const res = await request(server).get('/static/plain.txt')
expect(res.status).toBe(200)
expect(res.headers['content-type']).toBe('text/plain; charset=utf-8')
expect(res.text).toBe('This is plain.txt')
})
it('Should return correct headers for icons', async () => {
const res = await request(server).get('/favicon.ico')
expect(res.status).toBe(200)
expect(res.headers['content-type']).toBe('image/x-icon')
})
})
})
})

describe('Security tests', () => {
const app = new Hono()
const server = createAdaptorServer(app)
app.use('/static/*', serveStatic({ root: './test/assets' }))

it('Should prevent path traversal attacks with double dots', async () => {
const res = await request(server).get('/static/../secret.txt')
expect(res.status).toBe(404)
})

it('Should prevent path traversal attacks with multiple levels', async () => {
const res = await request(server).get('/static/../../package.json')
expect(res.status).toBe(404)
})

it('Should prevent path traversal attacks with mixed separators', async () => {
const res = await request(server).get('/static/..\\..\\package.json')
expect(res.status).toBe(404)
})

it('Should prevent path traversal attacks with encoded dots', async () => {
const res = await request(server).get('/static/%2e%2e%2fsecret.txt')
expect(res.status).toBe(404)
})
})
})
Loading