Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
b21c84f231 | ||
|
|
33ca4b2a6a | ||
|
|
694f1a4199 | ||
|
|
472dcee7ed | ||
|
|
633acd2d2a |
@@ -1,332 +0,0 @@
|
||||
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<void> => {} },
|
||||
execute: async (): Promise<{ rows: unknown[] }> => ({ rows: [] }),
|
||||
select: () => ({
|
||||
from: () => ({
|
||||
where: async (): Promise<Array<{ count: number }>> => [{ count: 1 }],
|
||||
}),
|
||||
}),
|
||||
insert: () => ({ values: async (): Promise<void> => {} }),
|
||||
};
|
||||
|
||||
const fakeProviderService = {
|
||||
onModuleInit: async (): Promise<void> => {},
|
||||
onModuleDestroy: (): void => {},
|
||||
getRegistry: () => ({ getAvailable: () => [], getAll: () => [], find: () => undefined }),
|
||||
getDefaultModel: () => undefined,
|
||||
listAvailableModels: () => [],
|
||||
listProviders: () => [],
|
||||
getAdapter: () => undefined,
|
||||
getProvidersHealth: () => [],
|
||||
};
|
||||
|
||||
function compileRealAppGraph(): Promise<TestingModule> {
|
||||
return Test.createTestingModule({ imports: [AppModule] })
|
||||
.overrideProvider('DB_HANDLE')
|
||||
.useValue({ db: fakeDb, close: async (): Promise<void> => {} })
|
||||
.overrideProvider('DB')
|
||||
.useValue(fakeDb)
|
||||
.overrideProvider('STORAGE_ADAPTER')
|
||||
.useValue({
|
||||
name: 'required-security-wiring-test',
|
||||
migrate: async (): Promise<void> => {},
|
||||
close: async (): Promise<void> => {},
|
||||
})
|
||||
.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<unknown>;
|
||||
token: Type<unknown>;
|
||||
useValue: object;
|
||||
}
|
||||
|
||||
async function compileWithoutProvider(
|
||||
moduleType: Type<unknown>,
|
||||
missingToken: Type<unknown>,
|
||||
maskingConsumer: MaskingConsumer,
|
||||
): Promise<{ error: unknown; moduleRef: TestingModule | undefined }> {
|
||||
const touchedModules = new Set([moduleType, maskingConsumer.moduleType]);
|
||||
const originals = Array.from(touchedModules, (touchedModule: Type<unknown>) => ({
|
||||
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<void> {
|
||||
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<typeof vi.fn>) {
|
||||
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<typeof vi.fn>): {
|
||||
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<string, AgentSession> };
|
||||
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);
|
||||
});
|
||||
});
|
||||
@@ -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,
|
||||
{ get: vi.fn().mockResolvedValue(null), renew: vi.fn().mockResolvedValue(undefined) } as never,
|
||||
null,
|
||||
null,
|
||||
{ collect: vi.fn().mockResolvedValue(undefined) } as never,
|
||||
operatorMemory as never,
|
||||
|
||||
@@ -132,8 +132,9 @@ 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,
|
||||
private readonly systemOverride: SystemOverrideService | null,
|
||||
@Optional()
|
||||
@Inject(PreferencesService)
|
||||
private readonly preferencesService: PreferencesService | null,
|
||||
@@ -708,22 +709,23 @@ 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).
|
||||
// Required instruction-authority wiring is consulted before session/provider effects.
|
||||
// Prepend session-scoped system override if present (renew TTL on each turn)
|
||||
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}`);
|
||||
}
|
||||
}
|
||||
|
||||
session.promptCount += 1;
|
||||
try {
|
||||
await session.piSession.prompt(effectiveMessage);
|
||||
} catch (err) {
|
||||
|
||||
@@ -80,10 +80,6 @@ const mockMcpClient = {
|
||||
getToolDefinitions: vi.fn(() => []),
|
||||
};
|
||||
|
||||
const allowAuthorization = {
|
||||
authorize: vi.fn().mockResolvedValue({ allowed: true }),
|
||||
};
|
||||
|
||||
function buildService(
|
||||
redis: typeof mockRedis | null = mockRedis,
|
||||
mcpClient: {
|
||||
@@ -102,7 +98,6 @@ function buildService(
|
||||
null,
|
||||
mockChatGateway as never,
|
||||
mcpClient as never,
|
||||
allowAuthorization as never,
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -35,8 +35,9 @@ export class CommandExecutorService {
|
||||
@Inject(forwardRef(() => ChatGateway))
|
||||
private readonly chatGateway: ChatGateway | null,
|
||||
@Inject(McpClientService) private readonly mcpClient: McpClientService,
|
||||
@Optional()
|
||||
@Inject(CommandAuthorizationService)
|
||||
private readonly authorization: CommandAuthorizationService,
|
||||
private readonly authorization: CommandAuthorizationService | null = null,
|
||||
) {}
|
||||
|
||||
async execute(
|
||||
@@ -56,13 +57,13 @@ export class CommandExecutorService {
|
||||
};
|
||||
}
|
||||
|
||||
const authorization = await this.authorization.authorize(
|
||||
const authorization = await this.authorization?.authorize(
|
||||
def,
|
||||
payload,
|
||||
userId,
|
||||
payload.approvalId,
|
||||
);
|
||||
if (!authorization.allowed) {
|
||||
if (authorization && !authorization.allowed) {
|
||||
return { command, conversationId, success: false, message: authorization.reason };
|
||||
}
|
||||
|
||||
@@ -170,7 +171,7 @@ export class CommandExecutorService {
|
||||
const def = this.registry
|
||||
.getManifest()
|
||||
.commands.find((command) => command.name === payload.command);
|
||||
if (!def) return null;
|
||||
if (!def || !this.authorization) return null;
|
||||
return this.authorization.createApproval(def, payload, scope.userId);
|
||||
}
|
||||
|
||||
|
||||
@@ -55,10 +55,6 @@ const mockMcpClient = {
|
||||
reconnectServer: vi.fn().mockResolvedValue(undefined),
|
||||
};
|
||||
|
||||
const allowAuthorization = {
|
||||
authorize: vi.fn().mockResolvedValue({ allowed: true }),
|
||||
};
|
||||
|
||||
// ─── Helpers ─────────────────────────────────────────────────────────────────
|
||||
|
||||
function buildRegistry(): CommandRegistryService {
|
||||
@@ -78,7 +74,6 @@ function buildExecutor(registry: CommandRegistryService): CommandExecutorService
|
||||
null, // reloadService (optional)
|
||||
null, // chatGateway (optional)
|
||||
mockMcpClient as never,
|
||||
allowAuthorization as never,
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -159,7 +159,6 @@ 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' };
|
||||
|
||||
@@ -1,77 +0,0 @@
|
||||
# #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.
|
||||
Reference in New Issue
Block a user