From 42ac15d2ac9007a1a952fdf20921a00761e181f3 Mon Sep 17 00:00:00 2001 From: Henry H Date: Tue, 6 Feb 2024 09:13:59 -0800 Subject: [PATCH 1/6] Refuse to overwrite the home directory --- __test__/help.test.ts | 10 ++++++++++ src/Backup.ts | 14 ++++++++++---- src/helper.ts | 7 +++++++ src/locales/en_US.json | 1 + 4 files changed, 28 insertions(+), 4 deletions(-) diff --git a/__test__/help.test.ts b/__test__/help.test.ts index 8d364a0..ecbce06 100644 --- a/__test__/help.test.ts +++ b/__test__/help.test.ts @@ -146,4 +146,14 @@ describe("Test helper", function () { ).toBe(testCase.expected); } }); + + test.each([ + [ "/tmp/this/is/a/test", "/tmp/this/is/a/test", true ], + [ "/tmp/test", "/tmp/test///", true ], + [ "/tmp/te", "/tmp/test", false ], + [ "a", "/a", false ], + [ "/a/b", "/b/c", false ], + ])("pathsEquivalent (%s ?= %s)", (path1, path2, expected) => { + expect(helper.pathsEquivalent(path1, path2)).toBe(expected); + }); }); diff --git a/src/Backup.ts b/src/Backup.ts index 2c0c174..6e19282 100644 --- a/src/Backup.ts +++ b/src/Backup.ts @@ -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"; @@ -288,11 +289,16 @@ class Backup { } } - if (path.normalize(profileDir) === this.backupBasePath) { + const handleInvalidPath = async (errorId: string) => { + const invalidBackupPath = this.backupBasePath; this.backupBasePath = null; - await this.showError( - i18n.__("msg.error.backupPathJoplinDir", path.normalize(profileDir)) - ); + await this.showError(i18n.__(errorId, invalidBackupPath)); + }; + + if (helper.pathsEquivalent(profileDir, this.backupBasePath)) { + await handleInvalidPath("msg.error.backupPathJoplinDir"); + } else if (helper.pathsEquivalent(os.homedir(), this.backupBasePath)) { + await handleInvalidPath("msg.error.backupPathHomeDir"); } } diff --git a/src/helper.ts b/src/helper.ts index 3726fc2..cf2ea6f 100644 --- a/src/helper.ts +++ b/src/helper.ts @@ -1,4 +1,5 @@ import joplin from "api"; +import * as path from "path"; export namespace helper { export async function validFileName(fileName: string) { @@ -65,4 +66,10 @@ export namespace helper { return result; } + + export function pathsEquivalent(path1: string, path2: string) { + // We use `resolve` and not `normalize` because `resolve` removes trailing + // slashes, while `normalize` does not. + return path.resolve(path1) === path.resolve(path2); + } } diff --git a/src/locales/en_US.json b/src/locales/en_US.json index 79b6d55..aadf2aa 100644 --- a/src/locales/en_US.json +++ b/src/locales/en_US.json @@ -14,6 +14,7 @@ "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!", + "backupPathHomeDir": "The backup path is the home directory (%s). Either enable \"createSubfolders\" or choose a different backup directory.", "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!" } From 2f8ccdab529566988b1a840b0ae98c60bc79133c Mon Sep 17 00:00:00 2001 From: Henry H Date: Tue, 6 Feb 2024 10:18:24 -0800 Subject: [PATCH 2/6] Don't allow overwriting a parent of the profile directory --- __test__/backup.test.ts | 2 +- __test__/help.test.ts | 29 +++++++++++++++++++++-------- src/Backup.ts | 8 ++++---- src/helper.ts | 20 ++++++++++++++++---- src/locales/de_DE.json | 2 +- src/locales/en_US.json | 4 ++-- 6 files changed, 45 insertions(+), 20 deletions(-) diff --git a/__test__/backup.test.ts b/__test__/backup.test.ts index c557dde..5e96ec9 100644 --- a/__test__/backup.test.ts +++ b/__test__/backup.test.ts @@ -168,7 +168,7 @@ describe("Backup", function () { }); it(`relative paths`, async () => { - const backupPath = "../"; + const backupPath = "../foo"; /* prettier-ignore */ when(spyOnsSettingsValue) .calledWith("path").mockImplementation(() => Promise.resolve(backupPath)); diff --git a/__test__/help.test.ts b/__test__/help.test.ts index ecbce06..a8966df 100644 --- a/__test__/help.test.ts +++ b/__test__/help.test.ts @@ -148,12 +148,25 @@ describe("Test helper", function () { }); test.each([ - [ "/tmp/this/is/a/test", "/tmp/this/is/a/test", true ], - [ "/tmp/test", "/tmp/test///", true ], - [ "/tmp/te", "/tmp/test", false ], - [ "a", "/a", false ], - [ "/a/b", "/b/c", false ], - ])("pathsEquivalent (%s ?= %s)", (path1, path2, expected) => { - expect(helper.pathsEquivalent(path1, path2)).toBe(expected); - }); + // 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 (is %s the parent of %s?)", + (path1, path2, expected) => { + expect(helper.isSubdirectoryOrEqual(path1, path2)).toBe(expected); + } + ); }); diff --git a/src/Backup.ts b/src/Backup.ts index 6e19282..1d817e2 100644 --- a/src/Backup.ts +++ b/src/Backup.ts @@ -295,10 +295,10 @@ class Backup { await this.showError(i18n.__(errorId, invalidBackupPath)); }; - if (helper.pathsEquivalent(profileDir, this.backupBasePath)) { - await handleInvalidPath("msg.error.backupPathJoplinDir"); - } else if (helper.pathsEquivalent(os.homedir(), this.backupBasePath)) { - await handleInvalidPath("msg.error.backupPathHomeDir"); + if (helper.isSubdirectoryOrEqual(this.backupBasePath, os.homedir())) { + await handleInvalidPath("msg.error.backupPathContainsHomeDir"); + } else if (helper.isSubdirectoryOrEqual(this.backupBasePath, profileDir)) { + await handleInvalidPath("msg.error.backupPathContainsJoplinDir"); } } diff --git a/src/helper.ts b/src/helper.ts index cf2ea6f..971a49e 100644 --- a/src/helper.ts +++ b/src/helper.ts @@ -67,9 +67,21 @@ export namespace helper { return result; } - export function pathsEquivalent(path1: string, path2: string) { - // We use `resolve` and not `normalize` because `resolve` removes trailing - // slashes, while `normalize` does not. - return path.resolve(path1) === path.resolve(path2); + // 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) { + // 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 = path.resolve(parent) + path.sep; + const normalizedChild = path.resolve(possibleChild) + path.sep; + + return normalizedChild.startsWith(normalizedParent); } } diff --git a/src/locales/de_DE.json b/src/locales/de_DE.json index 9749df5..1f6b902 100644 --- a/src/locales/de_DE.json +++ b/src/locales/de_DE.json @@ -13,7 +13,7 @@ "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!", + "backupPathContainsJoplinDir": "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!" } diff --git a/src/locales/en_US.json b/src/locales/en_US.json index aadf2aa..50ad38e 100644 --- a/src/locales/en_US.json +++ b/src/locales/en_US.json @@ -13,8 +13,8 @@ "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!", - "backupPathHomeDir": "The backup path is the home directory (%s). Either enable \"createSubfolders\" or choose a different backup directory.", + "backupPathContainsJoplinDir": "The backup path is or contains the Joplin profile directory (%s) without subfolders, this is not allowed!", + "backupPathContainsHomeDir": "The backup path is or contains the home directory (%s). 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!" } From 09f6cef9e7e4132c0a646623fb2dad971da973c0 Mon Sep 17 00:00:00 2001 From: Henry H Date: Tue, 6 Feb 2024 10:45:31 -0800 Subject: [PATCH 3/6] More tests --- __test__/help.test.ts | 21 +++++++++++++++++++-- src/helper.ts | 12 +++++++++--- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/__test__/help.test.ts b/__test__/help.test.ts index a8966df..9057163 100644 --- a/__test__/help.test.ts +++ b/__test__/help.test.ts @@ -1,3 +1,4 @@ +import * as path from "path"; import { helper } from "../src/helper"; describe("Test helper", function () { @@ -164,9 +165,25 @@ describe("Test helper", function () { ["a", "/a", false], ["/a/b", "/b/c", false], ])( - "isSubdirectoryOrEqual (is %s the parent of %s?)", + "isSubdirectoryOrEqual with POSIX paths(is %s the parent of %s?)", (path1, path2, expected) => { - expect(helper.isSubdirectoryOrEqual(path1, path2)).toBe(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 + ); } ); }); diff --git a/src/helper.ts b/src/helper.ts index 971a49e..45eba0c 100644 --- a/src/helper.ts +++ b/src/helper.ts @@ -70,7 +70,13 @@ export namespace helper { // 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) { + export function isSubdirectoryOrEqual( + parent: string, + possibleChild: string, + + // Testing only + pathModule: typeof path = path + ) { // Appending path.sep to handle this case: // parent: /a/b/test // possibleChild: /a/b/test2 @@ -79,8 +85,8 @@ export namespace helper { // // Note that .resolve removes trailing slashes. // - const normalizedParent = path.resolve(parent) + path.sep; - const normalizedChild = path.resolve(possibleChild) + path.sep; + const normalizedParent = pathModule.resolve(parent) + pathModule.sep; + const normalizedChild = pathModule.resolve(possibleChild) + pathModule.sep; return normalizedChild.startsWith(normalizedParent); } From a4fd675653677729984bcad869d53929bf700a5d Mon Sep 17 00:00:00 2001 From: Henry H Date: Tue, 6 Feb 2024 11:32:07 -0800 Subject: [PATCH 4/6] Add missing space --- __test__/help.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/__test__/help.test.ts b/__test__/help.test.ts index 9057163..69a9ce2 100644 --- a/__test__/help.test.ts +++ b/__test__/help.test.ts @@ -165,7 +165,7 @@ describe("Test helper", function () { ["a", "/a", false], ["/a/b", "/b/c", false], ])( - "isSubdirectoryOrEqual with POSIX paths(is %s the parent of %s?)", + "isSubdirectoryOrEqual with POSIX paths (is %s the parent of %s?)", (path1, path2, expected) => { expect(helper.isSubdirectoryOrEqual(path1, path2, path.posix)).toBe( expected From 9955a8da36c88842118767dd9bc0ab32ba3bc011 Mon Sep 17 00:00:00 2001 From: Henry H Date: Mon, 19 Feb 2024 08:29:09 -0800 Subject: [PATCH 5/6] Add additional directories to disallow list --- __test__/backup.test.ts | 21 +++++++++++++++++++++ src/Backup.ts | 30 +++++++++++++++++++++--------- src/locales/de_DE.json | 1 - src/locales/en_US.json | 3 +-- 4 files changed, 43 insertions(+), 12 deletions(-) diff --git a/__test__/backup.test.ts b/__test__/backup.test.ts index 5caf4c9..c71831f 100644 --- a/__test__/backup.test.ts +++ b/__test__/backup.test.ts @@ -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"; @@ -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(); @@ -192,6 +194,25 @@ 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"), + ])( + "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 () { diff --git a/src/Backup.ts b/src/Backup.ts index 4460112..5738c11 100644 --- a/src/Backup.ts +++ b/src/Backup.ts @@ -289,16 +289,28 @@ class Backup { } } - const handleInvalidPath = async (errorId: string) => { - const invalidBackupPath = this.backupBasePath; - this.backupBasePath = null; - await this.showError(i18n.__(errorId, invalidBackupPath)); - }; + // 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"), + "C:\\Windows", + ]; - if (helper.isSubdirectoryOrEqual(this.backupBasePath, os.homedir())) { - await handleInvalidPath("msg.error.backupPathContainsHomeDir"); - } else if (helper.isSubdirectoryOrEqual(this.backupBasePath, profileDir)) { - await handleInvalidPath("msg.error.backupPathContainsJoplinDir"); + 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; + } } } diff --git a/src/locales/de_DE.json b/src/locales/de_DE.json index 0cb7454..340eae5 100644 --- a/src/locales/de_DE.json +++ b/src/locales/de_DE.json @@ -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", - "backupPathContainsJoplinDir": "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!" } diff --git a/src/locales/en_US.json b/src/locales/en_US.json index 1f535eb..e5ed212 100644 --- a/src/locales/en_US.json +++ b/src/locales/en_US.json @@ -13,8 +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", - "backupPathContainsJoplinDir": "The backup path is or contains the Joplin profile directory (%s) without subfolders, this is not allowed!", - "backupPathContainsHomeDir": "The backup path is or contains the home directory (%s). Without enabling the subfolder setting, 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!" } From 029a6b7a0e0df761b8a41bb194b1a38b52aee624 Mon Sep 17 00:00:00 2001 From: Henry H Date: Mon, 19 Feb 2024 08:40:32 -0800 Subject: [PATCH 6/6] Add comment to tests, only add Windows system directoies to disallow list on Windows --- __test__/backup.test.ts | 4 ++++ src/Backup.ts | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/__test__/backup.test.ts b/__test__/backup.test.ts index c71831f..788c578 100644 --- a/__test__/backup.test.ts +++ b/__test__/backup.test.ts @@ -200,6 +200,10 @@ describe("Backup", function () { 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) => { diff --git a/src/Backup.ts b/src/Backup.ts index 5738c11..aeebbcc 100644 --- a/src/Backup.ts +++ b/src/Backup.ts @@ -299,9 +299,12 @@ class Backup { path.join(os.homedir(), "Documents"), path.join(os.homedir(), "Downloads"), path.join(os.homedir(), "Pictures"), - "C:\\Windows", ]; + if (os.platform() === "win32") { + systemDirectories.push("C:\\Windows"); + } + for (const systemDirectory of systemDirectories) { if (helper.isSubdirectoryOrEqual(this.backupBasePath, systemDirectory)) { const invalidBackupPath = this.backupBasePath;