diff --git a/.cspell.json b/.cspell.json index 490b85431..5fd258574 100644 --- a/.cspell.json +++ b/.cspell.json @@ -18,7 +18,8 @@ "configurated", "mycustom", "commitlint", - "nosniff" + "nosniff", + "deoptimize" ], "ignorePaths": [ "CHANGELOG.md", diff --git a/src/middleware.js b/src/middleware.js index a7de58d4e..fa507f885 100644 --- a/src/middleware.js +++ b/src/middleware.js @@ -80,6 +80,18 @@ function wrapper(context) { extra, ); + if (extra.errorCode) { + if (extra.errorCode === 403) { + context.logger.error(`Malicious path "${filename}".`); + } + + sendError(req, res, extra.errorCode, { + modifyResponseData: context.options.modifyResponseData, + }); + + return; + } + if (!filename) { await goNext(); @@ -164,6 +176,7 @@ function wrapper(context) { headers: { "Content-Range": res.getHeader("Content-Range"), }, + modifyResponseData: context.options.modifyResponseData, }); return; diff --git a/src/utils/compatibleAPI.js b/src/utils/compatibleAPI.js index e8e3f5b5f..b168f040d 100644 --- a/src/utils/compatibleAPI.js +++ b/src/utils/compatibleAPI.js @@ -177,6 +177,8 @@ function destroyStream(stream, suppress) { /** @type {Record} */ const statuses = { + 400: "Bad Request", + 403: "Forbidden", 404: "Not Found", 416: "Range Not Satisfiable", 500: "Internal Server Error", @@ -213,7 +215,7 @@ function sendError(req, res, status, options) { // Send basic response setStatusCode(res, status); - setHeaderForResponse(res, "Content-Type", "text/html; charset=UTF-8"); + setHeaderForResponse(res, "Content-Type", "text/html; charset=utf-8"); setHeaderForResponse(res, "Content-Security-Policy", "default-src 'none'"); setHeaderForResponse(res, "X-Content-Type-Options", "nosniff"); diff --git a/src/utils/getFilenameFromUrl.js b/src/utils/getFilenameFromUrl.js index 82fab7447..82a58115a 100644 --- a/src/utils/getFilenameFromUrl.js +++ b/src/utils/getFilenameFromUrl.js @@ -43,11 +43,28 @@ const mem = (fn, { cache = new Map() } = {}) => { }; const memoizedParse = mem(parse); +const UP_PATH_REGEXP = /(?:^|[\\/])\.\.(?:[\\/]|$)/; + /** * @typedef {Object} Extra * @property {import("fs").Stats=} stats + * @property {number=} errorCode + */ + +/** + * decodeURIComponent. + * + * Allows V8 to only deoptimize this fn instead of all of send(). + * + * @param {string} input + * @returns {string} */ +function decode(input) { + return querystring.unescape(input); +} + +// TODO refactor me in the next major release, this function should return `{ filename, stats, error }` /** * @template {IncomingMessage} Request * @template {ServerResponse} Response @@ -85,22 +102,35 @@ function getFilenameFromUrl(context, url, extra = {}) { continue; } - if ( - urlObject.pathname && - urlObject.pathname.startsWith(publicPathObject.pathname) - ) { - filename = outputPath; + const pathname = decode(urlObject.pathname); + const publicPathPathname = decode(publicPathObject.pathname); + + if (pathname && pathname.startsWith(publicPathPathname)) { + // Null byte(s) + if (pathname.includes("\0")) { + // eslint-disable-next-line no-param-reassign + extra.errorCode = 400; + + return; + } + + // ".." is malicious + if (UP_PATH_REGEXP.test(path.normalize(`./${pathname}`))) { + // eslint-disable-next-line no-param-reassign + extra.errorCode = 403; + + return; + } // Strip the `pathname` property from the `publicPath` option from the start of requested url // `/complex/foo.js` => `foo.js` - const pathname = urlObject.pathname.slice( - publicPathObject.pathname.length, + // and add outputPath + // `foo.js` => `/home/user/my-project/dist/foo.js` + filename = path.join( + outputPath, + pathname.slice(publicPathPathname.length), ); - if (pathname) { - filename = path.join(outputPath, querystring.unescape(pathname)); - } - try { // eslint-disable-next-line no-param-reassign extra.stats = diff --git a/test/middleware.test.js b/test/middleware.test.js index 755b92fd7..79288d0b5 100644 --- a/test/middleware.test.js +++ b/test/middleware.test.js @@ -99,6 +99,10 @@ describe.each([ path.resolve(outputPath, "image.svg"), "svg image", ); + instance.context.outputFileSystem.writeFileSync( + path.resolve(outputPath, "image image.svg"), + "svg image", + ); instance.context.outputFileSystem.writeFileSync( path.resolve(outputPath, "byte-length.html"), "\u00bd + \u00bc = \u00be", @@ -183,6 +187,36 @@ describe.each([ expect(response.headers["content-type"]).toEqual("image/svg+xml"); }); + it('should return the "200" code for the "GET" request to the "image.svg" file with "/../"', async () => { + const fileData = instance.context.outputFileSystem.readFileSync( + path.resolve(outputPath, "image.svg"), + ); + + const response = await req.get("/public/../image.svg"); + + expect(response.statusCode).toEqual(200); + expect(response.headers["content-length"]).toEqual( + fileData.byteLength.toString(), + ); + expect(response.headers["content-type"]).toEqual("image/svg+xml"); + }); + + it('should return the "200" code for the "GET" request to the "image.svg" file with "/../../../"', async () => { + const fileData = instance.context.outputFileSystem.readFileSync( + path.resolve(outputPath, "image.svg"), + ); + + const response = await req.get( + "/public/assets/images/../../../image.svg", + ); + + expect(response.statusCode).toEqual(200); + expect(response.headers["content-length"]).toEqual( + fileData.byteLength.toString(), + ); + expect(response.headers["content-type"]).toEqual("image/svg+xml"); + }); + it('should return the "200" code for the "GET" request to the directory', async () => { const fileData = fs.readFileSync( path.resolve(__dirname, "./fixtures/index.html"), @@ -263,7 +297,7 @@ describe.each([ `bytes */${codeLength}`, ); expect(response.headers["content-type"]).toEqual( - "text/html; charset=UTF-8", + "text/html; charset=utf-8", ); expect(response.text).toEqual( ` @@ -447,6 +481,29 @@ describe.each([ false, ); }); + + it('should return the "200" code for the "GET" request to the "image image.svg" file', async () => { + const fileData = instance.context.outputFileSystem.readFileSync( + path.resolve(outputPath, "image image.svg"), + ); + + const response = await req.get("/image image.svg"); + + expect(response.statusCode).toEqual(200); + expect(response.headers["content-length"]).toEqual( + fileData.byteLength.toString(), + ); + expect(response.headers["content-type"]).toEqual("image/svg+xml"); + }); + + it('should return the "404" code for the "GET" request to the "%FF" file', async () => { + const response = await req.get("/%FF"); + + expect(response.statusCode).toEqual(404); + expect(response.headers["content-type"]).toEqual( + "text/html; charset=utf-8", + ); + }); }); describe('should not work with the broken "publicPath" option', () => { @@ -2032,7 +2089,7 @@ describe.each([ expect(response.statusCode).toEqual(500); expect(response.headers["content-type"]).toEqual( - "text/html; charset=UTF-8", + "text/html; charset=utf-8", ); expect(response.text).toEqual( "\n" + @@ -2113,7 +2170,7 @@ describe.each([ expect(response.statusCode).toEqual(404); expect(response.headers["content-type"]).toEqual( - "text/html; charset=UTF-8", + "text/html; charset=utf-8", ); expect(response.text).toEqual( "\n" + @@ -2575,6 +2632,7 @@ describe.each([ output: { filename: "bundle.js", path: path.resolve(__dirname, "./outputs/write-to-disk-true"), + publicPath: "/public/", }, }); @@ -2598,7 +2656,7 @@ describe.each([ it("should find the bundle file on disk", (done) => { request(app) - .get("/bundle.js") + .get("/public/bundle.js") .expect(200, (error) => { if (error) { return done(error); @@ -2632,6 +2690,25 @@ describe.each([ ); }); }); + + it("should not allow to get files above root", async () => { + const response = await req.get("/public/..%2f../middleware.test.js"); + + expect(response.statusCode).toEqual(403); + expect(response.headers["content-type"]).toEqual( + "text/html; charset=utf-8", + ); + expect(response.text).toEqual(` + + + +Error + + +
Forbidden
+ +`); + }); }); describe('should work with "true" value when the `output.clean` is `true`', () => { diff --git a/types/utils/getFilenameFromUrl.d.ts b/types/utils/getFilenameFromUrl.d.ts index 7dfc8035d..0ab19b393 100644 --- a/types/utils/getFilenameFromUrl.d.ts +++ b/types/utils/getFilenameFromUrl.d.ts @@ -1,9 +1,5 @@ /// export = getFilenameFromUrl; -/** - * @typedef {Object} Extra - * @property {import("fs").Stats=} stats - */ /** * @template {IncomingMessage} Request * @template {ServerResponse} Response @@ -25,6 +21,7 @@ declare namespace getFilenameFromUrl { } type Extra = { stats?: import("fs").Stats | undefined; + errorCode?: number | undefined; }; type IncomingMessage = import("../index.js").IncomingMessage; type ServerResponse = import("../index.js").ServerResponse;