feat(connections): forward raw connection URIs to plugin drivers#495
feat(connections): forward raw connection URIs to plugin drivers#495Robbyfuu wants to merge 2 commits into
Conversation
Plugin drivers could not receive a full connection URI, which made `mongodb+srv://` unusable. Three gaps combined: - `buildProtocolRegistry` derived protocols only from a driver's id and the scheme of `connection_string_example`, so a plugin could not register `mongodb+srv` at all. - `parseConnectionString` decomposed the URL into host/port/user/pass/db and dropped the scheme and every query parameter. For `mongodb+srv` that is unrecoverable: the scheme mandates a DNS seedlist lookup, and Atlas hostnames publish SRV records but no A record. - A database path was mandatory, while real Atlas URIs routinely omit it. Drivers can now declare `connection_uri` to receive the string verbatim and `connection_uri_schemes` to register additional schemes. The URI travels on `ConnectionParams`, which reaches every driver call, rather than through plugin settings, which are global per plugin and are only sent once at `initialize`. The URI embeds credentials, so it is treated as a secret: stored in the OS keychain, never in connections.json. Saving requires the keychain and fails loudly instead of downgrading to plaintext; Test Connection may use an ephemeral URI; a failed persist rolls back both the secret and the cache. Import, export and the MCP surface handle it on the same terms. Declared schemes cannot displace a protocol another driver already owns, so a plugin cannot capture connection strings meant for a built-in. Refs TabularisDB#494
697eb41 to
93d59d3
Compare
Reconnecting to a saved connection sends the on-disk params to test_connection and list_databases, and the on-disk params never carry the URI — it lives in the OS keychain. The password already had a restore path in both commands; the URI did not, so a saved passthrough connection enumerated databases at creation time and then failed at reconnect with the decomposed host:port. Found driving a real Atlas cluster end to end.
|
Update: the "no plugin exercises this end to end" gap from the PR description is now closed — I drove the full loop against a live Atlas cluster with a real plugin, and it surfaced one bug in this PR, fixed in the new commit. Setup. A MongoDB plugin declaring the new capabilities ( What worked immediately:
The bug this found (fixed in After the fix: reconnect works, both databases enumerate, collections list, schema inference and data browsing all return real documents from the cluster. (Plugin-side, for anyone wiring their own driver to this: the host sends |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // connections.json never holds the URI, so a passthrough connection | ||
| // that works in the app would reach the driver without its endpoint | ||
| // and credentials here unless it is restored the same way. | ||
| if conn.params.connection_uri_in_keychain.unwrap_or(false) { |
There was a problem hiding this comment.
WARNING: URI restoration nested inside save_in_keychain conditional
A connection can legitimately have save_in_keychain = false (password stored inline or not required) while connection_uri_in_keychain = true (URI stored in the OS keychain). Because this restoration block is inside the if conn.params.save_in_keychain.unwrap_or(false) conditional, the URI is never restored for such connections in MCP requests. The driver receives the decomposed host/port instead of the full URI, causing a runtime connection failure.
Move this block outside the save_in_keychain conditional so it runs whenever connection_uri_in_keychain is true, matching the non-MCP restore path in commands.rs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (27 files)
Fix these issues in Kilo Cloud Reviewed by kimi-k2.6 · Input: 154.3K · Output: 48.7K · Cached: 3.1M |
1 participant
Closes #494.
Opened as a draft for one reason, stated up front: the Rust tests have never executed in my environment.
src-tauri/tests/test_ca.pemis tracked in git but absent from my worktree, andpool_manager_tests.rs:466pulls it in withinclude_bytes!, socargo test --libfails to compile before reaching anything. I wrote 13 Rust tests that compile cleanly undercargo check --testsbut have never run. I am opening this as a draft specifically so CI executes them. Everything else below was verified locally.The problem
Pasting a real MongoDB Atlas connection string fails. Three separate gaps, all on the connection-string import path:
The protocol is rejected.
buildProtocolRegistryderives protocols from only two sources per driver —normalizeProtocol(driver.id)and the scheme ofconnection_string_example.PROTOCOL_ALIAS_GROUPSis a hardcoded const. A plugin has no way to register a second scheme, so:The URI is destroyed even when accepted.
parseConnectionStringdecomposes the URL andtoConnectionParamsforwards only host/port/username/password/database. The scheme and the entire query string are discarded. Formongodb+srv://that cannot be recovered: the scheme mandates a DNS seedlist lookup, and Atlas cluster hostnames publish SRV records but no A record, so the decomposedhost:27017fails at resolution:Query parameters (
tls,retryWrites,w,authSource,replicaSet,appName) are lost with it.A database path is mandatory. Real Atlas URIs routinely omit it —
mongodb+srv://user:pass@cluster0.xxxxx.mongodb.net/?tls=true&retryWrites=true&w=majority&appName=Cluster0— but the parser rejects that with "Database name is required in connection string".Approach
Two new capabilities:
connection_uri(bool) to declare that a driver takes the string verbatim, andconnection_uri_schemes(string[]) to register additional schemes. Both accept camelCase aliases, matching the existing convention forconnection_string/connectionString.The URI travels on
ConnectionParamsrather than through plugin settings. That is the load-bearing decision, so it is worth stating why: plugin settings are pushed once via theinitializeRPC inRpcDriver::new, which makes them global per plugin — an Atlas cluster and a localmongodb://localhostcould not coexist.ConnectionParamsis passed to every driver method, so putting the URI there makes it per-connection. It also meansplugins/driver.rsneeded no production change: serde already serializes the whole struct into each request.Both new fields are
Option+#[serde(default)]+skip_serializing_if, so existing stored connections deserialize unchanged and re-serializing does not introduce the keys. There is a test asserting the serialized output of legacy params contains noconnection_urisubstring. Both new capability fields are#[serde(default)], so existing plugin manifests keep parsing.Non-passthrough parsing is untouched: passthrough is a property of the resolved driver and branches before the existing local/remote split. The database requirement still applies to every driver that does not opt in.
Note that fixing only the protocol rejection would have been strictly worse than the status quo — the URI would parse, get decomposed, and persist a connection that fails at DNS instead of showing a clear validation error. Both gaps land in the same function for that reason.
Security
The URI embeds credentials in cleartext inside the string, so it is treated as a secret throughout:
{id}:connection_uri, never inconnections.json.params_for_persistencestrips it on every write path and leaves only aconnection_uri_in_keychainmarker.connections.jsonpersistence fails after the secret was written, both the keychain entry and the in-memory cache roll back. If the rollback itself fails, both errors are reported and the cache entry is dropped rather than left disagreeing with the keychain.postgresand — because passthrough hands the string over verbatim — capture a URI the user pasted intending the built-in driver. This mirrors the guard the alias-expansion pass already had.Import, export and the MCP surface were each brought onto the same terms:
apply_export_payloadvalidates and routes an inline URI to the keychain instead of persisting it,export_connections_payloadresolves it from the keychain wheninclude_secretsis set so a restored backup is not left with a marker and no secret, andresolve_db_paramsrestores it so an MCP query reaches the driver with the same params the app would send.Verification
Run locally against this branch:
pnpm typecheck— cleanpnpm lint— cleanpnpm test --run tests/utils/connectionStringParser.test.ts— 47 passedcd src-tauri && cargo check— compiles, 2 warnings, both pre-existing and unrelated (ssh_tunnel.rs:91,plugins/driver.rs:34)The full JS suite reports 33 failures across
SettingsProvider,ThemeProvideranduseSidebarResize, all onlocalStorage.clear(). These are pre-existing onmain— I confirmed by stashing this entire change and reproducing the identical 3 files / 33 tests on a clean tree. None of those files are touched here.The regression test guarding scheme shadowing was verified to be meaningful rather than decorative: removing the guard makes it fail, restoring it makes it pass.
Known gaps
commands.rs,models_tests.rsandplugins/driver.rscompile but are unexecuted, for thetest_ca.pemreason above. This is the reason for the draft status.connection_uri: true, so the passthrough path has not been driven against a live cluster. The SRV resolution that motivates the change is the plugin's responsibility.saveKeychainlabel verbatim and uses its established keychain terminology, but they are not native-speaker reviewed.Out of scope deliberately: no UI affordance marking a connection as URI-backed, no reveal-stored-URI flow, and no plugin SDK changes under
packages/— external plugin authors will need the capability documented there before they can adopt it.Happy to adjust the capability naming or split this if you would rather see the parser and the secret-handling land separately.