Improve nested path detection across app

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;

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(() => {

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'));
+    }
+  });
+});

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`
     );

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)
+  );
+}