v6.0.0: fix functional bugs found by adversarial review, 100% test coverage

.github/workflows/tests.yaml

@@ -6,10 +6,10 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v1
-      - name: Setup Environment (Using NodeJS 22.x)
+      - name: Setup Environment (Using NodeJS 24.x)
         uses: actions/setup-node@v1
         with:
-          node-version: 22.x
+          node-version: 24.x
 
       - name: Install dependencies
         run: npm install

README.md

@@ -20,6 +20,7 @@ Read more online:
 ```js
 npm i fast-gateway
 ```
+> Requires Node.js 24 or newer. Upgrading from v5.x? See the [breaking changes](https://fgw.21no.de/#/?id=v6x).
 
 ## Usage 
 ### Gateway

docs/README.md

@@ -25,6 +25,8 @@ Read more online:
 npm i fast-gateway
 ```
 
+> Requires Node.js 24 or newer. Upgrading from v5.x? See the v6.x Breaking Changes below.
+
 # Usage
 
 Next we describe two examples proxying HTTP and Lambda downstream services.
@@ -120,6 +122,7 @@ module.exports.handler = serverless(service)
   server,
   // Optional restana library configuration (https://www.npmjs.com/package/restana#configuration)
   // Please note that if "server" is provided, this settings are ignored.
+  // Routes are matched case-sensitively, use { caseSensitive: false } to match them case-insensitively.
   restana: {},
   // Optional global middlewares in the format: (req, res, next) => next()
   // Default value: []
@@ -175,7 +178,7 @@ module.exports.handler = serverless(service)
       endpoint: '/api-docs',
       type: 'swagger'
     },
-    // Optional "prefix rewrite" before request is forwarded. Default value: ''
+    // Optional "prefix rewrite" before request is forwarded, used literally. Default value: ''
     prefixRewrite: '',
     // Optional "url rewrite" hook. If defined, the prefixRewrite setting is ignored.
     urlRewrite: (req) => req.url,
@@ -541,6 +544,40 @@ You can support the maintenance of this project:
 
 # Breaking Changes
 
+## v6.x
+
+**Runtime**
+
+- Node.js 24 or newer is required.
+- [restana 7](https://github.com/BackendStack21/restana/blob/master/MIGRATION.md) is the default HTTP framework. Its trie router matches paths **case-sensitively**: `GET /Users` no longer matches a route with `prefix: '/users'`. Set `restana: { caseSensitive: false }` to keep the previous behavior.
+- Errors handled by restana's default `errorHandler` respond with the standard status text, e.g. `{"code":404,"message":"Not Found"}` instead of `"Internal Server Error"`.
+
+**Routing**
+
+- Requests whose path contains `.` or `..` segments get a `400` response. This includes encoded (`%2e`, `%2f`, `%5c`), backslash and `;` parameter forms such as `/..;/`.
+- The prefix is only stripped from the start of the URL.
+- `prefixRewrite` is used literally, so `$&` and other replacement patterns are no longer expanded.
+- `prefix: '/'` works as a catch-all route, and prefixes ending in `/` match nested paths. A `prefixRewrite` ending in `/` no longer produces `//`.
+
+**Proxying**
+
+- Chunked `Connection: close` responses over 1MB are streamed instead of rejected with `502`.
+- A route's `timeout: 0` disables the global timeout. `hooks.request` is merged with the timeout instead of replacing it, and `req.timeout` takes precedence over `hooks.request.timeout`.
+- Repeated query parameters (`?a=1&a=2`) are forwarded as-is. No trailing `?` is appended when there is no query string, and `proxyOpts.queryString` is then `null`.
+- The routes and hooks passed to `gateway()` are no longer modified, so they can be shared between routes and gateways.
+- If `onError` sends a response, `next(err)` is skipped. A response already partly sent when the error happened is destroyed.
+
+**Hostnames hook**
+
+- Hostnames are matched case-insensitively. Matchers are compiled when the hook is created, so later changes to the `hostname2prefix` array are not picked up.
+
+**WebSockets**
+
+- Close codes and reasons are relayed in both directions. When the upstream closes with a code that can't be relayed (1001, 1006, 1011), the client connection is dropped.
+- Only the subprotocol agreed with the client is offered upstream, and the query string is forwarded unchanged.
+- Messages sent while an async `onOpen` hook runs are queued and forwarded. Past 1MB of queued data, the client is closed with code `4413`.
+- Upgrade requests that aren't WebSocket, or match no route, are ended.
+
 ## v3.x
 
 - The `fast-proxy-lite` module is used by default to support `http` proxy type 🔥. This means, no `undici` or `http2` are supported by default.

docs/index.html

@@ -7,7 +7,7 @@
   <meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1" />
   <meta name="description" content="A super fast, framework agnostic, Node.js API Gateway for the masses!">
   <meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0">
-  <link rel="stylesheet" href="//cdn.jsdelivr.net/npm/docsify@4/lib/themes/vue.css">
+  <link rel="stylesheet" href="//cdn.jsdelivr.net/npm/docsify@4.13.1/lib/themes/vue.css">
   <link href="https://fonts.googleapis.com/css?family=Outfit" rel="stylesheet">
 
   <style>
@@ -36,10 +36,10 @@
     }
   </script>
   <!-- Docsify v4 -->
-  <script src="//cdn.jsdelivr.net/npm/docsify@4"></script>
+  <script src="//cdn.jsdelivr.net/npm/docsify@4.13.1"></script>
 
-  <script src="https://unpkg.com/docsify-copy-code@2"></script>
-  <script src="https://unpkg.com/docsify-share/build/index.min.js"></script>.
+  <script src="https://unpkg.com/docsify-copy-code@2.1.1"></script>
+  <script src="https://unpkg.com/docsify-share@0.1.3/build/index.min.js"></script>
 
   <script type="text/javascript" charset="UTF-8"
     src="//cdn.cookie-script.com/s/28dc51cd1b79414f499d222fffd2095d.js"></script>

index.js

@@ -6,12 +6,16 @@ const defaultProxyFactory = require('./lib/proxy-factory')
 const restana = require('restana')
 const defaultProxyHandler = (req, res, url, proxy, proxyOpts) =>
   proxy(req, res, url, proxyOpts)
-const DEFAULT_METHODS = require('restana/libs/methods').BASE.filter(
-  (method) => method !== 'all'
-)
+// restana's base HTTP methods, without "all"
+const DEFAULT_METHODS = ['get', 'delete', 'patch', 'post', 'put', 'head', 'options']
 const NOOP = (req, res) => {}
-const PROXY_TYPES = ['http', 'lambda']
+const PROXY_TYPES = ['http', 'http-legacy', 'lambda']
 const registerWebSocketRoutes = require('./lib/ws-proxy')
+// "." and ".." path segments, also percent-encoded or separated by a backslash
+// or an encoded (back)slash, some upstreams decode those before normalizing
+// (a ";" path parameter is ignored by some upstreams, e.g. "/..;/")
+const DOT_SEGMENT_REGEX =
+  /(?:^|[/\\]|%2f|%5c)(?:\.|%2e){1,2}(?:;[^/\\]*)?(?:[/\\]|%2f|%5c|$)/i
 
 const gateway = (opts) => {
   let proxyFactory
@@ -43,7 +47,8 @@ const gateway = (opts) => {
 
   // registering services.json
   const services = opts.routes.map((route) => ({
-    prefix: route.prefix,
+    prefix:
+      route.prefix instanceof RegExp ? route.prefix.toString() : route.prefix,
     docs: route.docs
   }))
   if (opts.enableServicesEndpoint) {
@@ -74,7 +79,10 @@ const gateway = (opts) => {
   // processing non-websocket routes
   opts.routes
     .filter((route) => route.proxyType !== 'websocket')
-    .forEach((route) => {
+    .forEach((routeConfig) => {
+      // route configs can be shared between routes or gateways, don't mutate them
+      const route = Object.assign({}, routeConfig)
+
       if (undefined === route.prefixRewrite) {
         route.prefixRewrite = ''
       }
@@ -95,9 +103,11 @@ const gateway = (opts) => {
       const { onRequestNoOp = NOOP, onResponse = NOOP } = hooksForDefaultType
 
       // populating required NOOPS
-      route.hooks = route.hooks || {}
-      route.hooks.onRequest = route.hooks.onRequest || onRequestNoOp
-      route.hooks.onResponse = route.hooks.onResponse || onResponse
+      const hooks = route.hooks || {}
+      route.hooks = Object.assign({}, hooks, {
+        onRequest: hooks.onRequest || onRequestNoOp,
+        onResponse: hooks.onResponse || onResponse
+      })
 
       // populating route middlewares
       route.middlewares = route.middlewares || []
@@ -112,21 +122,30 @@ const gateway = (opts) => {
       // route proxy handler function
       const proxyHandler = route.proxyHandler || defaultProxyHandler
 
-      // populating timeout config
-      route.timeout = route.timeout || opts.timeout
+      // populating timeout config, 0 disables the timeout
+      route.timeout = route.timeout ?? opts.timeout
+
+      // avoid a double slash between prefix and pathRegex, e.g. prefix: '/'
+      let prefix = route.prefix
+      if (
+        typeof prefix === 'string' &&
+        prefix.endsWith('/') &&
+        typeof route.pathRegex === 'string' &&
+        route.pathRegex.startsWith('/')
+      ) {
+        prefix = prefix.slice(0, -1)
+      }
 
       // registering route handlers
       const methods = route.methods || DEFAULT_METHODS
 
       const args = [
         // path
-        route.prefix instanceof RegExp
-          ? route.prefix
-          : route.prefix + route.pathRegex,
+        prefix instanceof RegExp ? prefix : prefix + route.pathRegex,
         // route middlewares
         ...route.middlewares,
         // route handler
-        handler(route, proxy, proxyHandler)
+        handler(route, prefix, proxy, proxyHandler)
       ]
 
       methods.forEach((method) => {
@@ -140,10 +159,56 @@ const gateway = (opts) => {
   return router
 }
 
-const handler = (route, proxy, proxyHandler) => async (req, res, next) => {
+const hasDotSegment = (url) => {
+  const queryIndex = url.indexOf('?')
+
+  return DOT_SEGMENT_REGEX.test(
+    queryIndex === -1 ? url : url.slice(0, queryIndex)
+  )
+}
+
+// routes can be matched case-insensitively (e.g. restana's caseSensitive: false),
+// so the prefix is stripped the same way
+const stripPrefix = (url, prefix, prefixRewrite) => {
+  if (url.slice(0, prefix.length).toLowerCase() !== prefix.toLowerCase()) {
+    return url
+  }
+
+  // avoid a double slash when prefixRewrite ends with "/", e.g. '/api/'
+  const rest = url.slice(prefix.length)
+
+  return prefixRewrite.endsWith('/') && rest.startsWith('/')
+    ? prefixRewrite + rest.slice(1)
+    : prefixRewrite + rest
+}
+
+// URLSearchParams(object) joins repeated parameters (arrays) with commas,
+// iterating [name, value] pairs makes it append each value instead
+function * queryPairs () {
+  for (const name in this) {
+    for (const value of [].concat(this[name])) yield [name, value]
+  }
+}
+
+const toQueryString = (query, url) => {
+  if (!query) return query
+
+  const values = Object.values(query)
+  // nothing to forward, avoids a trailing "?" in the upstream url
+  if (values.length === 0 && !url.includes('?')) return null
+  if (!values.some(Array.isArray)) return query
+
+  // still a plain object for hooks and custom proxy handlers
+  return Object.defineProperty(
+    Object.assign(Object.create(null), query),
+    Symbol.iterator,
+    { value: queryPairs }
+  )
+}
+
+const handler = (route, prefix, proxy, proxyHandler) => async (req, res, next) => {
   const {
     urlRewrite,
-    prefix,
     prefixRewrite,
     hooks,
     timeout,
@@ -152,27 +217,45 @@ const handler = (route, proxy, proxyHandler) => async (req, res, next) => {
   const { onRequest } = hooks
 
   try {
+    // the upstream would resolve dot segments, escaping the route prefix
+    if (hasDotSegment(req.url)) {
+      res.statusCode = 400
+      res.end('Bad Request')
+
+      return
+    }
+
     if (typeof urlRewrite === 'function') {
       req.url = urlRewrite(req)
     } else if (typeof prefix === 'string') {
-      req.url = req.url.replace(prefix, prefixRewrite)
+      req.url = stripPrefix(req.url, prefix, prefixRewrite)
+    }
+
+    // restana shares a frozen empty req.query when there is no query string
+    if (req.query && Object.isFrozen(req.query)) {
+      req.query = Object.create(null)
     }
 
     const shouldAbortProxy = await onRequest(req, res)
     if (!shouldAbortProxy) {
       const proxyOpts = Object.assign(
         {
-          request: {
-            timeout: req.timeout || timeout
-          },
-          queryString: disableQsOverwrite ? null : req.query
+          queryString: disableQsOverwrite
+            ? null
+            : toQueryString(req.query, req.url)
         },
-        route.hooks
+        hooks,
+        {
+          request: Object.assign({}, hooks.request, {
+            timeout: req.timeout ?? hooks.request?.timeout ?? timeout
+          })
+        }
       )
 
-      proxyHandler(req, res, req.url, proxy, proxyOpts)
+      await proxyHandler(req, res, req.url, proxy, proxyOpts)
     }
   } catch (err) {
+    const headersSent = res.headersSent
     const { onError } = hooks
     if (typeof onError === 'function') {
       try {
@@ -182,7 +265,13 @@ const handler = (route, proxy, proxyHandler) => async (req, res, next) => {
       }
     }
 
-    return next(err)
+    if (headersSent) {
+      // a partially sent response can't be turned into an error response
+      if (!res.writableEnded) res.destroy()
+    } else if (!res.headersSent) {
+      // otherwise the onError hook already sent a response
+      return next(err)
+    }
   }
 }