From 246a06aa1d35aa206dcf11186b3aa90badb2de72 Mon Sep 17 00:00:00 2001 From: Yusuke Wada Date: Tue, 24 Sep 2024 12:23:35 +0900 Subject: [PATCH] feat(serve-static): support absolute root (#3420) * feat(serve-static): support absolute root * add bun runtime test * don't use `Request` * add code for deno * remove unnecessay `console.log` * use `normalizeFilePath` instead of `URL` * use `URL` for `options.root` * don't allow directory traversal and fix the behavior when root including `../` --- runtime-tests/bun/index.test.tsx | 16 +++-- .../bun/static-absolute-root/plain.txt | 1 + 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 | 15 +++++ .../deno/static-absolute-root/plain.txt | 1 + src/adapter/bun/serve-static.ts | 4 +- src/adapter/deno/serve-static.ts | 2 +- src/middleware/serve-static/index.test.ts | 58 +++++++++++++++++++ src/middleware/serve-static/index.ts | 17 +++++- src/utils/filepath.ts | 4 ++ 12 files changed, 137 insertions(+), 12 deletions(-) create mode 100644 runtime-tests/bun/static-absolute-root/plain.txt create mode 100644 runtime-tests/deno/static-absolute-root/plain.txt diff --git a/runtime-tests/bun/index.test.tsx b/runtime-tests/bun/index.test.tsx index 8aaed909..6dda78ab 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' @@ -11,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. @@ -42,7 +43,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 +108,8 @@ describe('Serve Static Middleware', () => { }) ) + app.all('/static-absolute-root/*', serveStatic({ root: path.dirname(__filename) })) + beforeEach(() => onNotFound.mockClear()) it('Should return static file correctly', async () => { @@ -157,6 +160,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('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 +320,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 00000000..4488acc6 --- /dev/null +++ b/runtime-tests/bun/static-absolute-root/plain.txt @@ -0,0 +1 @@ +Bun! \ No newline at end of file diff --git a/runtime-tests/deno/.vscode/settings.json b/runtime-tests/deno/.vscode/settings.json index 40cd470f..39e14c39 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 240c5d2e..6ad900d2 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 49d0b042..75d93277 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 ed55dc80..b3fc3fed 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,16 @@ Deno.test('Serve Static middleware', async () => { }) ) + app.get('/static-absolute-root/*', serveStatic({ root: dirname(fromFileUrl(import.meta.url)) })) + + 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 +143,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 00000000..77d6622f --- /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/bun/serve-static.ts b/src/adapter/bun/serve-static.ts index 10e5f6de..c5be32e6 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/adapter/deno/serve-static.ts b/src/adapter/deno/serve-static.ts index 056fae02..33786b20 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 diff --git a/src/middleware/serve-static/index.test.ts b/src/middleware/serve-static/index.test.ts index 3f0c493f..96814046 100644 --- a/src/middleware/serve-static/index.test.ts +++ b/src/middleware/serve-static/index.test.ts @@ -226,4 +226,62 @@ describe('Serve Static Middleware', () => { expect(res.status).toBe(200) expect(res.body).toBe(body) }) + + 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 d9cdfbc0..0f1837f2 100644 --- a/src/middleware/serve-static/index.ts +++ b/src/middleware/serve-static/index.ts @@ -39,6 +39,18 @@ 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 + } else { + root = options.root + } + } + return async (c, next) => { // Do nothing if Response is already set if (c.finalized) { @@ -48,7 +60,6 @@ export const serveStatic = ( let filename = options.path ?? decodeURI(c.req.path) filename = options.rewriteRequestPath ? options.rewriteRequestPath(filename) : filename - const root = options.root // If it was Directory, force `/` on the end. if (!filename.endsWith('/') && options.isDir) { @@ -71,6 +82,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.ts b/src/utils/filepath.ts index f3f2ea82..61471483 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 }