diff --git a/src/utils/getFilenameFromUrl.js b/src/utils/getFilenameFromUrl.js index d5b20c7..4316027 100644 --- a/src/utils/getFilenameFromUrl.js +++ b/src/utils/getFilenameFromUrl.js @@ -118,33 +118,29 @@ function getFilenameFromUrl(context, url) { throw new FilenameError("Bad Request", 400); } + // Strip the public path prefix from the decoded pathname: + // `/complex/foo.js` => `foo.js`. + const relativePathname = pathname.slice(publicPathPathname.length); + // Only decoded paths containing two consecutive dots can produce a // parent-directory segment through `path.join`. - const mayHaveUpPath = pathname.includes(".."); + const mayHaveUpPath = relativePathname.includes(".."); - // ".." is malicious + // Stripping a public path without a trailing slash can turn + // `/assets../secret` into `../secret`. Check the remainder before joining. if ( mayHaveUpPath && - UP_PATH_REGEXP.test(path.normalize(`./${pathname}`)) + UP_PATH_REGEXP.test(path.normalize(`./${relativePathname}`)) ) { throw new FilenameError("Forbidden", 403); } - // Strip the `pathname` property from the `publicPath` option from the start of requested url - // `/complex/foo.js` => `foo.js` - // and add outputPath - // `foo.js` => `/home/user/my-project/dist/foo.js` - filename = path.join( - outputPath, - pathname.slice(publicPathPathname.length), - ); + // Add outputPath: `foo.js` => `/home/user/my-project/dist/foo.js`. + filename = path.join(outputPath, relativePathname); if (mayHaveUpPath) { - // A public path without a trailing slash can consume the characters - // before `..`, turning a safe-looking pathname such as - // `/assets../secret` into `../secret` after the prefix is stripped. - // Validate the final candidate while keeping ordinary requests on the - // fast path. + // Keep the final containment check as a backstop for platform-specific + // path behavior, without adding work for ordinary requests. const relative = path.relative(outputPath, filename); if (relative === ".." || relative.startsWith(`..${path.sep}`)) { @@ -163,9 +159,8 @@ function getFilenameFromUrl(context, url) { // Lazy evaluate immutable because it may be expensive to get asset info get immutable() { if (immutable === undefined) { - const assetName = pathname.slice(publicPathPathname.length); immutable = Boolean( - compilation.getAsset(assetName)?.info?.immutable, + compilation.getAsset(relativePathname)?.info?.immutable, ); } return immutable; diff --git a/test/middleware.test.js b/test/middleware.test.js index e033731..53e4fba 100644 --- a/test/middleware.test.js +++ b/test/middleware.test.js @@ -990,6 +990,12 @@ describe.each([ expect(response.statusCode).toBe(403); expect(response.text).not.toContain("TOP SECRET"); }); + + it("should reject a path that leaves and re-enters the output path", async () => { + const response = await req.get("/static../dist/index.html"); + + expect(response.statusCode).toBe(403); + }); }); describe("should work", () => {