fix(expressions): avoid helper eval in literal checks#7321
Open
dwisiswant0 wants to merge 2 commits intodevfrom
Open
fix(expressions): avoid helper eval in literal checks#7321dwisiswant0 wants to merge 2 commits intodevfrom
dwisiswant0 wants to merge 2 commits intodevfrom
Conversation
Contributor
WalkthroughExpression evaluation now returns errors immediately on compile/eval failures and avoids executing expressions to determine "literals-only." Unresolved-marker replacement is gated by presence of unresolved markers and function tokens. Tests added to ensure helpers are not executed during unresolved-variable checks and to validate error messaging. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/protocols/common/expressions/expressions.go`:
- Around line 57-63: The error messages currently include the post-replacement
value because you call replacer.Replace on expression before
compiling/evaluating; capture the original template (e.g., save the incoming
expression as origExpression) before calling replacer.Replace and use
origExpression in any returned fmt.Errorf messages around
govaluate.NewEvaluableExpressionWithFunctions and evaluation (the compiled/eval
error paths such as where compiled/eval failures are reported) while still
wrapping/preserving the underlying err; keep the replaced expression for the
actual compile/eval calls (use the replaced value for
govaluate.NewEvaluableExpressionWithFunctions and subsequent evaluation) but
ensure errors reference origExpression, not the substituted value.
- Around line 65-70: The Evaluate function currently short-circuits expressions
when any input contains unresolved markers by calling unresolvedVarMarkers(...)
and using replacer.ReplaceOne(...), which rewrites inputs like "{{contact_id}}"
and causes correct expressions (e.g. Evaluate with "{{body != ''}}") to be
mis-evaluated; move this unresolved-marker propagation out of Evaluate and into
the validation path (the same check flow that uses ContainsUnresolvedVariables)
or restrict it only to helper/encoding code paths that can hide markers (e.g.,
base64 helpers), so Update the logic around unresolvedVarMarkers,
replacer.ReplaceOne and the Evaluate method to stop globally rewriting
expression inputs and instead perform this propagation during variable
validation or within specific helper functions; ensure SkipVariablesCheck
handling in pkg/protocols/http/build_request.go no longer receives mutated
values from Evaluate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e3975bb-9836-468b-afd4-cd76801b1714
📒 Files selected for processing (4)
pkg/protocols/common/expressions/expressions.gopkg/protocols/common/expressions/expressions_test.gopkg/protocols/common/expressions/variables.gopkg/protocols/common/expressions/variables_test.go
`hasLiteralsOnly()` currently evaluates helper
expressions while deciding whether "{{...}}"
contains unresolved variables, which makes
validation paths run side-effectful helpers.
Just replace that runtime eval with a
`Vars()` len check so unresolved-variable
detection literally stays literal (am I writing it
right?), and of course side-effect free.
Also make `Evaluate()` return template-authored
expression compile/eval errors instead of
logging and then keep continuing, so malformed
helper calls still fail in the rendering path.
Fixes #7320
Signed-off-by: Dwi Siswanto <git@dw1.io>
dwisiswant0
force-pushed
the
dwisiswant0/fix/expressions/avoid-helper-eval-in-literal-checks
branch
from
0734d48 to
a3f3c99
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/protocols/common/expressions/variables_test.go (1)
12-25: Note: global map mutation is not parallel-safe.
dsl.HelperFunctionsis a plainmap[string]govaluate.ExpressionFunctionwithout synchronization. The current implementation works because not.Parallel()is called, but if parallelism is added later, this would cause data races.Consider adding a comment or using
t.Setenv-style pattern if the test suite ever enables parallel execution.💡 Optional: Add clarifying comment
func withTestHelperFunction(t *testing.T, name string, fn govaluate.ExpressionFunction) { t.Helper() + // NOTE: dsl.HelperFunctions is not thread-safe; do not use t.Parallel() with this helper originalFn, hadFn := dsl.HelperFunctions[name]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/protocols/common/expressions/variables_test.go` around lines 12 - 25, The helper mutates the global dsl.HelperFunctions map which is not safe for parallel tests; update withTestHelperFunction to avoid races by either (a) serializing access with a package-level sync.Mutex around reads/writes to dsl.HelperFunctions when setting and restoring, or (b) making and assigning a shallow copy of the map (copy existing entries, set dsl.HelperFunctions = copiedMap) then restore the original map in the t.Cleanup closure; reference the function withTestHelperFunction, the global dsl.HelperFunctions, and the t.Cleanup restore logic when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/protocols/common/expressions/variables_test.go`:
- Around line 12-25: The helper mutates the global dsl.HelperFunctions map which
is not safe for parallel tests; update withTestHelperFunction to avoid races by
either (a) serializing access with a package-level sync.Mutex around
reads/writes to dsl.HelperFunctions when setting and restoring, or (b) making
and assigning a shallow copy of the map (copy existing entries, set
dsl.HelperFunctions = copiedMap) then restore the original map in the t.Cleanup
closure; reference the function withTestHelperFunction, the global
dsl.HelperFunctions, and the t.Cleanup restore logic when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58d91ea6-e30d-42e8-98b6-02bcc1c92185
📒 Files selected for processing (4)
pkg/protocols/common/expressions/expressions.gopkg/protocols/common/expressions/expressions_test.gopkg/protocols/common/expressions/variables.gopkg/protocols/common/expressions/variables_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/protocols/common/expressions/variables.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/protocols/common/expressions/expressions_test.go
`getArgsCopy()` logs and counts argument
evaluation failures, but continues to store Port
in `argsCopy` even when `evaluateArgs()` returns.
This causes malformed JavaScript args like
"{{base64()}}" to panic instead of returning the
original error.
Return the evaluation error immediately after
reporting it to prevent the panic and make sure
proper error propagation.
Signed-off-by: Dwi Siswanto <git@dw1.io>
Neo - PR Security ReviewNo security issues found Comment |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/protocols/javascript/js_test.go (1)
90-92: Consider adding a brief comment explaining why JsCompiler setup is needed.The test setup is correct, but a brief comment would help future maintainers understand that
JsCompileris required forExecuteWithResultsto function, even though this test focuses on argument evaluation errors that occur before JS execution.📝 Suggested documentation
executorOptions := testutils.NewMockExecuterOptions(options, tmplInfo) + // JsCompiler is required by ExecuteWithResults even though this test + // exercises argument evaluation failure before JS execution occurs executorOptions.JsCompiler = templates.GetJsCompiler()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/protocols/javascript/js_test.go` around lines 90 - 92, Add a one-line comment near where executorOptions.JsCompiler is set (after the call to testutils.NewMockExecuterOptions and before ExecuteWithResults is used) explaining that JsCompiler must be populated for ExecuteWithResults to operate (even though this test triggers argument evaluation errors prior to JS execution); reference the JsCompiler field on the returned executorOptions, the NewMockExecuterOptions helper, and the ExecuteWithResults call so future maintainers understand why the compiler is initialized in this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/protocols/javascript/js_test.go`:
- Around line 90-92: Add a one-line comment near where
executorOptions.JsCompiler is set (after the call to
testutils.NewMockExecuterOptions and before ExecuteWithResults is used)
explaining that JsCompiler must be populated for ExecuteWithResults to operate
(even though this test triggers argument evaluation errors prior to JS
execution); reference the JsCompiler field on the returned executorOptions, the
NewMockExecuterOptions helper, and the ExecuteWithResults call so future
maintainers understand why the compiler is initialized in this test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f794750-32e4-4427-a8f1-c14e66946a55
📒 Files selected for processing (2)
pkg/protocols/javascript/js.gopkg/protocols/javascript/js_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
fix(expressions): avoid helper eval in literal checks
hasLiteralsOnly()currently evaluates helperexpressions while deciding whether "{{...}}"
contains unresolved variables, which makes
validation paths run side-effectful helpers.
Just replace that runtime eval with a
Vars()len check so unresolved-variabledetection literally stays literal (am I writing it
right?), and of course side-effect free.
Also make
Evaluate()return template-authoredexpression compile/eval errors instead of
logging and then keep continuing, so malformed
helper calls still fail in the rendering path.
Fixes #7320
Proof
Checklist
Summary by CodeRabbit
Bug Fixes
Tests