fix(#4793): gRPC getDatabase rejects path-traversal names and enforces auth/authorization

[robfrank]

Issue

Fixes #4793 (HIGH severity, security).

ArcadeDbGrpcService.getDatabase resolved a request-supplied databaseName after only checking that a username was resolvable. validateCredentials never called ServerSecurity.authenticate. Two problems:

1. Path traversal - databaseName flowed unsanitized into new DatabaseFactory(databasePath + "/" + databaseName) (fallback path) and into arcadeServer.getDatabase(databaseName). A name containing .., / or \ escapes the configured databases directory, letting a caller open/create databases anywhere on disk.
2. Weak authentication / missing authorization - validation accepted any non-empty username; the password was never authenticated and the user was never authorized for the named database.

Fix

grpcw/.../ArcadeDbGrpcService.java:

• validateDatabaseName(String) (new): rejects null/blank names and any name containing /, \ or .., before any filesystem access. Mirrors the established pattern in PostServerCommandHandler.resolveBackupFile. Called at the top of getDatabase.
• validateCredentials now takes the database name and, when server security is active (at least one user configured), enforces real authentication + per-database authorization:
  • if the auth interceptor already verified the connection (context user set), the resolved user is authorized for the requested database;
  • otherwise the request-payload username/password are authenticated and the requested database authorized via ServerSecurity.authenticate.
  • when no users are configured (security intentionally disabled) only username presence is required, matching the interceptor's securityEnabled gate.

Tests

grpcw/.../Issue4793GrpcGetDatabaseSecurityIT.java (new): parent-dir .., embedded /, and \ database names are rejected (and no directory escapes the databases folder); blank name rejected; a legitimate authorized access still works. 3 of the 5 cases fail before the fix, all pass after.

Verification

• New IT: 3/5 fail pre-fix, 5/5 pass post-fix.
• grpcw unit tests (104, incl. ArcadeDbGrpcServiceExtendedTest) + GrpcAuthInterceptorTest (8): pass.
• grpcw ITs GrpcServerIT + GrpcFollowerForwardingIT (31, incl. HA leader-forwarding via the interceptor context user): pass.
• Pre-existing unrelated failure grpc-client Issue4562RollbackDeleteTest (HTTP-path schema-cache) verified to fail identically on the unmodified base - not caused by this change.

🤖 Generated with Claude Code

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Coverage 76.67% diff coverage · -7.26% coverage variation

Metric	Results
Coverage variation	✅ -7.26% coverage variation
Diff coverage	✅ 76.67% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (85768bf)	133181	98777	74.17%
Head commit (44b40be)	165012 (+31831)	110402 (+11625)	66.91% (-7.26%)

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 (#4815)	30	23	76.67%

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.}

[gemini-code-assist]

Code Review

This pull request addresses a security vulnerability (Issue #4793) in ArcadeDbGrpcService where unauthenticated users could perform path traversal or unauthorized database creation/access via gRPC. It introduces database name validation to prevent path traversal and enforces proper authentication and per-database authorization. The feedback points out a security concern where a database name of exactly "." is not rejected, which could resolve to the root databases directory, and suggests explicitly rejecting it.

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.

[gemini-code-assist]

The current validation checks for .., /, and \, but does not reject a database name of exactly .. If a client requests a database named ., the resolved path becomes databasePath + "/.", which points directly to the root databases directory. This allows a client to potentially open, modify, or drop the entire databases directory as if it were a single database. To prevent this directory escape, explicitly reject . as an invalid database name.

Suggested change

	if (databaseName.contains("/") || databaseName.contains("\\") || databaseName.contains(".."))
	throw new IllegalArgumentException("Invalid database name: " + databaseName);
	if (databaseName.equals(".") || databaseName.contains("/") || databaseName.contains("\\") || databaseName.contains(".."))
	throw new IllegalArgumentException("Invalid database name: " + databaseName);

[robfrank]

Good catch - fixed in c425c28. A bare "." resolves to the databases directory itself, so validateDatabaseName now rejects "." alongside the separator and parent-reference checks, with a regression case added to Issue4793GrpcGetDatabaseSecurityIT.

[claude]

Review: PR 4815 - gRPC getDatabase path-traversal + auth/authorization hardening

Thanks for this - it closes a genuine HIGH-severity gap and the change is well scoped, well documented, and centralized in the one method (getDatabase) that every gRPC entry point funnels through. A few observations below, mostly quality/correctness rather than blockers.

What is good

• The fix addresses more than just path traversal. The most valuable part is the per-database re-authorization in the contextUser branch. The interceptor (GrpcAuthInterceptor) authenticates against the x-arcade-database header (defaulting to default), while getDatabase resolves the request-body database. Those can differ, so before this change a caller authorized for DB A (in the header) could read DB B (in the body) - a cross-database authorization escape. Re-authorizing the resolved user against the actual databaseName is the right closure and worth calling out explicitly in the PR description.
• validateDatabaseName runs before any filesystem access and mirrors the established PostServerCommandHandler pattern. Good consistency.
• Tests are clean, exercise the real interceptor path (header DB = real DB, body DB = traversal name), and the on-disk doesNotExist() assertion is a nice belt-and-suspenders check.

Issues / suggestions

1. gRPC status code is wrong for these rejections (quality). validateDatabaseName throws IllegalArgumentException and the authz checks throw ServerSecurityException, but executeQuery (and the other handlers) catch Exception and wrap everything as Status.INTERNAL ("Query execution failed: ..."), see ArcadeDbGrpcService.java:934. So a path-traversal name and an authorization failure both surface to clients as INTERNAL (13), implying a server fault. Semantically these should be INVALID_ARGUMENT (bad name) and PERMISSION_DENIED / UNAUTHENTICATED (authz/authn). The new IT only asserts the message substring, so it passes regardless and does not pin the code. Consider mapping these explicitly, and asserting the Status.Code in the tests so the contract is locked.

2. Stale Javadoc reference. resolvedUsername Javadoc still references validateCredentials(DatabaseCredentials) - the single-arg signature no longer exists after this change. Update the link to the new (DatabaseCredentials, String) signature to avoid a broken javadoc reference.

3. Duplicated authorization logic. The contextUser branch re-implements the exact ANY/contains(databaseName) check that already lives in ServerSecurity.authenticate (and ServerSecurityUser.getAuthorizedDatabases). Since the password is already verified for the context user, you cannot reuse authenticate directly - but it would be worth extracting a small ServerSecurity.authorizeDatabase(user, databaseName) (or a method on ServerSecurityUser) so the rule has a single source of truth and cannot drift between the interceptor, authenticate, and this new branch.

4. Auto-create on a SELECT still possible (defense-in-depth note). Path traversal is closed, but for a legitimate, validated name the fallback branch still calls dbFactory.create() when the DB does not exist (around ArcadeDbGrpcService.java:3197). A user authorized with * (e.g. root) passes the new authz check for a non-existent DB name and a plain executeQuery would silently create it on disk. That is pre-existing and within the databases directory now, so not a traversal issue - but the issue title mentions unauth DB create, and creating a database as a side effect of a query is surprising. Might be worth a follow-up to gate creation behind an explicit admin path rather than the query handler.

Minor

• validateDatabaseName blocks /, \, .. - consistent with the HTTP handlers, so fine. If you ever want to tighten further, an allowlist (alphanumeric + _-) is stricter than a denylist, but matching the existing pattern is the right call for this PR.

Overall: solid, targeted security fix. Item 1 is the one I would most encourage addressing before merge since it affects the client-visible API contract; the rest are cleanups.

[claude]

Review: gRPC getDatabase path-traversal + auth hardening (#4793)

Reviewed the full diff and the surrounding code paths. This is a solid, well-scoped security fix. The central getDatabase(databaseName, credentials) helper is the single choke point used by every gRPC entry point (executeQuery, executeCommand, the streaming/batch paths, etc.), so adding the checks there closes the vector uniformly without touching each handler. Good javadoc, good test coverage, and the validation mirrors the established HTTP pattern. Below are observations and a few non-blocking suggestions.

Strengths

• Defense-in-depth ordering is correct: validateDatabaseName runs before any filesystem access and before validateCredentials, so a traversal name can never reach new DatabaseFactory(databasePath + "/" + databaseName) or arcadeServer.getDatabase(...).
• Closes a real authorization gap: the interceptor authenticates the user against the x-arcade-database header, but getDatabase resolves the database from the request payload, which can differ. Re-authorizing the context user against the payload databaseName is a genuine, meaningful check, not redundant with the interceptor.
• Validation is stricter than the existing engine check (LocalDatabase.checkDatabaseName only rejects * and ..); rejecting /, \, and bare . as well is the right call.
• Tests cover the important cases (parent .., embedded /, \, bare ., blank) plus a positive control, and assert no directory escapes onto disk.

Suggestions (non-blocking)

1. Reuse ServerSecurityUser.canAccessToDatabase(databaseName) in the context-user branch instead of re-implementing it:
   ```java
   if (!user.canAccessToDatabase(databaseName))
   throw new ServerSecurityException("User has not access to database '" + databaseName + "'");
   ```
   The current code hand-rolls allowedDatabases.contains(SecurityManager.ANY) && !allowedDatabases.contains(databaseName), which duplicates exactly what canAccessToDatabase already does (and what ServerSecurity.authenticate uses internally). This drops the new SecurityManager import and the Set local, and aligns with the CLAUDE.md "reuse existing components" guidance.

2. The fallback path (arcadeServer == null) silently skips all auth/authorization because validateCredentials only enforces security when arcadeServer != null (security is null otherwise). That path is the one doing new DatabaseFactory(databasePath + "/" + databaseName) and even dbFactory.create(). Path-traversal is still blocked there (good), and in a real server arcadeServer is always set so this branch is unreachable, but a one-line comment noting "no server => security not configured => name-sanitization only" would prevent a future reader from assuming this branch is also auth-gated.

3. contains("..") also rejects legitimate names like a..b. This matches the existing HTTP handler pattern so it is acceptable for consistency, but worth being aware of. Given there are now at least three near-identical implementations (PostServerCommandHandler.resolveBackupFile, LocalDatabase.checkDatabaseName, and this), a shared FileUtils-style validator would be the cleaner long-term home - a follow-up, not for this PR.

4. Test nit: GRPC_PORT = 50051 is hardcoded, which can collide if gRPC ITs ever run concurrently. Minor given ITs typically run serially.

Correctness check

• getUsers() returns users.keySet() (never null), and the security != null && getUsers() != null && !isEmpty() gate is consistent with the interceptor's securityEnabled definition - good.
• The explicit com.arcadedb.security.SecurityManager import correctly shadows java.lang.SecurityManager, so SecurityManager.ANY resolves as intended.
• ServerSecurityException is a runtime exception; confirm the gRPC error-mapping wraps it into a sensible StatusRuntimeException (the negative auth/authz cases are not asserted in the new IT, only the IllegalArgumentException "Invalid database name" cases are). Consider adding one test for "authenticated user lacks access to the requested DB" to lock in the authorization behavior, since that is the other half of the fix.

Overall: approve in spirit. Suggestion #1 (reuse canAccessToDatabase) and the extra authorization-failure test in #4 are the two I would most encourage before merge.

[robfrank]

Thanks for the thorough review. Addressed in 4e5a009:

• Item 1 (status codes) - fixed. validateDatabaseName now throws INVALID_ARGUMENT, authentication failures throw UNAUTHENTICATED, and per-database authorization failures throw PERMISSION_DENIED. executeQuery now preserves a StatusRuntimeException instead of masking it as INTERNAL, and the IT asserts Status.Code.INVALID_ARGUMENT so the client-visible contract is pinned.
• Item 2 (stale Javadoc) - fixed. resolvedUsername Javadoc now links the new validateCredentials(DatabaseCredentials, String) signature.
• Item 3 (extract a single-source authorizeDatabase) - good suggestion, deferring to a follow-up. It spans the server module (ServerSecurity/ServerSecurityUser) and the duplicated logic is the two-line ANY/contains check; keeping this PR scoped to the gRPC security boundary.
• Item 4 (auto-create DB on a SELECT) - agree this is surprising, but as you noted it is pre-existing and now confined to the databases directory (no traversal). Tracking as a separate follow-up to gate creation behind an explicit admin path.

[claude]

Review: PR #4815 — gRPC getDatabase path-traversal + auth/authz hardening

Solid, well-scoped security fix. Placing both checks at the single getDatabase(...) chokepoint means every gRPC entry point (executeQuery, command, createRecord, beginTransaction, streaming, etc.) is covered without touching each handler. The name validation mirrors the established PostServerCommandHandler.resolveBackupFile pattern, the gRPC status mapping (INVALID_ARGUMENT / UNAUTHENTICATED / PERMISSION_DENIED) is correct, and the test deliberately verifies no directory escaped on disk rather than just asserting the error. Good practice including the pre-fix fail / post-fix pass evidence and the design doc.

Issues & suggestions

1. Status-code preservation is only wired into executeQuery (medium)
The new catch (StatusRuntimeException e) was added only to executeQuery (line ~933). The other entry points that call getDatabase catch broadly and remap to INTERNAL:

• createRecord (line 513) — catch (Exception) → Status.INTERNAL
• beginTransaction (line 1133) — catch (Throwable) → Status.INTERNAL
• and the remaining getDatabase callers (lines 233, 627, 673, 837, 1276, 1848, 2953)

A StatusRuntimeException is neither IllegalArgumentException nor specially handled there, so a path-traversal name or an authz failure on those endpoints surfaces to the client as INTERNAL instead of INVALID_ARGUMENT / PERMISSION_DENIED. The security rejection still happens (request is denied), so this is a client-contract inconsistency rather than a hole — but it's worth either adding the same catch (StatusRuntimeException e) ahead of the generic catch in those handlers, or factoring the unwrap into a small helper so the contract is uniform.

2. The auth/authz half of the fix has no negative test coverage (medium)
The IT covers the path-traversal vector well (4 rejection cases + blank + 1 positive control), but the second, more complex half — real authentication + per-database authorization in validateCredentials(credentials, databaseName) — is only exercised by the positive path. There's no test asserting:

• wrong password → UNAUTHENTICATED
• a valid user not authorized for the requested DB → PERMISSION_DENIED (the cross-database escape the PR description highlights)

That branch contains the duplicated ANY/contains authorization logic, so a negative test pinning PERMISSION_DENIED would protect the most regression-prone part of the change.

3. arcadeServer == null skips security entirely while the fallback path can still create DBs (low)
In validateCredentials, security resolves to null when arcadeServer == null, so authentication/authorization is fully skipped — and the fallback branch (line 3133+) can still dbFactory.create() a non-existent database on a plain SELECT. Path traversal is now blocked (good), so this is confined to the databases directory, and it overlaps with deferred Item 4. Likely only embedded/test scenarios hit a null arcadeServer, but a short comment documenting that assumption (or guarding creation) would make the boundary explicit.

4. Duplicated authorization logic (low / nit)
The interceptor-context branch reimplements the exact !allowedDatabases.contains(ANY) && !allowedDatabases.contains(databaseName) check that already lives in ServerSecurity.authenticate. Deferring the ServerSecurity/ServerSecurityUser extraction (Item 3) is reasonable to keep the PR scoped, but until then the two copies can drift. A single security.authorizeDatabase(user, databaseName) helper would be a clean follow-up.

5. Nits

• security.getUsers() != null is always non-null (users.keySet()); the null check is harmless but redundant.
• databaseName.contains("..") also rejects legitimate names like foo..bar. This matches the backup-file pattern and is an acceptable trade-off, just flagging the behavior.

Verdict

The core security objectives are met and the chokepoint design is the right call. Recommend addressing #1 and #2 before merge (both small) and tracking #3/#4 as the already-noted follow-ups.

Reviewed against CLAUDE.md conventions (TDD, final params, status mapping, scoping).

[claude]

Review: fix(#4793) gRPC getDatabase path-traversal + auth/authorization

Thanks for this - the fix is well-targeted and the core security improvement is solid: validateDatabaseName + validateCredentials(credentials, databaseName) are both called at the top of getDatabase, which is the single chokepoint every gRPC entry point routes through, so the path-traversal block and per-database authorization apply uniformly across all ~10 callers (executeQuery, executeCommand, createRecord, lookupByRid, streamQuery, insertStream, graphBatchLoad, etc.). The traversal check correctly runs before any filesystem access and before arcadeServer.getDatabase(...). The validation mirrors the established PostServerCommandHandler.resolveBackupFile pattern (/, \, .., blank), so it's consistent with the HTTP side.

Suggestions

1. Reuse ServerSecurityUser.canAccessToDatabase() instead of inlining the check (recommended).
The context-user branch inlines the authorization test:

final Set<String> allowedDatabases = user.getAuthorizedDatabases();
if (!allowedDatabases.contains(SecurityManager.ANY) && !allowedDatabases.contains(databaseName))
  throw Status.PERMISSION_DENIED...

ServerSecurityUser already exposes exactly this as canAccessToDatabase(databaseName) (body: databasesNames.contains(ANY) || databasesNames.contains(databaseName)), and it's the established idiom everywhere else in the server module: DatabaseAbstractHandler, ToolDispatcher, AiChatHandler, MCPToolUtils, ArcadeDBServer. Replacing the inlined logic with if (!user.canAccessToDatabase(databaseName)) throw ... removes the duplication, drops the new SecurityManager import, and stays consistent with CLAUDE.md's "reuse existing components" guidance. This is "deferred item 3" from the docs, but it's a two-line in-module change against an existing public method, not a cross-module refactor - worth doing now.

2. Test coverage gap on the actual cross-database escape this PR closes.
The PR's headline authorization win (Impact section) is that the interceptor authenticates against the x-arcade-database header while getDatabase resolves the request-body database, so a user authorized for DB-A could previously target DB-B in the body. That exact path - a restricted (non-root) user authorized for DB-A sending a query whose body names DB-B, expecting PERMISSION_DENIED - is not tested. Every test in Issue4793GrpcGetDatabaseSecurityIT uses root, which has ANY access, so the authorization-denial branch never fires. I'd add one test with a user restricted to a single database to lock in the behavior the PR is primarily about.

3. The request-payload authentication branch (the else) is largely unreachable in production and untested.
When security is enabled, GrpcAuthInterceptor always either sets the context user or closes the call (UNAUTHENTICATED), so validateCredentials always takes the context-user branch in practice. The else branch calling security.authenticate(user, password, db) is effectively defense-in-depth the normal interceptor path never hits, and it has no test exercising wrong-password -> PERMISSION_DENIED or missing-password -> UNAUTHENTICATED. Fine to keep as a backstop, but the doc presents it as a primary flow - worth a sentence clarifying it's only reachable if the service is invoked without the interceptor, and/or a small unit test if you want it covered.

4. Status-code preservation is only wired into executeQuery.
The new catch (StatusRuntimeException) that re-surfaces INVALID_ARGUMENT/PERMISSION_DENIED/UNAUTHENTICATED instead of masking as INTERNAL was added only to executeQuery. The other entry points (executeCommand L239, createRecord, lookupByRid, updateRecord, deleteRecord, streamQuery L1333, insertStream, graphBatchLoad) still catch generic Exception and re-wrap the rejection as Status.INTERNAL. Security is still enforced everywhere (the throw happens in getDatabase), but a path-traversal name sent to e.g. executeCommand returns a confusing INTERNAL rather than INVALID_ARGUMENT. Not a blocker, but for a consistent client contract consider hoisting the StatusRuntimeException passthrough into a shared helper or replicating it in the other handlers.

Minor

• validateDatabaseName rejects any name containing .., so a legitimate name like my..db would be refused. This matches the HTTP handler's behavior, so it's acceptable/consistent - just flagging the (very unlikely) edge.
• The auto-create-on-SELECT in the arcadeServer == null fallback (dbFactory.create()) is correctly now confined to the databases directory; deferring the "don't create a DB on a read query" concern as a separate follow-up is reasonable.

Nice work on the IT design (real gRPC channel, interceptor, and the on-disk doesNotExist() assertion that proves no escape actually happened). Addressing #1 and #2 would make this a clean merge.

[codecov]

Codecov Report

❌ Patch coverage is 46.66667% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.18%. Comparing base (85768bf) to head (44b40be).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
.../com/arcadedb/server/grpc/ArcadeDbGrpcService.java	46.66%	7 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #4815   +/-   ##
=========================================
  Coverage     65.18%   65.18%           
  Complexity      715      715           
=========================================
  Files          1680     1680           
  Lines        133181   133209   +28     
  Branches      28467    28475    +8     
=========================================
+ Hits          86811    86832   +21     
+ Misses        34441    34438    -3     
- Partials      11929    11939   +10     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.