Compare commits
2 Commits
fix/ms22-a
...
fix/orches
| Author | SHA1 | Date | |
|---|---|---|---|
| 869bf88104 | |||
| d2e645a9b2 |
@@ -1,6 +1,6 @@
|
||||
import { Injectable, NotFoundException } from "@nestjs/common";
|
||||
import type { LlmProvider } from "@prisma/client";
|
||||
import { createHash, timingSafeEqual } from "node:crypto";
|
||||
import { timingSafeEqual } from "node:crypto";
|
||||
import { PrismaService } from "../prisma/prisma.service";
|
||||
import { CryptoService } from "../crypto/crypto.service";
|
||||
|
||||
@@ -143,23 +143,21 @@ export class AgentConfigService {
|
||||
}),
|
||||
]);
|
||||
|
||||
let match: ContainerTokenValidation | null = null;
|
||||
|
||||
for (const container of userContainers) {
|
||||
const storedToken = this.decryptContainerToken(container.gatewayToken);
|
||||
if (!match && storedToken && this.tokensEqual(storedToken, token)) {
|
||||
match = { type: "user", id: container.id };
|
||||
if (storedToken && this.tokensEqual(storedToken, token)) {
|
||||
return { type: "user", id: container.id };
|
||||
}
|
||||
}
|
||||
|
||||
for (const container of systemContainers) {
|
||||
const storedToken = this.decryptContainerToken(container.gatewayToken);
|
||||
if (!match && storedToken && this.tokensEqual(storedToken, token)) {
|
||||
match = { type: "system", id: container.id };
|
||||
if (storedToken && this.tokensEqual(storedToken, token)) {
|
||||
return { type: "system", id: container.id };
|
||||
}
|
||||
}
|
||||
|
||||
return match;
|
||||
return null;
|
||||
}
|
||||
|
||||
private buildOpenClawConfig(
|
||||
@@ -270,9 +268,14 @@ export class AgentConfigService {
|
||||
}
|
||||
|
||||
private tokensEqual(left: string, right: string): boolean {
|
||||
const leftDigest = createHash("sha256").update(left, "utf8").digest();
|
||||
const rightDigest = createHash("sha256").update(right, "utf8").digest();
|
||||
return timingSafeEqual(leftDigest, rightDigest);
|
||||
const leftBuffer = Buffer.from(left, "utf8");
|
||||
const rightBuffer = Buffer.from(right, "utf8");
|
||||
|
||||
if (leftBuffer.length !== rightBuffer.length) {
|
||||
return false;
|
||||
}
|
||||
|
||||
return timingSafeEqual(leftBuffer, rightBuffer);
|
||||
}
|
||||
|
||||
private hasModelId(modelEntry: unknown): modelEntry is { id: string } {
|
||||
|
||||
@@ -1,14 +1,4 @@
|
||||
import {
|
||||
Body,
|
||||
Controller,
|
||||
HttpException,
|
||||
Logger,
|
||||
Post,
|
||||
Req,
|
||||
Res,
|
||||
UnauthorizedException,
|
||||
UseGuards,
|
||||
} from "@nestjs/common";
|
||||
import { Body, Controller, Post, Req, Res, UnauthorizedException, UseGuards } from "@nestjs/common";
|
||||
import type { Response } from "express";
|
||||
import { AuthGuard } from "../auth/guards/auth.guard";
|
||||
import type { MaybeAuthenticatedRequest } from "../auth/types/better-auth-request.interface";
|
||||
@@ -18,8 +8,6 @@ import { ChatProxyService } from "./chat-proxy.service";
|
||||
@Controller("chat")
|
||||
@UseGuards(AuthGuard)
|
||||
export class ChatProxyController {
|
||||
private readonly logger = new Logger(ChatProxyController.name);
|
||||
|
||||
constructor(private readonly chatProxyService: ChatProxyService) {}
|
||||
|
||||
// POST /api/chat/stream
|
||||
@@ -70,11 +58,10 @@ export class ChatProxyController {
|
||||
res.write(Buffer.from(chunk));
|
||||
}
|
||||
} catch (error: unknown) {
|
||||
this.logStreamError(error);
|
||||
|
||||
if (!res.writableEnded && !res.destroyed) {
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
res.write("event: error\n");
|
||||
res.write(`data: ${JSON.stringify({ error: this.toSafeClientMessage(error) })}\n\n`);
|
||||
res.write(`data: ${JSON.stringify({ error: message })}\n\n`);
|
||||
}
|
||||
} finally {
|
||||
if (!res.writableEnded && !res.destroyed) {
|
||||
@@ -82,21 +69,4 @@ export class ChatProxyController {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private toSafeClientMessage(error: unknown): string {
|
||||
if (error instanceof HttpException && error.getStatus() < 500) {
|
||||
return "Chat request was rejected";
|
||||
}
|
||||
|
||||
return "Chat stream failed";
|
||||
}
|
||||
|
||||
private logStreamError(error: unknown): void {
|
||||
if (error instanceof Error) {
|
||||
this.logger.warn(`Chat stream failed: ${error.message}`);
|
||||
return;
|
||||
}
|
||||
|
||||
this.logger.warn(`Chat stream failed: ${String(error)}`);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -64,7 +64,6 @@ describe("ChatProxyService", () => {
|
||||
expect.objectContaining({
|
||||
method: "POST",
|
||||
headers: {
|
||||
Authorization: "Bearer gateway-token",
|
||||
"Content-Type": "application/json",
|
||||
},
|
||||
})
|
||||
|
||||
@@ -1,24 +1,12 @@
|
||||
import {
|
||||
BadGatewayException,
|
||||
Injectable,
|
||||
Logger,
|
||||
ServiceUnavailableException,
|
||||
} from "@nestjs/common";
|
||||
import { BadGatewayException, Injectable, ServiceUnavailableException } from "@nestjs/common";
|
||||
import { ContainerLifecycleService } from "../container-lifecycle/container-lifecycle.service";
|
||||
import { PrismaService } from "../prisma/prisma.service";
|
||||
import type { ChatMessage } from "./chat-proxy.dto";
|
||||
|
||||
const DEFAULT_OPENCLAW_MODEL = "openclaw:default";
|
||||
|
||||
interface ContainerConnection {
|
||||
url: string;
|
||||
token: string;
|
||||
}
|
||||
|
||||
@Injectable()
|
||||
export class ChatProxyService {
|
||||
private readonly logger = new Logger(ChatProxyService.name);
|
||||
|
||||
constructor(
|
||||
private readonly prisma: PrismaService,
|
||||
private readonly containerLifecycle: ContainerLifecycleService
|
||||
@@ -26,7 +14,8 @@ export class ChatProxyService {
|
||||
|
||||
// Get the user's OpenClaw container URL and mark it active.
|
||||
async getContainerUrl(userId: string): Promise<string> {
|
||||
const { url } = await this.getContainerConnection(userId);
|
||||
const { url } = await this.containerLifecycle.ensureRunning(userId);
|
||||
await this.containerLifecycle.touch(userId);
|
||||
return url;
|
||||
}
|
||||
|
||||
@@ -36,14 +25,11 @@ export class ChatProxyService {
|
||||
messages: ChatMessage[],
|
||||
signal?: AbortSignal
|
||||
): Promise<Response> {
|
||||
const { url: containerUrl, token: gatewayToken } = await this.getContainerConnection(userId);
|
||||
const containerUrl = await this.getContainerUrl(userId);
|
||||
const model = await this.getPreferredModel(userId);
|
||||
const requestInit: RequestInit = {
|
||||
method: "POST",
|
||||
headers: {
|
||||
"Content-Type": "application/json",
|
||||
Authorization: `Bearer ${gatewayToken}`,
|
||||
},
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body: JSON.stringify({
|
||||
messages,
|
||||
model,
|
||||
@@ -61,10 +47,10 @@ export class ChatProxyService {
|
||||
if (!response.ok) {
|
||||
const detail = await this.readResponseText(response);
|
||||
const status = `${String(response.status)} ${response.statusText}`.trim();
|
||||
this.logger.warn(
|
||||
detail ? `OpenClaw returned ${status}: ${detail}` : `OpenClaw returned ${status}`
|
||||
);
|
||||
throw new BadGatewayException(`OpenClaw returned ${status}`);
|
||||
const message = detail
|
||||
? `OpenClaw returned ${status}: ${detail}`
|
||||
: `OpenClaw returned ${status}`;
|
||||
throw new BadGatewayException(message);
|
||||
}
|
||||
|
||||
return response;
|
||||
@@ -74,17 +60,10 @@ export class ChatProxyService {
|
||||
}
|
||||
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
this.logger.warn(`Failed to proxy chat request: ${message}`);
|
||||
throw new ServiceUnavailableException("Failed to proxy chat to OpenClaw");
|
||||
throw new ServiceUnavailableException(`Failed to proxy chat to OpenClaw: ${message}`);
|
||||
}
|
||||
}
|
||||
|
||||
private async getContainerConnection(userId: string): Promise<ContainerConnection> {
|
||||
const connection = await this.containerLifecycle.ensureRunning(userId);
|
||||
await this.containerLifecycle.touch(userId);
|
||||
return connection;
|
||||
}
|
||||
|
||||
private async getPreferredModel(userId: string): Promise<string> {
|
||||
const config = await this.prisma.userAgentConfig.findUnique({
|
||||
where: { userId },
|
||||
|
||||
@@ -85,14 +85,6 @@ const INITIAL_FORM: ProviderFormState = {
|
||||
isActive: true,
|
||||
};
|
||||
|
||||
function mapProviderTypeToApi(type: string): "ollama" | "openai" | "claude" {
|
||||
if (type === "ollama" || type === "claude") {
|
||||
return type;
|
||||
}
|
||||
|
||||
return "openai";
|
||||
}
|
||||
|
||||
function getErrorMessage(error: unknown, fallback: string): string {
|
||||
if (error instanceof Error && error.message.trim().length > 0) {
|
||||
return error.message;
|
||||
@@ -101,6 +93,18 @@ function getErrorMessage(error: unknown, fallback: string): string {
|
||||
return fallback;
|
||||
}
|
||||
|
||||
function buildProviderName(displayName: string, type: string): string {
|
||||
const slug = displayName
|
||||
.trim()
|
||||
.toLowerCase()
|
||||
.replace(/[^a-z0-9]+/g, "-")
|
||||
.replace(/^-+/, "")
|
||||
.replace(/-+$/, "");
|
||||
|
||||
const candidate = `${type}-${slug.length > 0 ? slug : "provider"}`;
|
||||
return candidate.slice(0, 100);
|
||||
}
|
||||
|
||||
function normalizeProviderModels(models: unknown): FleetProviderModel[] {
|
||||
if (!Array.isArray(models)) {
|
||||
return [];
|
||||
@@ -149,11 +153,11 @@ function modelsToEditorText(models: unknown): string {
|
||||
.join("\n");
|
||||
}
|
||||
|
||||
function parseModelsText(value: string): string[] {
|
||||
function parseModelsText(value: string): FleetProviderModel[] {
|
||||
const seen = new Set<string>();
|
||||
|
||||
return value
|
||||
.split(/\r?\n/g)
|
||||
.split(/\n|,/g)
|
||||
.map((segment) => segment.trim())
|
||||
.filter((segment) => segment.length > 0)
|
||||
.filter((segment) => {
|
||||
@@ -162,7 +166,8 @@ function parseModelsText(value: string): string[] {
|
||||
}
|
||||
seen.add(segment);
|
||||
return true;
|
||||
});
|
||||
})
|
||||
.map((id) => ({ id, name: id }));
|
||||
}
|
||||
|
||||
function maskApiKey(value: string): string {
|
||||
@@ -274,7 +279,6 @@ export default function ProvidersSettingsPage(): ReactElement {
|
||||
}
|
||||
|
||||
const models = parseModelsText(form.modelsText);
|
||||
const providerModels = models.map((id) => ({ id, name: id }));
|
||||
const baseUrl = form.baseUrl.trim();
|
||||
const apiKey = form.apiKey.trim();
|
||||
|
||||
@@ -285,7 +289,7 @@ export default function ProvidersSettingsPage(): ReactElement {
|
||||
const updatePayload: UpdateFleetProviderRequest = {
|
||||
displayName,
|
||||
isActive: form.isActive,
|
||||
models: providerModels,
|
||||
models,
|
||||
};
|
||||
|
||||
if (baseUrl.length > 0) {
|
||||
@@ -299,27 +303,21 @@ export default function ProvidersSettingsPage(): ReactElement {
|
||||
await updateFleetProvider(editingProvider.id, updatePayload);
|
||||
setSuccessMessage(`Updated provider "${displayName}".`);
|
||||
} else {
|
||||
const config: CreateFleetProviderRequest["config"] = {};
|
||||
const createPayload: CreateFleetProviderRequest = {
|
||||
name: buildProviderName(displayName, form.type),
|
||||
displayName,
|
||||
type: form.type,
|
||||
models,
|
||||
};
|
||||
|
||||
if (baseUrl.length > 0) {
|
||||
config.endpoint = baseUrl;
|
||||
createPayload.baseUrl = baseUrl;
|
||||
}
|
||||
|
||||
if (apiKey.length > 0) {
|
||||
config.apiKey = apiKey;
|
||||
createPayload.apiKey = apiKey;
|
||||
}
|
||||
|
||||
if (models.length > 0) {
|
||||
config.models = models;
|
||||
}
|
||||
|
||||
const createPayload: CreateFleetProviderRequest = {
|
||||
displayName,
|
||||
providerType: mapProviderTypeToApi(form.type),
|
||||
config,
|
||||
isEnabled: form.isActive,
|
||||
};
|
||||
|
||||
await createFleetProvider(createPayload);
|
||||
setSuccessMessage(`Added provider "${displayName}".`);
|
||||
}
|
||||
|
||||
@@ -34,25 +34,17 @@ describe("createFleetProvider", (): void => {
|
||||
vi.mocked(client.apiPost).mockResolvedValueOnce({ id: "provider-1" } as never);
|
||||
|
||||
await createFleetProvider({
|
||||
providerType: "openai",
|
||||
name: "openai-main",
|
||||
displayName: "OpenAI Main",
|
||||
config: {
|
||||
endpoint: "https://api.openai.com/v1",
|
||||
apiKey: "sk-test",
|
||||
models: ["gpt-4.1-mini", "gpt-4o-mini"],
|
||||
},
|
||||
isEnabled: true,
|
||||
type: "openai",
|
||||
apiKey: "sk-test",
|
||||
});
|
||||
|
||||
expect(client.apiPost).toHaveBeenCalledWith("/api/fleet-settings/providers", {
|
||||
providerType: "openai",
|
||||
name: "openai-main",
|
||||
displayName: "OpenAI Main",
|
||||
config: {
|
||||
endpoint: "https://api.openai.com/v1",
|
||||
apiKey: "sk-test",
|
||||
models: ["gpt-4.1-mini", "gpt-4o-mini"],
|
||||
},
|
||||
isEnabled: true,
|
||||
type: "openai",
|
||||
apiKey: "sk-test",
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -16,16 +16,13 @@ export interface FleetProvider {
|
||||
}
|
||||
|
||||
export interface CreateFleetProviderRequest {
|
||||
providerType: "ollama" | "openai" | "claude";
|
||||
name: string;
|
||||
displayName: string;
|
||||
config: {
|
||||
endpoint?: string;
|
||||
apiKey?: string;
|
||||
models?: string[];
|
||||
timeout?: number;
|
||||
};
|
||||
isDefault?: boolean;
|
||||
isEnabled?: boolean;
|
||||
type: string;
|
||||
baseUrl?: string;
|
||||
apiKey?: string;
|
||||
apiType?: string;
|
||||
models?: FleetProviderModel[];
|
||||
}
|
||||
|
||||
export interface UpdateFleetProviderRequest {
|
||||
|
||||
@@ -1,157 +0,0 @@
|
||||
# MS22 Phase 1 Module Audit
|
||||
|
||||
Date: 2026-03-01
|
||||
Branch: `fix/ms22-audit`
|
||||
Scope:
|
||||
|
||||
- `apps/api/src/container-lifecycle/`
|
||||
- `apps/api/src/crypto/`
|
||||
- `apps/api/src/agent-config/`
|
||||
- `apps/api/src/onboarding/`
|
||||
- `apps/api/src/fleet-settings/`
|
||||
- `apps/api/src/chat-proxy/`
|
||||
|
||||
## Summary
|
||||
|
||||
Audit completed for module wiring, security controls, input validation, and error handling.
|
||||
|
||||
Findings:
|
||||
|
||||
1. `chat-proxy`: raw internal/upstream error messages were returned to clients over SSE (fixed).
|
||||
2. `chat-proxy`: proxy requests to OpenClaw did not forward the container bearer token returned by lifecycle startup (fixed).
|
||||
3. `agent-config`: token validation returned early and used length-gated compare logic, creating avoidable timing side-channel behavior (hardened).
|
||||
|
||||
## Module Review Results
|
||||
|
||||
### 1) `container-lifecycle`
|
||||
|
||||
- NestJS module dependency audit:
|
||||
- `ContainerLifecycleModule` imports `ConfigModule`, `PrismaModule`, and `CryptoModule` required by `ContainerLifecycleService`.
|
||||
- Providers/exports are correct (`ContainerLifecycleService` provided and exported).
|
||||
- Security review:
|
||||
- Container operations are user-scoped by `userId` and do not expose cross-user selectors in this module.
|
||||
- AES token generation/decryption delegated to `CryptoService`.
|
||||
- Input validation:
|
||||
- No controller endpoints in this module; no direct request DTO surface here.
|
||||
- Error handling:
|
||||
- No direct HTTP layer here; errors flow to callers/global filter.
|
||||
- Finding status: **No issues found in this module**.
|
||||
|
||||
### 2) `crypto`
|
||||
|
||||
- NestJS module dependency audit:
|
||||
- `CryptoModule` correctly imports `ConfigModule` for `ConfigService`.
|
||||
- `CryptoService` is correctly provided/exported.
|
||||
- Security review:
|
||||
- AES-256-GCM is implemented correctly.
|
||||
- 96-bit IV generated via `randomBytes(12)` per encryption.
|
||||
- Auth tag captured and verified on decrypt (`setAuthTag` + `decipher.final()`).
|
||||
- HKDF derives a fixed 32-byte key from `MOSAIC_SECRET_KEY`.
|
||||
- Input validation:
|
||||
- No DTO/request surface in this module.
|
||||
- Error handling:
|
||||
- Decrypt failures are normalized to `Failed to decrypt value`.
|
||||
- Finding status: **No issues found in this module**.
|
||||
|
||||
### 3) `agent-config`
|
||||
|
||||
- NestJS module dependency audit:
|
||||
- `AgentConfigModule` imports `PrismaModule` + `CryptoModule`; `AgentConfigService` and `AgentConfigGuard` are provided.
|
||||
- Controller/guard/service wiring is correct.
|
||||
- Security review:
|
||||
- Bearer token comparisons used `timingSafeEqual`, but returned early on first match and performed length-gated comparison.
|
||||
- Internal route (`/api/internal/agent-config/:id`) is access-controlled by bearer token guard and container-id match (`containerAuth.id === :id`).
|
||||
- Input validation:
|
||||
- Header token extraction and route param are manually handled (no DTO for `:id`, acceptable for current use but should remain constrained).
|
||||
- Error handling:
|
||||
- Service throws typed Nest exceptions for not-found paths.
|
||||
- Finding status: **Issue found and fixed**.
|
||||
|
||||
### 4) `onboarding`
|
||||
|
||||
- NestJS module dependency audit:
|
||||
- `OnboardingModule` imports required dependencies (`PrismaModule`, `CryptoModule`; `ConfigModule` currently unused but harmless).
|
||||
- Providers/controllers are correctly declared.
|
||||
- Security review:
|
||||
- `OnboardingGuard` blocks all mutating onboarding routes once `onboarding.completed=true`.
|
||||
- Onboarding cannot be re-run via guarded endpoints after completion.
|
||||
- Input validation:
|
||||
- DTOs use `class-validator` decorators for all request bodies.
|
||||
- Error handling:
|
||||
- Uses typed Nest exceptions (`ConflictException`, `BadRequestException`).
|
||||
- Finding status: **No issues found in this module**.
|
||||
|
||||
### 5) `fleet-settings`
|
||||
|
||||
- NestJS module dependency audit:
|
||||
- `FleetSettingsModule` imports `AuthModule`, `PrismaModule`, `CryptoModule` required by its controller/service.
|
||||
- Provider/export wiring is correct for `FleetSettingsService`.
|
||||
- Security review:
|
||||
- Class-level `AuthGuard` protects all routes.
|
||||
- Admin-only routes additionally use `AdminGuard` (`oidc` and `breakglass/reset-password`).
|
||||
- Provider list/get responses do not expose `apiKey`.
|
||||
- OIDC read response intentionally omits `clientSecret`.
|
||||
- Input validation:
|
||||
- DTOs are decorated with `class-validator`.
|
||||
- Error handling:
|
||||
- Ownership/not-found conditions use typed exceptions.
|
||||
- Finding status: **No issues found in this module**.
|
||||
|
||||
### 6) `chat-proxy`
|
||||
|
||||
- NestJS module dependency audit:
|
||||
- `ChatProxyModule` imports `AuthModule`, `PrismaModule`, `ContainerLifecycleModule` needed by controller/service.
|
||||
- Provider/controller wiring is correct.
|
||||
- Security review:
|
||||
- User identity comes from `AuthGuard`; no user-provided container selector, so no cross-user container proxy path found.
|
||||
- **Issue fixed:** gateway bearer token was not forwarded on proxied requests.
|
||||
- **Issue fixed:** SSE error events exposed raw internal exception messages.
|
||||
- Input validation:
|
||||
- `ChatStreamDto` + nested `ChatMessageDto` use `class-validator` decorators.
|
||||
- Error handling:
|
||||
- **Issue fixed:** controller now emits safe client error messages and logs details server-side.
|
||||
- Finding status: **Issues found and fixed**.
|
||||
|
||||
## Security Checklist Outcomes
|
||||
|
||||
- `fleet-settings`: admin-only routes are guarded; non-admin users cannot access OIDC or breakglass reset routes. Provider secrets are not returned in provider read endpoints.
|
||||
- `agent-config`: token comparison hardened; route remains gated by bearer token + container id binding.
|
||||
- `onboarding`: guarded mutating endpoints cannot run after completion.
|
||||
- `crypto`: AES-256-GCM usage is correct (random IV, auth-tag verification, fixed 32-byte key derivation).
|
||||
- `chat-proxy`: user cannot target another user’s container; proxy now authenticates to OpenClaw using per-container bearer token.
|
||||
|
||||
## Input Validation
|
||||
|
||||
- DTO coverage is present in onboarding, fleet-settings, and chat-proxy request bodies.
|
||||
- No critical unvalidated body inputs found in scoped modules.
|
||||
|
||||
## Error Handling
|
||||
|
||||
- Global API layer has a sanitizing `GlobalExceptionFilter`.
|
||||
- `chat-proxy` used manual response handling (`@Res`) and bypassed global filter; this was corrected by sending safe generic SSE errors.
|
||||
- No additional critical sensitive-data leaks found in reviewed scope.
|
||||
|
||||
## Changes Made
|
||||
|
||||
1. Hardened token comparison behavior in:
|
||||
- `apps/api/src/agent-config/agent-config.service.ts`
|
||||
- Changes:
|
||||
- Compare SHA-256 digests with `timingSafeEqual`.
|
||||
- Avoid early return during scan to reduce timing signal differences.
|
||||
|
||||
2. Fixed OpenClaw auth forwarding and error leak risk in:
|
||||
- `apps/api/src/chat-proxy/chat-proxy.service.ts`
|
||||
- `apps/api/src/chat-proxy/chat-proxy.controller.ts`
|
||||
- `apps/api/src/chat-proxy/chat-proxy.service.spec.ts`
|
||||
- Changes:
|
||||
- Forward `Authorization: Bearer <gatewayToken>` when proxying chat requests.
|
||||
- Stop returning raw internal/upstream error text to clients over SSE.
|
||||
- Log details server-side and return safe client-facing messages.
|
||||
|
||||
## Validation Commands
|
||||
|
||||
Required quality gate command run:
|
||||
|
||||
- `pnpm turbo lint typecheck --filter=@mosaic/api`
|
||||
|
||||
(Results captured in session logs.)
|
||||
Reference in New Issue
Block a user