From 8920b14069951953ed21884f8701982d94127c89 Mon Sep 17 00:00:00 2001 From: aarroyo Date: Sat, 19 Sep 2026 06:38:24 -0500 Subject: [PATCH] fix(security): escribir las tres guardas de #725 en la forma que CodeQL modela MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit La corrida de CodeQL sobre main (50121746) cerró 20 de las 37 alertas de #725 y dejó abiertas 9 js/path-injection y 1 js/prototype-pollution-utility: la contención es real pero el motor no la reconocía como sanitizador. - `resolveLegacyPath`: `path.resolve` + UN `startsWith(root + sep)`, la misma forma que `resolve()` en el mismo fichero (que sí pasa); la condición compuesta `resolved !== root && …` no se modela. El root no es un satélite, nada se pierde. - `InitializeProjectUseCase`: el nombre pasa a una sola variable y, además del regex, se comprueban `includes('..')` y `path.isAbsolute` — los dos guards que TaintedPath modela; el regex solo no cuenta como barrera. - `evolith-config-set`: la comparación literal con `__proto__`/`constructor`/`prototype` se hace sobre el segmento que se escribe (un Set en otra función no se modela); se quita el import sin uso de `sanitizePathInput`. Sin cambio de comportamiento: los mismos 56 tests de los tres sitios siguen verdes. Co-Authored-By: Claude Opus 5 --- .../workspace-reference-resolver.service.ts | 6 +++++- .../use-cases/initialize-project.use-case.ts | 16 ++++++++++++--- .../mcp-server/src/tools/config.tools.ts | 20 ++++++++++++++----- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/apps/core-api/src/application/services/workspace-reference-resolver.service.ts b/src/apps/core-api/src/application/services/workspace-reference-resolver.service.ts index ee3dc5cce..969834b2a 100644 --- a/src/apps/core-api/src/application/services/workspace-reference-resolver.service.ts +++ b/src/apps/core-api/src/application/services/workspace-reference-resolver.service.ts @@ -40,7 +40,11 @@ export class WorkspaceReferenceResolverService { } const root = path.resolve(this.config.getOrThrow('WORKSPACE_ROOT')); const resolved = path.resolve(root, rawPath); - if (resolved !== root && !resolved.startsWith(`${root}${path.sep}`)) { + // Same shape as `resolve()` above on purpose: a normalized path followed by ONE + // `startsWith(root + sep)` guard is what CodeQL models as containment; the + // compound `resolved !== root && …` form was not, and left js/path-injection open + // on every sink downstream. The root itself is not a satellite, so nothing is lost. + if (!resolved.startsWith(`${root}${path.sep}`)) { throw new BadRequestException( `${field} resolves outside the workspace root; send an opaque workspaceRef instead`, ); diff --git a/src/packages/core-domain/src/application/use-cases/initialize-project.use-case.ts b/src/packages/core-domain/src/application/use-cases/initialize-project.use-case.ts index 6b00f7a68..8747e03f4 100644 --- a/src/packages/core-domain/src/application/use-cases/initialize-project.use-case.ts +++ b/src/packages/core-domain/src/application/use-cases/initialize-project.use-case.ts @@ -1,3 +1,4 @@ +import * as path from 'path'; import { ICatalogLoader, IFileSystem } from '../../domain/interfaces'; import { IPlatformProviders } from '../ports/platform-detection.port'; import { InitProjectInput, InitProjectResult } from '../services/use-case.types'; @@ -27,9 +28,18 @@ export class InitializeProjectUseCase { const artifacts: string[] = []; try { - if (typeof input.name !== 'string' || !PROJECT_NAME.test(input.name)) { + const name = input.name; + // The regex already excludes separators and dot-segments; the two explicit + // checks restate the same fact in the form CodeQL models as a path sanitizer + // (no `..`, not absolute), so `${cwd}/${name}` reads as contained downstream. + if ( + typeof name !== 'string' || + !PROJECT_NAME.test(name) || + name.includes('..') || + path.isAbsolute(name) + ) { errors.push( - `Project name "${input.name}" is not a valid directory name: use letters, digits, ".", "-" or "_" (max 128 chars, cannot start with "." or "-")`, + `Project name "${name}" is not a valid directory name: use letters, digits, ".", "-" or "_" (max 128 chars, cannot start with "." or "-")`, ); return { success: false, artifacts, warnings, errors }; } @@ -55,7 +65,7 @@ export class InitializeProjectUseCase { return { success: false, artifacts, warnings, errors }; } - const projectDir = `${cwd}/${input.name}`; + const projectDir = `${cwd}/${name}`; await this.fs.ensureDir(projectDir); await this.projectScaffolder.scaffoldEvolithYaml(input, projectDir); diff --git a/src/packages/mcp-server/src/tools/config.tools.ts b/src/packages/mcp-server/src/tools/config.tools.ts index 2108a4266..313148ed0 100644 --- a/src/packages/mcp-server/src/tools/config.tools.ts +++ b/src/packages/mcp-server/src/tools/config.tools.ts @@ -2,7 +2,6 @@ import * as path from 'node:path'; import * as fs from 'fs-extra'; import * as yaml from 'yaml'; import { McpTool } from '../mcp/tool.interface'; -import { sanitizePathInput } from '../utils/path-security'; /** * Segments that would walk the key path onto `Object.prototype` instead of into @@ -60,13 +59,24 @@ export class ConfigToolService { const keys = keySegments(key); let target: Record = config; for (let i = 0; i < keys.length - 1; i++) { + const segment = keys[i]; + // keySegments() already refused these; restated here, on the value that is + // written, because this literal comparison is the guard CodeQL models for + // js/prototype-pollution-utility — a Set lookup in another function is not. + if (segment === '__proto__' || segment === 'constructor' || segment === 'prototype') { + throw new Error(`Invalid key "${key}": "${segment}" is not an allowed segment`); + } // Only descend into an OWN plain object; a scalar or an inherited property // is replaced, never written through. - const next = Object.prototype.hasOwnProperty.call(target, keys[i]) ? target[keys[i]] : undefined; - if (typeof next !== 'object' || next === null || Array.isArray(next)) target[keys[i]] = {}; - target = target[keys[i]] as Record; + const next = Object.prototype.hasOwnProperty.call(target, segment) ? target[segment] : undefined; + if (typeof next !== 'object' || next === null || Array.isArray(next)) target[segment] = {}; + target = target[segment] as Record; } - target[keys[keys.length - 1]] = value; + const leaf = keys[keys.length - 1]; + if (leaf === '__proto__' || leaf === 'constructor' || leaf === 'prototype') { + throw new Error(`Invalid key "${key}": "${leaf}" is not an allowed segment`); + } + target[leaf] = value; await fs.writeFile(configPath, yaml.stringify(config)); return { key, value, updated: true }; }