From dde0333b3d89807317405000d8233c445eb8df66 Mon Sep 17 00:00:00 2001 From: albertgwo Date: Tue, 17 Mar 2026 19:57:59 -0400 Subject: [PATCH 1/2] feat(engine): add security hardening (path containment, YAML limits, reserved IDs) - Add assertWithinProject() utility to prevent path traversal attacks - Use it in loadEntryPoint() and loadRulesFile() before resolving paths - Add YAML parse limits (maxAliasCount: 100, schema: core) to prevent YAML bombs - Add reserved node ID validation (input, state, Math, node, output) in structural validation --- packages/engine/src/index.ts | 3 +++ packages/engine/src/rules/evaluator.ts | 4 ++- packages/engine/src/runner/loader.ts | 2 ++ packages/engine/src/security/index.ts | 1 + .../engine/src/security/path-containment.ts | 16 ++++++++++++ packages/schema/src/structural.ts | 25 ++++++++++++++++--- 6 files changed, 47 insertions(+), 4 deletions(-) create mode 100644 packages/engine/src/security/index.ts create mode 100644 packages/engine/src/security/path-containment.ts diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 0bdebab..aee3669 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -34,6 +34,9 @@ export type { RulesTestResult, } from './rules/index.js' +// Security +export { assertWithinProject } from './security/index.js' + // Codegen export { generateCode } from './codegen/index.js' export type { GenerateResult, GenerateOptions, GeneratedFile } from './codegen/index.js' diff --git a/packages/engine/src/rules/evaluator.ts b/packages/engine/src/rules/evaluator.ts index 3b68a5f..cc54892 100644 --- a/packages/engine/src/rules/evaluator.ts +++ b/packages/engine/src/rules/evaluator.ts @@ -1,6 +1,7 @@ import { readFileSync } from 'node:fs' import { resolve } from 'node:path' import { parse } from 'yaml' +import { assertWithinProject } from '../security/index.js' import { validateRules } from '@ruminaider/flowprint-schema' import type { ExecutionContext } from '../runner/types.js' import { evaluateExpression } from '../runner/evaluator.js' @@ -24,6 +25,7 @@ import type { * @returns Parsed and validated RulesDocument */ export function loadRulesFile(filePath: string, projectRoot: string): RulesDocument { + assertWithinProject(filePath, projectRoot) const absolutePath = resolve(projectRoot, filePath) let content: string @@ -36,7 +38,7 @@ export function loadRulesFile(filePath: string, projectRoot: string): RulesDocum let doc: unknown try { - doc = parse(content) + doc = parse(content, { maxAliasCount: 100, schema: 'core' }) } catch (err: unknown) { const message = err instanceof Error ? err.message : String(err) throw new Error(`Failed to parse rules file "${filePath}": ${message}`) diff --git a/packages/engine/src/runner/loader.ts b/packages/engine/src/runner/loader.ts index c3de085..a551bd9 100644 --- a/packages/engine/src/runner/loader.ts +++ b/packages/engine/src/runner/loader.ts @@ -1,4 +1,5 @@ import { resolve } from 'node:path' +import { assertWithinProject } from '../security/index.js' /** * Dynamically import an entry point file and extract the named symbol. @@ -11,6 +12,7 @@ export async function loadEntryPoint( entry: { file: string; symbol: string }, projectRoot: string, ): Promise<(...args: unknown[]) => unknown> { + assertWithinProject(entry.file, projectRoot) const filePath = resolve(projectRoot, entry.file) let mod: Record diff --git a/packages/engine/src/security/index.ts b/packages/engine/src/security/index.ts new file mode 100644 index 0000000..4547fb8 --- /dev/null +++ b/packages/engine/src/security/index.ts @@ -0,0 +1 @@ +export { assertWithinProject } from './path-containment.js' diff --git a/packages/engine/src/security/path-containment.ts b/packages/engine/src/security/path-containment.ts new file mode 100644 index 0000000..4449781 --- /dev/null +++ b/packages/engine/src/security/path-containment.ts @@ -0,0 +1,16 @@ +import { resolve, sep, relative } from 'node:path' + +/** + * Assert that a file path resolves within the project root directory. + * Prevents path traversal attacks (e.g. `../../etc/passwd`). + * + * @param filePath - The file path to validate (absolute or relative) + * @param projectRoot - The root directory boundary + * @throws Error if the resolved path is outside the project root + */ +export function assertWithinProject(filePath: string, projectRoot: string): void { + const resolved = resolve(projectRoot, filePath) + if (!resolved.startsWith(projectRoot + sep) && resolved !== projectRoot) { + throw new Error(`Path "${relative(projectRoot, resolved)}" resolves outside project root`) + } +} diff --git a/packages/schema/src/structural.ts b/packages/schema/src/structural.ts index 3e489a3..08d4bca 100644 --- a/packages/schema/src/structural.ts +++ b/packages/schema/src/structural.ts @@ -1,11 +1,19 @@ import type { ValidationError } from './types.js' +/** + * Node IDs that conflict with expression sandbox globals. + * Using these as node IDs would shadow built-in variables in the + * expression evaluator, leading to subtle bugs or security issues. + */ +const RESERVED_NODE_IDS = new Set(['input', 'state', 'Math', 'node', 'output']) + /** * Perform structural validation on a schema-valid Flowprint document. * Checks for: - * 1. Dangling node references (next, cases[].next, branches[], join, error.catch, default, timeout_next) - * 2. Invalid lane references (node.lane must exist in lanes) - * 3. Orphan nodes (non-terminals: no incoming AND no outgoing; terminals: no incoming) + * 1. Reserved node IDs (conflict with expression sandbox globals) + * 2. Dangling node references (next, cases[].next, branches[], join, error.catch, default, timeout_next) + * 3. Invalid lane references (node.lane must exist in lanes) + * 4. Orphan nodes (non-terminals: no incoming AND no outgoing; terminals: no incoming) * * This function assumes the document has already passed schema validation. */ @@ -19,6 +27,17 @@ export function validateStructure(doc: Record): ValidationError return errors } + // Check for reserved node IDs + for (const nodeId of Object.keys(nodes)) { + if (RESERVED_NODE_IDS.has(nodeId)) { + errors.push({ + path: `/nodes/${nodeId}`, + message: `Node ID "${nodeId}" is reserved (conflicts with expression sandbox globals). Reserved IDs: ${[...RESERVED_NODE_IDS].join(', ')}`, + severity: 'error', + }) + } + } + const laneIds = new Set(Object.keys(lanes)) const nodeIds = new Set(Object.keys(nodes)) From 5283df2e1d24aafce64136fa7450a331f8b02b51 Mon Sep 17 00:00:00 2001 From: albertgwo Date: Tue, 17 Mar 2026 19:59:43 -0400 Subject: [PATCH 2/2] test(engine): add security hardening tests - Path containment: traversal attacks, absolute paths, prefix-matching edge cases - Reserved node IDs: all 5 reserved names rejected, non-reserved names accepted - YAML parsing limits: excessive alias count rejected, within-limit accepted --- .../security/path-containment.test.ts | 54 ++++++++++++ .../security/reserved-node-ids.test.ts | 87 +++++++++++++++++++ .../__tests__/security/yaml-limits.test.ts | 56 ++++++++++++ 3 files changed, 197 insertions(+) create mode 100644 packages/engine/src/__tests__/security/path-containment.test.ts create mode 100644 packages/engine/src/__tests__/security/reserved-node-ids.test.ts create mode 100644 packages/engine/src/__tests__/security/yaml-limits.test.ts diff --git a/packages/engine/src/__tests__/security/path-containment.test.ts b/packages/engine/src/__tests__/security/path-containment.test.ts new file mode 100644 index 0000000..03f8f91 --- /dev/null +++ b/packages/engine/src/__tests__/security/path-containment.test.ts @@ -0,0 +1,54 @@ +import { describe, it, expect } from 'vitest' +import { assertWithinProject } from '../../security/path-containment.js' + +describe('assertWithinProject', () => { + const projectRoot = '/home/user/project' + + it('allows a path within the project root', () => { + expect(() => assertWithinProject('src/main.ts', projectRoot)).not.toThrow() + }) + + it('allows a nested path within the project root', () => { + expect(() => assertWithinProject('src/rules/order.rules.yaml', projectRoot)).not.toThrow() + }) + + it('throws on ../../etc/passwd style traversal', () => { + expect(() => assertWithinProject('../../etc/passwd', projectRoot)).toThrow( + /resolves outside project root/, + ) + }) + + it('throws on absolute path outside project', () => { + expect(() => assertWithinProject('/etc/passwd', projectRoot)).toThrow( + /resolves outside project root/, + ) + }) + + it('error message shows relative path, not absolute', () => { + try { + assertWithinProject('../../etc/passwd', projectRoot) + expect.fail('should have thrown') + } catch (err) { + const message = (err as Error).message + expect(message).toContain('../../etc/passwd') + expect(message).not.toContain(projectRoot) + } + }) + + it('throws on path that starts with projectRoot as prefix but is not a child', () => { + // /home/user/project-extra is not inside /home/user/project + expect(() => assertWithinProject('../project-extra/file', projectRoot)).toThrow( + /resolves outside project root/, + ) + }) + + it('allows the project root itself', () => { + expect(() => assertWithinProject('.', projectRoot)).not.toThrow() + }) + + it('throws on sneaky path with encoded traversal', () => { + expect(() => assertWithinProject('src/../../../etc/shadow', projectRoot)).toThrow( + /resolves outside project root/, + ) + }) +}) diff --git a/packages/engine/src/__tests__/security/reserved-node-ids.test.ts b/packages/engine/src/__tests__/security/reserved-node-ids.test.ts new file mode 100644 index 0000000..2bda30d --- /dev/null +++ b/packages/engine/src/__tests__/security/reserved-node-ids.test.ts @@ -0,0 +1,87 @@ +import { describe, it, expect } from 'vitest' +import { validate } from '@ruminaider/flowprint-schema' + +function makeDoc(nodeId: string) { + return { + schema: 'flowprint/1.0', + name: 'test', + version: '1.0.0', + lanes: { main: { label: 'Main', visibility: 'external', order: 0 } }, + nodes: { + [nodeId]: { type: 'action', lane: 'main', label: 'Step', next: 'end' }, + end: { type: 'terminal', lane: 'main', label: 'End', outcome: 'success' }, + }, + } +} + +describe('reserved node IDs', () => { + it('rejects node ID "input"', () => { + const result = validate(makeDoc('input')) + expect(result.valid).toBe(false) + expect( + result.errors.some( + (e) => e.path === '/nodes/input' && e.message.includes('reserved'), + ), + ).toBe(true) + }) + + it('rejects node ID "state"', () => { + const result = validate(makeDoc('state')) + expect(result.valid).toBe(false) + expect( + result.errors.some( + (e) => e.path === '/nodes/state' && e.message.includes('reserved'), + ), + ).toBe(true) + }) + + it('rejects node ID "Math"', () => { + const result = validate(makeDoc('Math')) + expect(result.valid).toBe(false) + expect( + result.errors.some( + (e) => e.path === '/nodes/Math' && e.message.includes('reserved'), + ), + ).toBe(true) + }) + + it('rejects node ID "node"', () => { + const result = validate(makeDoc('node')) + expect(result.valid).toBe(false) + expect( + result.errors.some( + (e) => e.path === '/nodes/node' && e.message.includes('reserved'), + ), + ).toBe(true) + }) + + it('rejects node ID "output"', () => { + const result = validate(makeDoc('output')) + expect(result.valid).toBe(false) + expect( + result.errors.some( + (e) => e.path === '/nodes/output' && e.message.includes('reserved'), + ), + ).toBe(true) + }) + + it('allows non-reserved node ID "my_action"', () => { + const result = validate(makeDoc('my_action')) + expect(result.valid).toBe(true) + }) + + it('allows non-reserved node ID "process_input"', () => { + const result = validate(makeDoc('process_input')) + expect(result.valid).toBe(true) + }) + + it('error message lists all reserved IDs', () => { + const result = validate(makeDoc('input')) + const error = result.errors.find((e) => e.path === '/nodes/input') + expect(error?.message).toContain('input') + expect(error?.message).toContain('state') + expect(error?.message).toContain('Math') + expect(error?.message).toContain('node') + expect(error?.message).toContain('output') + }) +}) diff --git a/packages/engine/src/__tests__/security/yaml-limits.test.ts b/packages/engine/src/__tests__/security/yaml-limits.test.ts new file mode 100644 index 0000000..9ed849f --- /dev/null +++ b/packages/engine/src/__tests__/security/yaml-limits.test.ts @@ -0,0 +1,56 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { parse } from 'yaml' + +describe('YAML parsing limits', () => { + it('rejects YAML with excessive aliases (YAML bomb protection)', () => { + // Build a YAML bomb: define an anchor and reference it 101+ times + const lines = ['top: &a value'] + for (let i = 0; i < 101; i++) { + lines.push(`k${String(i)}: *a`) + } + const yaml = lines.join('\n') + + // With maxAliasCount: 100, parsing should throw + expect(() => parse(yaml, { maxAliasCount: 100, schema: 'core' })).toThrow( + /excessive alias count/i, + ) + }) + + it('accepts YAML with aliases within the limit', () => { + const lines = ['top: &a value'] + for (let i = 0; i < 50; i++) { + lines.push(`k${String(i)}: *a`) + } + const yaml = lines.join('\n') + + // With maxAliasCount: 100, parsing 50 aliases should succeed + const result = parse(yaml, { maxAliasCount: 100, schema: 'core' }) + expect(result).toBeDefined() + expect(result.top).toBe('value') + expect(result.k0).toBe('value') + }) + + it('verifies loadRulesFile rejects YAML bombs', async () => { + // Dynamically import to test the actual code path with mocked fs + vi.mock('node:fs', () => ({ + readFileSync: vi.fn(), + })) + + const { readFileSync } = await import('node:fs') + const mockedReadFileSync = vi.mocked(readFileSync) + const { loadRulesFile } = await import('../../rules/evaluator.js') + + // Build a YAML bomb with 101 aliases + const lines = ['schema: flowprint-rules/1.0', 'name: bomb', 'hit_policy: first', 'x: &a val'] + for (let i = 0; i < 101; i++) { + lines.push(`k${String(i)}: *a`) + } + lines.push('rules:', ' - then:', ' result: true') + + mockedReadFileSync.mockReturnValue(lines.join('\n')) + + expect(() => loadRulesFile('bomb.yaml', '/project')).toThrow(/parse/i) + + vi.restoreAllMocks() + }) +})