CVE-2024-23340: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: @hono/node-server is an adapter that allows users to run Hono applications on Node.js. Since v1.3.0, @hono/node-server has used its own Request object with `url` behavior that is unexpected. In the standard API, if the URL contains `..`, here called "double dots", the URL string returned by Request will be in the resolved path. However, the `url` in @hono/node-server's Request as does not resolve double dots, so `http://localhost/static/.. /foo.txt` is returned. This causes vulnerabilities when using `serveStatic`. Modern web browsers and a latest `curl` command resolve double dots on the client side, so this issue doesn't affect those using either of those tools. However, problems may occur if accessed by a client that does not resolve them. Version 1.4.1 includes the change to fix this issue. As a workaround, don't use `serveStatic`.


CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 5.3
Published: 2024-01-22T23:15Z
Repository: https://github.com/honojs/node-server
Language: TypeScript
Files changed: src/request.ts, test/assets/secret.txt, test/request.test.ts, test/serve-static.test.ts
Commit date: 2024-01-21 16:54:03 +0900
Commit message: fix(request): handle "double dots" in URL (#124)


Diff: diff --git a/src/request.ts b/src/request.ts
index a341847..7fa4308 100644
--- a/src/request.ts
+++ b/src/request.ts
@@ -1,10 +1,11 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 // Define prototype for lightweight pseudo Request object
 
 import type { IncomingMessage } from 'node:http'
 import type { Http2ServerRequest } from 'node:http2'
+import { resolve } from 'node:path'
 import { Readable } from 'node:stream'
 
 const newRequestFromIncoming = (
   method: string,
   url: string,
@@ -39,11 +40,17 @@ const requestPrototype: Record<string | symbol, any> = {
   get method() {
     return this[incomingKey].method || 'GET'
   },
 
   get url() {
-    return `http://${this[incomingKey].headers.host}${this[incomingKey].url}`
+    let path = this[incomingKey]['path']
+    if (!path) {
+      const originalPath = this[incomingKey].url
+      path = /\.\./.test(originalPath) ? resolve(originalPath) : originalPath
+      this[incomingKey]['path'] = path
+    }
+    return `http://${this[incomingKey].headers.host}${path}`
   },
 
   [getRequestCache]() {
     return (this[requestCache] ||= newRequestFromIncoming(this.method, this.url, this[incomingKey]))
   },
diff --git a/test/assets/secret.txt b/test/assets/secret.txt
new file mode 100644
index 0000000..536aca3
--- /dev/null
+++ b/test/assets/secret.txt
@@ -0,0 +1 @@
+secret
\ No newline at end of file
diff --git a/test/request.test.ts b/test/request.test.ts
index 756f2d8..4444c9c 100644
--- a/test/request.test.ts
+++ b/test/request.test.ts
@@ -15,6 +15,19 @@ describe('Request', () => {
     expect(req).toBeInstanceOf(global.Request)
     expect(req.method).toBe('GET')
     expect(req.url).toBe('http://localhost/')
     expect(req.headers.get('host')).toBe('localhost')
   })
+
+  it('Should resolve double dots in URL', async () => {
+    const req = newRequest({
+      headers: {
+        host: 'localhost',
+      },
+      url: '/static/../foo.txt',
+    } as IncomingMessage)
+    expect(req).toBeInstanceOf(global.Request)
+    expect(req.url).toBe('http://localhost/foo.txt')
+    // Check if cached value is returned correctly
+    expect(req.url).toBe('http://localhost/foo.txt')
+  })
 })
diff --git a/test/serve-static.test.ts b/test/serve-static.test.ts
index b8af601..b26a6e8 100644
--- a/test/serve-static.test.ts
+++ b/test/serve-static.test.ts
@@ -127,6 +127,11 @@ describe('Serve Static Middleware', () => {
     expect(res.status).toBe(404)
     expect(notFoundMessage).toBe(
       './not-found/on-not-found/foo.txt is not found, request to /on-not-found/foo.txt'
     )
   })
+
+  it('Should handle double dots in URL', async () => {
+    const res = await request(server).get('/static/../secret.txt')
+    expect(res.status).toBe(404)
+  })
 })

Vulnerable code:     return `http://${this[incomingKey].headers.host}${this[incomingKey].url}`
Fixed code: import { resolve } from 'node:path'
    let path = this[incomingKey]['path']
    if (!path) {
      const originalPath = this[incomingKey].url
      path = /\.\./.test(originalPath) ? resolve(originalPath) : originalPath
      this[incomingKey]['path'] = path
    }
    return `http://${this[incomingKey].headers.host}${path}`
secret

  it('Should resolve double dots in URL', async () => {
    const req = newRequest({
      headers: {
        host: 'localhost',
      },
      url: '/static/../foo.txt',
    } as IncomingMessage)
    expect(req).toBeInstanceOf(global.Request)
    expect(req.url).toBe('http://localhost/foo.txt')
    // Check if cached value is returned correctly
    expect(req.url).toBe('http://localhost/foo.txt')
  })

  it('Should handle double dots in URL', async () => {
    const res = await request(server).get('/static/../secret.txt')
    expect(res.status).toBe(404)
  })