Skip to content

Commit 199c4ab

Browse files
fix(eval): preserve sibling params when Filepicker Binary data is passed to action run (#42021)
## Description Filepicker Binary-format data passed alongside other params to `Api.run()` caused **every sibling param to become `null`** and `{{this.params}}` to become `null`. **Reproduce:** Filepicker with Data Format = Binary, then ```js await Api1.run({ name: "Test", sources: Filepicker1.files }) ``` `name` arrives as `null` (and `this.params` is `null`). Base64 format and the no-file case work fine. **Root cause:** `DataTreeEvaluator.evaluateActionBindings` serialized the whole params object into a single `{{ ${JSON.stringify(executionParams)} }}` binding and re-parsed it with the brace-counting `getDynamicStringSegments`. Filepicker Binary data is a raw `readAsBinaryString` byte string that routinely contains unescaped `{`/`}` bytes; `JSON.stringify` does not escape braces, so the counter unbalances and the **entire** params object collapses to `undefined`. Base64 works because its alphabet has no braces. **Fix:** `JSON.stringify` emits only literals, so that round-trip could only deep-clone the (already fully-evaluated) params — it never resolved nested bindings. Replace it with the already-imported JSON-safe deep clone `klonaJSON(executionParams)`. This is behavior-preserving for valid cases, faithfully passes JS values through, avoids re-serializing multi-MB binary payloads, and removes the brace vulnerability entirely. **Reviewer notes:** - Intentional (more-faithful) behavior change now covered by tests: `undefined`/`NaN`/`Date` param values pass through unnormalized instead of being JSON-coerced. - `generateOverrideContext` already receives the raw `executionParams` object, so its (EE) contract is unchanged. - Verified red→green: on the old code the regression test returns all-`undefined` params (the exact bug); on the fix all `evaluateActionBindings` tests pass. **TL;DR:** Filepicker Binary data in `Api.run()` params no longer nulls out the other params — the params object is now deep-cloned instead of round-tripped through the `{{ }}` binding parser. Fixes appsmithorg/appsmith-ee#8639 ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/30824007861> > Commit: f7bae03 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=30824007861&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Mon, 03 Aug 2026 15:45:44 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed action parameter evaluation when values contain unbalanced braces, preserving sibling parameters and correct results. * Prevented already-evaluated parameters from being altered through template parsing or type conversion. * Improved handling of execution parameter references during dynamic value evaluation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent ec663b7 commit 199c4ab

2 files changed

Lines changed: 63 additions & 35 deletions

File tree

app/client/src/workers/common/DataTreeEvaluator/dataTreeEvaluator.test.ts

Lines changed: 58 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,41 @@ describe("DataTreeEvaluator", () => {
160160
]);
161161
});
162162

163+
// Regression for #8639: Filepicker Binary data (a raw byte string containing
164+
// '{'/'}' chars) in one param must not corrupt sibling params. Previously the
165+
// whole params object was round-tripped through the {{ }} brace-counting parser,
166+
// so unbalanced braces in the binary value nulled out every other param.
167+
it("preserves sibling params when a param value contains unbalanced braces", () => {
168+
// Mimics a Filepicker "Binary" readAsBinaryString payload: raw bytes incl.
169+
// unbalanced braces, a null byte and a high byte (kept as \u escapes so the
170+
// source file stays ASCII and is not treated as binary).
171+
const binaryLike = ' {"junk": "}}{{" } \u0000\u00ff';
172+
const result = dataTreeEvaluator.evaluateActionBindings(
173+
["this.params.name", "this.params.sources", "executionParams.name"],
174+
{
175+
name: "Test",
176+
sources: binaryLike,
177+
},
178+
);
179+
180+
expect(result).toStrictEqual(["Test", binaryLike, "Test"]);
181+
});
182+
183+
// Locks the intended behavior change from the #8639 fix: params are cloned, not
184+
// re-evaluated/JSON-normalized. A value that looks like a binding is preserved
185+
// verbatim, and NaN survives (the old JSON.stringify round-trip coerced it to null).
186+
it("passes already-evaluated params through verbatim", () => {
187+
const result = dataTreeEvaluator.evaluateActionBindings(
188+
["this.params.binding", "this.params.notANumber"],
189+
{
190+
binding: "{{Api1.data}}",
191+
notANumber: NaN,
192+
},
193+
);
194+
195+
expect(result).toStrictEqual(["{{Api1.data}}", NaN]);
196+
});
197+
163198
// The test should verify that generateOverrideContext is called and passed as context to getDynamicValue
164199
it("should call generateOverrideContext and pass as context to getDynamicValue", () => {
165200
const overrideContextValue = { "ModuleInstance1.inputs.input1": "200" };
@@ -242,36 +277,30 @@ describe("DataTreeEvaluator", () => {
242277
"200",
243278
]);
244279

245-
// Verify getDynamicValue receives the correct parameters
246-
// The first call is always with executionParams
247-
[`${JSON.stringify(executionParams)}`, ...bindings].forEach(
248-
(binding, index) => {
249-
const replacedBinding = binding.replace(
250-
EXECUTION_PARAM_REFERENCE_REGEX,
251-
EXECUTION_PARAM_KEY,
252-
);
280+
// Verify getDynamicValue receives the correct parameters.
281+
// Execution params are now cloned directly (no {{ }} round-trip), so there is
282+
// no leading getDynamicValue call for them — only one call per binding, each
283+
// carrying the overrideContext.
284+
bindings.forEach((binding, index) => {
285+
const replacedBinding = binding.replace(
286+
EXECUTION_PARAM_REFERENCE_REGEX,
287+
EXECUTION_PARAM_KEY,
288+
);
253289

254-
let defaultExpectedValue = [
255-
`{{${replacedBinding}}}`,
256-
klona(dataTree),
257-
dataTreeEvaluator.oldConfigTree,
258-
EvaluationSubstitutionType.TEMPLATE,
259-
];
260-
261-
if (index !== 0) {
262-
defaultExpectedValue = [
263-
...defaultExpectedValue,
264-
expect.objectContaining({
265-
overrideContext: overrideContextValue,
266-
}),
267-
];
268-
}
269-
270-
expect(getDynamicValueCapturedParams[index]).toEqual(
271-
defaultExpectedValue,
272-
);
273-
},
274-
);
290+
const defaultExpectedValue = [
291+
`{{${replacedBinding}}}`,
292+
klona(dataTree),
293+
dataTreeEvaluator.oldConfigTree,
294+
EvaluationSubstitutionType.TEMPLATE,
295+
expect.objectContaining({
296+
overrideContext: overrideContextValue,
297+
}),
298+
];
299+
300+
expect(getDynamicValueCapturedParams[index]).toEqual(
301+
defaultExpectedValue,
302+
);
303+
});
275304

276305
// Restore the original function after the test
277306
(generateOverrideContext as jest.Mock).mockImplementation(

app/client/src/workers/common/DataTreeEvaluator/index.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2069,12 +2069,11 @@ export default class DataTreeEvaluator {
20692069
let overrideContext: Record<string, unknown>;
20702070

20712071
if (executionParams && isObject(executionParams)) {
2072-
evaluatedExecutionParams = this.getDynamicValue(
2073-
`{{${JSON.stringify(executionParams)}}}`,
2074-
this.evalTree,
2075-
this.oldConfigTree,
2076-
EvaluationSubstitutionType.TEMPLATE,
2077-
);
2072+
// Execution params are already fully-evaluated JS values here, so a JSON-safe
2073+
// deep clone is sufficient. Do NOT route them back through the {{ }} template
2074+
// parser: Filepicker Binary data can contain '{'/'}' bytes that unbalance the
2075+
// brace counter in getDynamicStringSegments and null out sibling params. (#8639)
2076+
evaluatedExecutionParams = klonaJSON(executionParams);
20782077

20792078
overrideContext = generateOverrideContext({
20802079
bindings,

0 commit comments

Comments
 (0)