fix: resolve CodeQL Java code-scanning alerts at their true sources#4383
Conversation
Triaged 30 open CodeQL alerts on main via the SARIF code-flows: the reported sink locations collapse to four root causes. - insecure-randomness (24 alerts): RandFunction returned Math.random(), which CodeQL follows through config-string resolution into security sinks. Switched to a per-thread SecureRandom (secure source, no cross-thread lock contention on the query hot path). - tainted-format-string (4 alerts): BinarySerializer concatenated an untrusted value into a log message; pass it as a %s argument instead. - path-injection (1 alert): untrusted Neo4j import labels became schema type names and on-disk bucket file names. Validate labels at the import boundary, rejecting path separators and parent traversal. - implicit-cast-in-compound-assignment (1 alert): widen totalRecords to long in CountStepTest. Added Neo4jImporterIT regression tests for malicious node/edge labels.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
🟢 Metrics 85 complexity
Metric Results Complexity 85
🟢 Coverage 90.91% diff coverage
Metric Results Coverage variation Report missing for 198d9051 Diff coverage ✅ 90.91% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (198d905) Report Missing Report Missing Report Missing Head commit (e57af51) 159513 105296 66.01% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4383) 11 10 90.91% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
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.
There was a problem hiding this comment.
Code Review
This pull request introduces path traversal validation for Neo4j importer labels to prevent writing files outside the database directory, adds corresponding integration tests, fixes a logging format string in BinarySerializer, and updates a test variable type from int to long. Additionally, it replaces Math.random() with a thread-local SecureRandom in RandFunction. The reviewer raised a performance concern regarding the use of SecureRandom for the SQL rand() function, recommending ThreadLocalRandom instead to avoid unnecessary overhead on the query hot path.
| // Per-thread SecureRandom: cryptographically strong source without cross-thread lock contention on the query hot path. | ||
| private static final ThreadLocal<SecureRandom> RANDOM = ThreadLocal.withInitial(SecureRandom::new); | ||
|
|
||
| @Override | ||
| public String getName() { | ||
| return "rand"; | ||
| } | ||
|
|
||
| @Override | ||
| public Object execute(final Object[] args, final CommandContext context) { | ||
| return Math.random(); | ||
| return RANDOM.get().nextDouble(); | ||
| } |
There was a problem hiding this comment.
Performance & Architectural Concern: Use of SecureRandom for SQL rand()
While using SecureRandom resolves the CodeQL java/insecure-randomness alert, doing so introduces a significant performance bottleneck on the query hot path:
- Performance Overhead:
SecureRandomis cryptographically secure but orders of magnitude slower thanThreadLocalRandomorMath.random(). SQLrand()is commonly used in queries for non-cryptographic purposes (e.g.,ORDER BY rand(), random sampling, or generating mock data) where performance is critical. - Seeding Cost:
ThreadLocal.withInitial(SecureRandom::new)seeds a newSecureRandominstance for every thread that executes a query containingrand(), which adds substantial latency to the first query execution on each thread. - Architectural Misalignment: A SQL
rand()function returns adouble(0.0 to 1.0), which is inherently unsuitable for generating high-entropy cryptographic keys or tokens. If security-sensitive sinks (like SSL helpers or socket factories) are accepting random values from SQL queries or configuration strings, the vulnerability should be addressed at those sinks (by usingSecureRandomdirectly there) rather than forcing all SQL queries to pay a performance penalty.
Recommendation:
Consider reverting to ThreadLocalRandom.current().nextDouble() for the SQL rand() function to maintain high query performance, and handle the CodeQL alert either by dismissing it as a false positive (since SQL rand() is not a security-sensitive source) or by securing the actual sinks.
| // Per-thread SecureRandom: cryptographically strong source without cross-thread lock contention on the query hot path. | |
| private static final ThreadLocal<SecureRandom> RANDOM = ThreadLocal.withInitial(SecureRandom::new); | |
| @Override | |
| public String getName() { | |
| return "rand"; | |
| } | |
| @Override | |
| public Object execute(final Object[] args, final CommandContext context) { | |
| return Math.random(); | |
| return RANDOM.get().nextDouble(); | |
| } | |
| @Override | |
| public String getName() { | |
| return "rand"; | |
| } | |
| @Override | |
| public Object execute(final Object[] args, final CommandContext context) { | |
| return java.util.concurrent.ThreadLocalRandom.current().nextDouble(); | |
| } |
There was a problem hiding this comment.
Keeping SecureRandom. Reverting to ThreadLocalRandom would re-open all 24 java/insecure-randomness alerts (CodeQL treats ThreadLocalRandom, a java.util.Random subclass, identically to Math.random()), so it trades the fix for a manual FP-dismissal rather than solving it - and the goal of this PR is to resolve the alerts in code.
The flows into the SSL/socket sinks are spurious (CodeQL routes the SQL rand() value through ContextConfiguration.toString()); there is no real sink receiving rand() output to harden, so "secure the sinks" has nothing to target.
The performance point is fair, so I added RandFunctionBenchmark (8d7c22a, @Tag("benchmark"), excluded from regular CI) to quantify it. Measured per-call cost on this machine:
| source | ns/op |
|---|---|
| RandFunction (ThreadLocal) | 104.2 |
| ThreadLocalRandom.nextDouble() | 2.6 |
| Math.random() | 9.6 |
So ORDER BY rand() over 1M rows is ~104ms vs ~3ms - measurable but bounded, and now tracked if it ever needs revisiting.
Code ReviewWell-structured security fix PR - the root-cause table in the description is a good read and the approach of patching the source rather than the sinks is correct. Some targeted observations below.
|
Address PR review: - RandFunctionBenchmark compares the rand() SecureRandom source against ThreadLocalRandom and Math.random() so the security/perf tradeoff is measurable (tagged benchmark, excluded from regular CI). - Collapse the validateLabel Javadoc block to a single-line comment per the project comment-style guidance.
|
Addressing the review points (claude[bot] + gemini): Done
rand() RNG - keeping Declining / deferring (out of scope for this CodeQL PR):
|
Code ReviewGood targeted security fix. The four root causes are clearly diagnosed and the fixes land at the right abstraction layer. A few notes below.
|
| Item | Assessment |
|---|---|
| SecureRandom taint fix | Correct; benchmark overhead needs a pass/fail threshold |
validateLabel .contains("..") |
Overly broad - worth narrowing to segment-boundary check |
| Format string fix | Clean |
int->long cast fix |
Correct |
| Test coverage | Good regression tests added |
Overall this is solid work - the approach of fixing at the source rather than the sink is the right call throughout.
|
Follow-up on the
So the ~95 ns/op cost is bounded by how often a user calls
Conclusion: not a hot-path bottleneck. Keeping |
Pass a shared Object[0] to RandFunction.execute instead of null - more explicit about the unused args contract and avoids per-call allocation inside the measured loop.
|
Addressing this round: Done
The cost is rejecting an otherwise-legal label containing literal The Windows-style |
|
test |
Code ReviewThis is a well-reasoned set of security fixes that addresses root causes rather than sinks. Four distinct issues are bundled, all related. Overall the approach is sound; a few targeted observations below.
|
| Change | Assessment |
|---|---|
SecureRandom via ThreadLocal |
Correct; performance justified by PR analysis |
| Format string fix | Clean |
int->long in test |
Correct |
validateLabel placement |
Correct trust-boundary approach |
Guard inconsistency in parseEdges() |
Pre-existing; worth fixing while the code is being touched |
| Regression tests | Good coverage of both separator types |
| Benchmark | Correctly tagged; class Javadoc slightly over style limit |
Overall this is solid security work. The one actionable suggestion is aligning the parseEdges() null guard with syncSchema() so that a missing "label" key is warned-and-skipped rather than thrown.
Code ReviewThis PR addresses 30 CodeQL language:java alerts on main by fixing four root causes. Well-scoped, well-justified, and fixes are at the correct trust boundary. RandFunction.javaGood: ThreadLocal is the right tradeoff - cryptographically strong without the synchronized overhead of a shared instance. The inline comment explains the rationale concisely. Minor follow-up worth considering: SQLFunctionRandomInt (engine/.../sql/math/SQLFunctionRandomInt.java) still holds a shared static SecureRandom. It is thread-safe (SecureRandom is internally synchronized) but will contend under parallel query execution. Worth aligning to ThreadLocal in a follow-up. BinarySerializer.javaGood: Correct one-line fix. Passing value as a format argument rather than concatenating into the template closes the taint sink. Follows the existing LogManager.log() pattern throughout the file. Neo4jImporter.javaGood: Validation at the import boundary is the right call (the PR body explains why a sink-side fix would break valid usage). All three extraction paths are covered: syncSchema edges, parseEdges, and typeNameFromLabels (both single and multi-label nodes). One concern - contains("..") is overly broad. This rejects any label with two consecutive dots anywhere, including legitimate names like "schema..version" or "v2..0". Real path traversal requires ".." to appear as a path segment (flanked by separators or at string boundaries). A more precise check would test only for "../" or "..\" or the label equaling ".." outright. If double-dot labels are genuinely not expected in Neo4j exports, the conservative check is fine - just worth a comment stating that intent. Missing test variants (not blockers):
CountStepTest.javaGood: Correct widening of totalRecords to long. The previous int += long narrowing would silently truncate on large row counts. RandFunctionBenchmark.javaGood: Properly tagged @tag("benchmark"), consistent with VarIntBenchmark and SelectCacheHitsBenchmark. The blackhole() pattern prevents dead-code elimination. Leaving the benchmark alongside the fix is good practice - the performance claim is now re-checkable. Minor nit: The private DoubleOp inner interface could be replaced with java.util.function.DoubleSupplier to avoid defining a new type. OverallFour root causes fixed cleanly at the right abstraction level, with regression tests for the highest-risk one (path injection). The contains("..") breadth in validateLabel is the only thing worth a follow-up discussion. The PR description quality is high and the benchmark is a valuable addition. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4383 +/- ##
=========================================
Coverage 64.33% 64.34%
+ Complexity 430 422 -8
=========================================
Files 1647 1647
Lines 127831 127835 +4
Branches 27403 27403
=========================================
+ Hits 82239 82250 +11
+ Misses 34043 34039 -4
+ Partials 11549 11546 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
Resolves the 30 open CodeQL (
language:java) code-scanning alerts onmain. Triaging the SARIF code-flows showed the reported sink locations collapse to four root causes - the alertpath:lineis the sink, while the real source lives upstream in the flow.java/insecure-randomnessRandFunction.execute()returnedMath.random(); CodeQL follows the SQLrand()value through config-string resolution into 24 security sinks (BoltSslHelper, SocketFactory, ServerSecurity, ArcadeDBServer, HttpServer, etc.)ThreadLocal<SecureRandom>- cryptographically strong source without cross-thread lock contention on the query hot pathjava/tainted-format-stringBinarySerializerconcatenated an untrustedvalueinto a log messagevalueas a%sargument instead of concatenating into the templatejava/path-injectionvalidateLabel()at the import boundary rejects/,\, NUL and..java/implicit-cast-in-compound-assignmentint += longnarrowing in a testtotalRecordstolongThe path-injection fix is at the import trust boundary rather than the file-open sink because at the sink the trusted directory and untrusted name are a single indistinguishable string, and ArcadeDB database paths can legitimately be relative (contain
..), so a sink-side..rejection would break valid usage.Test plan
engineandintegrationmodules compileCountStepTest,BinarySerializerTest,JavaBinarySerializerTest,JsonSerializerTestrand()(OpenCypherMathNumericFunctionsComprehensiveTest,CypherFunctionFactoryExtendedTest,FunctionCachingTest)Neo4jImporterIT- 8 tests incl. 2 new regressions (rejectNodeLabelWithPathTraversal,rejectEdgeLabelWithPathTraversal)Performance (rand() SecureRandom)
rand()now uses a thread-localSecureRandom. Measured per-call cost (RandFunctionBenchmark,@Tag("benchmark")):Verdict: within budget.
RandFunctionhas no internal engine callers - it backs only the user-facingrand()(Cypher + SQL). The ~95 ns/op is incurred only when a user writesrand(), and it sits on top of per-row record fetch + binary deserialization (hundreds of ns to several µs/row) that dominates by 1-2 orders of magnitude. Worst realistic caseORDER BY rand()over 1M rows adds ~92 ms vs ThreadLocalRandom, behind a sort+materialization that already costs far more. The benchmark stays so the tradeoff is re-checkable if a heavy random-sampling workload ever justifies revisiting.