diff --git a/src/main/app/composition.ts b/src/main/app/composition.ts index db6cb5534..626a4ce98 100644 --- a/src/main/app/composition.ts +++ b/src/main/app/composition.ts @@ -2796,7 +2796,7 @@ export async function createMainProcessControl(dependencies: { recordSettingsActivity: (input) => settingsDatabase.recordSettingsActivity(input) }) const toolRoutes = createToolRoutes(toolService) - const pluginRoutes = createPluginRoutes(pluginService) + const pluginRoutes = createPluginRoutes(pluginService, pluginSettingsWindow) const skillRoutes = createSkillRoutes({ skillService, skillSyncService, diff --git a/src/main/desktop/pluginSettingsWindow.ts b/src/main/desktop/pluginSettingsWindow.ts index 7e6b091a4..fd2c045d6 100644 --- a/src/main/desktop/pluginSettingsWindow.ts +++ b/src/main/desktop/pluginSettingsWindow.ts @@ -1,9 +1,11 @@ import { BrowserWindow } from 'electron' import path from 'node:path' +import { pathToFileURL } from 'node:url' import type { PluginSettingsWindowPort } from '@/plugin' export class PluginSettingsWindow implements PluginSettingsWindowPort { private readonly windows = new Map() + private readonly pluginIdByWebContentsId = new Map() async open(input: { pluginId: string; title: string; entry: string }): Promise { const existing = this.windows.get(input.pluginId) @@ -23,19 +25,32 @@ export class PluginSettingsWindow implements PluginSettingsWindowPort { nodeIntegration: false, contextIsolation: true, preload: path.join(__dirname, '../preload/pluginSettings.mjs'), - sandbox: false + sandbox: false, + additionalArguments: [`--deepchat-plugin-id=${encodeURIComponent(input.pluginId)}`] } }) + const webContentsId = settingsWindow.webContents.id + const entryPath = pathToFileURL(input.entry).pathname this.windows.set(input.pluginId, settingsWindow) + this.pluginIdByWebContentsId.set(webContentsId, input.pluginId) settingsWindow.webContents.setWindowOpenHandler(() => ({ action: 'deny' })) + settingsWindow.webContents.on('will-navigate', (event, url) => { + const target = new URL(url) + if (target.protocol !== 'file:' || target.pathname !== entryPath) { + event.preventDefault() + } + }) settingsWindow.on('ready-to-show', () => { if (!settingsWindow.isDestroyed()) { settingsWindow.show() } }) settingsWindow.on('closed', () => { - this.windows.delete(input.pluginId) + this.pluginIdByWebContentsId.delete(webContentsId) + if (this.windows.get(input.pluginId) === settingsWindow) { + this.windows.delete(input.pluginId) + } }) await settingsWindow.loadFile(input.entry, { @@ -45,6 +60,10 @@ export class PluginSettingsWindow implements PluginSettingsWindowPort { }) } + getPluginIdForWebContents(webContentsId: number): string | null { + return this.pluginIdByWebContentsId.get(webContentsId) ?? null + } + close(pluginId: string): void { const settingsWindow = this.windows.get(pluginId) if (settingsWindow && !settingsWindow.isDestroyed()) { diff --git a/src/main/plugin/index.ts b/src/main/plugin/index.ts index e74d9ffac..60e9dea61 100644 --- a/src/main/plugin/index.ts +++ b/src/main/plugin/index.ts @@ -80,6 +80,7 @@ export interface PluginSettingsWindowPort { open(input: { pluginId: string; title: string; entry: string }): Promise close(pluginId: string): void closeAll(): void + getPluginIdForWebContents(webContentsId: number): string | null } type PluginServiceDeps = { diff --git a/src/main/plugin/routes.ts b/src/main/plugin/routes.ts index 287fee2df..d1adad768 100644 --- a/src/main/plugin/routes.ts +++ b/src/main/plugin/routes.ts @@ -11,10 +11,25 @@ import { pluginsInvokeActionRoute, pluginsListRoute } from '@shared/contracts/routes' -import { createRouteMap, type DeepchatRouteMap } from '@/routes/routeRegistry' -import type { PluginServicePort } from './index' +import { createRouteMap, type DeepchatRouteMap, type RouteContext } from '@/routes/routeRegistry' +import type { PluginServicePort, PluginSettingsWindowPort } from './index' + +export function createPluginRoutes( + pluginService: PluginServicePort, + settingsWindow: PluginSettingsWindowPort +): DeepchatRouteMap { + const assertPluginSettingsCallerOwns = (context: RouteContext, pluginId: string): void => { + if (context.caller.kind !== 'renderer') { + return + } + const ownerPluginId = settingsWindow.getPluginIdForWebContents(context.caller.webContentsId) + if (ownerPluginId != null && ownerPluginId !== pluginId) { + throw new Error( + `Plugin settings window for "${ownerPluginId}" cannot control plugin "${pluginId}"` + ) + } + } -export function createPluginRoutes(pluginService: PluginServicePort): DeepchatRouteMap { return createRouteMap([ [ pluginsInspectSourceRoute.name, @@ -78,8 +93,9 @@ export function createPluginRoutes(pluginService: PluginServicePort): DeepchatRo ], [ pluginsGetRoute.name, - async (rawInput) => { + async (rawInput, context) => { const input = pluginsGetRoute.input.parse(rawInput) + assertPluginSettingsCallerOwns(context, input.pluginId) return pluginsGetRoute.output.parse({ plugin: await pluginService.getPlugin(input.pluginId) }) @@ -87,8 +103,9 @@ export function createPluginRoutes(pluginService: PluginServicePort): DeepchatRo ], [ pluginsEnableRoute.name, - async (rawInput) => { + async (rawInput, context) => { const input = pluginsEnableRoute.input.parse(rawInput) + assertPluginSettingsCallerOwns(context, input.pluginId) return pluginsEnableRoute.output.parse({ result: await pluginService.enablePlugin(input.pluginId) }) @@ -96,8 +113,9 @@ export function createPluginRoutes(pluginService: PluginServicePort): DeepchatRo ], [ pluginsDisableRoute.name, - async (rawInput) => { + async (rawInput, context) => { const input = pluginsDisableRoute.input.parse(rawInput) + assertPluginSettingsCallerOwns(context, input.pluginId) return pluginsDisableRoute.output.parse({ result: await pluginService.disablePlugin(input.pluginId) }) @@ -105,8 +123,9 @@ export function createPluginRoutes(pluginService: PluginServicePort): DeepchatRo ], [ pluginsInvokeActionRoute.name, - async (rawInput) => { + async (rawInput, context) => { const input = pluginsInvokeActionRoute.input.parse(rawInput) + assertPluginSettingsCallerOwns(context, input.pluginId) return pluginsInvokeActionRoute.output.parse({ result: await pluginService.invokeAction(input.pluginId, input.actionId, input.payload) }) diff --git a/src/main/provider/auth/index.ts b/src/main/provider/auth/index.ts index 52cf9f91b..c87db9099 100644 --- a/src/main/provider/auth/index.ts +++ b/src/main/provider/auth/index.ts @@ -397,6 +397,8 @@ export class OAuthService implements OAuthServicePort { const authUrl = this.buildAuthUrl(config) logger.info('Opening OAuth URL:', authUrl) + this.authWindow.webContents.setWindowOpenHandler(() => ({ action: 'deny' })) + // Load authorization page this.authWindow.loadURL(authUrl) this.authWindow.show() diff --git a/src/preload/plugin-settings-preload.ts b/src/preload/plugin-settings-preload.ts index f7bbd3046..cd4d171c1 100644 --- a/src/preload/plugin-settings-preload.ts +++ b/src/preload/plugin-settings-preload.ts @@ -9,8 +9,13 @@ import { } from '@shared/contracts/routes' import type { PluginSettingsApiStatus } from '@shared/types/plugin' +const PLUGIN_ID_ARG_PREFIX = '--deepchat-plugin-id=' + function readPluginId(): string { - const pluginId = new URL(window.location.href).searchParams.get('pluginId')?.trim() + const arg = process.argv.find((value) => value.startsWith(PLUGIN_ID_ARG_PREFIX)) + const pluginId = arg + ? decodeURIComponent(arg.slice(PLUGIN_ID_ARG_PREFIX.length)).trim() + : undefined if (!pluginId) { throw new Error('Plugin settings renderer is missing pluginId') } diff --git a/test/main/desktop/pluginSettingsWindow.test.ts b/test/main/desktop/pluginSettingsWindow.test.ts new file mode 100644 index 000000000..ba9f980d3 --- /dev/null +++ b/test/main/desktop/pluginSettingsWindow.test.ts @@ -0,0 +1,87 @@ +import { describe, expect, it, vi } from 'vitest' +import { BrowserWindow } from 'electron' +import { PluginSettingsWindow } from '@/desktop/pluginSettingsWindow' + +type Handler = (...args: any[]) => void + +type FakeWindow = { + webContents: { + id: number + on: ReturnType + setWindowOpenHandler: ReturnType + } + isDestroyed: ReturnType + on: ReturnType + show: ReturnType + focus: ReturnType + close: ReturnType + loadFile: ReturnType + handlers: Map + webContentsHandlers: Map +} + +function installBrowserWindowMock(): FakeWindow[] { + const created: FakeWindow[] = [] + vi.mocked(BrowserWindow).mockImplementation(function () { + const handlers = new Map() + const webContentsHandlers = new Map() + const win: FakeWindow = { + webContents: { + id: 100 + created.length, + on: vi.fn((event: string, cb: Handler) => webContentsHandlers.set(event, cb)), + setWindowOpenHandler: vi.fn() + }, + isDestroyed: vi.fn(() => false), + on: vi.fn((event: string, cb: Handler) => handlers.set(event, cb)), + show: vi.fn(), + focus: vi.fn(), + close: vi.fn(), + loadFile: vi.fn().mockResolvedValue(undefined), + handlers, + webContentsHandlers + } + created.push(win) + return win as any + }) + return created +} + +const input = { pluginId: 'p1', title: 'P1', entry: '/plugins/p1/settings.html' } + +describe('PluginSettingsWindow', () => { + it('keeps the reopened window record when a stale closed event fires late', async () => { + const created = installBrowserWindowMock() + const settingsWindow = new PluginSettingsWindow() + + await settingsWindow.open(input) + settingsWindow.close(input.pluginId) + await settingsWindow.open(input) + + created[0].handlers.get('closed')?.() + + settingsWindow.close(input.pluginId) + expect(created[1].close).toHaveBeenCalled() + expect(settingsWindow.getPluginIdForWebContents(created[1].webContents.id)).toBe('p1') + }) + + it('restricts navigation to the file entry', async () => { + const created = installBrowserWindowMock() + const settingsWindow = new PluginSettingsWindow() + + await settingsWindow.open(input) + + const onWillNavigate = created[0].webContentsHandlers.get('will-navigate') + expect(onWillNavigate).toBeDefined() + + const isAllowed = (url: string): boolean => { + const event = { preventDefault: vi.fn() } + onWillNavigate?.(event, url) + return event.preventDefault.mock.calls.length === 0 + } + + expect(isAllowed('file:///plugins/p1/settings.html?pluginId=p1')).toBe(true) + expect(isAllowed('file:///plugins/p1/settings.html#section')).toBe(true) + expect(isAllowed('file:///plugins/other/settings.html')).toBe(false) + expect(isAllowed('https://example.com/')).toBe(false) + }) +}) diff --git a/test/main/plugin/pluginRoutes.test.ts b/test/main/plugin/pluginRoutes.test.ts new file mode 100644 index 000000000..a6e481ad0 --- /dev/null +++ b/test/main/plugin/pluginRoutes.test.ts @@ -0,0 +1,101 @@ +import { describe, expect, it, vi } from 'vitest' +import { + pluginsDisableRoute, + pluginsEnableRoute, + pluginsGetRoute, + pluginsInvokeActionRoute +} from '@shared/contracts/routes' +import type { PluginServicePort, PluginSettingsWindowPort } from '@/plugin' +import { createPluginRoutes } from '@/plugin/routes' +import { createRendererRouteContext, type RouteContext } from '@/routes/routeRegistry' + +const actionResult = { ok: true } + +function setup(ownerPluginId: string | null) { + const pluginService = { + enablePlugin: vi.fn().mockResolvedValue(actionResult), + disablePlugin: vi.fn().mockResolvedValue(actionResult), + invokeAction: vi.fn().mockResolvedValue(actionResult), + getPlugin: vi.fn().mockResolvedValue({ id: 'plugin-a' }) + } + const settingsWindow: PluginSettingsWindowPort = { + open: async () => {}, + close: () => {}, + closeAll: () => {}, + getPluginIdForWebContents: () => ownerPluginId + } + const routes = createPluginRoutes(pluginService as unknown as PluginServicePort, settingsWindow) + return { pluginService, routes } +} + +const pluginWindowContext = (): RouteContext => createRendererRouteContext(42, 7) + +describe('createPluginRoutes settings-window ownership', () => { + it('rejects disable for another plugin from a plugin settings window', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsDisableRoute.name) + + await expect(handler?.({ pluginId: 'plugin-b' }, pluginWindowContext())).rejects.toThrow( + /cannot control plugin/ + ) + expect(pluginService.disablePlugin).not.toHaveBeenCalled() + }) + + it('rejects enable for another plugin from a plugin settings window', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsEnableRoute.name) + + await expect(handler?.({ pluginId: 'plugin-b' }, pluginWindowContext())).rejects.toThrow( + /cannot control plugin/ + ) + expect(pluginService.enablePlugin).not.toHaveBeenCalled() + }) + + it('rejects invokeAction for another plugin from a plugin settings window', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsInvokeActionRoute.name) + + await expect( + handler?.({ pluginId: 'plugin-b', actionId: 'act' }, pluginWindowContext()) + ).rejects.toThrow(/cannot control plugin/) + expect(pluginService.invokeAction).not.toHaveBeenCalled() + }) + + it('rejects get for another plugin from a plugin settings window', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsGetRoute.name) + + await expect(handler?.({ pluginId: 'plugin-b' }, pluginWindowContext())).rejects.toThrow( + /cannot control plugin/ + ) + expect(pluginService.getPlugin).not.toHaveBeenCalled() + }) + + it('allows a plugin settings window to control its own plugin', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsDisableRoute.name) + + await handler?.({ pluginId: 'plugin-a' }, pluginWindowContext()) + + expect(pluginService.disablePlugin).toHaveBeenCalledWith('plugin-a') + }) + + it('allows renderer callers that are not plugin settings windows', async () => { + const { pluginService, routes } = setup(null) + const handler = routes.get(pluginsDisableRoute.name) + + await handler?.({ pluginId: 'plugin-b' }, pluginWindowContext()) + + expect(pluginService.disablePlugin).toHaveBeenCalledWith('plugin-b') + }) + + it('allows non-renderer callers', async () => { + const { pluginService, routes } = setup('plugin-a') + const handler = routes.get(pluginsEnableRoute.name) + const context: RouteContext = { caller: { kind: 'internal', component: 'scheduler' } } + + await handler?.({ pluginId: 'plugin-b' }, context) + + expect(pluginService.enablePlugin).toHaveBeenCalledWith('plugin-b') + }) +}) diff --git a/test/main/routes/dispatcher.test.ts b/test/main/routes/dispatcher.test.ts index 9fce8acc3..8166ef495 100644 --- a/test/main/routes/dispatcher.test.ts +++ b/test/main/routes/dispatcher.test.ts @@ -1616,7 +1616,12 @@ function createRuntime() { recordSettingsActivity: (input) => sqlitePresenter.recordSettingsActivity(input) }) const toolRoutes = createToolRoutes(toolService) - const pluginRoutes = createPluginRoutes(pluginService) + const pluginRoutes = createPluginRoutes(pluginService, { + open: async () => {}, + close: () => {}, + closeAll: () => {}, + getPluginIdForWebContents: () => null + }) const assertSessionActiveSkillsMutable = vi.fn().mockResolvedValue(undefined) const skillRoutes = createSkillRoutes({ skillService, diff --git a/test/renderer/api/preloadBoundaries.test.ts b/test/renderer/api/preloadBoundaries.test.ts index 5795e2694..511bfa94f 100644 --- a/test/renderer/api/preloadBoundaries.test.ts +++ b/test/renderer/api/preloadBoundaries.test.ts @@ -229,49 +229,58 @@ describe('preload IPC boundaries', () => { it('backs plugin settings preload APIs with typed route bridge calls', async () => { const { ipcRenderer } = installElectronPreloadMock() - window.history.pushState({}, '', '/plugin-settings/?pluginId=plugin-1') - - await import('../../../src/preload/plugin-settings-preload') - - const deepchatPlugin = ( - window as Window & { - deepchatPlugin: { - getPluginId: () => string - getStatus: () => Promise<{ pluginId: string; enabled: boolean }> - enable: () => Promise - invokeAction: (actionId: string, payload?: Record) => Promise + const originalArgv = process.argv + process.argv = [...originalArgv, '--deepchat-plugin-id=plugin-1'] + + try { + await import('../../../src/preload/plugin-settings-preload') + + const deepchatPlugin = ( + window as Window & { + deepchatPlugin: { + getPluginId: () => string + getStatus: () => Promise<{ pluginId: string; enabled: boolean }> + enable: () => Promise + invokeAction: (actionId: string, payload?: Record) => Promise + } } - } - ).deepchatPlugin - - await expect(deepchatPlugin.getStatus()).resolves.toMatchObject({ - pluginId: 'plugin-1', - enabled: true - }) - - await deepchatPlugin.enable() - await deepchatPlugin.invokeAction('refresh', { force: true }) + ).deepchatPlugin - expect(deepchatPlugin.getPluginId()).toBe('plugin-1') - expect(ipcRenderer.invoke).toHaveBeenCalledWith(DEEPCHAT_ROUTE_INVOKE_CHANNEL, 'plugins.get', { - pluginId: 'plugin-1' - }) - expect(ipcRenderer.invoke).toHaveBeenCalledWith( - DEEPCHAT_ROUTE_INVOKE_CHANNEL, - 'plugins.enable', - { - pluginId: 'plugin-1' - } - ) - expect(ipcRenderer.invoke).toHaveBeenCalledWith( - DEEPCHAT_ROUTE_INVOKE_CHANNEL, - 'plugins.invokeAction', - { + await expect(deepchatPlugin.getStatus()).resolves.toMatchObject({ pluginId: 'plugin-1', - actionId: 'refresh', - payload: { force: true } - } - ) + enabled: true + }) + + await deepchatPlugin.enable() + await deepchatPlugin.invokeAction('refresh', { force: true }) + + expect(deepchatPlugin.getPluginId()).toBe('plugin-1') + expect(ipcRenderer.invoke).toHaveBeenCalledWith( + DEEPCHAT_ROUTE_INVOKE_CHANNEL, + 'plugins.get', + { + pluginId: 'plugin-1' + } + ) + expect(ipcRenderer.invoke).toHaveBeenCalledWith( + DEEPCHAT_ROUTE_INVOKE_CHANNEL, + 'plugins.enable', + { + pluginId: 'plugin-1' + } + ) + expect(ipcRenderer.invoke).toHaveBeenCalledWith( + DEEPCHAT_ROUTE_INVOKE_CHANNEL, + 'plugins.invokeAction', + { + pluginId: 'plugin-1', + actionId: 'refresh', + payload: { force: true } + } + ) + } finally { + process.argv = originalArgv + } }) it('replays a debug mode received before the splash renderer subscribes', async () => {