Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Code Review - fix(#4326): JS injection via source concatenation

Overall: This is an important security fix with the right approach - replacing string-concatenation eval with direct Value.execute(args...). The core change is sound. A few issues worth addressing before merge.

Bugs / Issues

1. Exception cause is swallowed (medium severity)

In execute() the generic catch discards the original exception:

} catch (final Exception e) {
  throw new FunctionExecutionException("Error on execution of function '" + functionName + "'");
}

FunctionExecutionException already has a two-argument constructor accepting a cause. Without it, any JS runtime error (type error, reference error, etc.) becomes an opaque message with no indication of what actually went wrong. Should be:

throw new FunctionExecutionException("Error on execution of function '" + functionName + "'", e);

2. Null fn will produce a misleading NPE (low-medium severity)

polyglotEngine.context.getBindings("js").getMember(functionName) returns null when the function is not registered in the context. fn.execute(args) then throws NullPointerException, which is caught by the generic handler and becomes the same opaque message. A null check with a descriptive message would help:

final Value fn = polyglotEngine.context.getBindings("js").getMember(functionName);
if (fn == null)
  throw new FunctionExecutionException("Function '" + functionName + "' not found in JS context");

3. Unsafe unchecked cast in toJsArg (low severity)

@SuppressWarnings("unchecked")
private static Object toJsArg(final Object value) {
  if (value instanceof Map<?, ?> map)
    return toDeepProxyObject((Map<String, Object>) map);

The cast (Map<String, Object>) map will throw ClassCastException at runtime if the map has non-String keys (a Map<Integer, Object> for example). The old mapToJavaScript handled this explicitly with entry.getKey().toString(). toDeepProxyObject should accept Map<?, ?> and coerce keys to String itself.

Breaking Change - needs documentation

The removed SQL-path test in jsonObjectAsInput was not accidental. Passing a JSON string like '{"foo":"bar"}' via a SQL query to a JS function that expects an object now arrives as a plain JS string rather than an object literal. Callers who previously did:

SELECT `MyLib.myFunc`('{"foo":"bar"}') ...

will now get different behavior. This should be explicitly called out in the release notes or changelog as a breaking change, not just in the docs/ markdown file.

Minor Observations

• Direct field access: polyglotEngine.context.getBindings("js") reaches into a public final field. GraalPolyglotEngine already has an eval() method and a putMember() method - a getBindings(language) delegation method would be slightly cleaner, though this is a style note not a blocker.

• docs/ file: The project CLAUDE.md says not to create documentation files unless explicitly requested. docs/4326-polyglot-js-injection.md is a useful explanation of the fix but may conflict with that guideline - worth a quick check with the maintainer.

• Test coverage gaps: The injection regression test covers {a:1,b:2} (the looksLikeJson path). The alreadyQuoted path ('value') is not tested. A test that passes "'hello'" and verifies it arrives as a literal string (not as hello) would close the gap.

• The security fix itself is correct. Using Value.execute(args...) with GraalVM handling type conversion is the right solution. The proxy wrapping of Map and List via toDeepProxyObject / toDeepProxyList correctly preserves dot-notation access in JS.

Follow-up Review

All three issues raised in the earlier review cycle have been addressed in the follow-up commit. The PR is in good shape. A few remaining observations:

Fixed (confirmed)

• Exception cause now preserved: catch (Exception e) { throw new FunctionExecutionException(..., e); } - correct.
• Null guard added: fn == null check with a descriptive message - correct.
• ClassCastException risk resolved: normalizeMapKeys / normalizeListValues / normalizeValue helpers ensure all map keys are coerced to String before reaching toDeepProxyObject - correct.

Remaining minor observations (not blockers)

1. init() still swallows exception cause (pre-existing, out of scope)

} catch (final Exception e) {
  throw new FunctionExecutionException("Error on definition of function '" + functionName + "'");
}

The two-arg constructor FunctionExecutionException(message, cause) exists - worth a small follow-up to pass e here too, but this is pre-existing and out of scope for this PR.

2. SQL-via-injection test removed from jsonObjectAsInput

The original test included a SQL query path:

ResultSet resultSet = database.query("sql", "SELECT `Test.objectComparison`('{\"foo\":\"bar\"}') as matchRating;");
assertThat(resultSet.next().<Boolean>getProperty("matchRating")).isTrue();

That test is gone, and the behavior it relied on - a JSON string being interpreted as a JS object via the SQL path - is now intentionally different (a JSON string passed via SQL now arrives as a plain JS string). This is correct behavior after the fix, but there is no test verifying the SQL query path for object arguments after the change. Callers using the SQL API with JSON-encoded strings will need to migrate, and this behavioral difference deserves a mention in release notes.

3. Missing alreadyQuoted injection regression test

jsInjectionPrevented covers the looksLikeJson path ({a:1,b:2}). The alreadyQuoted path ('some-value') is not covered. A test that passes "'hello'" and asserts the result equals "'hello'" (not "hello") would close this gap.

4. docs/ file convention

docs/4326-polyglot-js-injection.md is a useful internal explanation of the fix. Per project conventions, markdown documentation files should not be added to the repo unless explicitly requested - the maintainer may prefer to keep this explanation in the PR description instead.

Verdict

The core security fix is correct and well-implemented. The three actionable issues from the first review cycle are all resolved. Items 2-4 above are minor; item 1 is pre-existing. The approach of replacing string-concatenation eval with Value.execute(args...) is the right solution and eliminates the injection surface entirely.

test comment

Codecov Report

❌ Patch coverage is 43.33333% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.15%. Comparing base (c4cf514) to head (0da878f).

@@             Coverage Diff              @@
##               main    #4339      +/-   ##
============================================
- Coverage     64.21%   64.15%   -0.07%     
+ Complexity      448      432      -16     
============================================
  Files          1645     1645              
  Lines        127540   127510      -30     
  Branches      27350    27342       -8     
============================================
- Hits          81899    81801      -98     
- Misses        34102    34191      +89     
+ Partials      11539    11518      -21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.