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

Conversation

personalizedrefrigerator
Copy link
Contributor

@personalizedrefrigerator personalizedrefrigerator commented Feb 6, 2024

Summary

Prevents the plugin from overwriting os.homedir() or the Joplin profile directory.

Rationale

laurent22/joplin#9857 suggests backing up to a subdirectory of the home directory by default. One way to do this is to set Simple Backup's path setting to the HOME directory.

Without additional checks, this could be dangerous if a user unchecks the "create subfolder" option.

Alternatives

  • Don't overwrite any directory that contains the profile directory

Testing

This pull request has a related automated test. However, this only tests one part of this pull request.

To test the pull request manually,

  1. Create a new user account
  2. Build the plugin from this branch and load it as a development plugin
  3. Start Joplin
  4. Set the backup path to the home directory
  5. Create a backup
  6. Disable subfolder creation
  7. Try to create a backup
  8. Set the backup path to the .config directory (e.g. /home/user/.config/)
  9. Try to create a backup
  10. Set the backup path to the profile directory
  11. Try to create a backup

This has been tested successfully with commit 2f8ccda on OpenSUSE 20240202 with Joplin 2.14.12.

@@ -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.

@personalizedrefrigerator personalizedrefrigerator changed the title Refuse to overwrite the any directory containing the home directory or the profile directory Refuse to overwrite any directory containing the home directory or the profile directory Feb 7, 2024
@JackGruber
Copy link
Owner

Just so I understand it correctly, the PR is supposed to prevent backups from being made without a subfolder in .config.

@personalizedrefrigerator
Copy link
Contributor Author

personalizedrefrigerator commented Feb 17, 2024

Just so I understand it correctly, the PR is supposed to prevent backups from being made without a subfolder in .config.

The goal of this pull request is to prevent the plugin from deleting Joplin's profile directory or the user's home directory.

Example 1

Suppose that

  1. A user disables "create subfolder"
  2. Some time later, the user sets the backup location to /home/usernamehere/ (the home directory)

This change would then prevent the plugin from overwriting /home/usernamehere/.

Example 2

Suppose that

  1. A user installs Joplin Portable on a USB drive
    • This would put the JoplinProfile directory somewhere on the USB drive
  2. The user sets the backup directory to the root of the USB drive (e.g. D:\) and disables "create subfolder"
    • This might happen if, for example, the user selects the wrong backup location
  3. The user tries to create a backup

Because the root directory of the drive contains the profile directory, this change should prevent the contents of the USB drive from being overwritten.

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

src/Backup.ts Outdated
Comment on lines 298 to 302
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.

@JackGruber JackGruber merged commit 7cd50d0 into JackGruber:develop Feb 21, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants