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

Desktop: #11073: Fix directory being considered unsafe because of naming #11504

Closed
wants to merge 10 commits into from
1 change: 1 addition & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ packages/app-desktop/InteropServiceHelper.js
packages/app-desktop/app.reducer.test.js
packages/app-desktop/app.reducer.js
packages/app-desktop/app.js
packages/app-desktop/bridge.test.js
packages/app-desktop/bridge.js
packages/app-desktop/checkForUpdates.js
packages/app-desktop/commands/copyDevCommand.js
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ packages/app-desktop/InteropServiceHelper.js
packages/app-desktop/app.reducer.test.js
packages/app-desktop/app.reducer.js
packages/app-desktop/app.js
packages/app-desktop/bridge.test.js
packages/app-desktop/bridge.js
packages/app-desktop/checkForUpdates.js
packages/app-desktop/commands/copyDevCommand.js
Expand Down
53 changes: 53 additions & 0 deletions packages/app-desktop/bridge.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { createTempDir } from '@joplin/lib/testing/test-utils';
import bridge, { initBridge } from './bridge';
import { join } from 'path';
import { mkdirp } from 'fs-extra';
import { dialog, shell } from 'electron';
import ElectronAppWrapper from './ElectronAppWrapper';

jest.mock('@sentry/electron/main', () => ({
init: () => { },
}));
jest.mock('electron', () => ({
dialog: {
showMessageBox: jest.fn(() => Promise.resolve()),
},
shell: {
openExternal: jest.fn(),
openPath: jest.fn(),
},
}));
describe('bridge', () => {

beforeAll(async () => {
await initBridge({
activeWindow: () => {},
} as ElectronAppWrapper, '', '', '', false);
});

it('should open directory if name has no extension', async () => {
const unsafeExtension = 'name';

const tempDir = await createTempDir();
const fullPath = join(tempDir, unsafeExtension);
await mkdirp(fullPath);

await bridge().openItem(fullPath);
expect(dialog.showMessageBox).toHaveBeenCalledTimes(0);
expect(shell.openPath).toHaveBeenCalledTimes(1);
expect(shell.openPath).toHaveBeenLastCalledWith(fullPath);
});

it('should show warning if opening directory has a unsafe extension', async () => {
const unsafeExtension = 'name.unsafe';
dialog.showMessageBox = jest.fn().mockReturnValue({ response: 1 });

const tempDir = await createTempDir();
const fullPath = join(tempDir, unsafeExtension);
await mkdirp(fullPath);

await bridge().openItem(fullPath);
expect(dialog.showMessageBox).toHaveBeenCalledTimes(1);
expect(dialog.showMessageBox).toHaveBeenCalledWith(undefined, { 'buttons': ['Cancel', 'Learn more', 'Open it'], 'checkboxLabel': 'Always open ".unsafe" files without asking.', 'message': 'This file seems to be a directory, but Joplin doesn\'t recognise the ".unsafe" extension. Opening this could be dangerous. What would you like to do?', 'title': 'Unknown file type', 'type': 'warning' });
});
});
18 changes: 14 additions & 4 deletions packages/app-desktop/bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import * as Sentry from '@sentry/electron/main';
import { ErrorEvent } from '@sentry/types/types';
import { homedir } from 'os';
import { msleep } from '@joplin/utils/time';
import { pathExists, pathExistsSync, writeFileSync } from 'fs-extra';
import { pathExists, pathExistsSync, stat, writeFileSync } from 'fs-extra';
import { extname, normalize } from 'path';
import isSafeToOpen from './utils/isSafeToOpen';
import { closeSync, openSync, readSync, statSync } from 'fs';
Expand Down Expand Up @@ -431,16 +431,26 @@ export class Bridge {
if (await pathExists(fullPath)) {
const fileExtension = extname(fullPath);
const userAllowedExtension = this.extraAllowedOpenExtensions.includes(fileExtension);
if (userAllowedExtension || await isSafeToOpen(fullPath)) {

if (userAllowedExtension || (await isSafeToOpen(fullPath))) {
return shell.openPath(fullPath);
} else {
const allowOpenId = 2;
const learnMoreId = 1;
const fileExtensionDescription = JSON.stringify(fileExtension);

let warningMessage = _('Joplin doesn\'t recognise the %s extension. Opening this file could be dangerous. What would you like to do?', fileExtensionDescription);

// We recheck if it is a directory because there is a possibility that the file could be dangerous and a directory
// but we would like to not confuse users about what link is being open
const isDirectory = (await stat(fullPath)).isDirectory();
if (isDirectory) {
warningMessage = _('This file seems to be a directory, but Joplin doesn\'t recognise the %s extension. Opening this could be dangerous. What would you like to do?', fileExtensionDescription);
}

const result = await dialog.showMessageBox(this.activeWindow(), {
title: _('Unknown file type'),
message:
_('Joplin doesn\'t recognise the %s extension. Opening this file could be dangerous. What would you like to do?', fileExtensionDescription),
message: warningMessage,
type: 'warning',
checkboxLabel: _('Always open %s files without asking.', fileExtensionDescription),
buttons: [
Expand Down
Loading