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

2 changes: 1 addition & 1 deletion __test__/backup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,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 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
);
}
);
});
14 changes: 10 additions & 4 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,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.isSubdirectoryOrEqual(this.backupBasePath, os.homedir())) {
await handleInvalidPath("msg.error.backupPathContainsHomeDir");
} else if (helper.isSubdirectoryOrEqual(this.backupBasePath, profileDir)) {
await handleInvalidPath("msg.error.backupPathContainsJoplinDir");
}
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps we should also check a few other directories here.

  • Desktop
  • OS Folder (Like c:\windows, or other system folders)

Or perhaps the entire deletion routine should be reworked. So that it only deletes its own files ...

I think I'll have a look at it when this PR has been merged. Fits in with another idea I have.

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've changed this!

I'm not sure how necessary system folders are though — when I set the backup directory to C:\ProgramFiles or C:\Windows, I get an "operation not permitted" error.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes when the user runs his system with UAC and or a administrator user.

}

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);
}
}
2 changes: 1 addition & 1 deletion src/locales/de_DE.json
Original file line number Diff line number Diff line change
Expand Up @@ -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!"
}
Expand Down
3 changes: 2 additions & 1 deletion src/locales/en_US.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +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!",
"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!"
}
Expand Down
Loading