feat(engine): security hardening (path containment, YAML limits, reserved IDs)#9
feat(engine): security hardening (path containment, YAML limits, reserved IDs)#9
Conversation
…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
- 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
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| 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']) |
There was a problem hiding this comment.
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: Breaking Changes | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/schema/src/structural.ts around lines 1 to 8, the new hardcoded
RESERVED_NODE_IDS set incorrectly blocks names (state, node, output) that are not
actually injected into the evaluator sandbox. Replace the hardcoded Set with a single
source of truth: either import the reserved/global names from the evaluator module
(packages/engine/src/runner/evaluator.ts) or export a shared constant from a new/common
module that both the engine and schema packages consume. Update the top of this file to
import that shared list and use it to build RESERVED_NODE_IDS, and add a short comment
explaining that this list must match the evaluator sandbox globals to avoid breaking
changes. Ensure tests or a validation run still pass.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| 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`) |
There was a problem hiding this comment.
assertWithinProject compares the resolved path to an unnormalized projectRoot — should we resolve projectRoot first
const normalizedRoot = resolve(projectRoot)
resolved.startsWith(normalizedRoot + sep)
Finding type: Breaking Changes | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/security/path-containment.ts around lines 11 to 14, the
assertWithinProject function resolves filePath against the raw projectRoot but compares
the resolved path to projectRoot + sep, which breaks when projectRoot is a relative path
or has symlinks. Refactor by first normalizing/resolving projectRoot (e.g. const
normalizedRoot = resolve(projectRoot)), then use normalizedRoot in the prefix/equality
checks and in the relative() call for the error message (keep the same
startsWith(normalizedRoot + sep) && resolved !== normalizedRoot logic). Ensure imports
stay the same and update the thrown message to use relative(normalizedRoot, resolved).
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| 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`) |
There was a problem hiding this comment.
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: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/security/path-containment.ts around lines 11 to 14, the
assertWithinProject function compares the resolved file path against the raw projectRoot
string which fails for relative or unnormalized roots. Refactor by normalizing the
boundary first (e.g. const normalizedRoot = resolve(projectRoot) or use
realpathSync(projectRoot) if you need symlink resolution), then compute const resolved =
resolve(normalizedRoot, filePath) and perform the containment check against
normalizedRoot (use resolved.startsWith(normalizedRoot + sep) || resolved ===
normalizedRoot). Update variable names accordingly and keep the existing error message
logic.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| 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`) |
There was a problem hiding this comment.
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: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/security/path-containment.ts around lines 11-14, the
assertWithinProject function uses projectRoot + sep as the containment prefix without
normalizing projectRoot, which fails when projectRoot has a trailing separator. Refactor
by first normalizing the root (e.g. const normalizedRoot = resolve(projectRoot)), use
normalizedRoot when resolving the file path and for the prefix (normalizedRoot + sep),
and update the equality/startsWith checks and the error message to use normalizedRoot
and relative(normalizedRoot, resolved). Ensure behavior remains the same when resolved
=== normalizedRoot.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
| // Security | ||
| export { assertWithinProject } from './security/index.js' |
There was a problem hiding this comment.
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: 🟠 Medium
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/index.ts around lines 37-38, a new public export
`assertWithinProject` was added. Also account for structural validation changes in
packages/schema/src/structural.ts that alter package behavior. Add a .changeset/*.md
file that (1) describes the addition of the new public export and the structural
validation change, (2) lists the affected packages (at least packages/engine and
packages/schema), and (3) specifies appropriate version bumps (e.g., minor for the new
public API export and patch or minor as appropriate for the schema behavioral change, or
mark as breaking if the structural change is breaking). Ensure the changeset includes a
short human-readable summary and author name so the release tooling will include these
changes before merging.
Heads up!
Your free trial ends tomorrow.
To keep getting your PRs reviewed by Baz, update your team's subscription
Deploying flowprint with
|
| Latest commit: |
5283df2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2365ede9.flowprint.pages.dev |
| Branch Preview URL: | https://feat-engine-pr0a-security-ha.flowprint.pages.dev |
Code Review FindingsCritical (90-100 confidence)1. Path containment bypass with trailing slash on
const normalizedRoot = resolve(projectRoot)
const resolved = resolve(normalizedRoot, filePath)2. Symlink escapes not actually protected (90)
Important (80-89 confidence)3. Unused
4. YAML bomb test has fragile coupling to path containment (82)
5. PR description doesn't match code (80)
|
1 participant
User description
Summary
containsPath) to prevent escaping workspace boundaries__start,__end,__error) from user blueprintsDependency
Base:
main(no dependencies)PR 1 of 16 in the execution engine implementation.
Test plan
../traversals and symlink escapesGenerated description
Below is a concise technical summary of the changes proposed in this PR:
Fortify entry and rule loading by exporting
assertWithinProjectsoloadEntryPointandloadRulesFileare constrained within the workspace boundaries while path containment tests exercise the new security guard. Harden blueprint ingestion by limiting YAML alias counts and flagging reserved node IDs invalidateStructure, with tests covering both alias bombs and reserved identifier rejection.validateStructure, with companion tests that exercise YAML bomb rejection and reserved ID errors to keep blueprints confined to safe identifiers.Modified files (4)
Latest Contributors(1)
assertWithinProjectthrough the security module, calling it fromloadEntryPointandloadRulesFile, and validating the containment rule with targeted tests so the engine cannot resolve files outside the project root.Modified files (6)
Latest Contributors(1)