From 4c50a07ba27ee7172bef2e64ffa24e29d2c16a42 Mon Sep 17 00:00:00 2001 From: coder2 Date: Wed, 12 Aug 2026 19:58:13 -0500 Subject: [PATCH] fix(#1179): require security authority wiring --- .../required-security-wiring.test.ts | 332 ++++++++++++++++++ .../__tests__/agent-service-ownership.test.ts | 2 +- apps/gateway/src/agent/agent.service.ts | 20 +- .../commands/command-executor-p8012.spec.ts | 5 + .../src/commands/command-executor.service.ts | 9 +- .../src/commands/commands.integration.spec.ts | 5 + .../gateway/src/reload/reload.service.spec.ts | 1 + docs/scratchpads/1179-required-security-di.md | 77 ++++ 8 files changed, 434 insertions(+), 17 deletions(-) create mode 100644 apps/gateway/src/__tests__/required-security-wiring.test.ts create mode 100644 docs/scratchpads/1179-required-security-di.md diff --git a/apps/gateway/src/__tests__/required-security-wiring.test.ts b/apps/gateway/src/__tests__/required-security-wiring.test.ts new file mode 100644 index 00000000..3decf9d3 --- /dev/null +++ b/apps/gateway/src/__tests__/required-security-wiring.test.ts @@ -0,0 +1,332 @@ +import { type Type } from '@nestjs/common'; +import { Test, type TestingModule } from '@nestjs/testing'; +import type { SlashCommandPayload } from '@mosaicstack/types'; +import { describe, expect, it, vi } from 'vitest'; +import { AgentService, type AgentSession } from '../agent/agent.service.js'; +import { ProviderService } from '../agent/provider.service.js'; +import { AppModule } from '../app.module.js'; +import { CommandAuthorizationService } from '../commands/command-authorization.service.js'; +import { CommandExecutorService } from '../commands/command-executor.service.js'; +import { CommandsModule } from '../commands/commands.module.js'; +import { CommandRuntimeApprovalVerifier } from '../commands/runtime-approval-verifier.js'; +import { PreferencesModule } from '../preferences/preferences.module.js'; +import { SystemOverrideService } from '../preferences/system-override.service.js'; + +const fakeDb = { + $client: { exec: async (): Promise => {} }, + execute: async (): Promise<{ rows: unknown[] }> => ({ rows: [] }), + select: () => ({ + from: () => ({ + where: async (): Promise> => [{ count: 1 }], + }), + }), + insert: () => ({ values: async (): Promise => {} }), +}; + +const fakeProviderService = { + onModuleInit: async (): Promise => {}, + onModuleDestroy: (): void => {}, + getRegistry: () => ({ getAvailable: () => [], getAll: () => [], find: () => undefined }), + getDefaultModel: () => undefined, + listAvailableModels: () => [], + listProviders: () => [], + getAdapter: () => undefined, + getProvidersHealth: () => [], +}; + +function compileRealAppGraph(): Promise { + return Test.createTestingModule({ imports: [AppModule] }) + .overrideProvider('DB_HANDLE') + .useValue({ db: fakeDb, close: async (): Promise => {} }) + .overrideProvider('DB') + .useValue(fakeDb) + .overrideProvider('STORAGE_ADAPTER') + .useValue({ + name: 'required-security-wiring-test', + migrate: async (): Promise => {}, + close: async (): Promise => {}, + }) + .overrideProvider('AUTH') + .useValue({}) + .overrideProvider('BRAIN') + .useValue({ conversations: {}, agents: {} }) + .overrideProvider('LOG_SERVICE') + .useValue({}) + .overrideProvider('MEMORY') + .useValue({}) + .overrideProvider('MEMORY_ADAPTER') + .useValue({}) + .overrideProvider(ProviderService) + .useValue(fakeProviderService) + .compile(); +} + +function providerToken(provider: unknown): unknown { + return typeof provider === 'function' ? provider : (provider as { provide?: unknown })?.provide; +} + +interface MaskingConsumer { + moduleType: Type; + token: Type; + useValue: object; +} + +async function compileWithoutProvider( + moduleType: Type, + missingToken: Type, + maskingConsumer: MaskingConsumer, +): Promise<{ error: unknown; moduleRef: TestingModule | undefined }> { + const touchedModules = new Set([moduleType, maskingConsumer.moduleType]); + const originals = Array.from(touchedModules, (touchedModule: Type) => ({ + moduleType: touchedModule, + providers: (Reflect.getMetadata('providers', touchedModule) ?? []) as unknown[], + exports: (Reflect.getMetadata('exports', touchedModule) ?? []) as unknown[], + })); + + for (const original of originals) { + const providers = original.providers.flatMap((provider: unknown): unknown[] => { + const token = providerToken(provider); + if (original.moduleType === moduleType && token === missingToken) return []; + if (original.moduleType === maskingConsumer.moduleType && token === maskingConsumer.token) { + return [{ provide: maskingConsumer.token, useValue: maskingConsumer.useValue }]; + } + return [provider]; + }); + const exports = original.exports.filter( + (exported: unknown): boolean => + original.moduleType !== moduleType || providerToken(exported) !== missingToken, + ); + Reflect.defineMetadata('providers', providers, original.moduleType); + Reflect.defineMetadata('exports', exports, original.moduleType); + } + + let moduleRef: TestingModule | undefined; + let error: unknown; + try { + moduleRef = await compileRealAppGraph(); + } catch (caught: unknown) { + error = caught; + } finally { + for (const original of originals) { + Reflect.defineMetadata('providers', original.providers, original.moduleType); + Reflect.defineMetadata('exports', original.exports, original.moduleType); + } + } + return { error, moduleRef }; +} + +async function closeIfCompiled(moduleRef: TestingModule | undefined): Promise { + if (moduleRef) await moduleRef.close(); +} + +describe('required security wiring — real AppModule startup refusal', () => { + it('FL-01 positive control: the real graph compiles when CommandAuthorizationService is bound', async () => { + const moduleRef = await compileRealAppGraph(); + try { + expect(moduleRef.get(CommandAuthorizationService, { strict: false })).toBeInstanceOf( + CommandAuthorizationService, + ); + } finally { + await moduleRef.close(); + } + }); + + it('FL-01 negative control: absence read as permission is refused at module compilation', async () => { + const { error, moduleRef } = await compileWithoutProvider( + CommandsModule, + CommandAuthorizationService, + { + moduleType: CommandsModule, + token: CommandRuntimeApprovalVerifier, + useValue: {}, + }, + ); + await closeIfCompiled(moduleRef); + + expect( + error, + 'absence read as permission: AppModule compilation accepted a missing CommandAuthorizationService binding', + ).toBeInstanceOf(Error); + if (!(error instanceof Error)) return; + expect(error.message).toContain('CommandExecutorService'); + expect(error.message).toContain('CommandAuthorizationService'); + }); + + it('FL-11 positive control: the real graph compiles when SystemOverrideService is bound', async () => { + const moduleRef = await compileRealAppGraph(); + try { + expect(moduleRef.get(SystemOverrideService, { strict: false })).toBeInstanceOf( + SystemOverrideService, + ); + } finally { + await moduleRef.close(); + } + }); + + it('FL-11 negative control: absence read as permission is refused at module compilation', async () => { + const { error, moduleRef } = await compileWithoutProvider( + PreferencesModule, + SystemOverrideService, + { + moduleType: CommandsModule, + token: CommandExecutorService, + useValue: {}, + }, + ); + await closeIfCompiled(moduleRef); + + expect( + error, + 'absence read as permission: AppModule compilation accepted a missing SystemOverrideService binding', + ).toBeInstanceOf(Error); + if (!(error instanceof Error)) return; + expect(error.message).toContain('AgentService'); + expect(error.message).toContain('SystemOverrideService'); + }); +}); + +const actorScope = { userId: 'security-user', tenantId: 'security-tenant' }; +const conversationId = 'security-conversation'; + +function directExecutorWithoutAuthorization(systemOverrideSet: ReturnType) { + const registry = { + getManifest: vi.fn(() => ({ + version: 1, + commands: [ + { + name: 'system', + aliases: [], + description: 'Set instruction authority', + scope: 'agent' as const, + execution: 'socket' as const, + available: true, + }, + ], + skills: [], + })), + }; + return new CommandExecutorService( + registry as never, + { getSession: vi.fn() } as never, + { set: systemOverrideSet, clear: vi.fn() } as never, + { collect: vi.fn() } as never, + null, + { agents: {} } as never, + null, + null, + { getServerStatuses: vi.fn(() => []), getToolDefinitions: vi.fn(() => []) } as never, + undefined as never, + ); +} + +function directAgentWithoutSystemOverride(piPrompt: ReturnType): { + service: AgentService; + session: AgentSession; +} { + const service = new AgentService( + { + getDefaultModel: vi.fn(() => null), + getRegistry: vi.fn(() => ({})), + findModel: vi.fn(), + listAvailableModels: vi.fn(() => []), + } as never, + {} as never, + {} as never, + { available: false } as never, + {} as never, + { getToolDefinitions: vi.fn(() => []) } as never, + { loadForSession: vi.fn(async () => ({ metaTools: [], promptAdditions: [] })) } as never, + undefined as never, + null, + { collect: vi.fn().mockResolvedValue(undefined) } as never, + null, + ); + const session = { + id: conversationId, + provider: 'test-provider', + modelId: 'test-model', + piSession: { prompt: piPrompt }, + listeners: new Set(), + unsubscribe: vi.fn(), + createdAt: Date.now(), + promptCount: 0, + channels: new Set(), + skillPromptAdditions: [], + sandboxDir: process.cwd(), + allowedTools: null, + userId: actorScope.userId, + tenantId: actorScope.tenantId, + metrics: { + tokens: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + modelSwitches: 0, + messageCount: 0, + lastActivityAt: new Date(0).toISOString(), + }, + } as unknown as AgentSession; + const internals = service as unknown as { sessions: Map }; + internals.sessions.set(conversationId, session); + return { service, session }; +} + +describe('required security wiring — malformed direct absence has zero effects', () => { + it('FL-01 refuses command execution before any command effect when authorization is absent', async () => { + const systemOverrideSet = vi.fn().mockResolvedValue(undefined); + const executor = directExecutorWithoutAuthorization(systemOverrideSet); + const payload: SlashCommandPayload = { + command: 'system', + args: 'authority that must not be stored', + conversationId, + }; + let error: unknown; + + try { + await executor.execute(payload, actorScope); + } catch (caught: unknown) { + error = caught; + } + + expect + .soft( + error, + 'absence read as permission: direct executor accepted missing command authorization', + ) + .toBeInstanceOf(Error); + expect + .soft( + systemOverrideSet, + 'absence read as permission: command effect occurred without command authorization', + ) + .not.toHaveBeenCalled(); + }); + + it('FL-11 refuses prompt execution before any provider or session effect when system override authority is absent', async () => { + const piPrompt = vi.fn().mockResolvedValue(undefined); + const { service, session } = directAgentWithoutSystemOverride(piPrompt); + let error: unknown; + + try { + await service.prompt(conversationId, 'must not reach provider', actorScope); + } catch (caught: unknown) { + error = caught; + } + + expect + .soft( + error, + 'absence read as permission: direct session accepted missing system override authority', + ) + .toBeInstanceOf(Error); + expect + .soft( + piPrompt, + 'absence read as permission: provider prompt occurred without system override authority', + ) + .not.toHaveBeenCalled(); + expect + .soft( + session.promptCount, + 'absence read as permission: session state changed without system override authority', + ) + .toBe(0); + }); +}); diff --git a/apps/gateway/src/agent/__tests__/agent-service-ownership.test.ts b/apps/gateway/src/agent/__tests__/agent-service-ownership.test.ts index dc6b9bfd..53c45e19 100644 --- a/apps/gateway/src/agent/__tests__/agent-service-ownership.test.ts +++ b/apps/gateway/src/agent/__tests__/agent-service-ownership.test.ts @@ -26,7 +26,7 @@ function makeService(operatorMemory: unknown = null): AgentService { {} as never, { getToolDefinitions: vi.fn(() => []) } as never, { loadForSession: vi.fn(async () => ({ metaTools: [], promptAdditions: [] })) } as never, - null, + { get: vi.fn().mockResolvedValue(null), renew: vi.fn().mockResolvedValue(undefined) } as never, null, { collect: vi.fn().mockResolvedValue(undefined) } as never, operatorMemory as never, diff --git a/apps/gateway/src/agent/agent.service.ts b/apps/gateway/src/agent/agent.service.ts index 38192969..d9a06bef 100644 --- a/apps/gateway/src/agent/agent.service.ts +++ b/apps/gateway/src/agent/agent.service.ts @@ -132,9 +132,8 @@ export class AgentService implements OnModuleDestroy { @Inject(CoordService) private readonly coordService: CoordService, @Inject(McpClientService) private readonly mcpClientService: McpClientService, @Inject(SkillLoaderService) private readonly skillLoaderService: SkillLoaderService, - @Optional() @Inject(SystemOverrideService) - private readonly systemOverride: SystemOverrideService | null, + private readonly systemOverride: SystemOverrideService, @Optional() @Inject(PreferencesService) private readonly preferencesService: PreferencesService | null, @@ -709,23 +708,22 @@ export class AgentService implements OnModuleDestroy { throw new Error(`No agent session found: ${sessionId}`); } this.assertSessionScope(session, scope); - session.promptCount += 1; // Channel attachments are untrusted URI references. Preserve exact, // authenticated metadata for the agent without treating it as authority. const attachmentContext = this.attachmentContext(attachments); - // Prepend session-scoped system override if present (renew TTL on each turn) + // Prepend session-scoped system override if present (renew TTL on each turn). + // Required instruction-authority wiring is consulted before session/provider effects. let effectiveMessage = `${message}${attachmentContext}`; - if (this.systemOverride) { - const override = await this.systemOverride.get(sessionId, scope); - if (override) { - effectiveMessage = `[System Override]\n${override}\n\n${effectiveMessage}`; - await this.systemOverride.renew(sessionId, scope); - this.logger.debug(`Applied system override for session ${sessionId}`); - } + const override = await this.systemOverride.get(sessionId, scope); + if (override) { + effectiveMessage = `[System Override]\n${override}\n\n${effectiveMessage}`; + await this.systemOverride.renew(sessionId, scope); + this.logger.debug(`Applied system override for session ${sessionId}`); } + session.promptCount += 1; try { await session.piSession.prompt(effectiveMessage); } catch (err) { diff --git a/apps/gateway/src/commands/command-executor-p8012.spec.ts b/apps/gateway/src/commands/command-executor-p8012.spec.ts index 28fbedad..730ca2ff 100644 --- a/apps/gateway/src/commands/command-executor-p8012.spec.ts +++ b/apps/gateway/src/commands/command-executor-p8012.spec.ts @@ -80,6 +80,10 @@ const mockMcpClient = { getToolDefinitions: vi.fn(() => []), }; +const allowAuthorization = { + authorize: vi.fn().mockResolvedValue({ allowed: true }), +}; + function buildService( redis: typeof mockRedis | null = mockRedis, mcpClient: { @@ -98,6 +102,7 @@ function buildService( null, mockChatGateway as never, mcpClient as never, + allowAuthorization as never, ); } diff --git a/apps/gateway/src/commands/command-executor.service.ts b/apps/gateway/src/commands/command-executor.service.ts index 78a4efcb..0cee2721 100644 --- a/apps/gateway/src/commands/command-executor.service.ts +++ b/apps/gateway/src/commands/command-executor.service.ts @@ -35,9 +35,8 @@ export class CommandExecutorService { @Inject(forwardRef(() => ChatGateway)) private readonly chatGateway: ChatGateway | null, @Inject(McpClientService) private readonly mcpClient: McpClientService, - @Optional() @Inject(CommandAuthorizationService) - private readonly authorization: CommandAuthorizationService | null = null, + private readonly authorization: CommandAuthorizationService, ) {} async execute( @@ -57,13 +56,13 @@ export class CommandExecutorService { }; } - const authorization = await this.authorization?.authorize( + const authorization = await this.authorization.authorize( def, payload, userId, payload.approvalId, ); - if (authorization && !authorization.allowed) { + if (!authorization.allowed) { return { command, conversationId, success: false, message: authorization.reason }; } @@ -171,7 +170,7 @@ export class CommandExecutorService { const def = this.registry .getManifest() .commands.find((command) => command.name === payload.command); - if (!def || !this.authorization) return null; + if (!def) return null; return this.authorization.createApproval(def, payload, scope.userId); } diff --git a/apps/gateway/src/commands/commands.integration.spec.ts b/apps/gateway/src/commands/commands.integration.spec.ts index e1806bcd..99fbfb91 100644 --- a/apps/gateway/src/commands/commands.integration.spec.ts +++ b/apps/gateway/src/commands/commands.integration.spec.ts @@ -55,6 +55,10 @@ const mockMcpClient = { reconnectServer: vi.fn().mockResolvedValue(undefined), }; +const allowAuthorization = { + authorize: vi.fn().mockResolvedValue({ allowed: true }), +}; + // ─── Helpers ───────────────────────────────────────────────────────────────── function buildRegistry(): CommandRegistryService { @@ -74,6 +78,7 @@ function buildExecutor(registry: CommandRegistryService): CommandExecutorService null, // reloadService (optional) null, // chatGateway (optional) mockMcpClient as never, + allowAuthorization as never, ); } diff --git a/apps/gateway/src/reload/reload.service.spec.ts b/apps/gateway/src/reload/reload.service.spec.ts index 63a89781..244d6941 100644 --- a/apps/gateway/src/reload/reload.service.spec.ts +++ b/apps/gateway/src/reload/reload.service.spec.ts @@ -159,6 +159,7 @@ describe('ReloadService — /reload command sanitizes plugin errors', () => { reloadService, mockChatGateway as never, mockMcpClient as never, + { authorize: vi.fn().mockResolvedValue({ allowed: true }) } as never, ); const payload: SlashCommandPayload = { command: 'reload', conversationId: 'conv-1' }; diff --git a/docs/scratchpads/1179-required-security-di.md b/docs/scratchpads/1179-required-security-di.md new file mode 100644 index 00000000..c7719d39 --- /dev/null +++ b/docs/scratchpads/1179-required-security-di.md @@ -0,0 +1,77 @@ +# #1179 — Required security DI wiring + +## Objective + +Eliminate the shared fail-open defect class **absence read as permission**: + +- FL-01: missing `CommandAuthorizationService` must refuse Nest startup and must not permit command effects. +- FL-11: missing `SystemOverrideService` must refuse Nest startup and must not omit stored instruction authority while allowing provider/session effects. + +## Tracking + +- Issue: #1179, child of #1156 +- Branch: `fix/1179-required-security-di` +- Base: `origin/next` at `216cd72226cd9ee17eea461cfe7cd0e010a22f02` + +## Plan + +1. RED: compile the real `AppModule` graph with each required provider independently removed, with a positive control for each intact binding. +2. RED: directly exercise each malformed absence path and assert zero command/provider/session effects. +3. Stop and report RED to the coordinator before production implementation. +4. After authorization, make both constructor injections required, remove absence-as-permission branches, and update explicit legitimate optional test seams. +5. Run focused Gateway tests, typecheck, lint, format, build, independent exact-head verification, and focused security review. + +## Immutable path fence + +Production changes are confined to: + +- `apps/gateway/src/commands/command-executor.service.ts` +- `apps/gateway/src/agent/agent.service.ts` + +Tests and task evidence are confined to: + +- `apps/gateway/src/__tests__/required-security-wiring.test.ts` +- existing direct-constructor specs that require explicit required arguments +- `docs/scratchpads/1179-required-security-di.md` + +No files in #1178, #1072, #1080, or #1054 lanes are in scope. `docs/TASKS.md` is orchestrator-owned and will not be modified. + +## Budget + +No explicit token ceiling was provided. Working assumption: one narrow Gateway security packet; split and stop if either arm requires unrelated module rewiring. + +## Progress + +- Intake read from #1179 and parent #1156. +- Base independently resolved from the issue's pre-native-stage ordering and repository `origin/next` ref; branch HEAD verified byte-for-byte against the remote ref. +- Real consumers and direct constructors inventoried. + +## Tests + +### RED + +- `required-security-wiring.test.ts`: 4 failed, 2 passed before implementation. +- Both real-graph negative controls showed module compilation accepted the missing target binding. +- Direct FL-01 showed one unauthorized command effect; direct FL-11 showed one provider prompt and one session counter mutation. + +### GREEN + +- `required-security-wiring.test.ts`: 6/6 passed. +- FL-01-only production revert: exactly the two FL-01 test cases failed; all four other cases, including FL-11, passed. +- FL-11-only production revert: exactly the two FL-11 test cases failed; all four other cases, including FL-01, passed. +- Full Gateway suite: 74 files passed, 7 skipped; 831 tests passed, 17 skipped. +- Gateway typecheck: passed. +- Gateway lint: passed. +- Gateway build: passed. +- Changed-file Prettier check: passed. + +### Review + +- Codex code review: APPROVE, 0 findings. +- Codex focused security review: risk `none`, 0 findings. +- Independent exact-head review remains assigned to Scrappy through the coordinator. + +## Risks / blockers + +- `AgentModule` / `CommandsModule` / `ChatModule` contain a production cycle; the module test therefore uses the real top-level `AppModule` and replaces only storage/network leaves, preserving the target service in each arm while isolating the separate required consumer that would otherwise mask that arm's defect. +- No broad module rewrite was required.