fix(condition): stop a secret value from breaking or forging a condition - #6705
Conversation
Condition expressions pasted every environment variable value into the
expression as source. Block references in the same expression go through a
proper escape and get quoted; env vars went through neither. That left three
defects:
- A bare string placeholder was a SyntaxError. `{{NAME}} === 'alice'` resolved
to `alice === 'alice'`, so the form the Function block docs recommend could
not be used here at all.
- Ordinary data broke the block. An apostrophe (`O'Brien`) or a newline in a
legitimate value produced unparseable source and failed the run.
- The quoted form was injectable. A value of `x' || true || '` turned
`'{{NAME}}' === 'bob'` into `'x' || true || '' === 'bob'`, forging a true
branch out of a comparison that should be false.
Inline only structurally inert literals — numbers, booleans, and null, with
optional space/tab padding. Every other value keeps its `{{NAME}}` placeholder
and is bound as a string by the execution-boundary compiler, the same one
Function blocks and Custom Tools already use.
Legacy outcomes are preserved. `{{COUNT}} === 3` and `{{ENABLED}} === true`
still compare as literals, and an embedded `"Bearer {{API_KEY}}"` still
compares equal — now via compiled concatenation rather than a pasted value.
Padding is admitted rather than trimmed so the inlined text stays
byte-identical to the stored value, which is what keeps a padded number
correct both bare and quoted.
A resolved secret also no longer travels to the execution boundary inside the
condition source.
The one deliberate behavior change: a value whose text is itself a quoted JS
literal (a secret stored as `'foo'`, a plausible workaround for the bare-string
SyntaxError) now compares as the 5-character string rather than as source.
That form is the injectable one, so it cannot be kept.
Docs: state the placeholder type contract, which was described mechanically but
never in terms of what a reader gets. `{{KEY}}` in Function and Custom Tool code
always evaluates to a string, so a bare `if ({{FLAG}})` is always true and a list
has to be stored as JSON. This is what a customer hit after the resolver lift in
#6247 moved Function blocks off source inlining.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Docs now spell out that Tests add resolver → compiler → Reviewed by Cursor Bugbot for commit d2fbd20. Configure here. |
Greptile SummaryThis PR prevents Condition environment-variable values from being interpreted as executable source while preserving existing literal comparisons.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code failure remaining after review. The resolver now limits raw source insertion to valid inert JavaScript literals while the existing execution-boundary compiler safely binds all other environment values as strings, and the tests cover the reachable compatibility and injection cases.
|
| Filename | Overview |
|---|---|
| apps/sim/executor/variables/resolver.ts | Restricts Condition source substitution to inert literals and preserves other placeholders for safe execution-boundary binding; no actionable defect was identified. |
| apps/sim/executor/variables/resolver.test.ts | Adds resolver-to-compiler regression coverage for injection prevention, string handling, literal compatibility, and whitespace preservation. |
| apps/docs/content/docs/en/workflows/blocks/condition.mdx | Documents the distinct literal-versus-string behavior of environment placeholders in Conditions. |
| apps/docs/content/docs/en/workflows/blocks/function.mdx | Clarifies that Function and Custom Tool placeholders are strings and documents explicit conversions. |
| apps/docs/content/docs/en/workflows/variables.mdx | Adds a concise cross-reference describing the string-valued placeholder contract in executable code. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A["Condition expression with {{KEY}}"] --> B["VariableResolver"]
B --> C{"Value is number, boolean, or null?"}
C -->|Yes| D["Inline inert literal"]
C -->|No| E["Preserve placeholder"]
D --> F["Condition handler builds Boolean(expression)"]
E --> G["compileCodePlaceholders binds string value"]
G --> F
F --> H["Execute condition and select branch"]
Reviews (1): Last reviewed commit: "fix(condition): stop a secret value from..." | Re-trigger Greptile
Problem
Condition expressions pasted every environment variable value into the expression as source. Block references in the same expression go through a proper escape (
\,',\n,\r, U+2028/9) and get quoted — env vars went through neither.Verified against the real resolver:
<producer.result> === 'hello world''hello world' === 'hello world'{{NAME}} === 'hello world'hello world === 'hello world''{{NAME}}' === 'bob',NAME=O'Brien'O'Brien' === 'bob''{{NAME}}' === 'bob',NAME=x' || true || ''x' || true || '' === 'bob'{{N}} === 123,N='123'123 === 123Three defects: a bare string placeholder is a SyntaxError (so the form the Function block docs recommend can't be used here at all); an apostrophe or newline in a legitimate value fails the run; and the quoted form lets a secret value forge a true branch out of a comparison that should be false.
Fix
Inline only structurally inert literals — numbers, booleans,
null, with optional space/tab padding. Every other value keeps its{{NAME}}placeholder and is bound as a string bycompileCodePlaceholders, the same execution-boundary compiler Function blocks and Custom Tools already use.No change to the shared compiler, no new flag through the tool/route/contract. The whole implementation is one predicate plus a two-line call-site change in the condition-only resolution path (
resolveTemplateWithoutConditionFormatting, which has exactly one caller).Every character the predicate admits is inert in both places a placeholder can land: in expression position none introduces an operator or comment, and inside a string literal none terminates it. Line terminators are excluded — a raw newline would break a single-quoted string.
Nothing breaks
Full
apps/simsuite: 25,076 passed, 0 failures. Type-check and Biome clean,check:api-validationpasses.The legacy outcomes are preserved, and there's a new test that proves it end to end — resolver → compiler → the same
Boolean(...)wrappercondition-handler.tsbuilds:{{COUNT}} === 3→ still true (inlined literal, unchanged){{ENABLED}} === true→ still true (inlined literal, unchanged)"Bearer {{API_KEY}}" === "Bearer token"→ still true, now via compiled concatenation instead of a pasted valuenull, negative, and exponent values all still compare as literalsPadding is admitted rather than trimmed so the inlined text stays byte-identical to the stored value — whitespace is meaningless bare but significant inside quotes, and only the untrimmed value is correct in both. Env var values aren't trimmed on save, so this is reachable; covered by a test.
The 4 new tests were confirmed to fail against the old behavior and pass against the new. The end-to-end outcomes test passes under both, which is the point — it's the regression guard.
The one deliberate behavior change
A value whose text is itself a quoted JS literal — a secret stored as
'foo', a plausible workaround someone may have found for the bare-string SyntaxError — now compares as the 5-character string rather than as source. That form is precisely the injectable one, so it can't be kept. Worth a note in release comms.Also: the placeholder type contract in docs
A customer hit a related silent break after the resolver lift in #6247 moved Function blocks from source inlining to value binding. A workspace secret holding a JS array of regex literals started arriving as a 411-character string, their guardrail threw, and requests fell through to a fallback billed on GPT output tokens.
That change is correct and stays — it closed arbitrary code execution from a secret value. But the docs described the mechanism ("bound separately from the source") and never the consequence, while
function.mdxactively recommends the bare form whose meaning changed. Added the contract and the conversions:{{KEY}}in Function/Custom Tool code always evaluates to a stringif ({{FLAG}})is always true, because"false"is truthyEvery docs snippet was run through the real compiler before being written down.
Test plan
bun run test(fullapps/sim) — 25,076 passedbun run type-checkbunx biome checkon changed filesbun run check:api-validation🤖 Generated with Claude Code