diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4a4101d..1b34a58 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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 diff --git a/README.md b/README.md index 9ac509f..fd046c3 100644 --- a/README.md +++ b/README.md @@ -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. diff --git a/package.json b/package.json index f6e4dfc..3b6e9b8 100644 --- a/package.json +++ b/package.json @@ -54,7 +54,7 @@ } }, "scripts": { - "test": "node --expose-gc ./node_modules/.bin/jest", + "test": "node --expose-gc node_modules/jest/bin/jest.js", "build": "tsup --external hono", "watch": "tsup --watch", "postbuild": "publint", @@ -99,4 +99,4 @@ "peerDependencies": { "hono": "^4" } -} +} \ No newline at end of file diff --git a/src/serve-static.ts b/src/serve-static.ts index 87d4fae..d841f75 100644 --- a/src/serve-static.ts +++ b/src/serve-static.ts @@ -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 = { /** @@ -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 { @@ -60,6 +56,9 @@ const getStats = (path: string) => { export const serveStatic = ( options: ServeStaticOptions = { root: '' } ): MiddlewareHandler => { + const root = resolve(options.root || '.') + const optionPath = options.path + return async (c, next) => { // Do nothing if Response is already set if (c.finalized) { @@ -69,35 +68,40 @@ export const serveStatic = ( 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 - if (path) { - path = addCurrentDirPrefix(path) - } else { - return next() - } + let path = optionPath + ? options.root + ? resolve(join(root, optionPath)) + : optionPath + : resolve(join(root, 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(root)) { + await options.onNotFound?.(path, c) return next() } diff --git a/test/serve-static.test.ts b/test/serve-static.test.ts index 6ba4f07..1e9c5a3 100644 --- a/test/serve-static.test.ts +++ b/test/serve-static.test.ts @@ -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' @@ -68,7 +68,9 @@ describe('Serve Static Middleware', () => { expect(res.status).toBe(200) expect(res.text).toBe('

Hello Hono

') 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 () => { @@ -167,8 +169,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$/ ) }) @@ -226,4 +228,95 @@ 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('

Hello Hono

') + }) + + 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('Root and path combination tests', () => { + const rootPaths = [ + path.join(__dirname, 'assets'), + path.join(__dirname, 'assets'), + __dirname + path.sep + '..' + path.sep + 'test' + path.sep + 'assets', + ] + const optionPaths = ['favicon.ico', '/favicon.ico'] + rootPaths.forEach((root) => { + optionPaths.forEach((optionPath) => { + describe(`${root} + ${optionPath}`, () => { + const app = new Hono() + const server = createAdaptorServer(app) + + app.use( + '/favicon.ico', + serveStatic({ + root, + path: optionPath, + }) + ) + + it('Should return 200 response if both root and path set', 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) + }) + }) })