feat(serve-static): use join to correct path resolution (#4291)

* feat(serve-static): use `join` to correct path resolution

* add windows tests

* implement `defaultJoin`

* add retrun type

* bun test support win

* remove warning add doc

* skip test

* remove warning

* fix the styles

* make `defaultJoin` behave like `path.posix.join`

* refactored and make it supports only Windows

* use `join` from 'node:path' for Deno

* simplify the `defaultJoin`

Co-authored-by: Taku Amano <[email protected]>

---------

Co-authored-by: Taku Amano <[email protected]>

Author: yusukebe

runtime-tests/bun/index.test.tsx

@@ -124,7 +124,9 @@ describe('Serve Static Middleware', () => {
     expect(res.status).toBe(404)
     expect(res.headers.get('X-Custom')).toBe('Bun')
     expect(onNotFound).toHaveBeenCalledWith(
-      './runtime-tests/bun/favicon-notfound.ico',
+      process.platform === 'win32'
+        ? 'runtime-tests\\bun\\favicon-notfound.ico'
+        : 'runtime-tests/bun/favicon-notfound.ico',
       expect.anything()
     )
   })

src/adapter/bun/serve-static.ts

@@ -1,5 +1,6 @@
 /* eslint-disable @typescript-eslint/ban-ts-comment */
 import { stat } from 'node:fs/promises'
+import { join } from 'node:path'
 import { serveStatic as baseServeStatic } from '../../middleware/serve-static'
 import type { ServeStaticOptions } from '../../middleware/serve-static'
 import type { Env, MiddlewareHandler } from '../../types'
@@ -9,14 +10,10 @@ export const serveStatic = <E extends Env = Env>(
 ): MiddlewareHandler => {
   return async function serveStatic(c, next) {
     const getContent = async (path: string) => {
-      path = path.startsWith('/') ? path : `./${path}`
       // @ts-ignore
       const file = Bun.file(path)
       return (await file.exists()) ? file : null
     }
-    const pathResolve = (path: string) => {
-      return path.startsWith('/') ? path : `./${path}`
-    }
     const isDir = async (path: string) => {
       let isDir
       try {
@@ -28,7 +25,7 @@ export const serveStatic = <E extends Env = Env>(
     return baseServeStatic({
       ...options,
       getContent,
-      pathResolve,
+      join,
       isDir,
     })(c, next)
   }

src/adapter/cloudflare-workers/serve-static.test.ts

@@ -79,7 +79,8 @@ describe('ServeStatic Middleware', () => {
     expect(res.headers.get('Content-Type')).toBe('text/plain; charset=utf-8')
   })
 
-  it('Should return index.html', async () => {
+  // Serve static on Cloudflare Workers cannot determine whether the target path is a directory or not
+  it.skip('Should return index.html', async () => {
     const res = await app.request('http://localhost/static/top')
     expect(res.status).toBe(200)
     expect(await res.text()).toBe('<h1>Top</h1>')

src/adapter/deno/serve-static.ts

@@ -1,3 +1,4 @@
+import { join } from 'node:path'
 import type { ServeStaticOptions } from '../../middleware/serve-static'
 import { serveStatic as baseServeStatic } from '../../middleware/serve-static'
 import type { Env, MiddlewareHandler } from '../../types'
@@ -23,9 +24,6 @@ export const serveStatic = <E extends Env = Env>(
         return null
       }
     }
-    const pathResolve = (path: string) => {
-      return path.startsWith('/') ? path : `./${path}`
-    }
     const isDir = (path: string) => {
       let isDir
       try {
@@ -34,11 +32,10 @@ export const serveStatic = <E extends Env = Env>(
       } catch {}
       return isDir
     }
-
     return baseServeStatic({
       ...options,
       getContent,
-      pathResolve,
+      join,
       isDir,
     })(c, next)
   }

src/middleware/serve-static/index.test.ts

@@ -12,11 +12,10 @@ describe('Serve Static Middleware', () => {
 
   const serveStatic = baseServeStatic({
     getContent,
-    pathResolve: (path) => {
-      return `./${path}`
-    },
     isDir: (path) => {
-      return path === 'static/hello.world'
+      if (path === 'static/sub' || path === 'static/hello.world') {
+        return true
+      }
     },
     onFound: (path, c) => {
       if (path.endsWith('hello.html')) {
@@ -36,36 +35,29 @@ describe('Serve Static Middleware', () => {
     expect(res.status).toBe(200)
     expect(res.headers.get('Content-Encoding')).toBeNull()
     expect(res.headers.get('Content-Type')).toMatch(/^text\/html/)
-    expect(await res.text()).toBe('Hello in ./static/hello.html')
-    expect(res.headers.get('X-Custom')).toBe('Found the file at ./static/hello.html')
+    expect(await res.text()).toBe('Hello in static/hello.html')
+    expect(res.headers.get('X-Custom')).toBe('Found the file at static/hello.html')
   })
 
   it('Should return 200 response - /static/sub', async () => {
     const res = await app.request('/static/sub')
     expect(res.status).toBe(200)
     expect(res.headers.get('Content-Type')).toMatch(/^text\/html/)
-    expect(await res.text()).toBe('Hello in ./static/sub/index.html')
-  })
-
-  it('Should return 200 response - /static/helloworld', async () => {
-    const res = await app.request('/static/helloworld')
-    expect(res.status).toBe(200)
-    expect(res.headers.get('Content-Type')).toMatch(/^text\/html/)
-    expect(await res.text()).toBe('Hello in ./static/helloworld/index.html')
+    expect(await res.text()).toBe('Hello in static/sub/index.html')
   })
 
   it('Should return 200 response - /static/hello.world', async () => {
     const res = await app.request('/static/hello.world')
     expect(res.status).toBe(200)
     expect(res.headers.get('Content-Type')).toMatch(/^text\/html/)
-    expect(await res.text()).toBe('Hello in ./static/hello.world/index.html')
+    expect(await res.text()).toBe('Hello in static/hello.world/index.html')
   })
 
   it('Should decode URI strings - /static/%E7%82%8E.txt', async () => {
     const res = await app.request('/static/%E7%82%8E.txt')
     expect(res.status).toBe(200)
     expect(res.headers.get('Content-Type')).toMatch(/^text\/plain/)
-    expect(await res.text()).toBe('Hello in ./static/炎.txt')
+    expect(await res.text()).toBe('Hello in static/炎.txt')
   })
 
   it('Should return 404 response - /static/not-found.txt', async () => {
@@ -228,15 +220,10 @@ describe('Serve Static Middleware', () => {
   })
 
   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)
@@ -249,7 +236,6 @@ describe('Serve Static Middleware', () => {
       const app = new Hono()
       const serveStatic = baseServeStatic({
         getContent,
-        pathResolve,
         root: '/home/hono/../parent',
       })
       app.get('/static/*', serveStatic)
@@ -262,26 +248,24 @@ describe('Serve Static Middleware', () => {
       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')
+      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)
+      expect(await res.text()).toBe('Hello in etc/passwd')
     })
   })
 })

src/middleware/serve-static/index.ts

@@ -6,8 +6,8 @@
 import type { Context, Data } from '../../context'
 import type { Env, MiddlewareHandler } from '../../types'
 import { COMPRESSIBLE_CONTENT_TYPE_REGEX } from '../../utils/compress'
-import { getFilePath, getFilePathWithoutDefaultDocument } from '../../utils/filepath'
 import { getMimeType } from '../../utils/mime'
+import { defaultJoin } from './path'
 
 export type ServeStaticOptions<E extends Env = Env> = {
   root?: string
@@ -27,100 +27,63 @@ const ENCODINGS = {
 const ENCODINGS_ORDERED_KEYS = Object.keys(ENCODINGS) as (keyof typeof ENCODINGS)[]
 
 const DEFAULT_DOCUMENT = 'index.html'
-const defaultPathResolve = (path: string) => path
-
-const isAbsolutePath = (path: string) => {
-  const isUnixAbsolutePath = path.startsWith('/')
-  const hasDriveLetter = /^[a-zA-Z]:\\/.test(path)
-  const isUncPath = /^\\\\[^\\]+\\[^\\]+/.test(path)
-  return isUnixAbsolutePath || hasDriveLetter || isUncPath
-}
-
-const windowsPathToUnixPath = (path: string) => {
-  return path.replace(/^[a-zA-Z]:/, '').replace(/\\/g, '/')
-}
 
 /**
  * This middleware is not directly used by the user. Create a wrapper specifying `getContent()` by the environment such as Deno or Bun.
  */
 export const serveStatic = <E extends Env = Env>(
   options: ServeStaticOptions<E> & {
     getContent: (path: string, c: Context<E>) => Promise<Data | Response | null>
+    /**
+     *
+     * `join` option according to the runtime. Example `import { join } from 'node:path`. If not specified, it will fall back to the default join function.`
+     */
+    join?: (...paths: string[]) => string
+    /**
+     * @deprecated Currently, `pathResolve` is no longer used.
+     */
     pathResolve?: (path: string) => string
     isDir?: (path: string) => boolean | undefined | Promise<boolean | undefined>
   }
 ): MiddlewareHandler => {
-  let isAbsoluteRoot = false
-  let root: string
-
-  if (options.root) {
-    if (isAbsolutePath(options.root)) {
-      isAbsoluteRoot = true
-      root = windowsPathToUnixPath(options.root)
-      root = new URL(`file://${root}`).pathname
-    } else {
-      root = options.root
-    }
-  }
+  const root = options.root ?? './'
+  const optionPath = options.path
+  const join = options.join ?? defaultJoin
 
   return async (c, next) => {
     // Do nothing if Response is already set
     if (c.finalized) {
-      await next()
-      return
+      return next()
     }
 
-    let filename = options.path ?? decodeURI(c.req.path)
-    filename = options.rewriteRequestPath ? options.rewriteRequestPath(filename) : filename
-
-    // If it was Directory, force `/` on the end.
-    if (!filename.endsWith('/') && options.isDir) {
-      const path = getFilePathWithoutDefaultDocument({
-        filename,
-        root,
-      })
-      if (path && (await options.isDir(path))) {
-        filename += '/'
+    let filename: string
+
+    if (options.path) {
+      filename = options.path
+    } else {
+      try {
+        filename = decodeURIComponent(c.req.path)
+        if (/(?:^|[\/\\])\.\.(?:$|[\/\\])/.test(filename)) {
+          throw new Error()
+        }
+      } catch {
+        await options.onNotFound?.(c.req.path, c)
+        return next()
       }
     }
 
-    let path = getFilePath({
-      filename,
+    let path = join(
       root,
-      defaultDocument: DEFAULT_DOCUMENT,
-    })
-
-    if (!path) {
-      return await next()
-    }
+      !optionPath && options.rewriteRequestPath ? options.rewriteRequestPath(filename) : filename
+    )
 
-    if (isAbsoluteRoot) {
-      path = '/' + path
+    if (options.isDir && (await options.isDir(path))) {
+      path = join(path, DEFAULT_DOCUMENT)
     }
 
     const getContent = options.getContent
-    const pathResolve = options.pathResolve ?? defaultPathResolve
-    path = pathResolve(path)
     let content = await getContent(path, c)
 
-    if (!content) {
-      let pathWithoutDefaultDocument = getFilePathWithoutDefaultDocument({
-        filename,
-        root,
-      })
-      if (!pathWithoutDefaultDocument) {
-        return await next()
-      }
-      pathWithoutDefaultDocument = pathResolve(pathWithoutDefaultDocument)
-
-      if (pathWithoutDefaultDocument !== path) {
-        content = await getContent(pathWithoutDefaultDocument, c)
-        if (content) {
-          path = pathWithoutDefaultDocument
-        }
-      }
-    }
-
     if (content instanceof Response) {
       return c.newResponse(content.body, content)
     }

src/middleware/serve-static/path.test.ts

@@ -0,0 +1,50 @@
+import { join as posixJoin } from 'node:path/posix'
+import { defaultJoin } from './path'
+
+describe('defaultJoin', () => {
+  describe('Comparison with node:path/posix.join', () => {
+    it('Should behave like path.posix.join for all path operations', () => {
+      const testCases = [
+        // Basic path joining
+        ['/home/yusuke/work/app/public', 'static/main.html'],
+        ['public', 'sub/', 'file.html'],
+        ['', 'file.html'],
+        ['public', ''],
+        ['public/', 'file.html'],
+        ['public', 'static', 'main.html'],
+        ['assets', 'images', 'logo.png'],
+
+        // Parent directory references
+        ['public', '../parent/file.html'],
+        ['public', '../../grandparent/file.html'],
+        ['/abs/path', '../relative.html'],
+        ['a/b', '../c'],
+        ['a/b/c', '../../d'],
+
+        // Current directory references
+        ['./public', 'static/main.html'],
+        ['public', './file.html'],
+        ['./public', '/absolute/path.html'],
+        ['.', 'file.html'],
+        ['public/.', 'file.html'],
+
+        // Edge cases
+        [],
+        ['.'],
+        [''],
+        ['/'],
+        ['a', 'b', 'c'],
+
+        // Backslash handling (security)
+        ['static', 'test\\file.txt'],
+        ['public', 'path\\with\\backslash'],
+      ]
+
+      testCases.forEach(([...args]) => {
+        const expected = posixJoin(...args)
+        const actual = defaultJoin(...args)
+        expect(actual).toBe(expected)
+      })
+    })
+  })
+})