Merge remote-tracking branch 'upstream/main' into fix-dangeling-promise

Author: yusukebe

.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

README.md

@@ -125,6 +125,19 @@ serve({
 })
 ```
 
+### `autoCleanupIncoming`
+
+The default value is `true`. The Node.js Adapter automatically cleans up (explicitly call `destroy()` method) if application is not finished to consume the incoming request. If you don't want to do that, set `false`.
+
+If the application accepts connections from arbitrary clients, this cleanup must be done otherwise incomplete requests from clients may cause the application to stop responding. If your application only accepts connections from trusted clients, such as in a reverse proxy environment and there is no process that returns a response without reading the body of the POST request all the way through, you can improve performance by setting it to `false`.
+
+```ts
+serve({
+  fetch: app.fetch,
+  autoCleanupIncoming: false,
+})
+```
+
 ## Middleware
 
 Most built-in middleware also works with Node.js.
@@ -155,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.
 

jest.config.js

@@ -1,6 +1,6 @@
 module.exports = {
   testMatch: ['**/test/**/*.+(ts)', '**/src/**/(*.)+(test).+(ts)'],
-  modulePathIgnorePatterns: ["test/setup.ts"],
+  modulePathIgnorePatterns: ["test/setup.ts", "test/app.ts"],
   transform: {
     '^.+\\.(ts)$': 'ts-jest',
   },

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@hono/node-server",
-  "version": "1.15.0",
+  "version": "1.17.0",
   "description": "Node.js Adapter for Hono",
   "main": "dist/index.js",
   "types": "dist/index.d.ts",
@@ -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",

src/listener.ts

@@ -1,9 +1,12 @@
 import type { IncomingMessage, ServerResponse, OutgoingHttpHeaders } from 'node:http'
-import type { Http2ServerRequest, Http2ServerResponse } from 'node:http2'
+import { Http2ServerRequest } from 'node:http2'
+import type { Http2ServerResponse } from 'node:http2'
+import type { IncomingMessageWithWrapBodyStream } from './request'
 import {
   abortControllerKey,
   newRequest,
   Request as LightweightRequest,
+  wrapBodyStream,
   toRequestError,
 } from './request'
 import { cacheKey, Response as LightweightResponse } from './response'
@@ -13,6 +16,11 @@ import { writeFromReadableStream, buildOutgoingHttpHeaders } from './utils'
 import { X_ALREADY_SENT } from './utils/response/constants'
 import './globals'
 
+const outgoingEnded = Symbol('outgoingEnded')
+type OutgoingHasOutgoingEnded = Http2ServerResponse & {
+  [outgoingEnded]?: () => void
+}
+
 const regBuffer = /^no$/i
 const regContentType = /^(application\/json\b|text\/(?!event-stream\b))/i
 
@@ -78,10 +86,12 @@ const responseViaCache = async (
     outgoing.end(new Uint8Array(await body.arrayBuffer()))
   } else {
     flushHeaders(outgoing)
-    return writeFromReadableStream(body, outgoing)?.catch(
+    await writeFromReadableStream(body, outgoing)?.catch(
       (e) => handleResponseError(e, outgoing) as undefined
     )
   }
+
+  ;(outgoing as OutgoingHasOutgoingEnded)[outgoingEnded]?.()
 }
 
 const responseViaResponseObject = async (
@@ -154,6 +164,8 @@ const responseViaResponseObject = async (
     outgoing.writeHead(res.status, resHeaderRecord)
     outgoing.end()
   }
+
+  ;(outgoing as OutgoingHasOutgoingEnded)[outgoingEnded]?.()
 }
 
 export const getRequestListener = (
@@ -162,8 +174,10 @@ export const getRequestListener = (
     hostname?: string
     errorHandler?: CustomErrorHandler
     overrideGlobalObjects?: boolean
+    autoCleanupIncoming?: boolean
   } = {}
 ) => {
+  const autoCleanupIncoming = options.autoCleanupIncoming ?? true
   if (options.overrideGlobalObjects !== false && global.Request !== LightweightRequest) {
     Object.defineProperty(global, 'Request', {
       value: LightweightRequest,
@@ -185,17 +199,59 @@ export const getRequestListener = (
       // so generate a pseudo Request object with only the minimum required information.
       req = newRequest(incoming, options.hostname)
 
+      let incomingEnded =
+        !autoCleanupIncoming || incoming.method === 'GET' || incoming.method === 'HEAD'
+      if (!incomingEnded) {
+        ;(incoming as IncomingMessageWithWrapBodyStream)[wrapBodyStream] = true
+        incoming.on('end', () => {
+          incomingEnded = true
+        })
+
+        if (incoming instanceof Http2ServerRequest) {
+          // a Http2ServerResponse instance requires additional processing on exit
+          // since outgoing.on('close') is not called even after outgoing.end() is called
+          // when the state is incomplete
+          ;(outgoing as OutgoingHasOutgoingEnded)[outgoingEnded] = () => {
+            // incoming is not consumed to the end
+            if (!incomingEnded) {
+              setTimeout(() => {
+                // in the case of a simple POST request, the cleanup process may be done automatically
+                // and end is called at this point. At that point, nothing is done.
+                if (!incomingEnded) {
+                  setTimeout(() => {
+                    incoming.destroy()
+                    // a Http2ServerResponse instance will not terminate without also calling outgoing.destroy()
+                    outgoing.destroy()
+                  })
+                }
+              })
+            }
+          }
+        }
+      }
+
       // Detect if request was aborted.
       outgoing.on('close', () => {
         const abortController = req[abortControllerKey] as AbortController | undefined
-        if (!abortController) {
-          return
+        if (abortController) {
+          if (incoming.errored) {
+            req[abortControllerKey].abort(incoming.errored.toString())
+          } else if (!outgoing.writableFinished) {
+            req[abortControllerKey].abort('Client connection prematurely closed.')
+          }
         }
 
-        if (incoming.errored) {
-          req[abortControllerKey].abort(incoming.errored.toString())
-        } else if (!outgoing.writableFinished) {
-          req[abortControllerKey].abort('Client connection prematurely closed.')
+        // incoming is not consumed to the end
+        if (!incomingEnded) {
+          setTimeout(() => {
+            // in the case of a simple POST request, the cleanup process may be done automatically
+            // and end is called at this point. At that point, nothing is done.
+            if (!incomingEnded) {
+              setTimeout(() => {
+                incoming.destroy()
+              })
+            }
+          })
         }
       })
 

src/request.ts

@@ -4,6 +4,7 @@
 import type { IncomingMessage } from 'node:http'
 import { Http2ServerRequest } from 'node:http2'
 import { Readable } from 'node:stream'
+import type { ReadableStreamDefaultReader } from 'node:stream/web'
 import type { TLSSocket } from 'node:tls'
 
 export class RequestError extends Error {
@@ -41,6 +42,8 @@ export class Request extends GlobalRequest {
   }
 }
 
+export type IncomingMessageWithWrapBodyStream = IncomingMessage & { [wrapBodyStream]: boolean }
+export const wrapBodyStream = Symbol('wrapBodyStream')
 const newRequestFromIncoming = (
   method: string,
   url: string,
@@ -83,6 +86,23 @@ const newRequestFromIncoming = (
           controller.close()
         },
       })
+    } else if ((incoming as IncomingMessageWithWrapBodyStream)[wrapBodyStream]) {
+      let reader: ReadableStreamDefaultReader<Uint8Array> | undefined
+      init.body = new ReadableStream({
+        async pull(controller) {
+          try {
+            reader ||= Readable.toWeb(incoming).getReader()
+            const { done, value } = await reader.read()
+            if (done) {
+              controller.close()
+            } else {
+              controller.enqueue(value)
+            }
+          } catch (error) {
+            controller.error(error)
+          }
+        },
+      })
     } else {
       // lazy-consume request body
       init.body = Readable.toWeb(incoming) as ReadableStream<Uint8Array>

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<E extends Env = Env> = {
   /**
@@ -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 = <E extends Env = any>(
   options: ServeStaticOptions<E> = { root: '' }
 ): MiddlewareHandler<E> => {
+  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 = <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
 
-    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()
       }
 

src/server.ts

@@ -8,6 +8,7 @@ export const createAdaptorServer = (options: Options): ServerType => {
   const requestListener = getRequestListener(fetchCallback, {
     hostname: options.hostname,
     overrideGlobalObjects: options.overrideGlobalObjects,
+    autoCleanupIncoming: options.autoCleanupIncoming,
   })
   // ts will complain about createServerHTTP and createServerHTTP2 not being callable, which works just fine
   // eslint-disable-next-line @typescript-eslint/no-explicit-any

src/types.ts

@@ -70,6 +70,7 @@ export type ServerOptions =
 export type Options = {
   fetch: FetchCallback
   overrideGlobalObjects?: boolean
+  autoCleanupIncoming?: boolean
   port?: number
   hostname?: string
 } & ServerOptions

src/utils.ts

@@ -7,35 +7,43 @@ export function writeFromReadableStream(stream: ReadableStream<Uint8Array>, writ
   } else if (writable.destroyed) {
     return stream.cancel()
   }
+
   const reader = stream.getReader()
-  writable.on('close', cancel)
-  writable.on('error', cancel)
-  reader.read().then(flow, cancel)
+
+  const handleError = () => {
+    // ignore the error
+  }
+
+  writable.on('error', handleError)
+
+  reader.read().then(flow, handleStreamError)
+
   return reader.closed.finally(() => {
-    writable.off('close', cancel)
-    writable.off('error', cancel)
+    writable.off('error', handleError)
   })
+
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  function cancel(error?: any) {
-    reader.cancel(error).catch(() => {})
+  function handleStreamError(error: any) {
     if (error) {
       writable.destroy(error)
     }
   }
+
   function onDrain() {
-    reader.read().then(flow, cancel)
+    reader.read().then(flow, handleStreamError)
   }
+
   function flow({ done, value }: ReadableStreamReadResult<Uint8Array>): void | Promise<void> {
     try {
       if (done) {
         writable.end()
       } else if (!writable.write(value)) {
         writable.once('drain', onDrain)
       } else {
-        return reader.read().then(flow, cancel)
+        return reader.read().then(flow, handleStreamError)
       }
     } catch (e) {
-      cancel(e)
+      handleStreamError(e)
     }
   }
 }