fix(executor): stop resolved data from breaking out of generated condition and function code - #7300
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThe PR prevents resolved workflow data from becoming executable source in generated condition and function code while narrowing condition sandboxes to the secrets explicitly required.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/executor/variables/resolver.ts | Resolves dynamic function values through runtime bindings, hardens condition formatting, records authored environment-map access, and improves lexical-context tracking. |
| apps/sim/executor/handlers/condition/condition-handler.ts | Narrows condition sandbox secrets using pre-resolution environment-read metadata and explicitly named placeholders. |
| apps/sim/executor/utils/code-formatting.ts | Adds shared JavaScript string-content escaping for values inserted into condition expressions. |
| apps/sim/executor/variables/resolvers/block.ts | Uses the shared inert string formatter in the fallback condition-value formatting path. |
| apps/sim/executor/variables/resolver.test.ts | Adds regression coverage for injection payloads, object formatting, lexical-context detection, runtime bindings, and legacy condition outcomes. |
| apps/sim/executor/handlers/condition/condition-handler.test.ts | Verifies selected, empty, and full secret scopes and prevents resolved payload data from influencing mounted secrets. |
| apps/sim/executor/variables/resolvers/block.test.ts | Covers fallback formatter behavior across JavaScript quote terminators. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Author-written condition or function code] --> B[Resolve workflow references]
B --> C{Resolved value may remain an inert literal?}
C -->|Yes| D[Emit safe literal]
C -->|No| E[Bind runtime context variable]
D --> F[Generated sandbox code]
E --> F
A --> G[Detect authored secret references]
G --> H[Select sandbox secret scope]
H --> F
Reviews (13): Last reviewed commit: "fix(executor): start a new identifier at..." | Re-trigger Greptile
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
A Condition expression is compiled by inlining each resolved reference as
source text, and the literal that gets emitted only ever anticipated the
quoting it chose itself. The author's quoting decides the real context, so
`"<start.input>".includes('urgent')` — a shape that works correctly with
benign data — let webhook, chat, or form data close the author's string and
run as JavaScript in the condition sandbox, which receives the workspace's
whole decrypted environment as the `environmentVariables` global. Template
literals, regex literals, and a crafted object key reached the same place.
Both condition formatters now escape every terminator of every JavaScript
string context rather than the one they open. `\"`, `` \` ``, `\$` and `\/`
are identity escapes, so a condition compares exactly what it compared
before; only its ability to parse as anything but data changes. A quoted
object reference additionally escapes its JSON, the only reading of that
shape that was not already a syntax error.
Function blocks bind block outputs as context variables but inlined the
remaining resolved values as literals, so a workflow variable a Variables
block had assigned from trigger data, or a loop item, could close the
string it landed in. Those bind now too. Numbers, booleans, null, and
strings that name an environment variable stay inline: the first three
cannot terminate a literal, and the last has to stay in source because the
placeholder — never the secret — is what is inlined, and the
execution-boundary compiler binds it downstream.
Condition evaluation also stops shipping the full secret map to the
sandbox: it mounts only the names its script references, so a future defect
in this path reaches nothing the condition did not already name.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Escaping a quoted object's JSON left the quote scanner load-bearing for
injection: it does not track regex literals, so a quote inside one
desynchronizes it and a later object reference is reported as unquoted,
which put raw attacker-shaped JSON back into source. A reference inside a
regex literal reached the same place through its unescaped slashes.
Objects outside a string are now parsed at runtime from a fully escaped
literal. The value is identical to the object literal it replaces, and the
emitted form carries no quote, slash, backtick or `${`, so it stays inert
whichever context the scanner reports.
Scoping now reads the expressions for a direct `environmentVariables`
access rather than the whole generated script, so a source block's output
containing that word can no longer widen the mounted secret set. The
placeholder scan still reads the built script, which is the text the
execution-boundary compiler substitutes over.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A member-access pattern decides whether a condition keeps the full secret map, and every shape it fails to anticipate — `environmentVariables?.FLAG`, a read through `Object.keys` — silently narrows what that expression can see and routes the run down a branch the author did not write. Matching the bare identifier inside the expressions costs only the narrowing, and never mounts more than this path mounted before it existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A regex body is the one place a lone quote is not a string delimiter, and the scanner did not track regex literals at all: `/['"]/` left it believing everything after it sat inside a string. Every later reference was then formatted for a context it was not in — a quoted object reference stayed raw source, and after the previous commit a bare one was emitted as escaped JSON, which cannot parse, so a valid condition threw instead of routing. The scan now enters regex mode where a `/` can only be a regex — division always follows a value, so the preceding token decides — and tracks escapes and character classes until the closing delimiter. A reference inside a regex reports its own context, so its JSON is escaped as pattern text rather than spliced with delimiters the data could forge. The same scanner decides how function-block references are spliced, so this also repairs quoting for code that matches on a quote-bearing pattern. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The built script carries the source block's output as data, so scanning it
for placeholders let a caller choose which secret materializes beside its
own payload: `{{SECRET}}` in trigger data mounted that secret and had the
compiler expand it into the serialized context. Both scans now read the
expressions, which is where every legitimate route to a secret runs —
including a workflow variable holding `{{NAME}}`, since the resolver inlines
that value into the expression before this handler sees it.
`throw` joins the keywords a regex may follow. `throw /re/` is legal, and
without it the scan reads the pattern body as code and mis-reports the
context of everything after it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aa5c8f0 to
47da972
Compare
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
Treating a comment end as the answer was too blunt: `/* c */ if (x)` is a control-flow head, and calling it a method left a statement-position regex scanned as division — the failure the comment guard was added to prevent, moved one shape over. The scan now steps back over comments to the token that precedes them, so the dot in `p./* c */catch(fn)` is still found and a keyword after a comment is still a head. A condition's environment read is placed the same way references are: an occurrence inside a string, a template, or a regex is text and mounts nothing, while any executable read — whatever its shape — keeps the map. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 7 files
Confidence score: 3/5
apps/sim/executor/variables/resolver.tsinopensControlFlowHeadcan misclassify.catchwhen a block comment between the dot andcatchcontains another/*, becauselastIndexOfselects the embedded delimiter; use the comment’s actual opener and add a nested-delimiter regression test.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/executor/variables/resolver.ts">
<violation number="1" location="apps/sim/executor/variables/resolver.ts:1233">
P1: When a block comment between a property-access dot and `catch` contains another `/*`, `lastIndexOf` selects the embedded delimiter instead of the comment's actual opener. `opensControlFlowHead` then misclassifies `.catch(...)` as a control-flow head, so the following division slash can be scanned as a regex and corrupt later reference formatting. Track the actual non-nesting block-comment boundary when scanning backward.</violation>
</file>
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
…k back Reading backwards cannot tell which characters were code: a block comment opens at its first delimiter, so `p./* a /* b */catch(fn)` defeated a search for the nearest `/*` and the call read as a control-flow head again. The scan already knows — it stepped over that comment on the way in — so the two facts the check needs, the token before the parenthesis and whether it followed a property access, are now recorded as it passes and read from there. No search back through the source, and nothing left for a comment body to imitate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 7 files
Confidence score: 3/5
apps/sim/executor/variables/resolver.tsmisclassifies the division slash after valid postfix++or--as a regex opener, causing later references to be emitted as regex text and direct environment-map reads to be mishandled; update the token classification to distinguish division from regex starts.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/executor/variables/resolver.ts">
<violation number="1" location="apps/sim/executor/variables/resolver.ts:193">
P2: When valid JavaScript uses postfix `++` or `--` before division, this character-based set classifies the division slash as a regex opener. Later references are emitted as regex text and direct environment-map reads can be missed; track postfix operators as value tokens before deciding whether `/` starts a regex.</violation>
</file>
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
`+` and `-` precede a regex as operators, but doubled they end a value, so `i++ / 2` was scanning a regex from the division and swallowing whatever quotes followed it on that line. The check now reads the pair rather than the single character; a lone `+` still admits `params.n + /re/.test(x)`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
A token continues only when the character immediately before it belongs to the same token. Asking the previous *significant* character instead made a name after a line break look like a continuation, so it kept whatever property-access answer the last token had: `const seen = params.a.b` on one line left the `if` on the next carrying `b`'s, which turned the statement head into a method call and the regex after it into division. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cubic-dev-ai review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
Problem
A Condition expression is compiled by inlining each resolved reference into the author's JavaScript as source text. The literal that gets emitted only ever anticipated the quoting it chose itself — it escapes
\,', and line terminators, then wraps in'…'— but the author's quoting decides which context the value actually lands in.So trigger data (webhook body, chat message, form field) could close the author's string and execute:
That runs in the condition sandbox, which is handed the run's whole decrypted environment (
{...personalDecrypted, ...workspaceDecrypted}fromexecution-core.ts) as theenvironmentVariablesglobal, inside an async wrapper whereawait fetch(...)is available. The branch verdict is unaffected, so nothing in the run log shows it happened.Four shapes reached it — double-quoted, template literal, regex literal, and a crafted key on a quoted object reference. The dangerous ones are the shapes that also behave correctly with benign data, so they survive in production:
"<x>".includes(…),"<x>".toLowerCase().includes(…),`<x>`.length > n,/<x>/.test(…). ("<x>" === "admin"injects too, but is always false even benignly, so an author would have abandoned it.) A bare<x> === 'admin'was never vulnerable, and'<x>' === 'admin'is a syntax error today, so it never ships.Same root cause, second site: Function blocks bind block outputs as runtime context variables (safe), but inlined the remaining resolved values as literals. A Variables block can assign trigger data into a workflow variable at runtime, and
WorkflowResolverreads the live map — soconst x = '<variable.userinput>'spliced attacker text into code. Loop items reached the same path, in JavaScript, Python, and shell.Fix
Conditions — one shared literal formatter, no behavior change.
escapeInertStringContentescapes every terminator of every JavaScript string context, not just the one the emitted literal opens.\",\`,\$, and\/are identity escapes, so a condition compares byte-for-byte what it compared before; only its ability to parse as anything but data changes. Both condition formatters use it —resolveTemplateWithoutConditionFormatting(the live path) andstringifyForCondition(the fallback, which had the mirrored blind spot: wraps in", escaped only").Deliberately not the quote-context rewrite (
getCodeStringQuoteContext+quote + JSON.stringify(v) + quote) that the code-block path uses: it would make"<x>" === "admin"start matching where today it is always false. That is the correct long-term semantics, but it re-routes live workflows and belongs in its own announced change, not a security patch.A quoted object reference additionally escapes its JSON, since JSON's own structural quotes close the author's string. In expression position the JSON stays a literal (unchanged). In a quoted position the previous output was either a syntax error or an injection, so the escaped form is the only reading that was not already broken.
Function blocks — bind instead of splice. Resolved values now bind as context variables like block outputs always have. Same runtime value, never source. Two exceptions stay inline: numbers/booleans/null (digits and keywords cannot terminate a literal), and a string that names an environment variable and carries no quote — that shape has to stay in source because the placeholder, never the secret, is what is inlined, and the execution-boundary compiler binds it downstream (this is how
<variable.indirectSecret>reaches its value).Defence in depth. Condition evaluation stops shipping the full secret map to the sandbox: it mounts only the names its script references (
secretScope: 'selected'). A script that reads theenvironmentVariablesglobal directly keeps the full map, so nothing that works today breaks.Tests
resolver.test.ts— 5 quotings × 4 payloads must not execute; a crafted-key object must not close the string it is quoted inside; bare and quoted references must still compare what they compared before (including values carrying",$,/). Both injection tests were confirmed to fail without their respective fix.condition-handler.test.ts— mounts only named secrets, denies all when none are named, keepsallfor directenvironmentVariablesaccess.block.test.ts— the fallback formatter escapes the quotes it does not open.contextVariablesholds the element, never the manifest or its array).bun run test(apps/sim),bun run type-check,biome check, andbun run check:api-validationall pass.Blast radius
Bare references (the documented form,
<agent.score> > 75) are byte-identical. Quoted references keep the same runtime value. Function-block generated code changes shape for spliced scalars/strings — values and display source are unchanged. Quoted object references in conditions change from "syntax error / injectable" to "well-formed string compare".