From e10adc1d5c970af007b8a1f9809110c02961db89 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Sun, 1 Mar 2026 13:51:35 -0600 Subject: [PATCH] fix(api): MS22 Phase 1 audit report and fixes --- .../src/agent-config/agent-config.service.ts | 25 ++- .../src/chat-proxy/chat-proxy.controller.ts | 36 +++- .../src/chat-proxy/chat-proxy.service.spec.ts | 1 + apps/api/src/chat-proxy/chat-proxy.service.ts | 41 +++-- docs/audits/ms22-phase1-audit.md | 157 ++++++++++++++++++ 5 files changed, 233 insertions(+), 27 deletions(-) create mode 100644 docs/audits/ms22-phase1-audit.md diff --git a/apps/api/src/agent-config/agent-config.service.ts b/apps/api/src/agent-config/agent-config.service.ts index f4917c5..7d837b0 100644 --- a/apps/api/src/agent-config/agent-config.service.ts +++ b/apps/api/src/agent-config/agent-config.service.ts @@ -1,6 +1,6 @@ import { Injectable, NotFoundException } from "@nestjs/common"; import type { LlmProvider } from "@prisma/client"; -import { timingSafeEqual } from "node:crypto"; +import { createHash, timingSafeEqual } from "node:crypto"; import { PrismaService } from "../prisma/prisma.service"; import { CryptoService } from "../crypto/crypto.service"; @@ -143,21 +143,23 @@ export class AgentConfigService { }), ]); + let match: ContainerTokenValidation | null = null; + for (const container of userContainers) { const storedToken = this.decryptContainerToken(container.gatewayToken); - if (storedToken && this.tokensEqual(storedToken, token)) { - return { type: "user", id: container.id }; + if (!match && storedToken && this.tokensEqual(storedToken, token)) { + match = { type: "user", id: container.id }; } } for (const container of systemContainers) { const storedToken = this.decryptContainerToken(container.gatewayToken); - if (storedToken && this.tokensEqual(storedToken, token)) { - return { type: "system", id: container.id }; + if (!match && storedToken && this.tokensEqual(storedToken, token)) { + match = { type: "system", id: container.id }; } } - return null; + return match; } private buildOpenClawConfig( @@ -268,14 +270,9 @@ export class AgentConfigService { } private tokensEqual(left: string, right: string): boolean { - const leftBuffer = Buffer.from(left, "utf8"); - const rightBuffer = Buffer.from(right, "utf8"); - - if (leftBuffer.length !== rightBuffer.length) { - return false; - } - - return timingSafeEqual(leftBuffer, rightBuffer); + const leftDigest = createHash("sha256").update(left, "utf8").digest(); + const rightDigest = createHash("sha256").update(right, "utf8").digest(); + return timingSafeEqual(leftDigest, rightDigest); } private hasModelId(modelEntry: unknown): modelEntry is { id: string } { diff --git a/apps/api/src/chat-proxy/chat-proxy.controller.ts b/apps/api/src/chat-proxy/chat-proxy.controller.ts index dc38807..527580e 100644 --- a/apps/api/src/chat-proxy/chat-proxy.controller.ts +++ b/apps/api/src/chat-proxy/chat-proxy.controller.ts @@ -1,4 +1,14 @@ -import { Body, Controller, Post, Req, Res, UnauthorizedException, UseGuards } from "@nestjs/common"; +import { + Body, + Controller, + HttpException, + Logger, + 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"; @@ -8,6 +18,8 @@ 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 @@ -58,10 +70,11 @@ 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: message })}\n\n`); + res.write(`data: ${JSON.stringify({ error: this.toSafeClientMessage(error) })}\n\n`); } } finally { if (!res.writableEnded && !res.destroyed) { @@ -69,4 +82,21 @@ 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)}`); + } } diff --git a/apps/api/src/chat-proxy/chat-proxy.service.spec.ts b/apps/api/src/chat-proxy/chat-proxy.service.spec.ts index 17b3c33..63b595c 100644 --- a/apps/api/src/chat-proxy/chat-proxy.service.spec.ts +++ b/apps/api/src/chat-proxy/chat-proxy.service.spec.ts @@ -64,6 +64,7 @@ describe("ChatProxyService", () => { expect.objectContaining({ method: "POST", headers: { + Authorization: "Bearer gateway-token", "Content-Type": "application/json", }, }) diff --git a/apps/api/src/chat-proxy/chat-proxy.service.ts b/apps/api/src/chat-proxy/chat-proxy.service.ts index f936d6a..13334ca 100644 --- a/apps/api/src/chat-proxy/chat-proxy.service.ts +++ b/apps/api/src/chat-proxy/chat-proxy.service.ts @@ -1,12 +1,24 @@ -import { BadGatewayException, Injectable, ServiceUnavailableException } from "@nestjs/common"; +import { + BadGatewayException, + Injectable, + Logger, + 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 @@ -14,8 +26,7 @@ export class ChatProxyService { // Get the user's OpenClaw container URL and mark it active. async getContainerUrl(userId: string): Promise { - const { url } = await this.containerLifecycle.ensureRunning(userId); - await this.containerLifecycle.touch(userId); + const { url } = await this.getContainerConnection(userId); return url; } @@ -25,11 +36,14 @@ export class ChatProxyService { messages: ChatMessage[], signal?: AbortSignal ): Promise { - const containerUrl = await this.getContainerUrl(userId); + const { url: containerUrl, token: gatewayToken } = await this.getContainerConnection(userId); const model = await this.getPreferredModel(userId); const requestInit: RequestInit = { method: "POST", - headers: { "Content-Type": "application/json" }, + headers: { + "Content-Type": "application/json", + Authorization: `Bearer ${gatewayToken}`, + }, body: JSON.stringify({ messages, model, @@ -47,10 +61,10 @@ export class ChatProxyService { if (!response.ok) { const detail = await this.readResponseText(response); const status = `${String(response.status)} ${response.statusText}`.trim(); - const message = detail - ? `OpenClaw returned ${status}: ${detail}` - : `OpenClaw returned ${status}`; - throw new BadGatewayException(message); + this.logger.warn( + detail ? `OpenClaw returned ${status}: ${detail}` : `OpenClaw returned ${status}` + ); + throw new BadGatewayException(`OpenClaw returned ${status}`); } return response; @@ -60,10 +74,17 @@ export class ChatProxyService { } const message = error instanceof Error ? error.message : String(error); - throw new ServiceUnavailableException(`Failed to proxy chat to OpenClaw: ${message}`); + this.logger.warn(`Failed to proxy chat request: ${message}`); + throw new ServiceUnavailableException("Failed to proxy chat to OpenClaw"); } } + private async getContainerConnection(userId: string): Promise { + const connection = await this.containerLifecycle.ensureRunning(userId); + await this.containerLifecycle.touch(userId); + return connection; + } + private async getPreferredModel(userId: string): Promise { const config = await this.prisma.userAgentConfig.findUnique({ where: { userId }, diff --git a/docs/audits/ms22-phase1-audit.md b/docs/audits/ms22-phase1-audit.md new file mode 100644 index 0000000..f894c57 --- /dev/null +++ b/docs/audits/ms22-phase1-audit.md @@ -0,0 +1,157 @@ +# 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 ` 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.)