From 8e89d84edac90e397f2d60ffdd7d3bdf061a8cbe Mon Sep 17 00:00:00 2001 From: Jake Jarvis Date: Thu, 7 Oct 2021 11:07:31 -0400 Subject: [PATCH] Stricter paths to prevent traversal & remove custom tempDir option --- .gitignore | 1 + README.md | 9 +-------- index.d.ts | 12 ++---------- index.js | 35 +++++++++++++++++------------------ test/index.js | 35 ++++++++++++++++++++++++----------- 5 files changed, 45 insertions(+), 47 deletions(-) diff --git a/.gitignore b/.gitignore index c2658d7..c7ed7ea 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,2 @@ node_modules/ +test/temp/ diff --git a/README.md b/README.md index 21b8e2d..46777ea 100644 --- a/README.md +++ b/README.md @@ -72,19 +72,12 @@ Default: `false` Use [`decompress`](https://github.com/kevva/decompress) to extract the final download to the destination directory (assuming it's a `.zip`, `.tar`, `.tar.gz`, etc.). -##### tempDir - -Type: `string`\ -Default: [`tempy.directory()`](https://github.com/sindresorhus/tempy#tempydirectoryoptions) - -Path to temporary directory for unverified and/or unextracted downloads. Automatically generated if not set (recommended). If set manually, the directory isn't purged upon finishing for security reasons. - ##### destDir Type: `string`\ Default: `"./downloads"` -Full path or directory name relative to module to store the validated download. +Directory path relative to module where the validated download is saved or extracted. **Must be located within `process.cwd()` for security reasons.** ##### cleanDestDir diff --git a/index.d.ts b/index.d.ts index 7b026bc..8536e75 100644 --- a/index.d.ts +++ b/index.d.ts @@ -16,16 +16,8 @@ export interface Options { readonly extract?: boolean; /** - * Path to temporary directory for unverified and/or unextracted downloads. - * Automatically generated if not set (recommended). - * - * @default `tempy.directory()` - */ - readonly tempDir?: string; - - /** - * Full path or directory name relative to module to store the validated - * download. + * Directory path relative to module where the validated download is saved or + * extracted. Must be located within `process.cwd()` for security reasons. * * @default "./downloads" */ diff --git a/index.js b/index.js index d757970..a52ae66 100644 --- a/index.js +++ b/index.js @@ -12,34 +12,35 @@ export default async function downloader(downloadUrl, checksumUrl, options) { // intialize options if none are set options = options || {}; - // don't delete the temp dir if set manually and dir exists - let deleteTempDir = true; - if (options.tempDir && fs.pathExistsSync(options.tempDir)) { - deleteTempDir = false; - } - // normalize options and set defaults options = { filename: options.filename || urlParse(downloadUrl).pathname.split("/").pop(), extract: !!options.extract, - tempDir: options.tempDir ? path.resolve(process.cwd(), options.tempDir) : tempy.directory(), - destDir: options.destDir ? path.resolve(process.cwd(), options.destDir) : path.resolve(process.cwd(), "download"), + destDir: options.destDir ? path.resolve(process.cwd(), options.destDir) : path.resolve(process.cwd(), "downloads"), cleanDestDir: !!options.cleanDestDir, algorithm: options.algorithm || "sha256", encoding: options.encoding || "binary", }; + // throw an error if destDir is outside of the module to prevent path traversal for security reasons + if (!options.destDir.startsWith(process.cwd())) { + throw new Error(`destDir must be located within '${process.cwd()}', it's currently set to '${options.destDir}'.`); + } + + // initialize temporary directory + const tempDir = tempy.directory(); + try { // simultaneously download the desired file and its checksums await Promise.all([ - downloadFile(downloadUrl, path.join(options.tempDir, options.filename)), - downloadFile(checksumUrl, path.join(options.tempDir, "checksums.txt")), + downloadFile(downloadUrl, path.join(tempDir, options.filename)), + downloadFile(checksumUrl, path.join(tempDir, "checksums.txt")), ]); // validate the checksum of the download - if (await checkChecksum(options.tempDir, options.filename, "checksums.txt", options.algorithm, options.encoding)) { + if (await checkChecksum(tempDir, options.filename, "checksums.txt", options.algorithm, options.encoding)) { // optionally clear the target directory of existing files - if (options.cleanDestDir) { + if (options.cleanDestDir && fs.existsSync(options.destDir)) { await fs.remove(options.destDir); } @@ -48,21 +49,19 @@ export default async function downloader(downloadUrl, checksumUrl, options) { if (options.extract) { // decompress download and move resulting files to final destination - await decompress(path.join(options.tempDir, options.filename), options.destDir); + await decompress(path.join(tempDir, options.filename), options.destDir); return options.destDir; } else { // move verified download to final destination as-is - await fs.copy(path.join(options.tempDir, options.filename), path.join(options.destDir, options.filename)); + await fs.copy(path.join(tempDir, options.filename), path.join(options.destDir, options.filename)); return path.join(options.destDir, options.filename); } } else { throw new Error(`Invalid checksum for ${options.filename}.`); } } finally { - // delete temporary directory (except for edge cases above) - if (deleteTempDir) { - await fs.remove(options.tempDir); - } + // delete temporary directory + await fs.remove(tempDir); } } diff --git a/test/index.js b/test/index.js index f427aad..2458ce3 100644 --- a/test/index.js +++ b/test/index.js @@ -1,50 +1,63 @@ /* eslint-env mocha */ import fs from "fs-extra"; import path from "path"; -import tempy from "tempy"; +import { fileURLToPath } from "url"; import { expect } from "chai"; import downloader from "../index.js"; +// https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c#what-do-i-use-instead-of-__dirname-and-__filename +const __dirname = path.dirname(fileURLToPath(import.meta.url)); + it("verified checksum, hugo.exe was extracted", async function () { this.timeout(30000); // increase timeout to an excessive 30 seconds for CI - const outDir = path.join(tempy.directory()); - await downloader( "https://github.com/gohugoio/hugo/releases/download/v0.88.1/hugo_extended_0.88.1_Windows-64bit.zip", "https://github.com/gohugoio/hugo/releases/download/v0.88.1/hugo_0.88.1_checksums.txt", { - destDir: outDir, + destDir: path.join(__dirname, "temp"), algorithm: "sha256", encoding: "binary", extract: true, }, ); - expect(fs.existsSync(path.join(outDir, "hugo.exe"))).to.be.true; + expect(fs.existsSync(path.join(__dirname, "temp", "hugo.exe"))).to.be.true; - fs.removeSync(outDir); + // clean up + fs.removeSync(path.join(__dirname, "temp")); }); it("incorrect checksum, not extracted", async function () { this.timeout(30000); // increase timeout to an excessive 30 seconds for CI - const outDir = path.join(tempy.directory()); - expect(async () => downloader( // download mismatching versions to trigger error "https://github.com/gohugoio/hugo/releases/download/v0.88.0/hugo_0.88.0_Windows-64bit.zip", "https://github.com/gohugoio/hugo/releases/download/v0.88.1/hugo_0.88.1_checksums.txt", { - destDir: outDir, + destDir: path.join(__dirname, "temp"), algorithm: "sha256", encoding: "binary", extract: false, }, )).to.throw; - expect(fs.existsSync(path.join(outDir, "hugo.exe"))).to.be.false; + expect(fs.existsSync(path.join(__dirname, "temp", "hugo.exe"))).to.be.false; - fs.removeSync(outDir); + // clean up + fs.removeSync(path.join(__dirname, "temp")); +}); + +it("destDir located outside of module, throw error", async function () { + this.timeout(30000); // increase timeout to an excessive 30 seconds for CI + + expect(async () => downloader( + "https://github.com/gohugoio/hugo/releases/download/v0.88.1/hugo_0.88.1_Windows-64bit.zip", + "https://github.com/gohugoio/hugo/releases/download/v0.88.1/hugo_0.88.1_checksums.txt", + { + destDir: "../vendor", // invalid path + }, + )).to.throw; });