From b50dc6ab71abdc586cd770adfa726b5dc067a70b Mon Sep 17 00:00:00 2001 From: Luis Ibarra Date: Mon, 20 Jul 2026 16:37:37 -0500 Subject: [PATCH] fix(eval): preserve sibling params when Filepicker Binary data is passed to action run (#8639) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Passing Filepicker Binary-format data alongside other params to Api.run(), e.g. `Api1.run({ name: "Test", sources: Filepicker1.files })`, caused every sibling param to resolve to null and `this.params` to become null. Root cause: `evaluateActionBindings` serialized the whole params object into a single `{{ ${JSON.stringify(executionParams)} }}` binding and re-parsed it with the brace-counting `getDynamicStringSegments`. Binary file data (a raw readAsBinaryString byte string) routinely contains unescaped '{'/'}' bytes, which `JSON.stringify` does not escape, unbalancing the brace counter and collapsing the entire params object to undefined. Base64/no-file cases worked because their content has no braces. Since `JSON.stringify` emits only literals, 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`, which is behavior-preserving for valid cases, faithfully passes JS values through, avoids re-serializing multi-MB binary payloads, and removes the brace vulnerability entirely. Tests: add a regression test (unbalanced braces in a param preserve siblings), a behavior-lock test (binding-like string preserved verbatim, NaN survives), and update the getDynamicValue-call assertion to the new single-call-per-binding sequence. Co-Authored-By: Claude Opus 4.8 --- .../dataTreeEvaluator.test.ts | 87 ++++++++++++------- .../workers/common/DataTreeEvaluator/index.ts | 11 ++- 2 files changed, 63 insertions(+), 35 deletions(-) diff --git a/app/client/src/workers/common/DataTreeEvaluator/dataTreeEvaluator.test.ts b/app/client/src/workers/common/DataTreeEvaluator/dataTreeEvaluator.test.ts index f08cb4373a6a..fb812de4b6c2 100644 --- a/app/client/src/workers/common/DataTreeEvaluator/dataTreeEvaluator.test.ts +++ b/app/client/src/workers/common/DataTreeEvaluator/dataTreeEvaluator.test.ts @@ -160,6 +160,41 @@ describe("DataTreeEvaluator", () => { ]); }); + // Regression for #8639: Filepicker Binary data (a raw byte string containing + // '{'/'}' chars) in one param must not corrupt sibling params. Previously the + // whole params object was round-tripped through the {{ }} brace-counting parser, + // so unbalanced braces in the binary value nulled out every other param. + it("preserves sibling params when a param value contains unbalanced braces", () => { + // Mimics a Filepicker "Binary" readAsBinaryString payload: raw bytes incl. + // unbalanced braces, a null byte and a high byte (kept as \u escapes so the + // source file stays ASCII and is not treated as binary). + const binaryLike = ' {"junk": "}}{{" } \u0000\u00ff'; + const result = dataTreeEvaluator.evaluateActionBindings( + ["this.params.name", "this.params.sources", "executionParams.name"], + { + name: "Test", + sources: binaryLike, + }, + ); + + expect(result).toStrictEqual(["Test", binaryLike, "Test"]); + }); + + // Locks the intended behavior change from the #8639 fix: params are cloned, not + // re-evaluated/JSON-normalized. A value that looks like a binding is preserved + // verbatim, and NaN survives (the old JSON.stringify round-trip coerced it to null). + it("passes already-evaluated params through verbatim", () => { + const result = dataTreeEvaluator.evaluateActionBindings( + ["this.params.binding", "this.params.notANumber"], + { + binding: "{{Api1.data}}", + notANumber: NaN, + }, + ); + + expect(result).toStrictEqual(["{{Api1.data}}", NaN]); + }); + // The test should verify that generateOverrideContext is called and passed as context to getDynamicValue it("should call generateOverrideContext and pass as context to getDynamicValue", () => { const overrideContextValue = { "ModuleInstance1.inputs.input1": "200" }; @@ -242,36 +277,30 @@ describe("DataTreeEvaluator", () => { "200", ]); - // Verify getDynamicValue receives the correct parameters - // The first call is always with executionParams - [`${JSON.stringify(executionParams)}`, ...bindings].forEach( - (binding, index) => { - const replacedBinding = binding.replace( - EXECUTION_PARAM_REFERENCE_REGEX, - EXECUTION_PARAM_KEY, - ); + // Verify getDynamicValue receives the correct parameters. + // Execution params are now cloned directly (no {{ }} round-trip), so there is + // no leading getDynamicValue call for them — only one call per binding, each + // carrying the overrideContext. + bindings.forEach((binding, index) => { + const replacedBinding = binding.replace( + EXECUTION_PARAM_REFERENCE_REGEX, + EXECUTION_PARAM_KEY, + ); - let defaultExpectedValue = [ - `{{${replacedBinding}}}`, - klona(dataTree), - dataTreeEvaluator.oldConfigTree, - EvaluationSubstitutionType.TEMPLATE, - ]; - - if (index !== 0) { - defaultExpectedValue = [ - ...defaultExpectedValue, - expect.objectContaining({ - overrideContext: overrideContextValue, - }), - ]; - } - - expect(getDynamicValueCapturedParams[index]).toEqual( - defaultExpectedValue, - ); - }, - ); + const defaultExpectedValue = [ + `{{${replacedBinding}}}`, + klona(dataTree), + dataTreeEvaluator.oldConfigTree, + EvaluationSubstitutionType.TEMPLATE, + expect.objectContaining({ + overrideContext: overrideContextValue, + }), + ]; + + expect(getDynamicValueCapturedParams[index]).toEqual( + defaultExpectedValue, + ); + }); // Restore the original function after the test (generateOverrideContext as jest.Mock).mockImplementation( diff --git a/app/client/src/workers/common/DataTreeEvaluator/index.ts b/app/client/src/workers/common/DataTreeEvaluator/index.ts index 15fe9a0b4331..6bea51ea9448 100644 --- a/app/client/src/workers/common/DataTreeEvaluator/index.ts +++ b/app/client/src/workers/common/DataTreeEvaluator/index.ts @@ -2069,12 +2069,11 @@ export default class DataTreeEvaluator { let overrideContext: Record; if (executionParams && isObject(executionParams)) { - evaluatedExecutionParams = this.getDynamicValue( - `{{${JSON.stringify(executionParams)}}}`, - this.evalTree, - this.oldConfigTree, - EvaluationSubstitutionType.TEMPLATE, - ); + // Execution params are already fully-evaluated JS values here, so a JSON-safe + // deep clone is sufficient. Do NOT route them back through the {{ }} template + // parser: Filepicker Binary data can contain '{'/'}' bytes that unbalance the + // brace counter in getDynamicStringSegments and null out sibling params. (#8639) + evaluatedExecutionParams = klonaJSON(executionParams); overrideContext = generateOverrideContext({ bindings,