From 9e761e278d4b762c24b2b6948de01cc4b512a081 Mon Sep 17 00:00:00 2001 From: Yusuke Wada Date: Tue, 17 Sep 2024 16:53:40 +0900 Subject: [PATCH 1/8] feat(serve-static): support absolute root --- src/adapter/bun/serve-static.ts | 4 ++-- src/middleware/serve-static/index.ts | 5 +++++ src/utils/filepath.test.ts | 23 +++++++++++++++++++++++ src/utils/filepath.ts | 16 +++++++++++++++- 4 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/adapter/bun/serve-static.ts b/src/adapter/bun/serve-static.ts index 10e5f6deee..c5be32e6cb 100644 --- a/src/adapter/bun/serve-static.ts +++ b/src/adapter/bun/serve-static.ts @@ -9,13 +9,13 @@ export const serveStatic = ( ): MiddlewareHandler => { return async function serveStatic(c, next) { const getContent = async (path: string) => { - path = `./${path}` + path = path.startsWith('/') ? path : `./${path}` // @ts-ignore const file = Bun.file(path) return (await file.exists()) ? file : null } const pathResolve = (path: string) => { - return `./${path}` + return path.startsWith('/') ? path : `./${path}` } const isDir = async (path: string) => { let isDir diff --git a/src/middleware/serve-static/index.ts b/src/middleware/serve-static/index.ts index d9cdfbc063..49cbcd3c97 100644 --- a/src/middleware/serve-static/index.ts +++ b/src/middleware/serve-static/index.ts @@ -12,6 +12,7 @@ import { getMimeType } from '../../utils/mime' export type ServeStaticOptions = { root?: string path?: string + allowAbsoluteRoot?: boolean precompressed?: boolean mimes?: Record rewriteRequestPath?: (path: string) => string @@ -50,11 +51,14 @@ export const serveStatic = ( filename = options.rewriteRequestPath ? options.rewriteRequestPath(filename) : filename const root = options.root + const allowAbsoluteRoot = options.allowAbsoluteRoot ?? false + // If it was Directory, force `/` on the end. if (!filename.endsWith('/') && options.isDir) { const path = getFilePathWithoutDefaultDocument({ filename, root, + allowAbsoluteRoot, }) if (path && (await options.isDir(path))) { filename += '/' @@ -64,6 +68,7 @@ export const serveStatic = ( let path = getFilePath({ filename, root, + allowAbsoluteRoot, defaultDocument: DEFAULT_DOCUMENT, }) diff --git a/src/utils/filepath.test.ts b/src/utils/filepath.test.ts index 875ef8cb7c..be826a388c 100644 --- a/src/utils/filepath.test.ts +++ b/src/utils/filepath.test.ts @@ -60,6 +60,29 @@ describe('getFilePath', () => { expect(getFilePath({ filename: 'filename.suffix_index' })).toBe('filename.suffix_index') expect(getFilePath({ filename: 'filename.suffix-index' })).toBe('filename.suffix-index') }) + + it('Should return file path correctly with allowAbsoluteRoot', async () => { + const allowAbsoluteRoot = true + expect(getFilePath({ filename: 'foo.txt', allowAbsoluteRoot })).toBe('/foo.txt') + expect(getFilePath({ filename: 'foo.txt', root: '/p', allowAbsoluteRoot })).toBe('/p/foo.txt') + expect(getFilePath({ filename: 'foo', root: '/p', allowAbsoluteRoot })).toBe( + '/p/foo/index.html' + ) + expect(getFilePath({ filename: 'foo.txt', root: '/p/../p2', allowAbsoluteRoot })).toBe( + '/p2/foo.txt' + ) + expect(getFilePath({ filename: 'foo', root: '/p/bar', allowAbsoluteRoot })).toBe( + '/p/bar/foo/index.html' + ) + + expect( + getFilePath({ filename: 'foo.txt', root: slashToBackslash('/p'), allowAbsoluteRoot }) + ).toBe('/p/foo.txt') + + expect( + getFilePath({ filename: 'foo.txt', root: slashToBackslash('/p/../p2'), allowAbsoluteRoot }) + ).toBe('/p2/foo.txt') + }) }) function slashToBackslash(filename: string) { diff --git a/src/utils/filepath.ts b/src/utils/filepath.ts index f3f2ea82e3..d1fc3f49d6 100644 --- a/src/utils/filepath.ts +++ b/src/utils/filepath.ts @@ -7,6 +7,7 @@ type FilePathOptions = { filename: string root?: string defaultDocument?: string + allowAbsoluteRoot?: boolean } export const getFilePath = (options: FilePathOptions): string | undefined => { @@ -23,6 +24,7 @@ export const getFilePath = (options: FilePathOptions): string | undefined => { const path = getFilePathWithoutDefaultDocument({ root: options.root, + allowAbsoluteRoot: options.allowAbsoluteRoot, filename, }) @@ -42,6 +44,9 @@ export const getFilePathWithoutDefaultDocument = ( // /foo.html => foo.html filename = filename.replace(/^\.?[\/\\]/, '') + // assets\foo => assets/foo + root = root.replace(/\\/, '/') + // foo\bar.txt => foo/bar.txt filename = filename.replace(/\\/, '/') @@ -50,7 +55,16 @@ export const getFilePathWithoutDefaultDocument = ( // ./assets/foo.html => assets/foo.html let path = root ? root + '/' + filename : filename - path = path.replace(/^\.?\//, '') + + if (!options.allowAbsoluteRoot) { + path = path.replace(/^\.?\//, '') + } else { + // assets => /assets + path = path.replace(/^(?!\/)/, '/') + // Using URL to normalize the path. + const url = new URL(`file://${path}`) + path = url.pathname + } return path } From 46346a4846726102ea45c947d1f3bdc5045e03e9 Mon Sep 17 00:00:00 2001 From: Yusuke Wada Date: Tue, 17 Sep 2024 17:08:22 +0900 Subject: [PATCH 2/8] add bun runtime test --- runtime-tests/bun/index.test.tsx | 18 +++++++++++++++--- .../bun/static-absolute-root/plain.txt | 1 + src/utils/filepath.test.ts | 2 -- 3 files changed, 16 insertions(+), 5 deletions(-) create mode 100644 runtime-tests/bun/static-absolute-root/plain.txt diff --git a/runtime-tests/bun/index.test.tsx b/runtime-tests/bun/index.test.tsx index 8aaed909bd..40939a745d 100644 --- a/runtime-tests/bun/index.test.tsx +++ b/runtime-tests/bun/index.test.tsx @@ -1,4 +1,6 @@ import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import fs from 'fs/promises' +import path from 'path' import { stream, streamSSE } from '../..//src/helper/streaming' import { serveStatic, toSSG } from '../../src/adapter/bun' import { createBunWebSocket } from '../../src/adapter/bun/websocket' @@ -42,7 +44,7 @@ describe('Basic', () => { describe('Environment Variables', () => { it('Should return the environment variable', async () => { - const c = new Context(new HonoRequest(new Request('http://localhost/'))) + const c = new Context(new Request('http://localhost/')) const { NAME } = env<{ NAME: string }>(c) expect(NAME).toBe('Bun') }) @@ -107,6 +109,11 @@ describe('Serve Static Middleware', () => { }) ) + app.all( + '/static-absolute-root/*', + serveStatic({ root: path.dirname(__filename), allowAbsoluteRoot: true }) + ) + beforeEach(() => onNotFound.mockClear()) it('Should return static file correctly', async () => { @@ -157,6 +164,13 @@ describe('Serve Static Middleware', () => { expect(res.status).toBe(200) expect(await res.text()).toBe('Hi\n') }) + + it('Should return 200 response - /static-absolute-root/plain.txt', async () => { + const res = await app.request(new Request('http://localhost/static-absolute-root/plain.txt')) + expect(res.status).toBe(200) + expect(await res.text()).toBe('Bun!') + expect(onNotFound).not.toHaveBeenCalled() + }) }) // Bun support WebCrypto since v0.2.2 @@ -310,8 +324,6 @@ describe('WebSockets Helper', () => { expect(receivedMessage).toBe(message) }) }) -const fs = require('fs').promises -const path = require('path') async function deleteDirectory(dirPath) { if ( diff --git a/runtime-tests/bun/static-absolute-root/plain.txt b/runtime-tests/bun/static-absolute-root/plain.txt new file mode 100644 index 0000000000..4488acc6c2 --- /dev/null +++ b/runtime-tests/bun/static-absolute-root/plain.txt @@ -0,0 +1 @@ +Bun! \ No newline at end of file diff --git a/src/utils/filepath.test.ts b/src/utils/filepath.test.ts index be826a388c..6bc26524a7 100644 --- a/src/utils/filepath.test.ts +++ b/src/utils/filepath.test.ts @@ -74,11 +74,9 @@ describe('getFilePath', () => { expect(getFilePath({ filename: 'foo', root: '/p/bar', allowAbsoluteRoot })).toBe( '/p/bar/foo/index.html' ) - expect( getFilePath({ filename: 'foo.txt', root: slashToBackslash('/p'), allowAbsoluteRoot }) ).toBe('/p/foo.txt') - expect( getFilePath({ filename: 'foo.txt', root: slashToBackslash('/p/../p2'), allowAbsoluteRoot }) ).toBe('/p2/foo.txt') From 5d7a2af5caad8068b78a423daaff98b45e22b97f Mon Sep 17 00:00:00 2001 From: Yusuke Wada Date: Tue, 17 Sep 2024 17:09:28 +0900 Subject: [PATCH 3/8] don't use `Request` --- runtime-tests/bun/index.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime-tests/bun/index.test.tsx b/runtime-tests/bun/index.test.tsx index 40939a745d..e3ccecdef7 100644 --- a/runtime-tests/bun/index.test.tsx +++ b/runtime-tests/bun/index.test.tsx @@ -166,7 +166,7 @@ describe('Serve Static Middleware', () => { }) it('Should return 200 response - /static-absolute-root/plain.txt', async () => { - const res = await app.request(new Request('http://localhost/static-absolute-root/plain.txt')) + const res = await app.request('http://localhost/static-absolute-root/plain.txt') expect(res.status).toBe(200) expect(await res.text()).toBe('Bun!') expect(onNotFound).not.toHaveBeenCalled() From 28355705c9d5811edd401df6a8f1a19ea167e651 Mon Sep 17 00:00:00 2001 From: Yusuke Wada Date: Fri, 20 Sep 2024 18:46:44 +0900 Subject: [PATCH 4/8] add code for deno --- runtime-tests/deno/.vscode/settings.json | 2 +- runtime-tests/deno/deno.json | 3 ++- runtime-tests/deno/deno.lock | 26 +++++++++++++++++-- runtime-tests/deno/middleware.test.tsx | 20 ++++++++++++++ .../deno/static-absolute-root/plain.txt | 1 + src/adapter/deno/serve-static.ts | 2 +- 6 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 runtime-tests/deno/static-absolute-root/plain.txt diff --git a/runtime-tests/deno/.vscode/settings.json b/runtime-tests/deno/.vscode/settings.json index 40cd470fa5..39e14c39a5 100644 --- a/runtime-tests/deno/.vscode/settings.json +++ b/runtime-tests/deno/.vscode/settings.json @@ -1,7 +1,7 @@ { "eslint.validate": ["javascript", "javascriptreact", "typescript", "typescriptreact"], "editor.codeActionsOnSave": { - "source.fixAll.eslint": true + "source.fixAll.eslint": "explicit" }, "deno.enable": true } diff --git a/runtime-tests/deno/deno.json b/runtime-tests/deno/deno.json index 240c5d2e41..6ad900d241 100644 --- a/runtime-tests/deno/deno.json +++ b/runtime-tests/deno/deno.json @@ -13,7 +13,8 @@ ], "imports": { "@std/assert": "jsr:@std/assert@^1.0.3", + "@std/path": "jsr:@std/path@^1.0.3", "@std/testing": "jsr:@std/testing@^1.0.1", "hono/jsx/jsx-runtime": "../../src/jsx/jsx-runtime.ts" } -} +} \ No newline at end of file diff --git a/runtime-tests/deno/deno.lock b/runtime-tests/deno/deno.lock index 49d0b042e0..75d93277ce 100644 --- a/runtime-tests/deno/deno.lock +++ b/runtime-tests/deno/deno.lock @@ -2,9 +2,12 @@ "version": "3", "packages": { "specifiers": { - "jsr:@std/assert@^1.0.3": "jsr:@std/assert@1.0.3", + "jsr:@std/assert@^1.0.3": "jsr:@std/assert@1.0.5", + "jsr:@std/assert@^1.0.4": "jsr:@std/assert@1.0.5", "jsr:@std/internal@^1.0.2": "jsr:@std/internal@1.0.2", - "jsr:@std/testing@^1.0.1": "jsr:@std/testing@1.0.1" + "jsr:@std/internal@^1.0.3": "jsr:@std/internal@1.0.3", + "jsr:@std/path@^1.0.3": "jsr:@std/path@1.0.6", + "jsr:@std/testing@^1.0.1": "jsr:@std/testing@1.0.2" }, "jsr": { "@std/assert@1.0.3": { @@ -13,14 +16,32 @@ "jsr:@std/internal@^1.0.2" ] }, + "@std/assert@1.0.5": { + "integrity": "e37da8e4033490ce613eec4ac1d78dba1faf5b02a3f6c573a28f15365b9b440f", + "dependencies": [ + "jsr:@std/internal@^1.0.3" + ] + }, "@std/internal@1.0.2": { "integrity": "f4cabe2021352e8bfc24e6569700df87bf070914fc38d4b23eddd20108ac4495" }, + "@std/internal@1.0.3": { + "integrity": "208e9b94a3d5649bd880e9ca38b885ab7651ab5b5303a56ed25de4755fb7b11e" + }, + "@std/path@1.0.6": { + "integrity": "ab2c55f902b380cf28e0eec501b4906e4c1960d13f00e11cfbcd21de15f18fed" + }, "@std/testing@1.0.1": { "integrity": "9c25841137ee818933e1722091bb9ed5fdc251c35e84c97979a52196bdb6c5c3", "dependencies": [ "jsr:@std/assert@^1.0.3" ] + }, + "@std/testing@1.0.2": { + "integrity": "9e8a7f4e26c219addabe7942d09c3450fa0a74e9662341961bc0ef502274dec3", + "dependencies": [ + "jsr:@std/assert@^1.0.4" + ] } } }, @@ -28,6 +49,7 @@ "workspace": { "dependencies": [ "jsr:@std/assert@^1.0.3", + "jsr:@std/path@^1.0.3", "jsr:@std/testing@^1.0.1" ] } diff --git a/runtime-tests/deno/middleware.test.tsx b/runtime-tests/deno/middleware.test.tsx index ed55dc80d7..1954a0489c 100644 --- a/runtime-tests/deno/middleware.test.tsx +++ b/runtime-tests/deno/middleware.test.tsx @@ -1,4 +1,5 @@ import { assertEquals, assertMatch } from '@std/assert' +import { dirname, fromFileUrl } from '@std/path' import { assertSpyCall, assertSpyCalls, spy } from '@std/testing/mock' import { serveStatic } from '../../src/adapter/deno/index.ts' import { Hono } from '../../src/hono.ts' @@ -94,6 +95,21 @@ Deno.test('Serve Static middleware', async () => { }) ) + console.log(dirname(fromFileUrl(import.meta.url))) + + app.get( + '/static-absolute-root/*', + serveStatic({ root: dirname(fromFileUrl(import.meta.url)), allowAbsoluteRoot: true }) + ) + + app.get( + '/static/*', + serveStatic({ + root: './runtime-tests/deno', + onNotFound, + }) + ) + let res = await app.request('http://localhost/favicon.ico') assertEquals(res.status, 200) assertEquals(res.headers.get('Content-Type'), 'image/x-icon') @@ -132,6 +148,10 @@ Deno.test('Serve Static middleware', async () => { res = await app.request('http://localhost/static/hello.world') assertEquals(res.status, 200) assertEquals(await res.text(), 'Hi\n') + + res = await app.request('http://localhost/static-absolute-root/plain.txt') + assertEquals(res.status, 200) + assertEquals(await res.text(), 'Deno!') }) Deno.test('JWT Authentication middleware', async () => { diff --git a/runtime-tests/deno/static-absolute-root/plain.txt b/runtime-tests/deno/static-absolute-root/plain.txt new file mode 100644 index 0000000000..77d6622f4e --- /dev/null +++ b/runtime-tests/deno/static-absolute-root/plain.txt @@ -0,0 +1 @@ +Deno! \ No newline at end of file diff --git a/src/adapter/deno/serve-static.ts b/src/adapter/deno/serve-static.ts index 056fae0255..33786b203c 100644 --- a/src/adapter/deno/serve-static.ts +++ b/src/adapter/deno/serve-static.ts @@ -20,7 +20,7 @@ export const serveStatic = ( } } const pathResolve = (path: string) => { - return `./${path}` + return path.startsWith('/') ? path : `./${path}` } const isDir = (path: string) => { let isDir From a93839ae02e9eafc3f1c1a432e18788264a3622a Mon Sep 17 00:00:00 2001 From: Yusuke Wada Date: Sat, 21 Sep 2024 15:43:24 +0900 Subject: [PATCH 5/8] remove unnecessay `console.log` --- runtime-tests/deno/middleware.test.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/runtime-tests/deno/middleware.test.tsx b/runtime-tests/deno/middleware.test.tsx index 1954a0489c..7dd62c9dec 100644 --- a/runtime-tests/deno/middleware.test.tsx +++ b/runtime-tests/deno/middleware.test.tsx @@ -95,8 +95,6 @@ Deno.test('Serve Static middleware', async () => { }) ) - console.log(dirname(fromFileUrl(import.meta.url))) - app.get( '/static-absolute-root/*', serveStatic({ root: dirname(fromFileUrl(import.meta.url)), allowAbsoluteRoot: true }) From 5f07dd738a0f1629d2241e2d6b8c0d8cee6f48e7 Mon Sep 17 00:00:00 2001 From: Yusuke Wada Date: Sat, 21 Sep 2024 16:31:17 +0900 Subject: [PATCH 6/8] use `normalizeFilePath` instead of `URL` --- src/utils/filepath.test.ts | 11 +++++++++++ src/utils/filepath.ts | 23 ++++++++++++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/utils/filepath.test.ts b/src/utils/filepath.test.ts index 6bc26524a7..23f588dfa1 100644 --- a/src/utils/filepath.test.ts +++ b/src/utils/filepath.test.ts @@ -80,6 +80,17 @@ describe('getFilePath', () => { expect( getFilePath({ filename: 'foo.txt', root: slashToBackslash('/p/../p2'), allowAbsoluteRoot }) ).toBe('/p2/foo.txt') + expect( + getFilePath({ filename: 'foo.txt', root: slashToBackslash('/p/.../p2'), allowAbsoluteRoot }) + ).toBe('/p/.../p2/foo.txt') + + expect( + getFilePathWithoutDefaultDocument({ + filename: '/%2e%2e/%2e%2e/%2e%2e/%2e%2e/etc/passwd', + root: '/p/p2', + allowAbsoluteRoot: true, + }) + ).toBe('/p/p2/%2e%2e/%2e%2e/%2e%2e/%2e%2e/etc/passwd') // /etc/passwd }) }) diff --git a/src/utils/filepath.ts b/src/utils/filepath.ts index d1fc3f49d6..09c3f263dd 100644 --- a/src/utils/filepath.ts +++ b/src/utils/filepath.ts @@ -31,6 +31,24 @@ export const getFilePath = (options: FilePathOptions): string | undefined => { return path } +const normalizeFilePath = (filePath: string) => { + const parts = filePath.split(/[\/\\]/) + + const result = [] + + for (const part of parts) { + if (part === '' || part === '.') { + continue + } else if (part === '..') { + result.pop() + } else { + result.push(part) + } + } + + return '/' + (result.length === 1 ? result[0] : result.join('/')) +} + export const getFilePathWithoutDefaultDocument = ( options: Omit ): string | undefined => { @@ -61,9 +79,8 @@ export const getFilePathWithoutDefaultDocument = ( } else { // assets => /assets path = path.replace(/^(?!\/)/, '/') - // Using URL to normalize the path. - const url = new URL(`file://${path}`) - path = url.pathname + // /assets/foo/../bar => /assets/bar + path = normalizeFilePath(path) } return path From 5dd59c573360ee0e4808a10f4923b26f9235fd72 Mon Sep 17 00:00:00 2001 From: Yusuke Wada Date: Sat, 21 Sep 2024 20:32:20 +0900 Subject: [PATCH 7/8] use `URL` for `options.root` --- runtime-tests/bun/index.test.tsx | 6 +---- runtime-tests/deno/middleware.test.tsx | 5 +--- src/middleware/serve-static/index.test.ts | 16 +++++++++++ src/middleware/serve-static/index.ts | 20 +++++++++----- src/utils/filepath.test.ts | 32 ---------------------- src/utils/filepath.ts | 33 +---------------------- 6 files changed, 33 insertions(+), 79 deletions(-) diff --git a/runtime-tests/bun/index.test.tsx b/runtime-tests/bun/index.test.tsx index e3ccecdef7..6dda78abac 100644 --- a/runtime-tests/bun/index.test.tsx +++ b/runtime-tests/bun/index.test.tsx @@ -13,7 +13,6 @@ import { Hono } from '../../src/index' import { jsx } from '../../src/jsx' import { basicAuth } from '../../src/middleware/basic-auth' import { jwt } from '../../src/middleware/jwt' -import { HonoRequest } from '../../src/request' // Test just only minimal patterns. // Because others are tested well in Cloudflare Workers environment already. @@ -109,10 +108,7 @@ describe('Serve Static Middleware', () => { }) ) - app.all( - '/static-absolute-root/*', - serveStatic({ root: path.dirname(__filename), allowAbsoluteRoot: true }) - ) + app.all('/static-absolute-root/*', serveStatic({ root: path.dirname(__filename) })) beforeEach(() => onNotFound.mockClear()) diff --git a/runtime-tests/deno/middleware.test.tsx b/runtime-tests/deno/middleware.test.tsx index 7dd62c9dec..b3fc3fed5c 100644 --- a/runtime-tests/deno/middleware.test.tsx +++ b/runtime-tests/deno/middleware.test.tsx @@ -95,10 +95,7 @@ Deno.test('Serve Static middleware', async () => { }) ) - app.get( - '/static-absolute-root/*', - serveStatic({ root: dirname(fromFileUrl(import.meta.url)), allowAbsoluteRoot: true }) - ) + app.get('/static-absolute-root/*', serveStatic({ root: dirname(fromFileUrl(import.meta.url)) })) app.get( '/static/*', diff --git a/src/middleware/serve-static/index.test.ts b/src/middleware/serve-static/index.test.ts index 3f0c493f6b..f8ba7416f5 100644 --- a/src/middleware/serve-static/index.test.ts +++ b/src/middleware/serve-static/index.test.ts @@ -27,6 +27,16 @@ describe('Serve Static Middleware', () => { app.get('/static/*', serveStatic) + const serveStaticAbsoluteRoot = baseServeStatic({ + getContent, + pathResolve: (path) => { + return path + }, + root: '/home/hono/../foo', + }) + + app.get('/static-absolute/*', serveStaticAbsoluteRoot) + beforeEach(() => { getContent.mockClear() }) @@ -226,4 +236,10 @@ describe('Serve Static Middleware', () => { expect(res.status).toBe(200) expect(res.body).toBe(body) }) + + it('Should traverse directories with absolute root path', async () => { + const res = await app.request('/static-absolute/bar/hello.html') + expect(res.status).toBe(200) + expect(await res.text()).toBe('Hello in /home/foo/static-absolute/bar/hello.html') + }) }) diff --git a/src/middleware/serve-static/index.ts b/src/middleware/serve-static/index.ts index 49cbcd3c97..8bcede7c94 100644 --- a/src/middleware/serve-static/index.ts +++ b/src/middleware/serve-static/index.ts @@ -12,7 +12,6 @@ import { getMimeType } from '../../utils/mime' export type ServeStaticOptions = { root?: string path?: string - allowAbsoluteRoot?: boolean precompressed?: boolean mimes?: Record rewriteRequestPath?: (path: string) => string @@ -40,6 +39,16 @@ export const serveStatic = ( isDir?: (path: string) => boolean | undefined | Promise } ): MiddlewareHandler => { + let isAbsoluteRoot = false + let root: string + + if (options.root) { + if (options.root.startsWith('/')) { + isAbsoluteRoot = true + } + root = new URL(`file://${options.root}`).pathname + } + return async (c, next) => { // Do nothing if Response is already set if (c.finalized) { @@ -49,16 +58,12 @@ export const serveStatic = ( let filename = options.path ?? decodeURI(c.req.path) filename = options.rewriteRequestPath ? options.rewriteRequestPath(filename) : filename - const root = options.root - - const allowAbsoluteRoot = options.allowAbsoluteRoot ?? false // If it was Directory, force `/` on the end. if (!filename.endsWith('/') && options.isDir) { const path = getFilePathWithoutDefaultDocument({ filename, root, - allowAbsoluteRoot, }) if (path && (await options.isDir(path))) { filename += '/' @@ -68,7 +73,6 @@ export const serveStatic = ( let path = getFilePath({ filename, root, - allowAbsoluteRoot, defaultDocument: DEFAULT_DOCUMENT, }) @@ -76,6 +80,10 @@ export const serveStatic = ( return await next() } + if (isAbsoluteRoot) { + path = '/' + path + } + const getContent = options.getContent const pathResolve = options.pathResolve ?? defaultPathResolve path = pathResolve(path) diff --git a/src/utils/filepath.test.ts b/src/utils/filepath.test.ts index 23f588dfa1..875ef8cb7c 100644 --- a/src/utils/filepath.test.ts +++ b/src/utils/filepath.test.ts @@ -60,38 +60,6 @@ describe('getFilePath', () => { expect(getFilePath({ filename: 'filename.suffix_index' })).toBe('filename.suffix_index') expect(getFilePath({ filename: 'filename.suffix-index' })).toBe('filename.suffix-index') }) - - it('Should return file path correctly with allowAbsoluteRoot', async () => { - const allowAbsoluteRoot = true - expect(getFilePath({ filename: 'foo.txt', allowAbsoluteRoot })).toBe('/foo.txt') - expect(getFilePath({ filename: 'foo.txt', root: '/p', allowAbsoluteRoot })).toBe('/p/foo.txt') - expect(getFilePath({ filename: 'foo', root: '/p', allowAbsoluteRoot })).toBe( - '/p/foo/index.html' - ) - expect(getFilePath({ filename: 'foo.txt', root: '/p/../p2', allowAbsoluteRoot })).toBe( - '/p2/foo.txt' - ) - expect(getFilePath({ filename: 'foo', root: '/p/bar', allowAbsoluteRoot })).toBe( - '/p/bar/foo/index.html' - ) - expect( - getFilePath({ filename: 'foo.txt', root: slashToBackslash('/p'), allowAbsoluteRoot }) - ).toBe('/p/foo.txt') - expect( - getFilePath({ filename: 'foo.txt', root: slashToBackslash('/p/../p2'), allowAbsoluteRoot }) - ).toBe('/p2/foo.txt') - expect( - getFilePath({ filename: 'foo.txt', root: slashToBackslash('/p/.../p2'), allowAbsoluteRoot }) - ).toBe('/p/.../p2/foo.txt') - - expect( - getFilePathWithoutDefaultDocument({ - filename: '/%2e%2e/%2e%2e/%2e%2e/%2e%2e/etc/passwd', - root: '/p/p2', - allowAbsoluteRoot: true, - }) - ).toBe('/p/p2/%2e%2e/%2e%2e/%2e%2e/%2e%2e/etc/passwd') // /etc/passwd - }) }) function slashToBackslash(filename: string) { diff --git a/src/utils/filepath.ts b/src/utils/filepath.ts index 09c3f263dd..f3f2ea82e3 100644 --- a/src/utils/filepath.ts +++ b/src/utils/filepath.ts @@ -7,7 +7,6 @@ type FilePathOptions = { filename: string root?: string defaultDocument?: string - allowAbsoluteRoot?: boolean } export const getFilePath = (options: FilePathOptions): string | undefined => { @@ -24,31 +23,12 @@ export const getFilePath = (options: FilePathOptions): string | undefined => { const path = getFilePathWithoutDefaultDocument({ root: options.root, - allowAbsoluteRoot: options.allowAbsoluteRoot, filename, }) return path } -const normalizeFilePath = (filePath: string) => { - const parts = filePath.split(/[\/\\]/) - - const result = [] - - for (const part of parts) { - if (part === '' || part === '.') { - continue - } else if (part === '..') { - result.pop() - } else { - result.push(part) - } - } - - return '/' + (result.length === 1 ? result[0] : result.join('/')) -} - export const getFilePathWithoutDefaultDocument = ( options: Omit ): string | undefined => { @@ -62,9 +42,6 @@ export const getFilePathWithoutDefaultDocument = ( // /foo.html => foo.html filename = filename.replace(/^\.?[\/\\]/, '') - // assets\foo => assets/foo - root = root.replace(/\\/, '/') - // foo\bar.txt => foo/bar.txt filename = filename.replace(/\\/, '/') @@ -73,15 +50,7 @@ export const getFilePathWithoutDefaultDocument = ( // ./assets/foo.html => assets/foo.html let path = root ? root + '/' + filename : filename - - if (!options.allowAbsoluteRoot) { - path = path.replace(/^\.?\//, '') - } else { - // assets => /assets - path = path.replace(/^(?!\/)/, '/') - // /assets/foo/../bar => /assets/bar - path = normalizeFilePath(path) - } + path = path.replace(/^\.?\//, '') return path } From 6365ba90dc949b6886c12fb20e6e833e7fcff99c Mon Sep 17 00:00:00 2001 From: Yusuke Wada Date: Sun, 22 Sep 2024 10:00:39 +0900 Subject: [PATCH 8/8] don't allow directory traversal and fix the behavior when root including `../` --- src/middleware/serve-static/index.test.ts | 70 ++++++++++++++++++----- src/middleware/serve-static/index.ts | 4 +- src/utils/filepath.ts | 4 ++ 3 files changed, 63 insertions(+), 15 deletions(-) diff --git a/src/middleware/serve-static/index.test.ts b/src/middleware/serve-static/index.test.ts index f8ba7416f5..9681404673 100644 --- a/src/middleware/serve-static/index.test.ts +++ b/src/middleware/serve-static/index.test.ts @@ -27,16 +27,6 @@ describe('Serve Static Middleware', () => { app.get('/static/*', serveStatic) - const serveStaticAbsoluteRoot = baseServeStatic({ - getContent, - pathResolve: (path) => { - return path - }, - root: '/home/hono/../foo', - }) - - app.get('/static-absolute/*', serveStaticAbsoluteRoot) - beforeEach(() => { getContent.mockClear() }) @@ -237,9 +227,61 @@ describe('Serve Static Middleware', () => { expect(res.body).toBe(body) }) - it('Should traverse directories with absolute root path', async () => { - const res = await app.request('/static-absolute/bar/hello.html') - expect(res.status).toBe(200) - expect(await res.text()).toBe('Hello in /home/foo/static-absolute/bar/hello.html') + describe('Changing root path', () => { + const pathResolve = (path: string) => { + return path.startsWith('/') ? path : `./${path}` + } + + it('Should return the content with absolute root path', async () => { + const app = new Hono() + const serveStatic = baseServeStatic({ + getContent, + pathResolve, + root: '/home/hono/child', + }) + app.get('/static/*', serveStatic) + + const res = await app.request('/static/html/hello.html') + expect(await res.text()).toBe('Hello in /home/hono/child/static/html/hello.html') + }) + + it('Should traverse the directories with absolute root path', async () => { + const app = new Hono() + const serveStatic = baseServeStatic({ + getContent, + pathResolve, + root: '/home/hono/../parent', + }) + app.get('/static/*', serveStatic) + + const res = await app.request('/static/html/hello.html') + expect(await res.text()).toBe('Hello in /home/parent/static/html/hello.html') + }) + + it('Should treat the root path includes .. as relative path', async () => { + const app = new Hono() + const serveStatic = baseServeStatic({ + getContent, + pathResolve, + root: '../home/hono', + }) + app.get('/static/*', serveStatic) + + const res = await app.request('/static/html/hello.html') + expect(await res.text()).toBe('Hello in ./../home/hono/static/html/hello.html') + }) + + it('Should not allow directory traversal with . as relative path', async () => { + const app = new Hono() + const serveStatic = baseServeStatic({ + getContent, + pathResolve, + root: '.', + }) + app.get('*', serveStatic) + + const res = await app.request('///etc/passwd') + expect(res.status).toBe(404) + }) }) }) diff --git a/src/middleware/serve-static/index.ts b/src/middleware/serve-static/index.ts index 8bcede7c94..0f1837f22c 100644 --- a/src/middleware/serve-static/index.ts +++ b/src/middleware/serve-static/index.ts @@ -45,8 +45,10 @@ export const serveStatic = ( if (options.root) { if (options.root.startsWith('/')) { isAbsoluteRoot = true + root = new URL(`file://${options.root}`).pathname + } else { + root = options.root } - root = new URL(`file://${options.root}`).pathname } return async (c, next) => { diff --git a/src/utils/filepath.ts b/src/utils/filepath.ts index f3f2ea82e3..614714831f 100644 --- a/src/utils/filepath.ts +++ b/src/utils/filepath.ts @@ -52,5 +52,9 @@ export const getFilePathWithoutDefaultDocument = ( let path = root ? root + '/' + filename : filename path = path.replace(/^\.?\//, '') + if (root[0] !== '/' && path[0] === '/') { + return + } + return path }