From c4182a950f47d6a242ccd2d572221070f39a415b Mon Sep 17 00:00:00 2001 From: beyondkmp Date: Mon, 28 Jul 2025 15:45:52 +0800 Subject: [PATCH] fix: add isPathInside functionality to check path relationships (#8590) * feat(ipc): add isPathInside functionality to check path relationships - Introduced a new IPC channel for checking if a path is inside another path, enhancing path validation capabilities. - Implemented the isPathInside function in the file utility, which accurately determines parent-child path relationships, handling edge cases. - Updated relevant components to utilize the new isPathInside function for validating app data and backup paths, ensuring better user experience and error handling. - Added comprehensive tests for isPathInside to cover various scenarios, including edge cases and error handling. * format code --- packages/shared/IpcChannel.ts | 1 + src/main/ipc.ts | 7 +- src/main/utils/__tests__/file.test.ts | 161 +++++++++++++++++- src/main/utils/file.ts | 36 ++++ src/preload/index.ts | 2 + .../settings/DataSettings/DataSettings.tsx | 6 +- .../DataSettings/LocalBackupSettings.tsx | 4 +- 7 files changed, 211 insertions(+), 6 deletions(-) diff --git a/packages/shared/IpcChannel.ts b/packages/shared/IpcChannel.ts index f3894dc97b..cfc00913aa 100644 --- a/packages/shared/IpcChannel.ts +++ b/packages/shared/IpcChannel.ts @@ -21,6 +21,7 @@ export enum IpcChannel { App_Select = 'app:select', App_HasWritePermission = 'app:has-write-permission', App_ResolvePath = 'app:resolve-path', + App_IsPathInside = 'app:is-path-inside', App_Copy = 'app:copy', App_SetStopQuitApp = 'app:set-stop-quit-app', App_SetAppDataPath = 'app:set-app-data-path', diff --git a/src/main/ipc.ts b/src/main/ipc.ts index 899a280187..a3e3ef4432 100644 --- a/src/main/ipc.ts +++ b/src/main/ipc.ts @@ -55,7 +55,7 @@ import { setOpenLinkExternal } from './services/WebviewService' import { windowService } from './services/WindowService' import { calculateDirectorySize, getResourcePath } from './utils' import { decrypt, encrypt } from './utils/aes' -import { getCacheDir, getConfigDir, getFilesDir, hasWritePermission, untildify } from './utils/file' +import { getCacheDir, getConfigDir, getFilesDir, hasWritePermission, isPathInside, untildify } from './utils/file' import { updateAppDataConfig } from './utils/init' import { compress, decompress } from './utils/zip' @@ -294,6 +294,11 @@ export function registerIpc(mainWindow: BrowserWindow, app: Electron.App) { return path.resolve(untildify(filePath)) }) + // Check if a path is inside another path (proper parent-child relationship) + ipcMain.handle(IpcChannel.App_IsPathInside, async (_, childPath: string, parentPath: string) => { + return isPathInside(childPath, parentPath) + }) + // Set app data path ipcMain.handle(IpcChannel.App_SetAppDataPath, async (_, filePath: string) => { updateAppDataConfig(filePath) diff --git a/src/main/utils/__tests__/file.test.ts b/src/main/utils/__tests__/file.test.ts index 8c651f237c..a95665f815 100644 --- a/src/main/utils/__tests__/file.test.ts +++ b/src/main/utils/__tests__/file.test.ts @@ -9,7 +9,16 @@ import { detectAll as detectEncodingAll } from 'jschardet' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { readTextFileWithAutoEncoding } from '../file' -import { getAllFiles, getAppConfigDir, getConfigDir, getFilesDir, getFileType, getTempDir, untildify } from '../file' +import { + getAllFiles, + getAppConfigDir, + getConfigDir, + getFilesDir, + getFileType, + getTempDir, + isPathInside, + untildify +} from '../file' // Mock dependencies vi.mock('node:fs') @@ -343,4 +352,154 @@ describe('file', () => { expect(untildify('~/folder_with_underscores')).toBe('/mock/home/folder_with_underscores') }) }) + + describe('isPathInside', () => { + beforeEach(() => { + // Mock path.resolve to simulate path resolution + vi.mocked(path.resolve).mockImplementation((...args) => { + const joined = args.join('/') + return joined.startsWith('/') ? joined : `/${joined}` + }) + + // Mock path.normalize to simulate path normalization + vi.mocked(path.normalize).mockImplementation((p) => p.replace(/\/+/g, '/')) + + // Mock path.relative to calculate relative paths + vi.mocked(path.relative).mockImplementation((from, to) => { + // Simple mock implementation for testing + const fromParts = from.split('/').filter((p) => p) + const toParts = to.split('/').filter((p) => p) + + // Find common prefix + let i = 0 + while (i < fromParts.length && i < toParts.length && fromParts[i] === toParts[i]) { + i++ + } + + // Calculate relative path + const upLevels = fromParts.length - i + const downPath = toParts.slice(i) + + if (upLevels === 0 && downPath.length === 0) { + return '' + } + + const result = ['..'.repeat(upLevels), ...downPath].filter((p) => p).join('/') + return result || '.' + }) + + // Mock path.isAbsolute + vi.mocked(path.isAbsolute).mockImplementation((p) => p.startsWith('/')) + }) + + describe('basic parent-child relationships', () => { + it('should return true when child is inside parent', () => { + expect(isPathInside('/root/test/child', '/root/test')).toBe(true) + expect(isPathInside('/root/test/deep/child', '/root/test')).toBe(true) + expect(isPathInside('child/deep', 'child')).toBe(true) + }) + + it('should return false when child is not inside parent', () => { + expect(isPathInside('/root/test', '/root/test/child')).toBe(false) + expect(isPathInside('/root/other', '/root/test')).toBe(false) + expect(isPathInside('/different/path', '/root/test')).toBe(false) + expect(isPathInside('child', 'child/deep')).toBe(false) + }) + + it('should return true when paths are the same', () => { + expect(isPathInside('/root/test', '/root/test')).toBe(true) + expect(isPathInside('child', 'child')).toBe(true) + }) + }) + + describe('edge cases that startsWith cannot handle', () => { + it('should correctly distinguish similar path names', () => { + // The problematic case mentioned by user + expect(isPathInside('/root/test aaa', '/root/test')).toBe(false) + expect(isPathInside('/root/test', '/root/test aaa')).toBe(false) + + // More similar cases + expect(isPathInside('/home/user-data', '/home/user')).toBe(false) + expect(isPathInside('/home/user', '/home/user-data')).toBe(false) + expect(isPathInside('/var/log-backup', '/var/log')).toBe(false) + }) + + it('should handle paths with spaces correctly', () => { + expect(isPathInside('/path with spaces/child', '/path with spaces')).toBe(true) + expect(isPathInside('/path with spaces', '/path with spaces/child')).toBe(false) + }) + + it('should handle Windows-style paths', () => { + // Mock for Windows paths + vi.mocked(path.resolve).mockImplementation((...args) => { + const joined = args.join('\\').replace(/\//g, '\\') + return joined.match(/^[A-Z]:/) ? joined : `C:${joined}` + }) + + vi.mocked(path.normalize).mockImplementation((p) => p.replace(/\\+/g, '\\')) + + // Mock path.relative for Windows paths + vi.mocked(path.relative).mockImplementation((from, to) => { + const fromParts = from.split('\\').filter((p) => p && p !== 'C:') + const toParts = to.split('\\').filter((p) => p && p !== 'C:') + + // Find common prefix + let i = 0 + while (i < fromParts.length && i < toParts.length && fromParts[i] === toParts[i]) { + i++ + } + + // Calculate relative path + const upLevels = fromParts.length - i + const downPath = toParts.slice(i) + + if (upLevels === 0 && downPath.length === 0) { + return '' + } + + const upPath = Array(upLevels).fill('..').join('\\') + const result = [upPath, ...downPath].filter((p) => p).join('\\') + return result || '.' + }) + + expect(isPathInside('C:\\Users\\test\\child', 'C:\\Users\\test')).toBe(true) + expect(isPathInside('C:\\Users\\test aaa', 'C:\\Users\\test')).toBe(false) + }) + }) + + describe('error handling', () => { + it('should return false when path operations throw errors', () => { + vi.mocked(path.resolve).mockImplementation(() => { + throw new Error('Path resolution failed') + }) + + expect(isPathInside('/any/path', '/any/parent')).toBe(false) + }) + }) + + describe('comparison with startsWith behavior', () => { + const testCases: [string, string, boolean, boolean][] = [ + ['/root/test aaa', '/root/test', false, true], // isPathInside vs startsWith + ['/root/test', '/root/test aaa', false, false], + ['/root/test/child', '/root/test', true, true], + ['/home/user-data', '/home/user', false, true] + ] + + it.each(testCases)( + 'should correctly handle %s vs %s', + (child: string, parent: string, expectedIsPathInside: boolean, expectedStartsWith: boolean) => { + const isPathInsideResult = isPathInside(child, parent) + const startsWithResult = child.startsWith(parent) + + expect(isPathInsideResult).toBe(expectedIsPathInside) + expect(startsWithResult).toBe(expectedStartsWith) + + // Verify that isPathInside gives different (correct) result in problematic cases + if (expectedIsPathInside !== expectedStartsWith) { + expect(isPathInsideResult).not.toBe(startsWithResult) + } + } + ) + }) + }) }) diff --git a/src/main/utils/file.ts b/src/main/utils/file.ts index 28a04f30fe..e73e546022 100644 --- a/src/main/utils/file.ts +++ b/src/main/utils/file.ts @@ -46,6 +46,42 @@ export async function hasWritePermission(dir: string) { } } +/** + * Check if a path is inside another path (proper parent-child relationship) + * This function correctly handles edge cases that string.startsWith() cannot handle, + * such as distinguishing between '/root/test' and '/root/test aaa' + * + * @param childPath - The path that might be inside the parent path + * @param parentPath - The path that might contain the child path + * @returns true if childPath is inside parentPath, false otherwise + */ +export function isPathInside(childPath: string, parentPath: string): boolean { + try { + const resolvedChild = path.resolve(childPath) + const resolvedParent = path.resolve(parentPath) + + // Normalize paths to handle different separators + const normalizedChild = path.normalize(resolvedChild) + const normalizedParent = path.normalize(resolvedParent) + + // Check if they are the same path + if (normalizedChild === normalizedParent) { + return true + } + + // Get relative path from parent to child + const relativePath = path.relative(normalizedParent, normalizedChild) + + // If relative path is empty, they are the same + // If relative path starts with '..', child is not inside parent + // If relative path is absolute, child is not inside parent + return relativePath !== '' && !relativePath.startsWith('..') && !path.isAbsolute(relativePath) + } catch (error) { + logger.error('Failed to check path relationship:', error as Error) + return false + } +} + export function getFileType(ext: string): FileTypes { ext = ext.toLowerCase() return fileTypeMap.get(ext) || FileTypes.OTHER diff --git a/src/preload/index.ts b/src/preload/index.ts index abf3d39593..5b492846d1 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -60,6 +60,8 @@ const api = { select: (options: Electron.OpenDialogOptions) => ipcRenderer.invoke(IpcChannel.App_Select, options), hasWritePermission: (path: string) => ipcRenderer.invoke(IpcChannel.App_HasWritePermission, path), resolvePath: (path: string) => ipcRenderer.invoke(IpcChannel.App_ResolvePath, path), + isPathInside: (childPath: string, parentPath: string) => + ipcRenderer.invoke(IpcChannel.App_IsPathInside, childPath, parentPath), setAppDataPath: (path: string) => ipcRenderer.invoke(IpcChannel.App_SetAppDataPath, path), getDataPathFromArgs: () => ipcRenderer.invoke(IpcChannel.App_GetDataPathFromArgs), copy: (oldPath: string, newPath: string, occupiedDirs: string[] = []) => diff --git a/src/renderer/src/pages/settings/DataSettings/DataSettings.tsx b/src/renderer/src/pages/settings/DataSettings/DataSettings.tsx index 47c16859ea..87f1881fcc 100644 --- a/src/renderer/src/pages/settings/DataSettings/DataSettings.tsx +++ b/src/renderer/src/pages/settings/DataSettings/DataSettings.tsx @@ -210,13 +210,15 @@ const DataSettings: FC = () => { } // check new app data path is not in old app data path - if (newAppDataPath.startsWith(appInfo.appDataPath)) { + const isInOldPath = await window.api.isPathInside(newAppDataPath, appInfo.appDataPath) + if (isInOldPath) { window.message.error(t('settings.data.app_data.select_error_same_path')) return } // check new app data path is not in app install path - if (newAppDataPath.startsWith(appInfo.installPath)) { + const isInInstallPath = await window.api.isPathInside(newAppDataPath, appInfo.installPath) + if (isInInstallPath) { window.message.error(t('settings.data.app_data.select_error_in_app_path')) return } diff --git a/src/renderer/src/pages/settings/DataSettings/LocalBackupSettings.tsx b/src/renderer/src/pages/settings/DataSettings/LocalBackupSettings.tsx index 53bd9973fc..388a6b94ca 100644 --- a/src/renderer/src/pages/settings/DataSettings/LocalBackupSettings.tsx +++ b/src/renderer/src/pages/settings/DataSettings/LocalBackupSettings.tsx @@ -75,14 +75,14 @@ const LocalBackupSettings: React.FC = () => { // check new local backup dir is not in app data path // if is in app data path, show error - if (resolvedDir.startsWith(appInfo!.appDataPath)) { + if (await window.api.isPathInside(resolvedDir, appInfo!.appDataPath)) { window.message.error(t('settings.data.local.directory.select_error_app_data_path')) return false } // check new local backup dir is not in app install path // if is in app install path, show error - if (resolvedDir.startsWith(appInfo!.installPath)) { + if (await window.api.isPathInside(resolvedDir, appInfo!.installPath)) { window.message.error(t('settings.data.local.directory.select_error_in_app_install_path')) return false }