Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refuse to overwrite any directory containing the home directory or the profile directory #68

27 changes: 26 additions & 1 deletion __test__/backup.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Backup } from "../src/Backup";
import * as fs from "fs-extra";
import * as path from "path";
import * as os from "os";
import { when } from "jest-when";
import { sevenZip } from "../src/sevenZip";
import joplin from "api";
Expand Down Expand Up @@ -34,6 +35,7 @@ const spyOnGlobalValue = jest.spyOn(joplin.settings, "globalValue");
const spyOnSettingsSetValue = jest
.spyOn(joplin.settings, "setValue")
.mockImplementation();
const homeDirMock = jest.spyOn(os, "homedir");

async function createTestStructure() {
const test = await getTestPaths();
Expand Down Expand Up @@ -180,7 +182,7 @@ describe("Backup", function () {
});

it(`relative paths`, async () => {
const backupPath = "../";
const backupPath = "../foo";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, the backupPath was a parent directory of the test profile directory. As such, the relative path has been changed.

/* prettier-ignore */
when(spyOnsSettingsValue)
.calledWith("path").mockImplementation(() => Promise.resolve(backupPath));
Expand All @@ -192,6 +194,29 @@ describe("Backup", function () {
expect(backup.log.error).toHaveBeenCalledTimes(0);
expect(backup.log.warn).toHaveBeenCalledTimes(0);
});

it.each([
os.homedir(),
path.dirname(os.homedir()),
path.join(os.homedir(), "Desktop"),
path.join(os.homedir(), "Documents"),

// Avoid including system-specific paths here. For example,
// testing with "C:\Windows" fails on POSIX systems because it is interpreted
// as a relative path.
])(
"should not allow backup path (%s) to be an important system directory",
async (path) => {
when(spyOnsSettingsValue)
.calledWith("path")
.mockImplementation(() => Promise.resolve(path));
backup.createSubfolder = false;

await backup.loadBackupPath();

expect(backup.backupBasePath).toBe(null);
}
);
});

describe("Div", function () {
Expand Down
40 changes: 40 additions & 0 deletions __test__/help.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as path from "path";
import { helper } from "../src/helper";

describe("Test helper", function () {
Expand Down Expand Up @@ -146,4 +147,43 @@ describe("Test helper", function () {
).toBe(testCase.expected);
}
});

test.each([
// Equality
["/tmp/this/is/a/test", "/tmp/this/is/a/test", true],
["/tmp/test", "/tmp/test///", true],

// Subdirectories
["/tmp", "/tmp/test", true],
["/tmp/", "/tmp/test", true],
["/tmp/", "/tmp/..test", true],
["/tmp/test", "/tmp/", false],

// Different directories
["/tmp/", "/tmp/../test", false],
["/tmp/te", "/tmp/test", false],
["a", "/a", false],
["/a/b", "/b/c", false],
])(
"isSubdirectoryOrEqual with POSIX paths (is %s the parent of %s?)",
(path1, path2, expected) => {
expect(helper.isSubdirectoryOrEqual(path1, path2, path.posix)).toBe(
expected
);
}
);

test.each([
["C:\\Users\\User\\", "C:\\Users\\User\\", true],
["D:\\Users\\User\\", "C:\\Users\\User\\", false],
["C:\\Users\\Userr\\", "C:\\Users\\User\\", false],
["C:\\Users\\User\\", "C:\\Users\\User\\.config\\joplin-desktop", true],
])(
"isSubdirectoryOrEqual with Windows paths (is %s the parent of %s?)",
(path1, path2, expected) => {
expect(helper.isSubdirectoryOrEqual(path1, path2, path.win32)).toBe(
expected
);
}
);
});
31 changes: 26 additions & 5 deletions src/Backup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import joplin from "api";
import * as path from "path";
import backupLogging from "electron-log";
import * as fs from "fs-extra";
import * as os from "os";
import { sevenZip } from "./sevenZip";
import * as moment from "moment";
import { helper } from "./helper";
Expand Down Expand Up @@ -288,11 +289,31 @@ class Backup {
}
}

if (path.normalize(profileDir) === this.backupBasePath) {
this.backupBasePath = null;
await this.showError(
i18n.__("msg.error.backupPathJoplinDir", path.normalize(profileDir))
);
// Creating a backup can overwrite the backup directory. Thus,
// we mark several system and user directories as not-overwritable.
const systemDirectories = [
profileDir,
os.homedir(),

path.join(os.homedir(), "Desktop"),
path.join(os.homedir(), "Documents"),
path.join(os.homedir(), "Downloads"),
path.join(os.homedir(), "Pictures"),
];

if (os.platform() === "win32") {
systemDirectories.push("C:\\Windows");
}

for (const systemDirectory of systemDirectories) {
if (helper.isSubdirectoryOrEqual(this.backupBasePath, systemDirectory)) {
const invalidBackupPath = this.backupBasePath;
this.backupBasePath = null;
await this.showError(
i18n.__("msg.error.backupPathContainsImportantDir", invalidBackupPath)
);
break;
}
}
}

Expand Down
25 changes: 25 additions & 0 deletions src/helper.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import joplin from "api";
import * as path from "path";

export namespace helper {
export async function validFileName(fileName: string) {
Expand Down Expand Up @@ -65,4 +66,28 @@ export namespace helper {

return result;
}

// Doesn't resolve simlinks
// See https://stackoverflow.com/questions/44892672/how-to-check-if-two-paths-are-the-same-in-npm
// for possible alternative implementations.
export function isSubdirectoryOrEqual(
parent: string,
possibleChild: string,

// Testing only
pathModule: typeof path = path
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not possible otherwise, via a spy or the like?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the difficulties here are that

  1. path.sep is a readonly-nonfunction property, so we can't jest.spyOn(path, 'resolve')
  2. path is imported at the top of src/helper.ts, so jest.doMock will only affect the import at the top of the file.
  3. jest.spyOn(path, 'resolve').mockImplementation((...args) => path.posix.resolve(...args)) causes infinite recursion on POSIX systems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The third issue above seems to be the most significant.

Even this alternative implementation causes a stack overflow:

https://github.com/personalizedrefrigerator/joplin-plugin-backup-fork/blob/a6f2b93bc7fa7e8f994c1b28d077a891bd61c1aa/__test__/help.test.ts#L196

) {
// Appending path.sep to handle this case:
// parent: /a/b/test
// possibleChild: /a/b/test2
// "/a/b/test2".startsWith("/a/b/test") -> true, but
// "/a/b/test2/".startsWith("/a/b/test/") -> false
//
// Note that .resolve removes trailing slashes.
//
const normalizedParent = pathModule.resolve(parent) + pathModule.sep;
const normalizedChild = pathModule.resolve(possibleChild) + pathModule.sep;

return normalizedChild.startsWith(normalizedParent);
}
}
1 change: 0 additions & 1 deletion src/locales/de_DE.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
"Backup": "Backup Fehler für %s: %s",
"fileCopy": "Fehler beim kopieren von Datei/Ordner in %s: %s",
"deleteFile": "Fehler beim löschen von Datei/Ordner in %s: %s",
"backupPathJoplinDir": "Als Sicherungs Pfad wurde das Joplin profile Verzeichniss (%s) ohne Unterordner ausgewählt, dies ist nicht erlaubt!",
"BackupSetNotSupportedChars": "Der Name des Backup-Sets enthält nicht zulässige Zeichen ( %s )!",
"passwordDoubleQuotes": "Das Passwort enthält \" (Doppelte Anführungszeichen), diese sind wegen eines Bugs nicht erlaubt. Der Passwortschutz für die Backups wurde deaktivert!"
}
Expand Down
2 changes: 1 addition & 1 deletion src/locales/en_US.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"Backup": "Backup error for %s: %s",
"fileCopy": "Error on file/folder copy in %s: %s",
"deleteFile": "Error on file/folder delete in %s: %s",
"backupPathJoplinDir": "The backup path is the Joplin profile directory (%s) without subfolders, this is not allowed!",
"backupPathContainsImportantDir": "The backup path is or contains an important directory (%s) that could be overwritten by a backup. Without enabling the subfolder setting, this is not allowed!",
"BackupSetNotSupportedChars": "Backup set name does contain not allowed characters ( %s )!",
"passwordDoubleQuotes": "Password contains \" (double quotes), these are not allowed because of a bug. Password protection for the backup is deactivated!"
}
Expand Down
Loading