From 5b60bd79514875986c8a7f45e8c7b4f86464bdae Mon Sep 17 00:00:00 2001 From: Baldri Date: Thu, 27 Aug 2026 20:23:02 +0200 Subject: [PATCH] fix(security): delegation must pass the same guard as every other LLM call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit executeDelegation handed subTask.content — the user's own text — straight to clientManager.sendMessageNonStreaming. Neither the orchestrator nor its IPC handler ran any guard; `requireFeature('agents')` gates the licence, not the content. Measured 2026-08-27: no guard call anywhere under src/main/routing, while the same search found preflightGuard in four other files. That bypassed the injection scan, the sensitive-data consent, the budget check, the trust routing, the residency policy and the audit trail — every one of them, on a path a user reaches from the UI. Each sub-task now runs preflightGuard before anything is sent. A refusal means NOT SENT, recorded as a refusal in the result rather than silently dropped. Where the guard settles on a different provider than the proposal named, that provider receives the sub-task AND is what gets recorded — otherwise the cost and audit trail would name a provider that never saw the content. The local-provider exclusion goes with it. `ollama` and `local` were filtered out before routing, so a sub-task the policy only permits on-device had nowhere to go and delegation could only refuse it. They are candidates now; the guard still has the last word. The current provider stays excluded — there is nothing to delegate to yourself. Adding the guard import surfaced a second problem: `src/renderer` imported the orchestrator's types from `src/main`, so the renderer's type-check followed the new import chain down to the database layer and its untyped `sql.js` dependency. The types the UI renders now live in `src/shared/orchestrator-types.ts` and are re-exported for existing importers. Two renderer -> main type imports remain, from other modules; they are pre-existing and untouched here. Sabotages, each verified red: - drop the guard call -> four tests fail - restore the local-provider exclusion -> the candidate-set test fails Co-Authored-By: Claude Opus 5 --- src/main/routing/hybrid-orchestrator.ts | 115 +++++++------- src/main/routing/intelligent-router.ts | 3 +- .../components/DelegationProposalDialog.tsx | 2 +- src/renderer/stores/orchestrator-store.ts | 2 +- src/shared/orchestrator-types.ts | 68 +++++++++ tests/unit/hybrid-orchestrator-guard.test.ts | 140 ++++++++++++++++++ 6 files changed, 266 insertions(+), 64 deletions(-) create mode 100644 src/shared/orchestrator-types.ts create mode 100644 tests/unit/hybrid-orchestrator-guard.test.ts diff --git a/src/main/routing/hybrid-orchestrator.ts b/src/main/routing/hybrid-orchestrator.ts index 71a09da..056f06f 100644 --- a/src/main/routing/hybrid-orchestrator.ts +++ b/src/main/routing/hybrid-orchestrator.ts @@ -16,68 +16,25 @@ */ import { getClientManager } from '../llm-clients/client-manager' -import { getRouter, type RequestCategory } from './intelligent-router' +import { getRouter } from './intelligent-router' +import type { + RequestCategory, + SubTask, + DelegationProposal, + DelegationResult, + OrchestratorConfig +} from '../../shared/orchestrator-types' + +// Re-exported so existing importers keep working; the definitions moved to +// shared because the renderer needs them (see that file's header). +export type { SubTask, DelegationProposal, DelegationResult, OrchestratorConfig } import { generateId } from '../utils/id-generator' +import { preflightGuard } from '../security/request-guard' +import { getGuardDeps } from '../security/request-guard-deps' import type { Message } from '../../shared/types' // ── Types ─────────────────────────────────────────────────────── -export interface SubTask { - id: string - description: string - category: RequestCategory - content: string - suggestedProvider: string - suggestedModel: string - confidence: number - reasoning: string -} - -export interface DelegationProposal { - id: string - /** The original user message */ - originalMessage: string - /** The primary LLM's analysis of why delegation helps */ - analysis: string - /** Sub-tasks to delegate */ - subTasks: SubTask[] - /** Total estimated cost if delegated */ - estimatedCost: number - /** Status */ - status: 'pending' | 'approved' | 'denied' | 'completed' | 'failed' - /** Timestamp */ - createdAt: number -} - -export interface DelegationResult { - proposalId: string - subTaskResults: Array<{ - subTaskId: string - provider: string - model: string - response: string - tokens?: number - cost?: number - latencyMs: number - }> - composedResponse: string - totalCost: number - totalLatencyMs: number -} - -export interface OrchestratorConfig { - /** Enable hybrid orchestration */ - enabled: boolean - /** Minimum confidence to suggest delegation (0-1) */ - delegationThreshold: number - /** Auto-delegate below this cost threshold (USD) without user approval */ - autoApproveThreshold: number - /** Maximum sub-tasks per delegation */ - maxSubTasks: number - /** Preferred models for each category */ - preferredModels: Partial> -} - export const DEFAULT_ORCHESTRATOR_CONFIG: OrchestratorConfig = { enabled: true, delegationThreshold: 0.75, @@ -125,9 +82,15 @@ export class HybridOrchestrator { const clientManager = getClientManager() - // Get available cloud providers + // Candidates for delegation: everything with credentials except the + // provider the user is already talking to — there is nothing to delegate + // to yourself. + // + // Local providers used to be filtered out here as well, which meant a + // sub-task the policy only permits on-device had nowhere to go. They are + // candidates now; the guard in executeDelegation still has the last word. const availableProviders = clientManager.getProvidersWithApiKeys() - .filter(p => p !== 'ollama' && p !== 'local' && p !== currentProvider) + .filter(p => p !== currentProvider) if (availableProviders.length === 0) return null @@ -276,8 +239,38 @@ export class HybridOrchestrator { } ] + // Delegation sends the user's own text to another provider, so it runs + // the same guard as every other path that does: injection scan, + // sensitive-data consent, budget, trust routing and residency policy. + // Until this was here, the licence check in the IPC handler was the + // only gate — and that gates the feature, not the content. + // + // The guard may settle on a DIFFERENT provider than the proposal + // named. What it settles on is what receives the sub-task and what + // gets recorded, or the two would disagree in the cost and audit + // trail. + const preflight = await preflightGuard( + { + texts: [subTask.content], + provider: subTask.suggestedProvider, + model: subTask.suggestedModel + }, + getGuardDeps() + ) + + if (!preflight.ok) { + results.push({ + subTaskId: subTask.id, + provider: subTask.suggestedProvider, + model: subTask.suggestedModel, + response: `[Delegation refused: ${preflight.reason ?? 'blocked by security guard'}]`, + latencyMs: Date.now() - taskStartTime + }) + continue + } + const response = await clientManager.sendMessageNonStreaming( - subTask.suggestedProvider, + preflight.provider, messages, subTask.suggestedModel, 0.7 @@ -285,7 +278,7 @@ export class HybridOrchestrator { results.push({ subTaskId: subTask.id, - provider: subTask.suggestedProvider, + provider: preflight.provider, model: subTask.suggestedModel, response, latencyMs: Date.now() - taskStartTime diff --git a/src/main/routing/intelligent-router.ts b/src/main/routing/intelligent-router.ts index 90e7c15..038465f 100644 --- a/src/main/routing/intelligent-router.ts +++ b/src/main/routing/intelligent-router.ts @@ -2,7 +2,8 @@ import { Ollama } from 'ollama' import type { LLMProvider } from '../llm-clients/client-manager' import { getProviderRegistry } from './provider-registry' -export type RequestCategory = 'code' | 'creative' | 'analysis' | 'general' | 'conversation' +import type { RequestCategory } from '../../shared/orchestrator-types' +export type { RequestCategory } export interface RoutingResult { category: RequestCategory diff --git a/src/renderer/components/DelegationProposalDialog.tsx b/src/renderer/components/DelegationProposalDialog.tsx index 19998f7..239149f 100644 --- a/src/renderer/components/DelegationProposalDialog.tsx +++ b/src/renderer/components/DelegationProposalDialog.tsx @@ -1,5 +1,5 @@ import { memo, useState, useCallback } from 'react' -import type { DelegationProposal } from '../../main/routing/hybrid-orchestrator' +import type { DelegationProposal } from '../../shared/orchestrator-types' interface DelegationProposalDialogProps { isOpen: boolean diff --git a/src/renderer/stores/orchestrator-store.ts b/src/renderer/stores/orchestrator-store.ts index 7f097ac..4f31de6 100644 --- a/src/renderer/stores/orchestrator-store.ts +++ b/src/renderer/stores/orchestrator-store.ts @@ -1,5 +1,5 @@ import { create } from 'zustand' -import type { DelegationProposal, DelegationResult, OrchestratorConfig } from '../../main/routing/hybrid-orchestrator' +import type { DelegationProposal, DelegationResult, OrchestratorConfig } from '../../shared/orchestrator-types' interface OrchestratorState { /** Whether orchestrator is enabled */ diff --git a/src/shared/orchestrator-types.ts b/src/shared/orchestrator-types.ts new file mode 100644 index 0000000..6070492 --- /dev/null +++ b/src/shared/orchestrator-types.ts @@ -0,0 +1,68 @@ +/** + * Types shared between the orchestrator (main process) and the UI. + * + * They live here rather than in `src/main/routing/hybrid-orchestrator.ts` + * because the renderer needs them: importing them from main pulled the whole + * main-process module graph into the renderer's type-check, down to the + * database layer and its untyped `sql.js` dependency. A type the UI renders + * belongs to neither process in particular. + */ + +/** What a request is asking for. Drives both routing and delegation. */ +export type RequestCategory = 'code' | 'creative' | 'analysis' | 'general' | 'conversation' + +export interface SubTask { + id: string + description: string + category: RequestCategory + content: string + suggestedProvider: string + suggestedModel: string + confidence: number + reasoning: string +} + +export interface DelegationProposal { + id: string + /** The original user message */ + originalMessage: string + /** The primary LLM's analysis of why delegation helps */ + analysis: string + /** Sub-tasks to delegate */ + subTasks: SubTask[] + /** Total estimated cost if delegated */ + estimatedCost: number + /** Status */ + status: 'pending' | 'approved' | 'denied' | 'completed' | 'failed' + /** Timestamp */ + createdAt: number +} + +export interface DelegationResult { + proposalId: string + subTaskResults: Array<{ + subTaskId: string + provider: string + model: string + response: string + tokens?: number + cost?: number + latencyMs: number + }> + composedResponse: string + totalCost: number + totalLatencyMs: number +} + +export interface OrchestratorConfig { + /** Enable hybrid orchestration */ + enabled: boolean + /** Minimum confidence to suggest delegation (0-1) */ + delegationThreshold: number + /** Auto-delegate below this cost threshold (USD) without user approval */ + autoApproveThreshold: number + /** Maximum sub-tasks per delegation */ + maxSubTasks: number + /** Preferred models for each category */ + preferredModels: Partial> +} diff --git a/tests/unit/hybrid-orchestrator-guard.test.ts b/tests/unit/hybrid-orchestrator-guard.test.ts new file mode 100644 index 0000000..7999b45 --- /dev/null +++ b/tests/unit/hybrid-orchestrator-guard.test.ts @@ -0,0 +1,140 @@ +/** + * The delegation path must pass the same guard as every other path that + * sends content to an LLM. + * + * Measured 2026-08-27: executeDelegation handed subTask.content — the user's + * own text — straight to clientManager.sendMessageNonStreaming, and neither + * the orchestrator nor its IPC handler called any guard. `requireFeature` + * gates the licence, not the content. That bypassed the injection scan, the + * sensitive-data consent, the budget check, the trust routing, the residency + * policy and the audit trail at once. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest' + +// vi.mock factories are hoisted above the file body, so anything they close +// over has to be hoisted too — otherwise the factory runs before the const is +// initialised. +const { sendMessageNonStreaming, routeCalls, preflightGuard } = vi.hoisted(() => ({ + sendMessageNonStreaming: vi.fn(async () => 'Delegated response content'), + routeCalls: [] as Array<{ content: string; providers: string[] }>, + preflightGuard: vi.fn(async () => ({ ok: true, provider: 'anthropic', warnings: [] })) +})) + +vi.mock('../../src/main/llm-clients/client-manager', () => ({ + getClientManager: vi.fn(() => ({ + getProvidersWithApiKeys: vi.fn(() => ['anthropic', 'openai', 'google', 'ollama', 'local']), + sendMessageNonStreaming + })) +})) + +vi.mock('../../src/main/routing/intelligent-router', () => ({ + getRouter: vi.fn(() => ({ + route: vi.fn(async (content: string, providers: string[]) => { + routeCalls.push({ content, providers }) + return { suggestedProvider: 'anthropic', confidence: 0.9, category: 'code', reasoning: 'Code task' } + }) + })) +})) + +vi.mock('../../src/main/utils/id-generator', () => ({ + generateId: vi.fn(() => `test-${Math.random().toString(36).slice(2, 10)}`) +})) + +vi.mock('../../src/main/security/request-guard', () => ({ preflightGuard })) +vi.mock('../../src/main/security/request-guard-deps', () => ({ getGuardDeps: vi.fn(() => ({})) })) + +import { HybridOrchestrator } from '../../src/main/routing/hybrid-orchestrator' + +const MESSAGE = '1. Schreibe eine Funktion zum Sortieren.\n2. Erklaere den Algorithmus dahinter.' + +async function approvedProposal(orchestrator: HybridOrchestrator) { + const proposal = await orchestrator.analyzeForDelegation(MESSAGE, 'openai', 'gpt-4') + expect(proposal).not.toBeNull() + orchestrator.approveProposal(proposal!.id) + return proposal! +} + +describe('delegation runs the shared guard', () => { + let orchestrator: HybridOrchestrator + + beforeEach(() => { + orchestrator = new HybridOrchestrator() + routeCalls.length = 0 + sendMessageNonStreaming.mockClear() + preflightGuard.mockClear() + preflightGuard.mockResolvedValue({ ok: true, provider: 'anthropic', warnings: [] }) + }) + + it('sends nothing when the guard refuses', async () => { + const proposal = await approvedProposal(orchestrator) + preflightGuard.mockResolvedValue({ + ok: false, provider: 'anthropic', blockedKind: 'routing', + reason: 'Content at protection level "high" may not go to "anthropic".', warnings: [] + } as never) + + const result = await orchestrator.executeDelegation(proposal.id) + + // The whole point: refused means NOT SENT, not "sent and flagged". + expect(sendMessageNonStreaming).not.toHaveBeenCalled() + expect(result?.subTaskResults.every((r) => /may not go to/.test(r.response))).toBe(true) + }) + + it('sends to the provider the guard settled on, not the one proposed', async () => { + const proposal = await approvedProposal(orchestrator) + preflightGuard.mockResolvedValue({ ok: true, provider: 'ollama', warnings: [] } as never) + + await orchestrator.executeDelegation(proposal.id) + + expect(sendMessageNonStreaming).toHaveBeenCalled() + for (const call of sendMessageNonStreaming.mock.calls) { + expect((call as unknown[])[0]).toBe('ollama') + } + }) + + it('records the provider that actually received the sub-task', async () => { + const proposal = await approvedProposal(orchestrator) + preflightGuard.mockResolvedValue({ ok: true, provider: 'ollama', warnings: [] } as never) + + const result = await orchestrator.executeDelegation(proposal.id) + + // An audit or cost trail naming the proposed provider instead of the one + // that was used would be wrong in exactly the way that matters. + expect(result?.subTaskResults.every((r) => r.provider === 'ollama')).toBe(true) + }) + + it('guards every sub-task, not just the first', async () => { + const proposal = await approvedProposal(orchestrator) + expect(proposal.subTasks.length).toBeGreaterThan(1) + + await orchestrator.executeDelegation(proposal.id) + + expect(preflightGuard).toHaveBeenCalledTimes(proposal.subTasks.length) + }) + + it('still sends when the guard allows', async () => { + const proposal = await approvedProposal(orchestrator) + + const result = await orchestrator.executeDelegation(proposal.id) + + expect(sendMessageNonStreaming).toHaveBeenCalled() + expect(result?.subTaskResults.every((r) => r.response === 'Delegated response content')).toBe(true) + }) +}) + +describe('delegation candidates', () => { + it('no longer excludes local providers from the candidate set', async () => { + // They were filtered out before routing, so a sub-task the policy would + // only permit on-device had nowhere to go and delegation simply refused. + routeCalls.length = 0 + const orchestrator = new HybridOrchestrator() + + await orchestrator.analyzeForDelegation(MESSAGE, 'openai', 'gpt-4') + + expect(routeCalls.length).toBeGreaterThan(0) + expect(routeCalls[0].providers).toContain('ollama') + // The provider the user is already talking to stays excluded — there is + // nothing to delegate to yourself. + expect(routeCalls[0].providers).not.toContain('openai') + }) +})