Skip to content

Commit

Permalink
fix(security): do not allow to read files above (#1771)
Browse files Browse the repository at this point in the history
  • Loading branch information
alexander-akait authored Mar 19, 2024
1 parent ab533de commit e10008c
Show file tree
Hide file tree
Showing 6 changed files with 141 additions and 21 deletions.
3 changes: 2 additions & 1 deletion .cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
"configurated",
"mycustom",
"commitlint",
"nosniff"
"nosniff",
"deoptimize"
],
"ignorePaths": [
"CHANGELOG.md",
Expand Down
13 changes: 13 additions & 0 deletions src/middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -164,6 +176,7 @@ function wrapper(context) {
headers: {
"Content-Range": res.getHeader("Content-Range"),
},
modifyResponseData: context.options.modifyResponseData,
});

return;
Expand Down
4 changes: 3 additions & 1 deletion src/utils/compatibleAPI.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@ function destroyStream(stream, suppress) {

/** @type {Record<number, string>} */
const statuses = {
400: "Bad Request",
403: "Forbidden",
404: "Not Found",
416: "Range Not Satisfiable",
500: "Internal Server Error",
Expand Down Expand Up @@ -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");

Expand Down
52 changes: 41 additions & 11 deletions src/utils/getFilenameFromUrl.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 =
Expand Down
85 changes: 81 additions & 4 deletions test/middleware.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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(
`<!DOCTYPE html>
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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(
"<!DOCTYPE html>\n" +
Expand Down Expand Up @@ -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(
"<!DOCTYPE html>\n" +
Expand Down Expand Up @@ -2575,6 +2632,7 @@ describe.each([
output: {
filename: "bundle.js",
path: path.resolve(__dirname, "./outputs/write-to-disk-true"),
publicPath: "/public/",
},
});

Expand All @@ -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);
Expand Down Expand Up @@ -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(`<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Error</title>
</head>
<body>
<pre>Forbidden</pre>
</body>
</html>`);
});
});

describe('should work with "true" value when the `output.clean` is `true`', () => {
Expand Down
5 changes: 1 addition & 4 deletions types/utils/getFilenameFromUrl.d.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
/// <reference types="node" />
export = getFilenameFromUrl;
/**
* @typedef {Object} Extra
* @property {import("fs").Stats=} stats
*/
/**
* @template {IncomingMessage} Request
* @template {ServerResponse} Response
Expand All @@ -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;

0 comments on commit e10008c

Please sign in to comment.