Skip to content

fix(executor): escape resolved references in while/doWhile loop conditions - #6181

Open
waleedlatif1 wants to merge 1 commit into
stagingfrom
fix/loop-condition-js-injection
Open

fix(executor): escape resolved references in while/doWhile loop conditions#6181
waleedlatif1 wants to merge 1 commit into
stagingfrom
fix/loop-condition-js-injection

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Loop conditions inlined each resolved <reference> as a bare double-quoted literal, then compiled the result in the execution isolate — a quote anywhere in the referenced value terminated the literal and ran the rest as JavaScript. Reachable by an external caller on any deployed workflow with a data-driven while condition; no workflow authorship needed.
  • Serialize condition operands as proper JS literals (JSON.stringify plus U+2028/U+2029 escaping) instead of hand-quoting, matching the escaping the condition block already does via stringifyForCondition.
  • The number / boolean / "true" / "false" / object / null branches are untouched, so output is byte-identical for every value that didn't already produce broken code.
  • Scoped deliberately to the escaping fix. The unbounded-iteration and error-swallowing items from the report are left for separate follow-up: once injection is closed the runaway-loop primitive is no longer externally reachable, and both changes carry regression risk for currently-working workflows that this one does not.

Type of Change

  • Bug fix

Testing

  • New tests cover the injection payload (asserting the generated code evaluates to false and the operand is a proper literal), backslash/newline/line-separator escaping, and the full coercion table.
  • Reverted the fix and confirmed the injection tests go red — the PoC payload evaluated to true against the old code.
  • Differential harness comparing old vs new formatting across every BMP code point, realistic workflow values, and 60k fuzz strings over 7 condition templates: 1,753,381 condition evaluations, 0 cases where behavior changed without the old code being uncompilable or corrupted. Non-string types (NaN, Infinity, -0, 1e21, nested objects): 0 differences.
  • loop.test.ts 20/20; full executor/ suite passing apart from failures unrelated to these files.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Aug 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 2, 2026 2:04am

Request Review

@cursor

cursor Bot commented Aug 2, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Changes how user-controlled data is embedded into compiled loop-condition code in the execution isolate; the fix is security-critical and must not regress safe literals while closing injection.

Overview
Closes a code-injection path in while/doWhile loop conditions where resolved <reference> values were inlined as hand-quoted strings before executeInIsolatedVM compiled return Boolean(...). A double quote (or crafted payload) in referenced data could break out of the literal and run arbitrary JavaScript in the isolate.

Operands are now built via formatConditionOperand / toJsLiteral: numbers, booleans, and trimmed "true"/"false" strings keep the same emission rules; everything else uses JSON.stringify plus escaping of U+2028/U+2029 so the generated source stays valid JS—aligned with how condition blocks already treat values via stringifyForCondition.

Tests cover injection payloads, backslashes/newlines/line separators, and the coercion table for common resolved types.

Reviewed by Cursor Bugbot for commit fc4f4c7. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR prevents resolved loop-condition references from escaping their generated JavaScript literals.

  • Replaces hand-quoted string interpolation with JSON-based JavaScript literal serialization.
  • Escapes U+2028 and U+2029 before isolate compilation.
  • Adds regression tests for injection payloads, escaping, and existing coercion behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
apps/sim/executor/orchestrators/loop.ts Safely serializes resolved loop-condition operands before compiling the generated expression in the execution isolate.
apps/sim/executor/orchestrators/loop.test.ts Adds focused coverage for expression injection, source-character escaping, and preserved operand coercion.

Reviews (2): Last reviewed commit: "fix(executor): escape resolved reference..." | Re-trigger Greptile

…tions

Loop conditions inlined each resolved reference as a bare double-quoted
literal before compiling the expression in the execution isolate, so a
quote anywhere in the referenced value broke out of the literal and ran
as code. Serialize operands as proper JS literals instead, matching the
escaping the condition block already does via stringifyForCondition.
@waleedlatif1
waleedlatif1 force-pushed the fix/loop-condition-js-injection branch from d3d686b to fc4f4c7 Compare August 2, 2026 02:04
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit fc4f4c7. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant