diff --git a/.gitignore b/.gitignore index 9d2578965f..5d8b24a501 100644 --- a/.gitignore +++ b/.gitignore @@ -249,6 +249,7 @@ packages/app-desktop/gui/KeymapConfig/utils/useCommandStatus.js packages/app-desktop/gui/KeymapConfig/utils/useKeymap.js packages/app-desktop/gui/MainLayoutPane.js packages/app-desktop/gui/MainScreen.js +packages/app-desktop/gui/MainScreen/handleCallbackUrl.test.js packages/app-desktop/gui/MainScreen/handleCallbackUrl.js packages/app-desktop/gui/MasterPasswordDialog/Dialog.js packages/app-desktop/gui/MenuBar.js diff --git a/.ignore.eslint b/.ignore.eslint index 766c79f363..81ecf8eb66 100644 --- a/.ignore.eslint +++ b/.ignore.eslint @@ -274,6 +274,7 @@ packages/app-desktop/gui/KeymapConfig/utils/useCommandStatus.js packages/app-desktop/gui/KeymapConfig/utils/useKeymap.js packages/app-desktop/gui/MainLayoutPane.js packages/app-desktop/gui/MainScreen.js +packages/app-desktop/gui/MainScreen/handleCallbackUrl.test.js packages/app-desktop/gui/MainScreen/handleCallbackUrl.js packages/app-desktop/gui/MasterPasswordDialog/Dialog.js packages/app-desktop/gui/MenuBar.js diff --git a/packages/app-desktop/gui/MainScreen/handleCallbackUrl.test.ts b/packages/app-desktop/gui/MainScreen/handleCallbackUrl.test.ts new file mode 100644 index 0000000000..a90081e888 --- /dev/null +++ b/packages/app-desktop/gui/MainScreen/handleCallbackUrl.test.ts @@ -0,0 +1,62 @@ +import { utils as commandUtils } from '@joplin/lib/services/CommandService'; +import executeCallbackUrl from './handleCallbackUrl'; + +const mockOpenExternal = jest.fn(); + +jest.mock('../../services/bridge', () => ({ + __esModule: true, + default: () => ({ + openExternal: (url: string) => mockOpenExternal(url), + }), +})); + +const setSelectedNoteId = (noteId: string | null) => { + // executeCallbackUrl reads the selected note id via commandUtils.store; a + // null id makes handleGetCurrentNote take its x-error branch, which routes + // the caller-supplied target through respond() without touching the DB. + commandUtils.store = { + getState: () => ({ selectedNoteIds: noteId ? [noteId] : [] }), + }; +}; + +describe('handleCallbackUrl', () => { + beforeEach(() => { + mockOpenExternal.mockClear(); + setSelectedNoteId(null); + }); + + test.each([ + 'x-custom-scheme://arbitrary-scheme-reached', + 'file:///etc/passwd', + 'ms-msdt://something', + 'mailto:someone@example.com', + 'not a url', + ])('should not dispatch a callback target with a disallowed scheme (%s)', async (target) => { + await executeCallbackUrl(`joplin://x-callback-url/getCurrentNote?x-error=${encodeURIComponent(target)}`); + expect(mockOpenExternal).not.toHaveBeenCalled(); + }); + + test.each([ + 'http://example.com/cb', + 'https://example.com/cb', + ])('should dispatch a callback target with an allowed scheme (%s)', async (target) => { + await executeCallbackUrl(`joplin://x-callback-url/getCurrentNote?x-error=${encodeURIComponent(target)}`); + expect(mockOpenExternal).toHaveBeenCalledTimes(1); + expect(mockOpenExternal.mock.calls[0][0]).toContain(`${target}?`); + }); + + it('should not leak the raw error message on the error path', async () => { + // Force a throw inside the try block so the catch path builds the response. + commandUtils.store = { + getState: () => { throw new Error('secret path /home/victim/.config/joplin/database.sqlite'); }, + }; + + await executeCallbackUrl(`joplin://x-callback-url/getCurrentNote?x-error=${encodeURIComponent('https://example.com/cb')}`); + + expect(mockOpenExternal).toHaveBeenCalledTimes(1); + const responseUrl = mockOpenExternal.mock.calls[0][0]; + expect(responseUrl).not.toContain('secret path'); + expect(responseUrl).not.toContain('database.sqlite'); + expect(new URL(responseUrl).searchParams.get('errorMessage')).toBe('The command could not be completed'); + }); +}); diff --git a/packages/app-desktop/gui/MainScreen/handleCallbackUrl.ts b/packages/app-desktop/gui/MainScreen/handleCallbackUrl.ts index cb9a2e56f0..cfa8fe0bce 100644 --- a/packages/app-desktop/gui/MainScreen/handleCallbackUrl.ts +++ b/packages/app-desktop/gui/MainScreen/handleCallbackUrl.ts @@ -12,6 +12,25 @@ const logger = Logger.create('handleCallbackUrl'); // Opens the caller's x-success / x-error URL with the result appended as query params. const respond = (target: string, params: Record = {}) => { if (!target) return; + + // The callback target is untrusted input from the x-callback-url. Anything + // that is not file: is handed straight to shell.openExternal(), i.e. the OS + // URI dispatcher, so restrict it to schemes we're willing to invoke. This + // also covers the x-error path below, which fires on any thrown exception. + let protocol; + try { + protocol = new URL(target).protocol; + } catch (error) { + // Don't log the caller-supplied target: it's untrusted and may contain + // secrets in its query or fragment. + logger.warn('Rejected malformed callback target'); + return; + } + if (protocol !== 'http:' && protocol !== 'https:') { + logger.warn(`Rejected callback target with disallowed scheme "${protocol}"`); + return; + } + const query = Object.entries(params) .map(([key, value]) => `${encodeURIComponent(key)}=${encodeURIComponent(value)}`) .join('&'); @@ -95,7 +114,9 @@ const executeCallbackUrl = async (url: string) => { } } catch (error) { logger.error(`Error handling callback URL command "${info.command}":`, error); - respond(info.params['x-error'], { errorMessage: error.message }); + // Return an opaque status rather than error.message, which can leak + // profile paths and item ids to the caller-supplied x-error target. + respond(info.params['x-error'], { errorMessage: 'The command could not be completed' }); } };