diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index c9311e325ec9bb8..af8fd9ce89667da 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/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 | null = null + private getFilesystemPaths(): Set { + 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([ + ...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 { diff --git a/packages/next/server/next-dev-server.ts b/packages/next/server/next-dev-server.ts index 6b9169800e48be8..f64907f5810f1d4 100644 --- a/packages/next/server/next-dev-server.ts +++ b/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 + } } diff --git a/test/integration/basic/test/index.test.js b/test/integration/basic/test/index.test.js index 1a40c3d06f7503e..27d47aefeac0d74 100644 --- a/test/integration/basic/test/index.test.js +++ b/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) }) diff --git a/test/integration/basic/test/security.js b/test/integration/basic/test/security.js new file mode 100644 index 000000000000000..b22b6aebc625715 --- /dev/null +++ b/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`) + } + } + }) + }) +} diff --git a/test/integration/dynamic-routing/static/hello copy.txt b/test/integration/dynamic-routing/static/hello copy.txt new file mode 100644 index 000000000000000..48941bc083e7e77 --- /dev/null +++ b/test/integration/dynamic-routing/static/hello copy.txt @@ -0,0 +1 @@ +hello world copy \ No newline at end of file diff --git a/test/integration/dynamic-routing/static/hello%20copy.txt b/test/integration/dynamic-routing/static/hello%20copy.txt new file mode 100644 index 000000000000000..c619e3b1f68fbf7 --- /dev/null +++ b/test/integration/dynamic-routing/static/hello%20copy.txt @@ -0,0 +1 @@ +hello world %20 \ No newline at end of file diff --git a/test/integration/dynamic-routing/static/hello+copy.txt b/test/integration/dynamic-routing/static/hello+copy.txt new file mode 100644 index 000000000000000..2c6733445659c96 --- /dev/null +++ b/test/integration/dynamic-routing/static/hello+copy.txt @@ -0,0 +1 @@ +hello world + \ No newline at end of file diff --git a/test/integration/dynamic-routing/static/hello.txt b/test/integration/dynamic-routing/static/hello.txt new file mode 100644 index 000000000000000..95d09f2b1015934 --- /dev/null +++ b/test/integration/dynamic-routing/static/hello.txt @@ -0,0 +1 @@ +hello world \ No newline at end of file diff --git a/test/integration/dynamic-routing/test/index.test.js b/test/integration/dynamic-routing/test/index.test.js index 4c73f4430f5507c..72f44167617c689 100644 --- a/test/integration/dynamic-routing/test/index.test.js +++ b/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') diff --git a/test/integration/production/test/security.js b/test/integration/production/test/security.js index 59ad0d15b5db25c..a7a0cd60eee8633 100644 --- a/test/integration/production/test/security.js +++ b/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')