-
Notifications
You must be signed in to change notification settings - Fork 0
feat(engine): security hardening (path containment, YAML limits, reserved IDs) #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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/, | ||
| ) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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') | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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() | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { assertWithinProject } from './path-containment.js' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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`) | ||
|
Comment on lines
+11
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assertWithinProject compares the resolved path to an unnormalized projectRoot — should we resolve projectRoot first const normalizedRoot = resolve(projectRoot) Finding type: Want Baz to fix this for you? Activate Fixer Other fix methodsPrompt for AI Agents: Heads up! Your free trial ends tomorrow.
Comment on lines
+11
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assertWithinProject compares a resolved path to the raw projectRoot so relative/unnormalized roots can fail; should we normalize projectRoot with resolve or realpath before the startsWith/equals check? Finding type: Want Baz to fix this for you? Activate Fixer Other fix methodsPrompt for AI Agents: Heads up! Your free trial ends tomorrow.
Comment on lines
+11
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assertWithinProject concatenates projectRoot + sep without normalizing, so a trailing slash can produce a double separator and break the .startsWith containment check; should we normalize projectRoot first (e.g. const normalizedRoot = resolve(projectRoot)) and use normalizedRoot + sep? Finding type: Want Baz to fix this for you? Activate Fixer Other fix methodsPrompt for AI Agents: Heads up! Your free trial ends tomorrow. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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']) | ||
|
Comment on lines
1
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RESERVED_NODE_IDS rejects names like state, node, and output that the evaluator sandbox doesn't expose — should we derive the reserved list from the sandbox globals or export a shared literal? Finding type: Want Baz to fix this for you? Activate Fixer Other fix methodsPrompt for AI Agents: Heads up! Your free trial ends tomorrow. |
||
|
|
||
| /** | ||
| * 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<string, unknown>): 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)) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR adds a new public export assertWithinProject and schema structural changes but doesn't include a .changeset/*.md — should we add one?
Finding type:
AI Coding Guidelines| Severity: 🟠 MediumWant Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription