Fix static/ file name encoding (#11373)

* Test `static/` file name encoding

* Fix `static/` file name encoding

Author: Timer

packages/next/next-server/server/next-server.ts

@@ -3,7 +3,7 @@ import fs from 'fs'
 import { IncomingMessage, ServerResponse } from 'http'
 import Proxy from 'http-proxy'
 import nanoid from 'next/dist/compiled/nanoid/index.js'
-import { join, resolve, sep } from 'path'
+import { join, relative, resolve, sep } from 'path'
 import { parse as parseQs, ParsedUrlQuery } from 'querystring'
 import { format as formatUrl, parse as parseUrl, UrlWithParsedQuery } from 'url'
 import { PrerenderManifest } from '../../build'
@@ -346,7 +346,11 @@ export default class Server {
             match: route('/static/:path*'),
             name: 'static catchall',
             fn: async (req, res, params, parsedUrl) => {
-              const p = join(this.dir, 'static', ...(params.path || []))
+              const p = join(
+                this.dir,
+                'static',
+                ...(params.path || []).map(encodeURIComponent)
+              )
               await this.serveStatic(req, res, p, parsedUrl)
               return {
                 finished: true,
@@ -705,14 +709,15 @@ export default class Server {
         match: route('/:path*'),
         name: 'public folder catchall',
         fn: async (req, res, params, parsedUrl) => {
-          const path = `/${(params.path || []).join('/')}`
+          const pathParts: string[] = params.path || []
+          const path = `/${pathParts.join('/')}`
 
           if (publicFiles.has(path)) {
             await this.serveStatic(
               req,
               res,
               // we need to re-encode it since send decodes it
-              join(this.dir, 'public', encodeURIComponent(path)),
+              join(this.publicDir, ...pathParts.map(encodeURIComponent)),
               parsedUrl
             )
             return {
@@ -1350,18 +1355,77 @@ export default class Server {
     }
   }
 
-  private isServeableUrl(path: string): boolean {
-    const resolved = resolve(path)
+  private _validFilesystemPathSet: Set<string> | null = null
+  private getFilesystemPaths(): Set<string> {
+    if (this._validFilesystemPathSet) {
+      return this._validFilesystemPathSet
+    }
+
+    const pathUserFilesStatic = join(this.dir, 'static')
+    let userFilesStatic: string[] = []
+    if (this.hasStaticDir && fs.existsSync(pathUserFilesStatic)) {
+      userFilesStatic = recursiveReadDirSync(pathUserFilesStatic).map(f =>
+        join('.', 'static', f)
+      )
+    }
+
+    let userFilesPublic: string[] = []
+    if (this.publicDir && fs.existsSync(this.publicDir)) {
+      userFilesPublic = recursiveReadDirSync(this.publicDir).map(f =>
+        join('.', 'public', f)
+      )
+    }
+
+    let nextFilesStatic: string[] = []
+    nextFilesStatic = recursiveReadDirSync(
+      join(this.distDir, 'static')
+    ).map(f => join('.', relative(this.dir, this.distDir), 'static', f))
+
+    return (this._validFilesystemPathSet = new Set<string>([
+      ...nextFilesStatic,
+      ...userFilesPublic,
+      ...userFilesStatic,
+    ]))
+  }
+
+  protected isServeableUrl(untrustedFileUrl: string): boolean {
+    // This method mimics what the version of `send` we use does:
+    // 1. decodeURIComponent:
+    //    https://github.com/pillarjs/send/blob/0.17.1/index.js#L989
+    //    https://github.com/pillarjs/send/blob/0.17.1/index.js#L518-L522
+    // 2. resolve:
+    //    https://github.com/pillarjs/send/blob/de073ed3237ade9ff71c61673a34474b30e5d45b/index.js#L561
+
+    let decodedUntrustedFilePath: string
+    try {
+      // (1) Decode the URL so we have the proper file name
+      decodedUntrustedFilePath = decodeURIComponent(untrustedFileUrl)
+    } catch {
+      return false
+    }
+
+    // (2) Resolve "up paths" to determine real request
+    const untrustedFilePath = resolve(decodedUntrustedFilePath)
+
+    // don't allow null bytes anywhere in the file path
+    if (untrustedFilePath.indexOf('\0') !== -1) {
+      return false
+    }
+
+    // Check if .next/static, static and public are in the path.
+    // If not the path is not available.
     if (
-      resolved.indexOf(join(this.distDir) + sep) !== 0 &&
-      resolved.indexOf(join(this.dir, 'static') + sep) !== 0 &&
-      resolved.indexOf(join(this.dir, 'public') + sep) !== 0
+      (untrustedFilePath.startsWith(join(this.distDir, 'static') + sep) ||
+        untrustedFilePath.startsWith(join(this.dir, 'static') + sep) ||
+        untrustedFilePath.startsWith(join(this.dir, 'public') + sep)) === false
     ) {
-      // Seems like the user is trying to traverse the filesystem.
       return false
     }
 
-    return true
+    // Check against the real filesystem paths
+    const filesystemUrls = this.getFilesystemPaths()
+    const resolved = relative(this.dir, untrustedFilePath)
+    return filesystemUrls.has(resolved)
   }
 
   protected readBuildId(): string {

packages/next/server/next-dev-server.ts

@@ -3,7 +3,8 @@ import crypto from 'crypto'
 import findUp from 'find-up'
 import fs from 'fs'
 import { IncomingMessage, ServerResponse } from 'http'
-import { join, relative } from 'path'
+import Worker from 'jest-worker'
+import { join, relative, resolve, sep } from 'path'
 import React from 'react'
 import { UrlWithParsedQuery } from 'url'
 import { promisify } from 'util'
@@ -30,7 +31,6 @@ import { Telemetry } from '../telemetry/storage'
 import ErrorDebug from './error-debug'
 import HotReloader from './hot-reloader'
 import { findPageFile } from './lib/find-page-file'
-import Worker from 'jest-worker'
 
 if (typeof React.Suspense === 'undefined') {
   throw new Error(
@@ -279,7 +279,8 @@ export default class DevServer extends Server {
     parsedUrl: UrlWithParsedQuery
   ) {
     const { pathname } = parsedUrl
-    const path = `/${(params.path || []).join('/')}`
+    const pathParts = params.path || []
+    const path = `/${pathParts.join('/')}`
     // check for a public file, throwing error if there's a
     // conflicting page
     if (await this.hasPublicFile(path)) {
@@ -291,7 +292,7 @@ export default class DevServer extends Server {
         await this.renderError(err, req, res, pathname!, {})
         return true
       }
-      await this.servePublic(req, res, path)
+      await this.servePublic(req, res, pathParts)
       return true
     }
 
@@ -536,9 +537,12 @@ export default class DevServer extends Server {
     res.setHeader('Cache-Control', 'no-store, must-revalidate')
   }
 
-  servePublic(req: IncomingMessage, res: ServerResponse, path: string) {
-    const p = join(this.publicDir, encodeURIComponent(path))
-    // we need to re-encode it since send decodes it
+  private servePublic(
+    req: IncomingMessage,
+    res: ServerResponse,
+    pathParts: string[]
+  ) {
+    const p = join(this.publicDir, ...pathParts.map(encodeURIComponent))
     return this.serveStatic(req, res, p)
   }
 
@@ -558,4 +562,44 @@ export default class DevServer extends Server {
     // Return the very first error we found.
     return errors[0]
   }
+
+  protected isServeableUrl(untrustedFileUrl: string): boolean {
+    // This method mimics what the version of `send` we use does:
+    // 1. decodeURIComponent:
+    //    https://github.com/pillarjs/send/blob/0.17.1/index.js#L989
+    //    https://github.com/pillarjs/send/blob/0.17.1/index.js#L518-L522
+    // 2. resolve:
+    //    https://github.com/pillarjs/send/blob/de073ed3237ade9ff71c61673a34474b30e5d45b/index.js#L561
+
+    let decodedUntrustedFilePath: string
+    try {
+      // (1) Decode the URL so we have the proper file name
+      decodedUntrustedFilePath = decodeURIComponent(untrustedFileUrl)
+    } catch {
+      return false
+    }
+
+    // (2) Resolve "up paths" to determine real request
+    const untrustedFilePath = resolve(decodedUntrustedFilePath)
+
+    // don't allow null bytes anywhere in the file path
+    if (untrustedFilePath.indexOf('\0') !== -1) {
+      return false
+    }
+
+    // During development mode, files can be added while the server is running.
+    // Checks for .next/static, .next/server, static and public.
+    // Note that in development .next/server is available for error reporting purposes.
+    // see `packages/next/next-server/server/next-server.ts` for more details.
+    if (
+      untrustedFilePath.startsWith(join(this.distDir, 'static') + sep) ||
+      untrustedFilePath.startsWith(join(this.distDir, 'server') + sep) ||
+      untrustedFilePath.startsWith(join(this.dir, 'static') + sep) ||
+      untrustedFilePath.startsWith(join(this.dir, 'public') + sep)
+    ) {
+      return true
+    }
+
+    return false
+  }
 }

test/integration/basic/test/index.test.js

@@ -9,6 +9,7 @@ import errorRecovery from './error-recovery'
 import dynamic from './dynamic'
 import processEnv from './process-env'
 import publicFolder from './public-folder'
+import security from './security'
 
 const context = {}
 jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 5
@@ -35,4 +36,5 @@ describe('Basic Features', () => {
   errorRecovery(context, (p, q) => renderViaHTTP(context.appPort, p, q))
   processEnv(context)
   publicFolder(context)
+  security(context)
 })

test/integration/basic/test/security.js

@@ -0,0 +1,23 @@
+/* eslint-env jest */
+import { fetchViaHTTP } from 'next-test-utils'
+
+module.exports = context => {
+  describe('With Security Related Issues', () => {
+    it('should not allow accessing files outside .next/static and .next/server directory', async () => {
+      const pathsToCheck = [
+        `/_next/static/../BUILD_ID`,
+        `/_next/static/../routes-manifest.json`,
+      ]
+      for (const path of pathsToCheck) {
+        const res = await fetchViaHTTP(context.appPort, path)
+        const text = await res.text()
+        try {
+          expect(res.status).toBe(404)
+          expect(text).toMatch(/This page could not be found/)
+        } catch (err) {
+          throw new Error(`Path ${path} accessible from the browser`)
+        }
+      }
+    })
+  })
+}

test/integration/dynamic-routing/static/hello copy.txt

@@ -0,0 +1 @@
+hello world copy

test/integration/dynamic-routing/static/hello%20copy.txt

@@ -0,0 +1 @@
+hello world %20

test/integration/dynamic-routing/static/hello+copy.txt

@@ -0,0 +1 @@
+hello world +

test/integration/dynamic-routing/static/hello.txt

@@ -0,0 +1 @@
+hello world

test/integration/dynamic-routing/test/index.test.js

@@ -443,6 +443,34 @@ function runTests(dev) {
     expect(res.status).toBe(200)
   })
 
+  it('should serve file with space from static folder', async () => {
+    const res = await fetchViaHTTP(appPort, '/static/hello copy.txt')
+    const text = (await res.text()).trim()
+    expect(text).toBe('hello world copy')
+    expect(res.status).toBe(200)
+  })
+
+  it('should serve file with plus from static folder', async () => {
+    const res = await fetchViaHTTP(appPort, '/static/hello+copy.txt')
+    const text = (await res.text()).trim()
+    expect(text).toBe('hello world +')
+    expect(res.status).toBe(200)
+  })
+
+  it('should serve file from static folder encoded', async () => {
+    const res = await fetchViaHTTP(appPort, '/static/hello%20copy.txt')
+    const text = (await res.text()).trim()
+    expect(text).toBe('hello world copy')
+    expect(res.status).toBe(200)
+  })
+
+  it('should serve file with %20 from static folder', async () => {
+    const res = await fetchViaHTTP(appPort, '/static/hello%2520copy.txt')
+    const text = (await res.text()).trim()
+    expect(text).toBe('hello world %20')
+    expect(res.status).toBe(200)
+  })
+
   if (dev) {
     it('should work with HMR correctly', async () => {
       const browser = await webdriver(appPort, '/post-1/comments')

test/integration/production/test/security.js

@@ -50,6 +50,25 @@ module.exports = context => {
       }
     })
 
+    it('should not allow accessing files outside .next/static directory', async () => {
+      const pathsToCheck = [
+        `/_next/static/../server/pages-manifest.json`,
+        `/_next/static/../server/build-manifest.json`,
+        `/_next/static/../BUILD_ID`,
+        `/_next/static/../routes-manifest.json`,
+      ]
+      for (const path of pathsToCheck) {
+        const res = await fetchViaHTTP(context.appPort, path)
+        const text = await res.text()
+        try {
+          expect(res.status).toBe(404)
+          expect(text).toMatch(/This page could not be found/)
+        } catch (err) {
+          throw new Error(`Path ${path} accessible from the browser`)
+        }
+      }
+    })
+
     it("should not leak the user's home directory into the build", async () => {
       const buildId = readFileSync(join(__dirname, '../.next/BUILD_ID'), 'utf8')