Add workflow expression security validation#8
Conversation
dhairyashiil
left a comment
There was a problem hiding this comment.
Hi Sezal, thanks for adding the expression security layer. Since this is the sandbox boundary, could we fix two things before this lands?
Could we block computed access to dangerous properties like constructor, prototype, and __proto__? Right now direct access is blocked, but an expression can still build the property name dynamically and reach those properties through computed access. That means a transform can mutate prototypes, which breaks the security guarantee this PR is trying to provide.
Could we also add acorn as a runtime dependency in the root package? expression-security.ts imports it at runtime, but the root package.json does not list it under dependencies. It is only present through dev tooling, so production installs can fail.
Also, npm run lint starts failing on this stack around this area, so could we clean that up here as well?
This is a strong direction, I just think the security boundary should be strict from the first version.
Address review feedback on PR #8: - Block computed member access with dynamic keys so constructor/prototype/__proto__ cannot be reached via runtime-built property names - Add acorn as a runtime dependency (was only present transitively via dev tooling) - Fix lint failures (attach cause on rethrow, justify structural any) - Switch to an allowlist identifier model, bound AST depth, block reserved-name shadowing, and handle spread elements
Summary
Depends on
#7