diff --git a/app/attachments.js b/app/attachments.js index ef2191d696..c5a6f73bb9 100644 --- a/app/attachments.js +++ b/app/attachments.js @@ -10,6 +10,7 @@ const toArrayBuffer = require('to-arraybuffer'); const { map, isArrayBuffer, isString } = require('lodash'); const normalizePath = require('normalize-path'); const sanitizeFilename = require('sanitize-filename'); +const { isPathInside } = require('../ts/util/isPathInside'); const getGuid = require('uuid/v4'); let xattr; @@ -26,6 +27,8 @@ const STICKER_PATH = 'stickers.noindex'; const TEMP_PATH = 'temp'; const DRAFT_PATH = 'drafts.noindex'; +const getApp = () => app || remote.app; + exports.getAllAttachments = async userDataPath => { const dir = exports.getPath(userDataPath); const pattern = normalizePath(path.join(dir, '**', '*')); @@ -113,7 +116,7 @@ exports.createReader = root => { const absolutePath = path.join(root, relativePath); const normalized = path.normalize(absolutePath); - if (!normalized.startsWith(root)) { + if (!isPathInside(normalized, root)) { throw new Error('Invalid relative path'); } const buffer = await fse.readFile(normalized); @@ -133,7 +136,7 @@ exports.createDoesExist = root => { const absolutePath = path.join(root, relativePath); const normalized = path.normalize(absolutePath); - if (!normalized.startsWith(root)) { + if (!isPathInside(normalized, root)) { throw new Error('Invalid relative path'); } try { @@ -150,16 +153,24 @@ exports.copyIntoAttachmentsDirectory = root => { throw new TypeError("'root' must be a path"); } + const userDataPath = getApp().getPath('userData'); + return async sourcePath => { if (!isString(sourcePath)) { throw new TypeError('sourcePath must be a string'); } + if (!isPathInside(sourcePath, userDataPath)) { + throw new Error( + "'sourcePath' must be relative to the user config directory" + ); + } + const name = exports.createName(); const relativePath = exports.getRelativePath(name); const absolutePath = path.join(root, relativePath); const normalized = path.normalize(absolutePath); - if (!normalized.startsWith(root)) { + if (!isPathInside(normalized, root)) { throw new Error('Invalid relative path'); } @@ -170,7 +181,7 @@ exports.copyIntoAttachmentsDirectory = root => { }; exports.writeToDownloads = async ({ data, name }) => { - const appToUse = app || remote.app; + const appToUse = getApp(); const downloadsPath = appToUse.getPath('downloads') || appToUse.getPath('home'); const sanitized = sanitizeFilename(name); @@ -189,7 +200,7 @@ exports.writeToDownloads = async ({ data, name }) => { const target = path.join(downloadsPath, candidateName); const normalized = path.normalize(target); - if (!normalized.startsWith(downloadsPath)) { + if (!isPathInside(normalized, downloadsPath)) { throw new Error('Invalid filename!'); } @@ -223,14 +234,14 @@ async function writeWithAttributes(target, data) { exports.openFileInDownloads = async name => { const shellToUse = shell || remote.shell; - const appToUse = app || remote.app; + const appToUse = getApp(); const downloadsPath = appToUse.getPath('downloads') || appToUse.getPath('home'); const target = path.join(downloadsPath, name); const normalized = path.normalize(target); - if (!normalized.startsWith(downloadsPath)) { + if (!isPathInside(normalized, downloadsPath)) { throw new Error('Invalid filename!'); } @@ -310,7 +321,7 @@ exports.createWriterForExisting = root => { const buffer = Buffer.from(arrayBuffer); const absolutePath = path.join(root, relativePath); const normalized = path.normalize(absolutePath); - if (!normalized.startsWith(root)) { + if (!isPathInside(normalized, root)) { throw new Error('Invalid relative path'); } @@ -335,7 +346,7 @@ exports.createDeleter = root => { const absolutePath = path.join(root, relativePath); const normalized = path.normalize(absolutePath); - if (!normalized.startsWith(root)) { + if (!isPathInside(normalized, root)) { throw new Error('Invalid relative path'); } await fse.remove(absolutePath); @@ -402,7 +413,7 @@ exports.getRelativePath = name => { exports.createAbsolutePathGetter = rootPath => relativePath => { const absolutePath = path.join(rootPath, relativePath); const normalized = path.normalize(absolutePath); - if (!normalized.startsWith(rootPath)) { + if (!isPathInside(normalized, rootPath)) { throw new Error('Invalid relative path'); } return normalized; diff --git a/test/app/attachments_test.js b/test/app/attachments_test.js index 4b4c0c9661..3a49379979 100644 --- a/test/app/attachments_test.js +++ b/test/app/attachments_test.js @@ -1,7 +1,9 @@ +const fs = require('fs'); const fse = require('fs-extra'); const path = require('path'); const tmp = require('tmp'); const { assert } = require('chai'); +const { app } = require('electron'); const Attachments = require('../../app/attachments'); const { @@ -152,6 +154,91 @@ describe('Attachments', () => { }); }); + describe('copyIntoAttachmentsDirectory', () => { + // These tests use the `userData` path. In `electron-mocha`, these are temporary + // directories; no need to be concerned about messing with the "real" directory. + before(function thisNeeded() { + this.filesToRemove = []; + this.getFakeAttachmentsDirectory = () => { + const result = path.join( + app.getPath('userData'), + `fake-attachments-${Date.now()}-${Math.random() + .toString() + .substring(2)}` + ); + this.filesToRemove.push(result); + return result; + }; + this.getTempFile = () => { + const result = tmp.fileSync().name; + this.filesToRemove.push(result); + return result; + }; + }); + + after(async function thisNeeded() { + await Promise.all( + this.filesToRemove.map(toRemove => fse.remove(toRemove)) + ); + }); + + it('throws if passed a non-string', () => { + assert.throws(() => { + Attachments.copyIntoAttachmentsDirectory(1234); + }, TypeError); + assert.throws(() => { + Attachments.copyIntoAttachmentsDirectory(null); + }, TypeError); + }); + + it('returns a function that rejects if the source path is not a string', async function thisNeeded() { + const copier = Attachments.copyIntoAttachmentsDirectory( + await this.getFakeAttachmentsDirectory() + ); + return copier(123) + .then(() => { + assert.fail('This should never be run'); + }) + .catch(err => { + assert.instanceOf(err, TypeError); + }); + }); + + it('returns a function that rejects if the source path is not in the user config directory', async function thisNeeded() { + const copier = Attachments.copyIntoAttachmentsDirectory( + await this.getFakeAttachmentsDirectory() + ); + return copier(this.getTempFile()) + .then(() => { + assert.fail('This should never be run'); + }) + .catch(err => { + assert.instanceOf(err, Error); + assert.strictEqual( + err.message, + "'sourcePath' must be relative to the user config directory" + ); + }); + }); + + it('returns a function that copies the source path into the attachments directory', async function thisNeeded() { + const attachmentsPath = await this.getFakeAttachmentsDirectory(); + const someOtherPath = path.join(app.getPath('userData'), 'somethingElse'); + await fse.outputFile(someOtherPath, 'hello world'); + this.filesToRemove.push(someOtherPath); + + const copier = Attachments.copyIntoAttachmentsDirectory(attachmentsPath); + const relativePath = await copier(someOtherPath); + + const absolutePath = path.join(attachmentsPath, relativePath); + assert.notEqual(someOtherPath, absolutePath); + assert.strictEqual( + await fs.promises.readFile(absolutePath, 'utf8'), + 'hello world' + ); + }); + }); + describe('createDeleter', () => { let tempRootDirectory = null; before(() => { diff --git a/ts/test/util/isPathInside_test.ts b/ts/test/util/isPathInside_test.ts new file mode 100644 index 0000000000..2ec8edaddf --- /dev/null +++ b/ts/test/util/isPathInside_test.ts @@ -0,0 +1,42 @@ +import { assert } from 'chai'; + +import { isPathInside } from '../../util/isPathInside'; + +describe('isPathInside', () => { + it('returns false if the child path is not inside the parent path', () => { + assert.isFalse(isPathInside('x', 'a/b')); + assert.isFalse(isPathInside('a/b', '/a/b')); + assert.isFalse(isPathInside('/a/b', 'a/b')); + assert.isFalse(isPathInside('/x', '/a/b')); + assert.isFalse(isPathInside('/x/y', '/a/b')); + assert.isFalse(isPathInside('/a/x', '/a/b')); + assert.isFalse(isPathInside('/a/bad', '/a/b')); + assert.isFalse(isPathInside('/a/x', '/a/b')); + assert.isFalse(isPathInside('/a/b', '/a/b')); + assert.isFalse(isPathInside('/a/b/c/..', '/a/b')); + assert.isFalse(isPathInside('/', '/')); + assert.isFalse(isPathInside('/x/..', '/')); + + if (process.platform === 'win32') { + assert.isFalse(isPathInside('C:\\a\\x\\y', 'C:\\a\\b')); + assert.isFalse(isPathInside('D:\\a\\b\\c', 'C:\\a\\b')); + } + }); + + it('returns true if the child path is inside the parent path', () => { + assert.isTrue(isPathInside('a/b/c', 'a/b')); + assert.isTrue(isPathInside('a/b/c/d', 'a/b')); + assert.isTrue(isPathInside('/a/b/c', '/a/b')); + assert.isTrue(isPathInside('/a/b/c', '/a/b/')); + assert.isTrue(isPathInside('/a/b/c/', '/a/b')); + assert.isTrue(isPathInside('/a/b/c/', '/a/b/')); + assert.isTrue(isPathInside('/a/b/c/d', '/a/b')); + assert.isTrue(isPathInside('/a/b/c/d/..', '/a/b')); + assert.isTrue(isPathInside('/x/y/z/z/y', '/')); + assert.isTrue(isPathInside('x/y/z/z/y', '/')); + + if (process.platform === 'win32') { + assert.isTrue(isPathInside('C:\\a\\b\\c', 'C:\\a\\b')); + } + }); +}); diff --git a/ts/updater/common.ts b/ts/updater/common.ts index 3d5119b62f..cc1ef4fbaf 100644 --- a/ts/updater/common.ts +++ b/ts/updater/common.ts @@ -26,6 +26,7 @@ import { Dialogs } from '../types/Dialogs'; // @ts-ignore import * as packageJson from '../../package.json'; import { getSignatureFileName } from './signature'; +import { isPathInside } from '../util/isPathInside'; import { LocaleType } from '../types/I18N'; import { LoggerType } from '../types/Logging'; @@ -71,7 +72,7 @@ export async function checkForUpdates( export function validatePath(basePath: string, targetPath: string) { const normalized = normalize(targetPath); - if (!normalized.startsWith(basePath)) { + if (!isPathInside(normalized, basePath)) { throw new Error( `validatePath: Path ${normalized} is not under base path ${basePath}` ); @@ -352,7 +353,7 @@ export async function deleteTempDir(targetDir: string) { } const baseTempDir = getBaseTempDir(); - if (!targetDir.startsWith(baseTempDir)) { + if (!isPathInside(targetDir, baseTempDir)) { throw new Error( `deleteTempDir: Cannot delete path '${targetDir}' since it is not within base temp dir` ); diff --git a/ts/util/isPathInside.ts b/ts/util/isPathInside.ts new file mode 100644 index 0000000000..98698092ab --- /dev/null +++ b/ts/util/isPathInside.ts @@ -0,0 +1,16 @@ +// This is inspired by the `is-path-inside` module on npm. +import * as path from 'path'; + +export function isPathInside(childPath: string, parentPath: string): boolean { + const childPathResolved = path.resolve(childPath); + + let parentPathResolved = path.resolve(parentPath); + if (!parentPathResolved.endsWith(path.sep)) { + parentPathResolved += path.sep; + } + + return ( + childPathResolved !== parentPathResolved && + childPathResolved.startsWith(parentPathResolved) + ); +}