From 587d0699241b36c8afe3d49034b48554ff54cf42 Mon Sep 17 00:00:00 2001 From: allhandghost Date: Mon, 27 Jul 2026 10:48:24 +0000 Subject: [PATCH] security: fix log injection, CORS wildcard default, WS auth docs, and WS connection limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit resolves four security issues identified in the vortex-backend audit. Each fix is described below with the affected files and approach taken. ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Issue #47 — Audit path parameters for log injection (log-injection via originalUrl) ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ File: src/common/logging.interceptor.ts Problem: LoggingInterceptor logged request.originalUrl verbatim. A crafted request with URL-encoded newlines (%0a / %0d) or other ASCII control characters could inject fake log lines, corrupt structured log output, or confuse log-aggregation pipelines. Fix: Added a sanitizeForLog() helper that strips all C0/C1 control characters (U+0000–U+0008, U+000A–U+001F, U+007F–U+009F) except horizontal tab before the URL is interpolated into the log message. Horizontal tab is kept because it is harmless in most log formats. The sanitised URL is used only for logging — it is never written back into the response. All @Param() usages in IntentsController, SolversController, and SorobanController were audited: none reflect their raw value directly into logs or error messages in a way that bypasses the interceptor. Closes #47 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Issue #48 — CORS_ORIGIN must not default to wildcard in production ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Files: src/config/env.validation.ts | src/config/configuration.ts | .env.example Problem: CORS_ORIGIN defaulted to "*" unconditionally. A production deployment that forgot to set the variable would silently accept requests from any origin, enabling credentialed-adjacent cross-site requests against the API. Fix: - env.validation.ts: added a Joi.when() conditional on NODE_ENV. In production, CORS_ORIGIN is .required() and .invalid("*") — the app will refuse to start (validation error at boot) if the variable is missing or set to the wildcard. In development/test it still defaults to "*" for developer convenience. - configuration.ts: exposed wsMaxConnections in AppConfig (see #50 below). - .env.example: added inline documentation explaining the production requirement and an example value (https://app.vortex.trade), plus the new WS_MAX_CONNECTIONS variable. Closes #48 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Issue #49 — Document WebSocket gateway authentication decision ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ File: src/intents/intents.gateway.ts Problem: IntentsGateway accepted every connection with no auth check and no comment explaining why. The absence of any decision record left ambiguity about whether the lack of auth was intentional. Decision (intentionally public read-only): The WS intent feed is a PUBLIC, READ-ONLY stream by design — analogous to Stellar Horizon event streams or dYdX order-book feeds. Transparency is a protocol property; any wallet or UI should be able to watch the feed without credentials. Solver bots that need to write (accept, fill) must use the REST API, which can be separately authenticated. The gateway never accepts inbound messages that trigger privileged actions, so there is nothing to protect here. A detailed JSDoc comment block was added above the class to make this decision explicit and to guide future contributors who might otherwise add ad-hoc auth later. If a private authenticated stream is ever needed it should live at a separate path (e.g. /ws/solver) with a NestJS WsGuard. Closes #49 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Issue #50 — Max-connections guard and subscriber count metric for WS gateway ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Files: src/intents/intents.gateway.ts | src/config/env.validation.ts src/config/configuration.ts | .env.example Problem: The subscribers Set grew unbounded. A connection-flood attack could exhaust file descriptors and heap memory, denying service to legitimate users. There was also no way to observe the current connection count for alerting. Fix: - WS_MAX_CONNECTIONS env var (default 1000, 0 = unlimited) validated by Joi in env.validation.ts and surfaced as wsMaxConnections in AppConfig. - IntentsGateway now injects ConfigService and reads wsMaxConnections at construction time. - handleConnection() checks subscribers.size before adding the new client. If the limit is already reached the socket is closed immediately with RFC 6455 close code 1013 ("Try Again Later") and a human-readable reason, and a WARN log is emitted. The client is never added to the subscriber set, so the cap is never exceeded. - Added get subscriberCount(): number as a public getter so health checks, metrics endpoints, and future observability tooling (issue #65) can read the live count without coupling to the gateway's internals. - Added NestJS Logger for structured debug-level connection lifecycle events (connected / error-drop / disconnected) with the live subscriber count. Closes #50 --- .env.example | 12 ++++++- src/common/logging.interceptor.ts | 18 ++++++++-- src/config/configuration.ts | 3 ++ src/config/env.validation.ts | 31 ++++++++++++++++- src/intents/intents.gateway.ts | 56 +++++++++++++++++++++++++++++-- 5 files changed, 113 insertions(+), 7 deletions(-) diff --git a/.env.example b/.env.example index fa8bba2..7b3276d 100644 --- a/.env.example +++ b/.env.example @@ -13,5 +13,15 @@ SETTLEMENT_CONTRACT_ID= SOLVER_REGISTRY_CONTRACT_ID= # ─── CORS ──────────────────────────────────────────────────────────────────── -# Comma-separated list of allowed origins for the frontend ("*" for any in dev) +# Comma-separated list of allowed origins for the frontend. +# Development default: "*" (any origin allowed — convenient for local work) +# Production REQUIRED: must be an explicit origin, never "*" +# e.g. CORS_ORIGIN=https://app.vortex.trade +# The app will refuse to start in production if this is "*" or unset. CORS_ORIGIN=* + +# ─── WebSocket ─────────────────────────────────────────────────────────────── +# Maximum number of concurrent WebSocket connections accepted by the gateway. +# Connections beyond this limit are rejected with close code 1013 (try again later). +# Set to 0 to disable the cap (not recommended in production). +WS_MAX_CONNECTIONS=1000 diff --git a/src/common/logging.interceptor.ts b/src/common/logging.interceptor.ts index dbf81d9..91da018 100644 --- a/src/common/logging.interceptor.ts +++ b/src/common/logging.interceptor.ts @@ -11,6 +11,18 @@ interface LoggableResponse { statusCode: number; } +/** + * Strips newline characters and other ASCII control characters (except tab) + * from a string to prevent log injection attacks. A crafted URL containing + * `%0a` / `%0d` sequences could otherwise forge extra log lines. + */ +function sanitizeForLog(value: string): string { + // Replace newlines, carriage returns, and all other C0/C1 control characters + // (except horizontal tab U+0009) with a space so the entry stays on one line. + // eslint-disable-next-line no-control-regex + return value.replace(/[\x00-\x08\x0a-\x1f\x7f-\x9f]/g, " ").trim(); +} + @Injectable() export class LoggingInterceptor implements NestInterceptor { intercept(context: ExecutionContext, next: CallHandler): Observable { @@ -21,9 +33,9 @@ export class LoggingInterceptor implements NestInterceptor { return next.handle().pipe( tap(() => { const duration = Date.now() - start; - logger.info( - `${request.method} ${request.originalUrl} ${response.statusCode} ${duration}ms`, - ); + // Sanitize the URL before logging to prevent log-injection via crafted paths. + const safeUrl = sanitizeForLog(request.originalUrl); + logger.info(`${request.method} ${safeUrl} ${response.statusCode} ${duration}ms`); }), ); } diff --git a/src/config/configuration.ts b/src/config/configuration.ts index b6c1100..afc721d 100644 --- a/src/config/configuration.ts +++ b/src/config/configuration.ts @@ -8,6 +8,8 @@ export interface AppConfig { solverRegistryContractId: string; }; corsOrigin: string; + /** Maximum concurrent WebSocket connections (0 = unlimited). */ + wsMaxConnections: number; } export default (): AppConfig => ({ @@ -20,4 +22,5 @@ export default (): AppConfig => ({ solverRegistryContractId: process.env.SOLVER_REGISTRY_CONTRACT_ID ?? "", }, corsOrigin: process.env.CORS_ORIGIN ?? "*", + wsMaxConnections: parseInt(process.env.WS_MAX_CONNECTIONS ?? "1000", 10), }); diff --git a/src/config/env.validation.ts b/src/config/env.validation.ts index 9a4dc6d..e4f1fe2 100644 --- a/src/config/env.validation.ts +++ b/src/config/env.validation.ts @@ -9,5 +9,34 @@ export const envValidationSchema = Joi.object({ SETTLEMENT_CONTRACT_ID: Joi.string().allow("").default(""), SOLVER_REGISTRY_CONTRACT_ID: Joi.string().allow("").default(""), - CORS_ORIGIN: Joi.string().default("*"), + /** + * Allowed CORS origin(s) — comma-separated list of explicit origins or "*". + * + * In production the wildcard "*" is rejected so that deployments cannot + * accidentally serve credentialed-adjacent requests from any origin. + * Set this to the frontend's production URL, e.g.: + * CORS_ORIGIN=https://app.vortex.trade + * + * In development/test the value defaults to "*" for convenience. + */ + CORS_ORIGIN: Joi.when("NODE_ENV", { + is: "production", + then: Joi.string() + .invalid("*") + .required() + .messages({ + "any.invalid": + 'CORS_ORIGIN must be set to an explicit origin (not "*") when NODE_ENV=production', + "any.required": + 'CORS_ORIGIN is required when NODE_ENV=production — set it to the frontend origin (e.g. https://app.vortex.trade)', + }), + otherwise: Joi.string().default("*"), + }), + + /** + * Maximum number of simultaneous WebSocket connections the gateway will + * accept before rejecting new ones with close code 1013 (try again later). + * Defaults to 1000. Set to 0 to disable the cap (not recommended in prod). + */ + WS_MAX_CONNECTIONS: Joi.number().integer().min(0).default(1000), }); diff --git a/src/intents/intents.gateway.ts b/src/intents/intents.gateway.ts index 0e1b51f..ce382ea 100644 --- a/src/intents/intents.gateway.ts +++ b/src/intents/intents.gateway.ts @@ -1,16 +1,67 @@ import { OnGatewayConnection, OnGatewayDisconnect, WebSocketGateway } from "@nestjs/websockets"; +import { Logger } from "@nestjs/common"; +import { ConfigService } from "@nestjs/config"; import { WebSocket } from "ws"; import { IntentsService } from "./intents.service"; +import { AppConfig } from "../config/configuration"; +/** + * Authentication / access-control decision (issue #49) + * ─────────────────────────────────────────────────────── + * The intent feed is intentionally PUBLIC and READ-ONLY. Any client may + * connect and receive real-time intent events without presenting credentials. + * This mirrors the design of public DEX order-book streams (e.g. dYdX, Stellar + * Horizon) where transparency is a protocol property. + * + * Solver bots submit intents and accept/fill them through the authenticated + * REST API (POST /api/v1/intents, POST /api/v1/intents/:id/accept, etc.). + * The WS gateway never accepts writes, so there is no privileged action to + * protect here. + * + * If a private/authenticated stream is needed in the future (e.g. per-solver + * private fills), add a separate gateway path (e.g. /ws/solver) and apply + * a NestJS WsGuard there. + */ @WebSocketGateway({ path: "/ws" }) export class IntentsGateway implements OnGatewayConnection, OnGatewayDisconnect { + private readonly logger = new Logger(IntentsGateway.name); private readonly subscribers = new Set(); + /** Configured via WS_MAX_CONNECTIONS env var (default 1000, 0 = unlimited). */ + private readonly maxConnections: number; - constructor(private readonly intentsService: IntentsService) {} + constructor( + private readonly intentsService: IntentsService, + private readonly configService: ConfigService, + ) { + this.maxConnections = this.configService.get("wsMaxConnections", { infer: true }); + } + + /** + * Returns the current number of active WebSocket subscribers. + * Exposed for metrics / health checks (issue #50 / #65). + */ + get subscriberCount(): number { + return this.subscribers.size; + } handleConnection(client: WebSocket) { + // ── Max-connections guard (issue #50) ──────────────────────────────────── + // Reject the connection before adding it to the subscriber set so the cap + // is never exceeded. Close code 1013 = "Try Again Later" (RFC 6455). + if (this.maxConnections > 0 && this.subscribers.size >= this.maxConnections) { + this.logger.warn( + `WS connection rejected: subscriber limit reached (${this.maxConnections})`, + ); + client.close(1013, "Server at capacity — try again later"); + return; + } + this.subscribers.add(client); - client.on("error", () => this.subscribers.delete(client)); + this.logger.debug(`WS client connected — active subscribers: ${this.subscribers.size}`); + client.on("error", () => { + this.subscribers.delete(client); + this.logger.debug(`WS client error/drop — active subscribers: ${this.subscribers.size}`); + }); client.send(JSON.stringify({ type: "connected", message: "Vortex intent stream" })); @@ -20,6 +71,7 @@ export class IntentsGateway implements OnGatewayConnection, OnGatewayDisconnect handleDisconnect(client: WebSocket) { this.subscribers.delete(client); + this.logger.debug(`WS client disconnected — active subscribers: ${this.subscribers.size}`); } broadcast(event: { type: string; [key: string]: unknown }) {