Skip to content

fix: use dynamic database lookup instead of hardcoded #mongodb-mcp in isSearchSupported #1091

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Will-hxw wants to merge 2 commits into from
Closed

fix: use dynamic database lookup instead of hardcoded #mongodb-mcp in isSearchSupported #1091

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7464fcd
fix(connectionManager): use dynamic database lookup for search index …
Will-hxw Apr 20, 2026
72654ab
fix(connectionManager): clarify comment for listDatabases call
Will-hxw Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions src/common/connectionManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ export interface ConnectionState {
connectedAtlasCluster?: AtlasClusterConnectionInfo;
}

const MCP_TEST_DATABASE = "#mongodb-mcp";
// Fallback database for search index check when no accessible database is found
const MCP_FALLBACK_TEST_DATABASE = "#mongodb-mcp";
// System databases that should be skipped when searching for accessible databases
const SYSTEM_DATABASES = new Set(["admin", "local", "config", "$external"]);

export const defaultDriverOptions: ConnectionInfo["driverOptions"] = {
readConcern: {
Expand Down Expand Up @@ -62,7 +65,10 @@ export class ConnectionStateConnected implements ConnectionState {
// with a cursor otherwise will throw an Error.
// the Search Index Management Service might not be ready yet, but
// we assume that the agent can retry in that situation.
await this.serviceProvider.getSearchIndexes(MCP_TEST_DATABASE, "test");
// Try to find an accessible database dynamically instead of using
// a hardcoded database name that may not exist or be accessible.
const databaseName = await this.findAccessibleDatabase();
await this.serviceProvider.getSearchIndexes(databaseName, "test");
this._isSearchSupported = true;
} catch {
this._isSearchSupported = false;
Expand All @@ -71,6 +77,27 @@ export class ConnectionStateConnected implements ConnectionState {

return this._isSearchSupported;
}

/**
* Find an accessible database for search index operations.
* Tries to list databases and find a non-system database that the user can access.
*/
private async findAccessibleDatabase(): Promise<string> {
try {
// List all databases from admin
const dbs = (await this.serviceProvider.listDatabases("")).databases as {
name: string;
}[];
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "List all databases from admin" but the call is listDatabases(""). If the empty string is intentional (as in list-databases tool), please update the comment to match the actual behavior to avoid confusion for future maintainers.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@Will-hxw Will-hxw Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the comment is misleading. The call listDatabases("") uses an empty string to target the admin database. I'll update the comment to be more accurate.

// Find first non-system database
const accessibleDb = dbs.find((db) => !SYSTEM_DATABASES.has(db.name));
if (accessibleDb) {
return accessibleDb.name;
Comment on lines +88 to +94
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findAccessibleDatabase() selects the first non-system DB name from listDatabases(), but that does not guarantee the user can actually access/read that database (a role may have listDatabases on the cluster but privileges only on specific DBs). This can lead to false negatives in isSearchSupported() if getSearchIndexes() is attempted against an unauthorized DB. Consider iterating candidate DBs and picking one that is actually usable (e.g., try listCollections/a lightweight read or directly attempt getSearchIndexes and continue on auth errors).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@Will-hxw Will-hxw Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. The suggestion to verify actual accessibility by trying listCollections or getSearchIndexes is valid, but implementing it would significantly expand the PR scope.

The current approach is a reasonable first step: it uses listDatabases which is already permitted for users who can call it, and filters out system databases. If listDatabases succeeds, the user has cluster-level access and any non-system database is likely accessible.

The fallback to #mongodb-mcp only triggers when listDatabases fails (permission denied), which means the user lacks cluster-level privileges. In that case, trying multiple databases would likely all fail with auth errors anyway.

If you'd prefer the more thorough approach, I can explore it as a follow-up, but it would be a larger change.

}
} catch {
// If listing databases fails (e.g., permission issues), fall back to default
}
return MCP_FALLBACK_TEST_DATABASE;
Comment on lines +85 to +99
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New behavior (findAccessibleDatabase() selection + system DB filtering + fallback) is not covered by tests. Please add unit tests that verify: (1) a non-system DB is chosen when present, (2) system DBs are skipped, and (3) fallback behavior when listDatabases throws or returns only system DBs.

Copilot generated this review using guidance from repository custom instructions.
Copy link
Copy Markdown
Author

@Will-hxw Will-hxw Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree comprehensive tests would be valuable. However, as I mentioned in the issue comment, setting up a restricted DB user in the test harness would be quite complex since there's no precedent for it in the current test infrastructure. The existing tests cover the fallback behavior through the regular connection flow.\n\nIf test coverage is a blocker for merging, I'd need some guidance on how to approach setting up a restricted user in the test environment. Alternatively, if the maintainers prefer, I can close this PR and the fix can be implemented by someone with more context on the test setup.

}
Comment on lines +96 to +100
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When listDatabases() fails (commonly due to missing listDatabases privilege), the code falls back to #mongodb-mcp, which is the same hardcoded DB that caused the original issue for restricted users. To fully address the reported failure mode, use a fallback that is likely to be permitted (e.g., prefer a DB name from the connection string / authSource / a configurable default, or try a small set like the current DB before falling back).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@Will-hxw Will-hxw Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. The fallback to #mongodb-mcp occurs when listDatabases fails due to permissions. In that scenario, the user has database-level (not cluster-level) access, so trying other databases would likely fail too. The fallback to #mongodb-mcp is the best available option since it's a commonly used database name for MCP tools.

The primary fix is the dynamic lookup when listDatabases succeeds, which helps users with cluster-level access. The fallback case is an edge case where we don't have good alternatives.

}

export interface ConnectionStateConnecting extends ConnectionState {
Expand Down
Loading