diff --git a/.github/workflows/ci-cd.yml b/.github/workflows/ci-cd.yml index 7a0b52351..4cc7f23db 100644 --- a/.github/workflows/ci-cd.yml +++ b/.github/workflows/ci-cd.yml @@ -21,6 +21,11 @@ on: branches: [main, develop] workflow_dispatch: +# Scorecard Token-Permissions: the default GITHUB_TOKEN is read-only for every job; +# a job that has to write declares exactly what it writes on itself. +permissions: + contents: read + jobs: test: name: Test @@ -525,6 +530,10 @@ jobs: governance-guards: name: Governance guards (GT-578) runs-on: ubuntu-latest + # Guard 50 lists the open pull requests and reads each one's diff. + permissions: + contents: read + pull-requests: read steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Setup Node.js diff --git a/.github/workflows/docker-images.yml b/.github/workflows/docker-images.yml index 257c6ec78..55c1c5936 100644 --- a/.github/workflows/docker-images.yml +++ b/.github/workflows/docker-images.yml @@ -18,13 +18,17 @@ on: tags: - "v*" +# Scorecard Token-Permissions: read-only by default; the build job below is the +# one that pushes to GHCR and declares `packages: write` itself. permissions: contents: read - packages: write jobs: build: runs-on: ubuntu-latest + permissions: + contents: read + packages: write strategy: fail-fast: false matrix: diff --git a/.github/workflows/docs-release.yml b/.github/workflows/docs-release.yml index d4ae3d9a8..53665e4ca 100644 --- a/.github/workflows/docs-release.yml +++ b/.github/workflows/docs-release.yml @@ -13,6 +13,11 @@ on: - 'release/docs-*' - 'hotfix/docs-*' +# Scorecard Token-Permissions: the default GITHUB_TOKEN is read-only for every job; +# a job that has to write declares exactly what it writes on itself. +permissions: + contents: read + jobs: release-trigger-check: if: github.ref == 'refs/heads/main' && github.event_name == 'push' @@ -59,6 +64,9 @@ jobs: needs: release-trigger-check if: needs.release-trigger-check.outputs.version != '' runs-on: ubuntu-latest + # Commits the version log back to main. + permissions: + contents: write steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -93,6 +101,9 @@ jobs: needs: [release-trigger-check, update-version-log] if: needs.release-trigger-check.outputs.version != '' runs-on: ubuntu-latest + # Pushes the tag and creates the GitHub Release. + permissions: + contents: write steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: diff --git a/.github/workflows/enforce-root-cleanliness.yml b/.github/workflows/enforce-root-cleanliness.yml index 81d393f5f..0816db1c5 100644 --- a/.github/workflows/enforce-root-cleanliness.yml +++ b/.github/workflows/enforce-root-cleanliness.yml @@ -12,6 +12,11 @@ on: branches: - main +# Scorecard Token-Permissions: the default GITHUB_TOKEN is read-only for every job; +# a job that has to write declares exactly what it writes on itself. +permissions: + contents: read + jobs: validate-root: runs-on: ubuntu-latest diff --git a/.github/workflows/reliability.yml b/.github/workflows/reliability.yml index f10e17c0e..cc8aaee79 100644 --- a/.github/workflows/reliability.yml +++ b/.github/workflows/reliability.yml @@ -78,6 +78,11 @@ defaults: run: shell: bash +# Scorecard Token-Permissions: the default GITHUB_TOKEN is read-only for every job; +# a job that has to write declares exactly what it writes on itself. +permissions: + contents: read + jobs: k6-load: name: k6 load profile (thresholds + published metrics) diff --git a/.github/workflows/sdk-cli-ci.yml b/.github/workflows/sdk-cli-ci.yml index 35c6ba213..387e69e10 100644 --- a/.github/workflows/sdk-cli-ci.yml +++ b/.github/workflows/sdk-cli-ci.yml @@ -50,6 +50,11 @@ env: # Bumping it is a reviewed change; guard 60 re-derives nothing from it. GITLEAKS_VERSION: '8.30.1' +# Scorecard Token-Permissions: the default GITHUB_TOKEN is read-only for every job; +# a job that has to write declares exactly what it writes on itself. +permissions: + contents: read + jobs: # ============================================ # JOB 1: Evolith Core Validation diff --git a/.github/workflows/sdk-cli-release.yml b/.github/workflows/sdk-cli-release.yml index b5f7c87c7..b74a46ad5 100644 --- a/.github/workflows/sdk-cli-release.yml +++ b/.github/workflows/sdk-cli-release.yml @@ -75,6 +75,11 @@ env: NODE_VERSION: '20' CLI_DIR: 'src/sdk/cli' +# Scorecard Token-Permissions: the default GITHUB_TOKEN is read-only for every job; +# a job that has to write declares exactly what it writes on itself. +permissions: + contents: read + jobs: # ============================================ # GATE 0: Evolith Core Validation @@ -629,6 +634,9 @@ jobs: # a GitHub Release must not be cut when the npm publish failed. if: ${{ needs.release-gate.outputs.release_created }} runs-on: ubuntu-latest + # Creates the GitHub Release and uploads the binaries to it. + permissions: + contents: write steps: - name: Checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 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 73249dbd6..ee3dc5cce 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 @@ -27,6 +27,42 @@ export class WorkspaceReferenceResolverService { return resolved; } + /** + * Legacy `satellitePath` (pre-ADR-0101 callers still send a filesystem path). + * Accepted only when it resolves INSIDE `WORKSPACE_ROOT` — relative to it or + * absolute beneath it — so a caller cannot point the Core at `/etc` or at the + * Core's own checkout. Anything else is a 400 that names `workspaceRef` as the + * replacement. + */ + resolveLegacyPath(rawPath: string, field = 'satellitePath'): string { + if (typeof rawPath !== 'string' || rawPath.length === 0 || rawPath.length > 1024 || rawPath.includes('\0')) { + throw new BadRequestException(`${field} must be a non-empty path`); + } + const root = path.resolve(this.config.getOrThrow('WORKSPACE_ROOT')); + const resolved = path.resolve(root, rawPath); + if (resolved !== root && !resolved.startsWith(`${root}${path.sep}`)) { + throw new BadRequestException( + `${field} resolves outside the workspace root; send an opaque workspaceRef instead`, + ); + } + return resolved; + } + + /** + * A caller-supplied `corePath` override. The Core rules live at the configured + * `CORE_PATH`; the only other place a request may point at is a Core checkout + * inside its own workspace. Absent → the configured path. + */ + resolveCorePathOverride(rawPath: string | undefined): string { + const configured = path.resolve(this.corePath()); + if (rawPath === undefined || rawPath === null || rawPath === '') return configured; + if (typeof rawPath !== 'string' || rawPath.includes('\0')) { + throw new BadRequestException('corePath must be a path'); + } + if (path.resolve(rawPath) === configured) return configured; + return this.resolveLegacyPath(rawPath, 'corePath'); + } + corePath(): string { return this.config.getOrThrow('CORE_PATH'); } diff --git a/src/apps/core-api/src/presentation/controllers/architecture.controller.spec.ts b/src/apps/core-api/src/presentation/controllers/architecture.controller.spec.ts index 6e64f672a..8f529e486 100644 --- a/src/apps/core-api/src/presentation/controllers/architecture.controller.spec.ts +++ b/src/apps/core-api/src/presentation/controllers/architecture.controller.spec.ts @@ -128,10 +128,25 @@ describe('ArchitectureController', () => { expect(validateUseCase.execute).toHaveBeenCalledWith({ satellitePath: '/workspaces/op_01', corePath: '/core', + manifest: undefined, }); expect(workspaceResolver.resolve).toHaveBeenCalledWith('op_01'); }); + // CWE-22: the manifest wins inside the use case and carried its own paths, + // so a client could evaluate any directory by sending them in the manifest. + it('pins the manifest paths to the resolved workspace and the configured Core', async () => { + await controller.validateSatellite({ + workspaceRef: 'op_01', + manifest: { satellitePath: '/etc', corePath: '/somewhere/else', topology: 'modular-monolith' } as any, + }); + expect(validateUseCase.execute).toHaveBeenCalledWith({ + satellitePath: '/workspaces/op_01', + corePath: '/core', + manifest: { satellitePath: '/workspaces/op_01', corePath: '/core', topology: 'modular-monolith' }, + }); + }); + it('should propagate validation errors', async () => { validateUseCase.execute.mockRejectedValue(new Error('Invalid satellite')); await expect( diff --git a/src/apps/core-api/src/presentation/controllers/architecture.controller.ts b/src/apps/core-api/src/presentation/controllers/architecture.controller.ts index e4d1883b6..ac216db29 100644 --- a/src/apps/core-api/src/presentation/controllers/architecture.controller.ts +++ b/src/apps/core-api/src/presentation/controllers/architecture.controller.ts @@ -119,10 +119,15 @@ export class ArchitectureController { @ApiBody({ type: ValidateSatelliteDto }) @ApiEnvelopeResponse(undefined, { description: 'Validation results or ADR-0073 envelope if manifest is provided' }) async validateSatellite(@Body() body: ValidateSatelliteDto) { + const satellitePath = this.workspaceResolver.resolve(body.workspaceRef); + const corePath = this.workspaceResolver.corePath(); const response = await this.validateSatelliteUseCase.execute({ - satellitePath: this.workspaceResolver.resolve(body.workspaceRef), - corePath: this.workspaceResolver.corePath(), - manifest: body.manifest, + satellitePath, + corePath, + // CWE-22: a manifest takes precedence in the use case and carries its own + // `satellitePath`/`corePath`, so those are pinned to the resolved ones — + // the client's values never reach the filesystem. + manifest: body.manifest ? { ...body.manifest, satellitePath, corePath } : undefined, }); return response.evaluationVerdict?.outputEnvelope ?? response.result; } diff --git a/src/apps/core-api/src/presentation/controllers/evaluation.consumer-contract.spec.ts b/src/apps/core-api/src/presentation/controllers/evaluation.consumer-contract.spec.ts index fcaae5c43..9d15396c5 100644 --- a/src/apps/core-api/src/presentation/controllers/evaluation.consumer-contract.spec.ts +++ b/src/apps/core-api/src/presentation/controllers/evaluation.consumer-contract.spec.ts @@ -107,7 +107,10 @@ const configParser = { stringify: (data: unknown) => JSON.stringify(data), }; const rulesetRepo = { getRuleset: async () => null, listRulesets: async () => [] } as any; -const workspaceResolver = { corePath: () => CORE_PATH } as any; +const workspaceResolver = { + corePath: () => CORE_PATH, + resolveCorePathOverride: (raw?: string) => raw ?? CORE_PATH, +} as any; function buildController(fallback: IFileSystem) { return new EvaluationController( diff --git a/src/apps/core-api/src/presentation/controllers/evaluation.controller.spec.ts b/src/apps/core-api/src/presentation/controllers/evaluation.controller.spec.ts index 6994aba66..9b99e2be7 100644 --- a/src/apps/core-api/src/presentation/controllers/evaluation.controller.spec.ts +++ b/src/apps/core-api/src/presentation/controllers/evaluation.controller.spec.ts @@ -4,6 +4,7 @@ import { ValidateSatelliteUseCase } from '@beyondnet/evolith-core-domain/applica import { EvaluationOrchestrator } from '@beyondnet/evolith-core-domain/evaluation'; import type { DirEntry, IFileSystem } from '@beyondnet/evolith-core-domain/domain/interfaces'; import { EvaluateSatelliteDto, EvaluationContextDto } from '../dtos/evaluation.dto'; +import { WorkspaceReferenceResolverService } from '../../application/services/workspace-reference-resolver.service'; const SAMPLE_GATE_EVAL = { artifactEvaluations: [ @@ -62,6 +63,14 @@ describe('EvaluationController (GT-361)', () => { providers: [ { provide: EvaluationOrchestrator, useValue: orchestrator }, { provide: ValidateSatelliteUseCase, useValue: mockUseCase }, + // The real resolver over a fake config: the legacy branch must contain + // `satellitePath` under WORKSPACE_ROOT and `corePath` to CORE_PATH. + { + provide: WorkspaceReferenceResolverService, + useValue: new WorkspaceReferenceResolverService({ + getOrThrow: (key: string) => ({ WORKSPACE_ROOT, CORE_PATH: CORE })[key], + } as any), + }, ], }).compile(); @@ -71,8 +80,11 @@ describe('EvaluationController (GT-361)', () => { afterEach(() => jest.clearAllMocks()); + const WORKSPACE_ROOT = '/workspaces'; + const CORE = '/core'; + const body: EvaluateSatelliteDto = { - satellitePath: '/satellite', + satellitePath: '/workspaces/satellite', corePath: '/core', topology: 'modular-monolith', phase: 'f1', @@ -132,10 +144,10 @@ describe('EvaluationController (GT-361)', () => { await controller.evaluate(body); expect(useCase.execute).toHaveBeenCalledWith({ - satellitePath: '/satellite', + satellitePath: '/workspaces/satellite', corePath: '/core', manifest: { - satellitePath: '/satellite', + satellitePath: '/workspaces/satellite', corePath: '/core', topology: 'modular-monolith', phase: 'f1', @@ -143,28 +155,61 @@ describe('EvaluationController (GT-361)', () => { }); }); - it('works with minimal body (only satellitePath)', async () => { + it('works with minimal body (only satellitePath); corePath defaults to the configured Core', async () => { const minimalEnvelope = { ...SAMPLE_ENVELOPE }; useCase.execute.mockResolvedValue({ result: {} as any, evaluationVerdict: { ...({} as any), outputEnvelope: minimalEnvelope }, }); - const minimalBody: EvaluateSatelliteDto = { satellitePath: '/satellite' }; + // Relative legacy paths resolve against WORKSPACE_ROOT. + const minimalBody: EvaluateSatelliteDto = { satellitePath: 'satellite' }; const response = await controller.evaluate(minimalBody); expect(useCase.execute).toHaveBeenCalledWith({ - satellitePath: '/satellite', - corePath: undefined, + satellitePath: '/workspaces/satellite', + corePath: '/core', manifest: { - satellitePath: '/satellite', - corePath: undefined, + satellitePath: '/workspaces/satellite', + corePath: '/core', topology: undefined, phase: undefined, }, }); expect(response).toBe(minimalEnvelope); }); + + // CWE-22: the legacy fields used to reach the filesystem verbatim, so an + // authenticated caller could evaluate (read) any directory on the host. + it('refuses a legacy satellitePath outside WORKSPACE_ROOT before touching the use case', async () => { + for (const satellitePath of ['/etc', '../outside', '/workspaces/../etc', '/workspacesX/sat']) { + await expect(controller.evaluate({ satellitePath })).rejects.toThrow(/outside the workspace root/); + } + expect(useCase.execute).not.toHaveBeenCalled(); + }); + + it('refuses a legacy corePath that is neither the configured Core nor inside the workspace', async () => { + await expect( + controller.evaluate({ satellitePath: 'sat', corePath: '/somewhere/else' }), + ).rejects.toThrow(/corePath resolves outside the workspace root/); + expect(useCase.execute).not.toHaveBeenCalled(); + + // A Core checkout inside the workspace is still an allowed override. + useCase.execute.mockResolvedValue({ + result: {} as any, + evaluationVerdict: { ...({} as any), outputEnvelope: SAMPLE_ENVELOPE }, + }); + await controller.evaluate({ satellitePath: 'sat', corePath: '/workspaces/core-checkout' }); + expect(useCase.execute).toHaveBeenCalledWith( + expect.objectContaining({ satellitePath: '/workspaces/sat', corePath: '/workspaces/core-checkout' }), + ); + }); + + it('fails closed when the instance has no workspace resolver', async () => { + const bare = new EvaluationController({ evaluate: jest.fn() } as any, useCase); + await expect(bare.evaluate({ satellitePath: '/workspaces/satellite' })).rejects.toThrow(/not available on this Core instance/); + expect(useCase.execute).not.toHaveBeenCalled(); + }); }); describe('canonical EvaluationContext path (GT-378)', () => { @@ -268,7 +313,9 @@ describe('EvaluationController — inline evaluationInput path', () => { stringify: (data: unknown) => JSON.stringify(data), }; const rulesetRepo = { getRuleset: async () => null, listRulesets: async () => [] } as any; - const workspaceResolver = { corePath: () => CORE_PATH } as any; + const workspaceResolver = new WorkspaceReferenceResolverService({ + getOrThrow: (key: string) => ({ CORE_PATH, WORKSPACE_ROOT: '/workspaces' })[key], + } as any); function buildController(fallback: IFileSystem) { return new EvaluationController( diff --git a/src/apps/core-api/src/presentation/controllers/evaluation.controller.ts b/src/apps/core-api/src/presentation/controllers/evaluation.controller.ts index 0a425cc95..83d9a352a 100644 --- a/src/apps/core-api/src/presentation/controllers/evaluation.controller.ts +++ b/src/apps/core-api/src/presentation/controllers/evaluation.controller.ts @@ -163,17 +163,20 @@ export class EvaluationController { // Legacy path: satellite filesystem path (pre-ADR-0101). Returns the legacy verdict envelope. if (body.satellitePath) { + // CWE-22: the raw path only reaches the filesystem once the resolver has + // pinned it inside WORKSPACE_ROOT (and corePath to the configured Core). + const { satellitePath, corePath } = this.containLegacyPaths(body.satellitePath, body.corePath); const { evaluationVerdict } = await this.validateSatelliteUseCase.execute( { - satellitePath: body.satellitePath, - corePath: body.corePath, + satellitePath, + corePath, rulesetRefs: selectionFrom(body), // GT-676 — the floor the caller asked for reaches the engine on this // surface too, so a REST verdict is reproducible from a CLI one. maxSkippedFraction: body.maxSkippedFraction, manifest: { - satellitePath: body.satellitePath, - corePath: body.corePath, + satellitePath, + corePath, topology: body.topology, phase: body.phase, }, @@ -189,6 +192,27 @@ export class EvaluationController { ); } + /** + * The legacy branch used to hand `body.satellitePath` / `body.corePath` straight + * to the pipeline, which reads directories under both — so an authenticated + * caller could point the Core at any path on the host. Both now go through the + * resolver's containment; an instance wired without one fails closed. + */ + private containLegacyPaths( + satellitePath: string, + corePath: string | undefined, + ): { satellitePath: string; corePath: string } { + if (!this.workspaceResolver) { + throw new BadRequestException( + 'Legacy `satellitePath` evaluation is not available on this Core instance; send `workspaceRef` or `evaluationInput.files`', + ); + } + return { + satellitePath: this.workspaceResolver.resolveLegacyPath(satellitePath), + corePath: this.workspaceResolver.resolveCorePathOverride(corePath), + }; + } + /** * Runs the SAME validation/evaluation the satellite path uses, but against a * synthetic satellite root served from the in-memory `files` map. The Core's @@ -209,8 +233,11 @@ export class EvaluationController { } // corePath resolved exactly as the existing flows do: explicit override, or - // the Core's configured CORE_PATH (so the Core rules are found on disk). - const corePath = body.corePath || this.workspaceResolver?.corePath(); + // the Core's configured CORE_PATH (so the Core rules are found on disk). An + // override is honoured only where the resolver allows it (CWE-22). + const corePath = this.workspaceResolver + ? this.workspaceResolver.resolveCorePathOverride(body.corePath) + : undefined; if (!corePath) { throw new BadRequestException( 'Unable to resolve corePath for inline evaluation', diff --git a/src/apps/core-api/src/presentation/dtos/projects.dto.ts b/src/apps/core-api/src/presentation/dtos/projects.dto.ts index 958a34ed6..9ac112d48 100644 --- a/src/apps/core-api/src/presentation/dtos/projects.dto.ts +++ b/src/apps/core-api/src/presentation/dtos/projects.dto.ts @@ -1,5 +1,5 @@ import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; -import { IsString, IsOptional, IsBoolean, MinLength } from 'class-validator'; +import { IsString, IsOptional, IsBoolean, Matches, MinLength } from 'class-validator'; export class InitProjectDto { @ApiProperty({ description: 'Opaque workspace reference issued by the Tracker BFF', example: 'op_01j7wq8e2n' }) @@ -7,9 +7,12 @@ export class InitProjectDto { @MinLength(1) workspaceRef!: string; - @ApiProperty({ description: 'Project name', example: 'my-service' }) + @ApiProperty({ description: 'Project name — becomes the directory under the workspace, so one path segment only', example: 'my-service' }) @IsString() @MinLength(1) + @Matches(/^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$/, { + message: 'name must be a single directory name: letters, digits, ".", "-" or "_" (cannot start with "." or "-")', + }) name!: string; @ApiProperty({ description: 'Project type', example: 'nestjs' }) diff --git a/src/apps/core-api/src/presentation/dtos/satellite-manifest.dto.ts b/src/apps/core-api/src/presentation/dtos/satellite-manifest.dto.ts index 210450d3d..39b8bc89b 100644 --- a/src/apps/core-api/src/presentation/dtos/satellite-manifest.dto.ts +++ b/src/apps/core-api/src/presentation/dtos/satellite-manifest.dto.ts @@ -99,9 +99,13 @@ export class EvaluationFactsDto { } export class SatelliteManifestDto { - @ApiProperty({ description: 'Filesystem path to the satellite repository', example: '/path/to/satellite' }) + @ApiPropertyOptional({ + description: 'Ignored on the REST surface: the satellite path is derived from `workspaceRef` (kept optional for wire compatibility)', + example: '/path/to/satellite', + }) + @IsOptional() @IsString() - satellitePath!: string; + satellitePath?: string; @ApiPropertyOptional({ description: 'Optional explicit path to the Evolith Core repository' }) @IsOptional() diff --git a/src/packages/agent-runtime/src/adapters/approval/tracker-approval.http-client.ts b/src/packages/agent-runtime/src/adapters/approval/tracker-approval.http-client.ts index 0c46bf0fc..bd4899f4d 100644 --- a/src/packages/agent-runtime/src/adapters/approval/tracker-approval.http-client.ts +++ b/src/packages/agent-runtime/src/adapters/approval/tracker-approval.http-client.ts @@ -32,6 +32,13 @@ import type { TrackerApprovalSubmission, } from './tracker-approval.adapter'; +/** Char-based trim of trailing `/` — no `\/+$` regex, which backtracks polynomially (ReDoS). */ +function stripTrailingSlashes(s: string): string { + let end = s.length; + while (end > 0 && s.charCodeAt(end - 1) === 47 /* '/' */) end -= 1; + return s.slice(0, end); +} + interface FetchResponse { readonly ok: boolean; readonly status: number; @@ -69,7 +76,7 @@ export class TrackerApprovalHttpClient implements TrackerApprovalClient { } this.fetchImpl = impl; // Trim trailing slashes so a base with or without one yields one clean join. - this.endpoint = `${options.baseUrl.replace(/\/+$/, '')}/runtime-approvals`; + this.endpoint = `${stripTrailingSlashes(options.baseUrl)}/runtime-approvals`; this.apiKey = options.apiKey; this.timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; } diff --git a/src/packages/agent-runtime/src/domain/retrieval/tokenize.ts b/src/packages/agent-runtime/src/domain/retrieval/tokenize.ts index b515ec167..bd364b8bd 100644 --- a/src/packages/agent-runtime/src/domain/retrieval/tokenize.ts +++ b/src/packages/agent-runtime/src/domain/retrieval/tokenize.ts @@ -55,7 +55,20 @@ const SEPARATORS = /[_\-./]+/g; const SEPARATOR_SPLIT = /[_\-./]+/; /** Leading/trailing separators are punctuation, not part of the word. */ -const TRIM_SEPARATORS = /^[_\-./]+|[_\-./]+$/g; +const SEPARATOR_CHARS = new Set(['_', '-', '.', '/']); + +/** + * Char-based trim of leading/trailing separators. The former + * `/^[_\-./]+|[_\-./]+$/` backtracks polynomially on a long run of separators + * (ReDoS on the retrieval corpus, which is caller-supplied text). + */ +function trimSeparators(word: string): string { + let start = 0; + let end = word.length; + while (start < end && SEPARATOR_CHARS.has(word[start])) start += 1; + while (end > start && SEPARATOR_CHARS.has(word[end - 1])) end -= 1; + return word.slice(start, end); +} /** Tokens shorter than this are dropped unless they contain a digit. */ const MIN_TOKEN_LENGTH = 2; @@ -85,7 +98,7 @@ export function tokenize(text: string): string[] { const out: string[] = []; for (const rawWord of text.split(WORD_SPLIT)) { - const word = rawWord.replace(TRIM_SEPARATORS, ''); + const word = trimSeparators(rawWord); if (!word) continue; const compound = compoundForm(word); diff --git a/src/packages/core-domain/src/application/use-cases/initialize-project.use-case.spec.ts b/src/packages/core-domain/src/application/use-cases/initialize-project.use-case.spec.ts index e60f18805..1bca52413 100644 --- a/src/packages/core-domain/src/application/use-cases/initialize-project.use-case.spec.ts +++ b/src/packages/core-domain/src/application/use-cases/initialize-project.use-case.spec.ts @@ -70,6 +70,25 @@ async function init(overrides: Partial = {}) { return { fs, result, root: `/tmp/${(overrides.name ?? INPUT.name)}` }; } +describe('InitializeProjectUseCase · the name is a directory, not a path (CWE-22)', () => { + it.each(['../escape', 'a/b', '..', '.hidden', '-flag', 'nul\0byte', ''])( + 'refuses %j before writing anything', + async (name) => { + const fs = memoryFs(); + const result = await new InitializeProjectUseCase(fs, catalogLoader).execute({ ...INPUT, name } as any, '/tmp'); + expect(result.success).toBe(false); + expect(result.errors[0]).toMatch(/not a valid directory name/); + expect(fs.dirs.size).toBe(0); + expect(fs.files.size).toBe(0); + }, + ); + + it('keeps accepting the dotted and dashed names npm does', async () => { + const { root } = await init({ name: 'acme.billing-api_v2' }); + expect(root).toBe('/tmp/acme.billing-api_v2'); + }); +}); + describe('InitializeProjectUseCase · GIT-08 — the scaffold enforces what it mandates', () => { it('writes a commitlint config carrying the commit types GIT-08 names', async () => { const { fs, root } = await init(); 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 f0034b6f8..6b00f7a68 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 @@ -3,6 +3,13 @@ import { IPlatformProviders } from '../ports/platform-detection.port'; import { InitProjectInput, InitProjectResult } from '../services/use-case.types'; import { ProjectScaffolderService } from '../services/project-scaffolder.service'; +/** + * The project name becomes the directory under `cwd` (and a path prefix on every + * artifact), so it has to be exactly one path segment: no separators, no `.`/`..`, + * no leading dash. Same alphabet npm accepts for an unscoped package name. + */ +const PROJECT_NAME = /^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$/; + export class InitializeProjectUseCase { private readonly fs: IFileSystem; private readonly catalogLoader: ICatalogLoader; @@ -20,6 +27,13 @@ export class InitializeProjectUseCase { const artifacts: string[] = []; try { + if (typeof input.name !== 'string' || !PROJECT_NAME.test(input.name)) { + errors.push( + `Project name "${input.name}" is not a valid directory name: use letters, digits, ".", "-" or "_" (max 128 chars, cannot start with "." or "-")`, + ); + return { success: false, artifacts, warnings, errors }; + } + const runtimes = this.catalogLoader.loadRuntimeCatalog(); const runtime = runtimes.find((r: any) => (r as any).id === input.runtime); if (!runtime) { diff --git a/src/packages/core-domain/src/application/use-cases/initialize-satellite.use-case.ts b/src/packages/core-domain/src/application/use-cases/initialize-satellite.use-case.ts index 35b5b98c3..4e64327e2 100644 --- a/src/packages/core-domain/src/application/use-cases/initialize-satellite.use-case.ts +++ b/src/packages/core-domain/src/application/use-cases/initialize-satellite.use-case.ts @@ -157,9 +157,14 @@ export class InitializeSatelliteUseCase { }; } - /** Parse owner and repo name from a GitHub URL such as https://github.com/owner/repo */ + /** + * Parse owner and repo name from a GitHub URL such as https://github.com/owner/repo + * (also `git@github.com:owner/repo.git` and `ssh://git@github.com/owner/repo`). + * Anchored at the start so `github.com` has to BE the host, not merely appear + * in the string, and every quantifier is possessive-shaped (no overlap → linear). + */ private parseRepoUrl(url: string): { owner: string; name: string } { - const match = url.match(/github\.com[/:]([^/]+)\/([^/.]+?)(?:\.git)?$/); + const match = url.trim().match(/^(?:[a-z][a-z0-9+.-]*:\/\/)?(?:[^@/\s]+@)?github\.com[/:]([^/\s]+)\/([^/.\s]+)(?:\.git)?\/?$/i); if (!match) { throw new Error(`Cannot parse GitHub URL: ${url}`); } diff --git a/src/packages/core-domain/src/application/use-cases/sync-satellite.use-case.ts b/src/packages/core-domain/src/application/use-cases/sync-satellite.use-case.ts index b63a2347d..6cf1006c8 100644 --- a/src/packages/core-domain/src/application/use-cases/sync-satellite.use-case.ts +++ b/src/packages/core-domain/src/application/use-cases/sync-satellite.use-case.ts @@ -11,6 +11,13 @@ import { IGitHubApiClient } from '../../domain/github-api-client.interface'; import { ILogger } from '../../domain/interfaces'; import { enumerateWorkspaceProjects } from '../../domain/workspace-descriptor'; +/** Char-based trim of trailing `/` — no `\/+$` regex, which backtracks polynomially (ReDoS). */ +function stripTrailingSlashes(s: string): string { + let end = s.length; + while (end > 0 && s.charCodeAt(end - 1) === 47 /* '/' */) end -= 1; + return s.slice(0, end); +} + export interface SyncSatelliteInput { satellite: SatelliteRecord; corePath: string; @@ -116,7 +123,7 @@ function walkDir(dir: string): string[] { function destinationPath(satellite: SatelliteRecord, relativePath: string): string { const sub = satellite.subpath; if (!sub || sub === '.' || sub === '') return relativePath; - return path.posix.join(sub.replace(/\\/g, '/').replace(/\/+$/, ''), relativePath); + return path.posix.join(stripTrailingSlashes(sub.replace(/\\/g, '/')), relativePath); } /** diff --git a/src/packages/core-domain/src/domain/interfaces.ts b/src/packages/core-domain/src/domain/interfaces.ts index c9b8a5783..2a6049c96 100644 --- a/src/packages/core-domain/src/domain/interfaces.ts +++ b/src/packages/core-domain/src/domain/interfaces.ts @@ -63,8 +63,20 @@ export interface PlatformCheck { } export interface ICommandExecutor { + /** + * @deprecated Runs through a shell: any caller-controlled fragment in + * `command` is an injection vector (CWE-78). Prefer {@link executeFile}. + */ execute(command: string, cwd?: string): Promise; + /** @deprecated Shell-backed like {@link execute}; prefer {@link executeFileOrThrow}. */ executeOrThrow(command: string, cwd?: string): Promise; + /** + * Shell-free execution: `args` reach the binary verbatim, so metacharacters + * in caller-supplied values are data, never syntax. + */ + executeFile(file: string, args: string[], cwd?: string): Promise; + /** {@link executeFile} that throws on a non-zero exit instead of returning it. */ + executeFileOrThrow(file: string, args: string[], cwd?: string): Promise; checkTool(name: string, versionCommand: string): Promise; } @@ -182,11 +194,6 @@ export interface PhaseTransition { timestamp: string; } -export interface ICommandExecutor { - execute(command: string, cwd?: string): Promise; - executeOrThrow(command: string, cwd?: string): Promise; - checkTool(name: string, versionCommand: string): Promise; -} export interface ICatalogLoader { loadRuntimeCatalog(): Runtime[]; diff --git a/src/packages/core-domain/src/domain/metrics/git-log-reader.ts b/src/packages/core-domain/src/domain/metrics/git-log-reader.ts index 9ba93c882..16410c20f 100644 --- a/src/packages/core-domain/src/domain/metrics/git-log-reader.ts +++ b/src/packages/core-domain/src/domain/metrics/git-log-reader.ts @@ -1,12 +1,13 @@ /** * Thin wrapper around `git log` for reading commit history. - * Uses child_process.exec so it works without any git library dependency. + * Uses child_process.execFile so it works without any git library dependency + * and without a shell: the options travel as argv, never as a command line. */ -import { exec } from 'child_process'; +import { execFile } from 'child_process'; import { promisify } from 'util'; -const execAsync = promisify(exec); +const execFileAsync = promisify(execFile); export interface GitCommit { hash: string; @@ -38,8 +39,9 @@ export async function readGitLog(opts: GitLogOptions): Promise { // format: hash|ISO-date|subject|parent1 parent2... const format = '%H|%aI|%s|%P'; - const { stdout } = await execAsync( - `git log --format="${format}" --since="${since}" --max-count=${maxCount}`, + const { stdout } = await execFileAsync( + 'git', + ['log', `--format=${format}`, `--since=${since}`, `--max-count=${maxCount}`], { cwd }, ); @@ -66,7 +68,7 @@ export async function readGitLog(opts: GitLogOptions): Promise { */ export async function isGitRepo(cwd: string): Promise { try { - await execAsync('git rev-parse --git-dir', { cwd }); + await execFileAsync('git', ['rev-parse', '--git-dir'], { cwd }); return true; } catch { return false; diff --git a/src/packages/core-domain/src/domain/ownership.ts b/src/packages/core-domain/src/domain/ownership.ts index 9ec46be33..b53ec8af9 100644 --- a/src/packages/core-domain/src/domain/ownership.ts +++ b/src/packages/core-domain/src/domain/ownership.ts @@ -14,6 +14,13 @@ import type { Violation } from './violation'; +/** Char-based trim of trailing `/` — no `\/+$` regex, which backtracks polynomially (ReDoS). */ +function stripTrailingSlashes(s: string): string { + let end = s.length; + while (end > 0 && s.charCodeAt(end - 1) === 47 /* '/' */) end -= 1; + return s.slice(0, end); +} + /** A component's ownership, normalized across sources. */ export interface OwnershipEntry { /** Component/service name. */ @@ -28,7 +35,8 @@ export interface OwnershipEntry { /** Normalize a path for prefix comparison (posix separators, strip `./` and trailing `/`). */ function normalizePath(p: string): string { - return p.replace(/\\/g, '/').replace(/^\.\//, '').replace(/\/+$/, ''); + const posix = p.replace(/\\/g, '/'); + return stripTrailingSlashes(posix.startsWith('./') ? posix.slice(2) : posix); } /** diff --git a/src/packages/core-domain/src/domain/transparency/codec.ts b/src/packages/core-domain/src/domain/transparency/codec.ts index a5c8d064b..eb8d22e03 100644 --- a/src/packages/core-domain/src/domain/transparency/codec.ts +++ b/src/packages/core-domain/src/domain/transparency/codec.ts @@ -48,7 +48,10 @@ export function toBase64(bytes: Uint8Array): string { /** Standard base64 decoding. Throws on a character outside the alphabet. */ export function fromBase64(b64: string): Uint8Array { - const clean = b64.replace(/=+$/, ''); + // Char-based trim of the `=` padding — no `=+$` regex (polynomial backtracking, ReDoS). + let end = b64.length; + while (end > 0 && b64.charCodeAt(end - 1) === 61 /* '=' */) end -= 1; + const clean = b64.slice(0, end); const out = new Uint8Array(Math.floor((clean.length * 3) / 4)); let acc = 0; let bits = 0; diff --git a/src/packages/core-domain/src/domain/workspace-descriptor.ts b/src/packages/core-domain/src/domain/workspace-descriptor.ts index e46f7df9c..dffbc7b3c 100644 --- a/src/packages/core-domain/src/domain/workspace-descriptor.ts +++ b/src/packages/core-domain/src/domain/workspace-descriptor.ts @@ -48,8 +48,15 @@ export function enumerateWorkspaceProjects(doc: unknown): WorkspaceProject[] { .map(p => ({ name: p.name, path: normalizeProjectPath(p.path) })); } +/** Char-based trim of trailing `/` — no `\/+$` regex, which backtracks polynomially (ReDoS). */ +function stripTrailingSlashes(s: string): string { + let end = s.length; + while (end > 0 && s.charCodeAt(end - 1) === 47 /* '/' */) end -= 1; + return s.slice(0, end); +} + /** Strip a leading `./` and any trailing slash; empty/root normalizes to '.'. */ export function normalizeProjectPath(rawPath: string): string { - const trimmed = rawPath.replace(/^\.\//, '').replace(/\/+$/, ''); + const trimmed = stripTrailingSlashes(rawPath.startsWith('./') ? rawPath.slice(2) : rawPath); return trimmed.length === 0 ? '.' : trimmed; } diff --git a/src/packages/infra-providers/src/architecture/nx-workspace.strategy.ts b/src/packages/infra-providers/src/architecture/nx-workspace.strategy.ts index d6b84011b..d4f059f4f 100644 --- a/src/packages/infra-providers/src/architecture/nx-workspace.strategy.ts +++ b/src/packages/infra-providers/src/architecture/nx-workspace.strategy.ts @@ -10,6 +10,29 @@ import * as path from 'path'; */ const MFE_CAPABLE_FRAMEWORKS = new Set(['react', 'angular']); +/** + * Every value interpolated into an `nx g` / `npm install` argument must be a + * plain identifier. The commands run shell-free (argv, never a command line), + * so this is not what stops `; rm -rf`: it is what stops `--directory=apps/../..` + * escaping the workspace and `--name=--flag` being read as an option. Nx project + * names are the same alphabet, so nothing legitimate is refused. + */ +const SAFE_IDENTIFIER = /^[A-Za-z0-9][A-Za-z0-9_-]{0,127}$/; + +/** + * Validates one caller-supplied name and returns it. Throws on anything outside + * {@link SAFE_IDENTIFIER}, naming the field so the error is actionable at the + * CLI prompt and in the MCP envelope alike. + */ +export function assertSafeIdentifier(value: string, field: string): string { + if (typeof value !== 'string' || !SAFE_IDENTIFIER.test(value)) { + throw new Error( + `Invalid ${field} "${value}": use letters, digits, "-" or "_" only (max 128 chars, cannot start with "-").`, + ); + } + return value; +} + export interface NxWorkspaceStrategyOptions { /** Progress sink for human-facing step messages. Defaults to a no-op. */ progress?: (message: string) => void; @@ -53,52 +76,61 @@ export class NxWorkspaceStrategy implements WorkspaceManagerStrategy { return path.join(this.baseDir, 'src'); } - private async runNx(command: string): Promise { + /** + * Shell-free: `npx` receives `['nx', ...args, '--no-interactive']` as argv. + * The progress line joins them for the human only; nothing is re-parsed. + */ + private async runNx(args: string[]): Promise { const targetDir = this.getTargetDir(); + const shown = `npx nx ${args.join(' ')}`; if (this.dryRun) { - this.progress(`[DRY-RUN] Would execute in ${targetDir}: npx nx ${command}`); + this.progress(`[DRY-RUN] Would execute in ${targetDir}: ${shown}`); return; } - this.progress(`> Executing in ${targetDir}: npx nx ${command}`); - // executeOrThrow surfaces a standard error on non-zero exit. - await this.commandExecutor.executeOrThrow(`npx nx ${command} --no-interactive`, targetDir); + this.progress(`> Executing in ${targetDir}: ${shown}`); + // executeFileOrThrow surfaces a standard error on non-zero exit. + await this.commandExecutor.executeFileOrThrow('npx', ['nx', ...args, '--no-interactive'], targetDir); } - private async runNpm(command: string): Promise { + private async runNpm(args: string[]): Promise { const targetDir = this.getTargetDir(); + const shown = `npm ${args.join(' ')}`; if (this.dryRun) { - this.progress(`[DRY-RUN] Would execute in ${targetDir}: npm ${command}`); + this.progress(`[DRY-RUN] Would execute in ${targetDir}: ${shown}`); return; } - this.progress(`> Executing in ${targetDir}: npm ${command}`); - await this.commandExecutor.executeOrThrow(`npm ${command} --legacy-peer-deps`, targetDir); + this.progress(`> Executing in ${targetDir}: ${shown}`); + await this.commandExecutor.executeFileOrThrow('npm', [...args, '--legacy-peer-deps'], targetDir); } async installDependencies(frontendFramework: string, orm: string): Promise { - const fw = frontendFramework.toLowerCase(); + const fw = assertSafeIdentifier(frontendFramework.toLowerCase(), 'frontend framework'); this.frontendFramework = fw; this.progress(`Installing Nx Plugins for ${fw.toUpperCase()} and NestJS...`); - await this.runNpm(`install -D @nx/nest @nx/${fw} @nx/webpack`); + await this.runNpm(['install', '-D', '@nx/nest', `@nx/${fw}`, '@nx/webpack']); if (orm.toLowerCase() === 'prisma') { - await this.runNpm(`install -D prisma @prisma/client`); + await this.runNpm(['install', '-D', 'prisma', '@prisma/client']); } else if (orm.toLowerCase() === 'typeorm') { - await this.runNpm(`install -D typeorm`); + await this.runNpm(['install', '-D', 'typeorm']); } } async generateStandardWebApp(name: string, framework: string): Promise { - const fw = framework.toLowerCase(); + const fw = assertSafeIdentifier(framework.toLowerCase(), 'frontend framework'); + const app = assertSafeIdentifier(name, 'web app name'); this.frontendFramework = fw; - this.progress(`Generating Standard Web App (Phase 1) [${name}] (${fw.toUpperCase()})...`); - await this.runNx(`g @nx/${fw}:app --name=${name} --directory=apps/${name}`); + this.progress(`Generating Standard Web App (Phase 1) [${app}] (${fw.toUpperCase()})...`); + await this.runNx(['g', `@nx/${fw}:app`, `--name=${app}`, `--directory=apps/${app}`]); } async generateHostApp(name: string, remotes: string[], framework: string): Promise { - const fw = framework.toLowerCase(); + const fw = assertSafeIdentifier(framework.toLowerCase(), 'frontend framework'); + const host = assertSafeIdentifier(name, 'host app name'); + const safeRemotes = remotes.map((r) => assertSafeIdentifier(r, 'remote name')); this.frontendFramework = fw; - this.progress(`Generating MFE Host App [${name}] with Remotes [${remotes.join(', ')}] (${fw.toUpperCase()})...`); + this.progress(`Generating MFE Host App [${host}] with Remotes [${safeRemotes.join(', ')}] (${fw.toUpperCase()})...`); if (!MFE_CAPABLE_FRAMEWORKS.has(fw)) { this.progress( @@ -106,31 +138,33 @@ export class NxWorkspaceStrategy implements WorkspaceManagerStrategy { `Generating a standard ${fw.toUpperCase()} app instead; configure MFE manually ` + `(e.g. @originjs/vite-plugin-federation for Vue).`, ); - await this.runNx(`g @nx/${fw}:app --name=${name} --directory=apps/${name}`); + await this.runNx(['g', `@nx/${fw}:app`, `--name=${host}`, `--directory=apps/${host}`]); return; } - const remotesFlag = remotes.length > 0 ? `--remotes=${remotes.join(',')}` : ''; - await this.runNx(`g @nx/${fw}:host --name=${name} ${remotesFlag} --directory=apps/${name}`); + const remotesFlag = safeRemotes.length > 0 ? [`--remotes=${safeRemotes.join(',')}`] : []; + await this.runNx(['g', `@nx/${fw}:host`, `--name=${host}`, ...remotesFlag, `--directory=apps/${host}`]); } async generateApiApp(name: string): Promise { - this.progress(`Generating NestJS API App [${name}]...`); - await this.runNx(`g @nx/nest:app --name=${name} --directory=apps/${name}`); + const api = assertSafeIdentifier(name, 'API app name'); + this.progress(`Generating NestJS API App [${api}]...`); + await this.runNx(['g', '@nx/nest:app', `--name=${api}`, `--directory=apps/${api}`]); } async generateLibrary(name: string, type: 'domain' | 'shell' | 'shared'): Promise { - this.progress(`Generating Library [${type}/${name}]...`); + const lib = assertSafeIdentifier(name, `${type} library name`); + this.progress(`Generating Library [${type}/${lib}]...`); - if (type === 'shared' && name.includes('ui')) { + if (type === 'shared' && lib.includes('ui')) { // UI libraries use the active frontend framework — e.g. @nx/react:library, // @nx/angular:library, @nx/vue:library. const fw = this.frontendFramework; this.progress(`Using @nx/${fw}:library for shared UI library.`); - await this.runNx(`g @nx/${fw}:library --name=${name} --directory=libs/${type}/${name}`); + await this.runNx(['g', `@nx/${fw}:library`, `--name=${lib}`, `--directory=libs/${type}/${lib}`]); } else { // Domain, shell and non-UI shared libs always use NestJS (backend) generator. - await this.runNx(`g @nx/nest:library --name=${name} --directory=libs/${type}/${name}`); + await this.runNx(['g', '@nx/nest:library', `--name=${lib}`, `--directory=libs/${type}/${lib}`]); } } } diff --git a/src/packages/infra-providers/src/tracker/evaluation-ingest.client.ts b/src/packages/infra-providers/src/tracker/evaluation-ingest.client.ts index 481ccc238..eee297330 100644 --- a/src/packages/infra-providers/src/tracker/evaluation-ingest.client.ts +++ b/src/packages/infra-providers/src/tracker/evaluation-ingest.client.ts @@ -48,6 +48,13 @@ import { type EvaluationIngestSurface, } from '@beyondnet/evolith-contracts/ingest'; +/** Char-based trim of trailing `/` — no `\/+$` regex, which backtracks polynomially (ReDoS). */ +function stripTrailingSlashes(s: string): string { + let end = s.length; + while (end > 0 && s.charCodeAt(end - 1) === 47 /* '/' */) end -= 1; + return s.slice(0, end); +} + // --------------------------------------------------------------------------- // Transport seam // --------------------------------------------------------------------------- @@ -114,7 +121,7 @@ export class TrackerEvaluationIngestClient { // The route is taken from the published contract rather than typed here, so a // path change upstream moves this client with it instead of leaving a caller // that 404s against a route nobody remembers renaming. - this.endpoint = `${options.baseUrl.replace(/\/+$/, '')}${EVALUATION_INGEST_ENDPOINT_CONTRACT.path}`; + this.endpoint = `${stripTrailingSlashes(options.baseUrl)}${EVALUATION_INGEST_ENDPOINT_CONTRACT.path}`; this.apiKey = options.apiKey; this.timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; } diff --git a/src/packages/mcp-server/src/tools/config.tools.spec.ts b/src/packages/mcp-server/src/tools/config.tools.spec.ts index b7e777684..cb5a54c08 100644 --- a/src/packages/mcp-server/src/tools/config.tools.spec.ts +++ b/src/packages/mcp-server/src/tools/config.tools.spec.ts @@ -31,6 +31,16 @@ describe('config tools', () => { expect(reread.value).toBe('phase-1'); }); + it('refuses a key path that walks onto Object.prototype (CWE-1321)', async () => { + const set = byName(tools, 'evolith-config-set'); + await expect(set.execute({ key: '__proto__.polluted', value: 'yes', dir })).rejects.toThrow(/not an allowed segment/); + await expect(set.execute({ key: 'product.constructor.prototype.polluted', value: 'yes', dir })).rejects.toThrow(/not an allowed segment/); + await expect(set.execute({ key: 'product..phase', value: 'x', dir })).rejects.toThrow(/empty segment/); + expect(({} as Record).polluted).toBeUndefined(); + // The file is untouched by a refused write. + expect(await fsExtra.readFile(path.join(dir, 'evolith.yaml'), 'utf-8')).toBe('product:\n name: demo\n phase: phase-0\n'); + }); + it('throws when evolith.yaml is absent', async () => { const empty = await fsExtra.mkdtemp(path.join(os.tmpdir(), 'evolith-empty-')); try { diff --git a/src/packages/mcp-server/src/tools/config.tools.ts b/src/packages/mcp-server/src/tools/config.tools.ts index ef196c4a5..2108a4266 100644 --- a/src/packages/mcp-server/src/tools/config.tools.ts +++ b/src/packages/mcp-server/src/tools/config.tools.ts @@ -4,6 +4,24 @@ 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 + * the document (CWE-1321). `evolith.yaml` never legitimately holds them. + */ +const FORBIDDEN_KEY_SEGMENTS = new Set(['__proto__', 'constructor', 'prototype']); + +/** Split a dot-path into segments, refusing empties and prototype walkers. */ +function keySegments(key: string): string[] { + const segments = key.split('.'); + for (const segment of segments) { + if (segment.length === 0) throw new Error(`Invalid key "${key}": empty segment`); + if (FORBIDDEN_KEY_SEGMENTS.has(segment)) { + throw new Error(`Invalid key "${key}": "${segment}" is not an allowed segment`); + } + } + return segments; +} + /** * Config Tool Service — reads/writes evolith.yaml. * Converted from procedural module to class for testability (SRP). @@ -21,7 +39,7 @@ export class ConfigToolService { const config = yaml.parse(await fs.readFile(configPath, 'utf-8')); let value: unknown = config; - for (const k of key.split('.')) { + for (const k of keySegments(key)) { value = (value as Record)?.[k]; } return { key, value: value ?? null }; @@ -39,10 +57,13 @@ export class ConfigToolService { if (!(await fs.pathExists(configPath))) throw new Error('evolith.yaml not found'); const config = yaml.parse(await fs.readFile(configPath, 'utf-8')) ?? {}; - const keys = key.split('.'); + const keys = keySegments(key); let target: Record = config; for (let i = 0; i < keys.length - 1; i++) { - if (!target[keys[i]]) target[keys[i]] = {}; + // 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; } target[keys[keys.length - 1]] = value; diff --git a/src/packages/mcp-server/src/tools/scaffold.tool.spec.ts b/src/packages/mcp-server/src/tools/scaffold.tool.spec.ts index 67c0cb08f..4dbb169da 100644 --- a/src/packages/mcp-server/src/tools/scaffold.tool.spec.ts +++ b/src/packages/mcp-server/src/tools/scaffold.tool.spec.ts @@ -4,24 +4,33 @@ import fs from 'node:fs/promises'; /** * Mock `node:child_process` so the embedded NodeCommandExecutor never spawns a - * real process. `scaffold.tool.ts` builds `execAsync = promisify(exec)` at module - * load, so we attach a `util.promisify.custom` implementation to the mocked - * `exec`: promisify returns it directly. Behaviour is switched per-test through a + * real process. `scaffold.tool.ts` builds `promisify(exec)` / `promisify(execFile)` + * at module load, so we attach a `util.promisify.custom` implementation to both + * mocks: promisify returns it directly. Behaviour is switched per-test through a * global flag (jest.mock factories cannot close over test-scoped variables). + * The strategy runs shell-free, so `execFile` is the one that actually fires; + * every invocation is recorded in `__EVOLITH_EXEC_CALLS__` for the argv assertions. */ jest.mock('node:child_process', () => { const util = require('node:util') as typeof import('node:util'); - const exec: any = jest.fn(); - exec[util.promisify.custom] = (command: string) => { + const respond = (shown: string) => { const mode = (globalThis as any).__EVOLITH_EXEC_MODE__ ?? 'ok'; if (mode === 'fail') { return Promise.reject( Object.assign(new Error('spawn boom'), { stdout: '', stderr: 'nx exploded', code: 2 }), ); } - return Promise.resolve({ stdout: `ran: ${command}`, stderr: '' }); + return Promise.resolve({ stdout: `ran: ${shown}`, stderr: '' }); + }; + const exec: any = jest.fn(); + exec[util.promisify.custom] = (command: string) => respond(command); + const execFile: any = jest.fn(); + execFile[util.promisify.custom] = (file: string, args: string[]) => { + const calls = ((globalThis as any).__EVOLITH_EXEC_CALLS__ ??= []) as Array<{ file: string; args: string[] }>; + calls.push({ file, args }); + return respond([file, ...args].join(' ')); }; - return { exec }; + return { exec, execFile }; }); // Imported AFTER jest.mock so the mocked child_process is in place. @@ -32,6 +41,10 @@ function setExecMode(mode: 'ok' | 'fail'): void { (globalThis as any).__EVOLITH_EXEC_MODE__ = mode; } +function execCalls(): Array<{ file: string; args: string[] }> { + return ((globalThis as any).__EVOLITH_EXEC_CALLS__ ?? []) as Array<{ file: string; args: string[] }>; +} + async function tmpDir(): Promise { return fs.mkdtemp(path.join(os.tmpdir(), 'evolith-scaffold-')); } @@ -48,6 +61,7 @@ describe('createScaffoldTools — evolith-scaffold', () => { afterEach(() => { stderrSpy.mockRestore(); delete (globalThis as any).__EVOLITH_EXEC_MODE__; + delete (globalThis as any).__EVOLITH_EXEC_CALLS__; }); it('exposes a single mutative write-scoped tool with the expected schema', () => { @@ -137,6 +151,47 @@ describe('createScaffoldTools — evolith-scaffold', () => { domains: ['identity'], baseDir: dir, }); + + // CWE-78: every command goes out as argv through execFile — never a shell line. + const calls = execCalls(); + expect(calls.length).toBeGreaterThan(0); + expect(calls.every((c) => c.file === 'npx' || c.file === 'npm')).toBe(true); + expect(calls).toContainEqual({ + file: 'npx', + args: ['nx', 'g', '@nx/react:app', '--name=web-spa', '--directory=apps/web-spa', '--no-interactive'], + }); + expect(calls).toContainEqual({ + file: 'npx', + args: ['nx', 'g', '@nx/nest:library', '--name=identity', '--directory=libs/domain/identity', '--no-interactive'], + }); + }); + + it('refuses a caller-supplied name that is not a plain identifier before spawning anything', async () => { + const [tool] = createScaffoldTools(); + const dir = await tmpDir(); + + await expect( + tool.execute({ + path: dir, + frontend: 'react', + orm: 'typeorm', + phase: '1', + apiName: 'api; rm -rf /', + dryRun: false, + }), + ).rejects.toThrow(/Invalid API app name/); + expect(execCalls().filter((c) => c.args.includes('g'))).toHaveLength(0); + + await expect( + tool.execute({ + path: dir, + frontend: 'react', + orm: 'typeorm', + phase: '2', + remotes: ['catalog', '../../escape'], + dryRun: false, + }), + ).rejects.toThrow(/Invalid remote name/); }); it('surfaces a command failure as a thrown error (NodeCommandExecutor.executeOrThrow)', async () => { diff --git a/src/packages/mcp-server/src/tools/scaffold.tool.ts b/src/packages/mcp-server/src/tools/scaffold.tool.ts index 6a387ccd9..9ddfaa01b 100644 --- a/src/packages/mcp-server/src/tools/scaffold.tool.ts +++ b/src/packages/mcp-server/src/tools/scaffold.tool.ts @@ -1,4 +1,4 @@ -import { exec } from 'node:child_process'; +import { exec, execFile } from 'node:child_process'; import { promisify } from 'node:util'; import { NxWorkspaceStrategy } from '@beyondnet/evolith-infra-providers'; import type { @@ -9,6 +9,10 @@ import type { import { McpTool } from '../mcp/tool.interface'; const execAsync = promisify(exec); +const execFileAsync = promisify(execFile); + +// Node-based CLIs ship as `.cmd` shims on Windows; execFile needs the explicit name. +const WINDOWS_CMD_TOOLS = new Set(['npm', 'npx', 'nx']); /** Allowlists for user-controlled scaffold parameters (CWE-78 mitigation). */ const ALLOWED_FRONTEND = new Set(['react', 'angular', 'vue']); @@ -16,9 +20,10 @@ const ALLOWED_ORM = new Set(['prisma', 'typeorm', 'drizzle']); /** * Minimal {@link ICommandExecutor} for the MCP gateway: runs `npx nx` / `npm` - * via child_process. Only `executeOrThrow` is exercised by - * {@link NxWorkspaceStrategy}; `execute`/`checkTool` are provided to satisfy the - * port. Kept here (not in the CLI's richer executor) so the MCP server has no + * via child_process. Only `executeFileOrThrow` is exercised by + * {@link NxWorkspaceStrategy} (argv, no shell — the tool's `apiName`, + * `hostName`, `remotes` and `domains` are caller-controlled); `execute`, + * `executeOrThrow` and `checkTool` are provided to satisfy the port. Kept here (not in the CLI's richer executor) so the MCP server has no * cross-package dependency on the CLI. */ class NodeCommandExecutor implements ICommandExecutor { @@ -40,6 +45,25 @@ class NodeCommandExecutor implements ICommandExecutor { return result.stdout; } + async executeFile(file: string, args: string[], cwd?: string): Promise { + const bin = process.platform === 'win32' && WINDOWS_CMD_TOOLS.has(file) ? `${file}.cmd` : file; + try { + const { stdout, stderr } = await execFileAsync(bin, args, { cwd, env: process.env, timeout: Number(process.env.MCP_SCAFFOLD_TIMEOUT_MS) || 120000 }); + return { success: true, stdout, stderr, exitCode: 0 }; + } catch (err) { + const e = err as { stdout?: string; stderr?: string; code?: number; message?: string }; + return { success: false, stdout: e.stdout ?? '', stderr: e.stderr ?? e.message ?? '', exitCode: e.code ?? 1 }; + } + } + + async executeFileOrThrow(file: string, args: string[], cwd?: string): Promise { + const result = await this.executeFile(file, args, cwd); + if (!result.success) { + throw new Error(`Command failed (exit ${result.exitCode}): ${[file, ...args].join(' ')}\n${result.stderr}`); + } + return result.stdout; + } + async checkTool(name: string, versionCommand: string): Promise { const result = await this.execute(versionCommand); return { name, command: versionCommand, available: result.success, version: result.stdout.trim() || undefined }; diff --git a/src/sdk/cli/src/commands/satellite/satellite-adopt.command.ts b/src/sdk/cli/src/commands/satellite/satellite-adopt.command.ts index 5bdbf4ae4..dae39d876 100644 --- a/src/sdk/cli/src/commands/satellite/satellite-adopt.command.ts +++ b/src/sdk/cli/src/commands/satellite/satellite-adopt.command.ts @@ -67,7 +67,7 @@ export class SatelliteAdoptCommand extends BaseEvolithCommand { if (!v.startsWith('https://github.com/')) { return 'URL must start with https://github.com/'; } - if (!/github\.com\/[^/]+\/[^/]+/.test(v)) { + if (!/^https:\/\/github\.com\/[^/]+\/[^/]+/.test(v)) { return 'URL must be in the form https://github.com/owner/repo'; } return undefined; diff --git a/src/sdk/cli/src/infrastructure/cli/command-executor.ts b/src/sdk/cli/src/infrastructure/cli/command-executor.ts index 9b95de9e7..bb4ef809b 100644 --- a/src/sdk/cli/src/infrastructure/cli/command-executor.ts +++ b/src/sdk/cli/src/infrastructure/cli/command-executor.ts @@ -116,6 +116,15 @@ export class CommandExecutor implements ICommandExecutor { return result.stdout; } + /** {@link executeFile} that throws {@link CommandExecutionError} on a non-zero exit. */ + async executeFileOrThrow(file: string, args: string[], cwd?: string): Promise { + const result = await this.executeFile(file, args, cwd); + if (!result.success) { + throw new CommandExecutionError([file, ...args].join(' '), result.exitCode, result.stderr); + } + return result.stdout; + } + private getInstallHint(tool: string): string { const hints: Record = { npm: 'Install Node.js from https://nodejs.org/',