fix(#4456): bind authenticated user on DatabaseContext in BoltNetworkExecutor#4457
Conversation
…Executor BoltNetworkExecutor.ensureDatabase() resolved the database but never called DatabaseContext.INSTANCE.init().setCurrentUser(), so follower-to-leader Raft forwarding threw "no authenticated user in the current security context" for every write that landed on a follower pod. Fix mirrors PostgresNetworkExecutor.openDatabase() (line 1509) and the identical gRPC fix: after server.getDatabase() succeeds, bind user.getDatabaseUser(database) onto the thread-local DatabaseContext. Regression test BoltFollowerForwardingIT: 3-node Raft cluster, Neo4j driver writes to a follower, asserts the vertex replicates to all nodes. Closes #4456
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
🟢 Coverage 100.00% diff coverage · -7.74% coverage variation
Metric Results Coverage variation ✅ -7.74% coverage variation Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (cbf9008) 124318 92530 74.43% Head commit (09336df) 156031 (+31713) 104051 (+11521) 66.69% (-7.74%) 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 (#4457) 7 7 100.00% 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%
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 addresses an issue where Bolt protocol write queries fail on Raft HA followers by binding the authenticated user to the thread-local DatabaseContext in BoltNetworkExecutor.ensureDatabase(). It also adds a regression integration test and documentation. However, a critical security vulnerability was identified: during connection re-authentication (via LOGOFF/LOGON), ensureDatabase() returns early and fails to update the DatabaseContext with the new user's context, potentially leading to privilege escalation. It is recommended to ensure the context is properly updated or cleared upon re-authentication.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (user != null) | ||
| DatabaseContext.INSTANCE.init((DatabaseInternal) database).setCurrentUser(user.getDatabaseUser(database)); |
There was a problem hiding this comment.
Critical Security Vulnerability: Security Context Mismatch on Re-authentication
While binding the authenticated user to the DatabaseContext in ensureDatabase() fixes the forwarding issue, it introduces a critical security vulnerability when a connection is re-authenticated.
Bolt 5.1+ supports LOGOFF and LOGON messages on the same connection (handled in handleLogoff() and handleLogon()). When a client logs off and logs on as a different user, the user field is updated to the new user. However, the next time ensureDatabase() is called, it returns early because database != null && database.isOpen() is true:
if (database != null && database.isOpen()) {
// Check if we need to switch to a different database
...
} else {
return true;
}
}Because of this early return, the code at lines 971-972 is skipped, and the thread-local DatabaseContext is NOT updated with the new user's security context. Consequently, all subsequent operations/queries executed by the new user will run under the security context (and permissions) of the previously authenticated user, leading to privilege escalation or unauthorized data access.
Recommendation
To fix this, you should ensure that the DatabaseContext is updated with the new user even when ensureDatabase() returns early, or explicitly update/clear the DatabaseContext during handleLogon() and handleLogoff().
For example, you can modify the early return block in ensureDatabase() to update the current user on the existing context:
} else {
if (user != null) {
final DatabaseContext.DatabaseContextTL context = DatabaseContext.INSTANCE.getContextIfExists(database.getDatabasePath());
if (context != null) {
context.setCurrentUser(user.getDatabaseUser(database));
}
}
return true;
}Or, clear the current user in handleLogoff():
private void handleLogoff() throws IOException {
if (database != null) {
final DatabaseContext.DatabaseContextTL context = DatabaseContext.INSTANCE.getContextIfExists(database.getDatabasePath());
if (context != null) {
context.setCurrentUser(null);
}
}
user = null;
sendSuccess(Map.of());
state = State.AUTHENTICATION;
}
Code Review - fix(#4456): bind authenticated user on DatabaseContext in BoltNetworkExecutorOverviewThis PR fixes a real bug: Bolt protocol write queries fail with The Fix (BoltNetworkExecutor.java)if (user != null)
DatabaseContext.INSTANCE.init((DatabaseInternal) database).setCurrentUser(user.getDatabaseUser(database));Correct and minimal. This exactly mirrors
pom.xmlThe BoltFollowerForwardingITThe test structure is solid:
One suggestion: The follower selection loop always picks the first non-leader server. This is fine, but a comment explaining that this is intentional (any follower is representative) would help future readers understand it isn't missing randomization by accident. Potential IssuesMinor - Follow-up scope - mongodbw and redisw: The PR body correctly identifies that both SummaryThe fix is correct, minimal, and well-tested. The test faithfully reproduces the failure scenario and validates the fix end-to-end on a real 3-node cluster. No concerns about correctness or performance. The only items worth addressing are the docs file (policy question) and filing the mongodbw/redisw follow-up issues. |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
Code Review - fix(#4456): bind authenticated user on DatabaseContext in BoltNetworkExecutorSummaryThis PR fixes a real and impactful bug: Bolt write queries fail with Core Fix (
|
Code Review - Cycle 2This is the second review cycle. The Cycle 1 security concern (LOGOFF/LOGON privilege escalation) was correctly addressed. Here is the full review of the current state. Core fix (new-database path) - Correct Minimal, correct, and mirrors PostgresNetworkExecutor line 1509 and the gRPC fix exactly. Placement after Cycle 2 fix (early-return path) - Correct Using Minor observation: silent gap when ctx == null in the early-return path When A defensive fallback when pom.xml - Correct
BoltFollowerForwardingIT - Good
One minor point: the follower-selection loop always picks the first non-leader index. This is fine (any follower is equally representative), but a short comment would help future readers understand the omission of randomization is intentional. docs/4456-bolt-follower-forwarding-no-user.md Per CLAUDE.md, documentation files should not be created unless explicitly requested. The content is already covered by the PR description and the test Javadoc. Worth removing unless this project has a convention for per-issue design docs in docs/. Follow-up: mongodbw and redisw The PR body correctly identifies that both modules have zero Summary The fix is correct and the Cycle 1 security concern is properly addressed. The only actionable items are: (1) the defensive ctx == null fallback in the early-return path (low priority), (2) the docs markdown file (policy question), and (3) filing mongodbw/redisw follow-up issues. The core logic and test are ready to merge. |
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
Code Review - fix(#4456): bind authenticated user on DatabaseContext in BoltNetworkExecutorOverviewThis PR fixes a real bug: CorrectnessCore fix (new-database path, line 979-980) - correct. Using Early-return path fix (lines 942-949) - mostly correct, but has a subtle gap: final DatabaseContext.DatabaseContextTL ctx =
DatabaseContext.INSTANCE.getContextIfExists(((DatabaseInternal) database).getDatabasePath());
if (ctx != null)
ctx.setCurrentUser(user.getDatabaseUser(database));The use of if (user != null) {
final DatabaseContext.DatabaseContextTL ctx =
DatabaseContext.INSTANCE.getContextIfExists(((DatabaseInternal) database).getDatabasePath());
if (ctx != null)
ctx.setCurrentUser(user.getDatabaseUser(database));
else
DatabaseContext.INSTANCE.init((DatabaseInternal) database).setCurrentUser(user.getDatabaseUser(database));
}Since Code StyleComment length - CLAUDE.md specifies: "Don't write multi-line comment blocks - one short line max." The comment at line 942-943 spans two lines. The WHY is genuinely non-obvious here (using // getContextIfExists avoids init() which would rollback open transactions
|
| Area | Verdict |
|---|---|
| Core fix (new-database path) | Correct |
| Early-return fix (ctx == null gap) | Minor issue - silent failure if context was cleaned up |
| Comment style | Violates one-line max from CLAUDE.md |
| Docs file review history | Should be stripped before merge |
| Test | Good coverage, minor port hardcoding concern |
| POM | Clean |
The fix addresses the root cause correctly. The two items worth addressing before merge are the ctx == null silent gap in the early-return path and stripping the Review Cycles section from the docs file.
📜 License Compliance Check✅ License check passed. See artifacts for full report. License Summary (first 50 lines) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4457 +/- ##
==========================================
+ Coverage 65.24% 65.26% +0.02%
==========================================
Files 1612 1612
Lines 124318 124325 +7
Branches 26910 26913 +3
==========================================
+ Hits 81111 81145 +34
+ Misses 31816 31785 -31
- Partials 11391 11395 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #4456
Summary
BoltNetworkExecutor.ensureDatabase()never calledDatabaseContext.INSTANCE.init().setCurrentUser()after resolving the database. On a Raft HA follower,RaftReplicatedDatabase.forwardCommandToLeaderViaRaftreadsproxied.getCurrentUserName()to set theX-ArcadeDB-Forwarded-Userheader; because it was never bound, it returnednulland threwSecurityException("Cannot forward command to leader: no authenticated user in the current security context"), surfaced to the client asNeo.DatabaseError.General.UnknownError.PostgresNetworkExecutor.openDatabase()(line 1509) and the prior gRPC fix: afterserver.getDatabase()succeeds, binduser.getDatabaseUser(database)onto the thread-localDatabaseContext.BoltFollowerForwardingIT: 3-node Raft cluster, Neo4j driver writesCREATEto a follower, asserts replication to all 3 nodes. Addsarcadedb-ha-rafttest deps tobolt/pom.xmlfollowing thegrpcw/pom.xmlprecedent.Test plan
BoltFollowerForwardingIT(new,@Tag("slow")): 3-node cluster, write via follower succeeds and vertex count is 1 on every nodeBoltProtocolIT(existing, 71 tests): all pass —concurrentSessionsskipped (pre-existing failure onmain)BoltMultiLabelIT,BoltPortConfigIT(existing): passBoltStructureTest,BoltMessageTest,BoltVersionNegotiationTest,BoltChunkedIOTest,PackStreamTest,PackStreamEdgeCasesTest,BoltExceptionTest,BoltSslHelperTest: passmvn -pl bolt -am clean install -DskipTestssucceeds🤖 Generated with Claude Code