From 1f47bf0e02168a71d8c34784b5904a918f853c03 Mon Sep 17 00:00:00 2001 From: Andrea Debernardi Date: Sun, 18 Oct 2026 10:39:58 +0200 Subject: [PATCH 1/2] feat(plugins): support plugin-owned connection fields and secrets Driver plugins can now adapt the host connection inputs and keep their own credentials in the OS keychain. - Manifest `connection_fields`: optional hidden/label/placeholder overrides for host, port, username, password and database. A hidden database input is no longer required, and hiding host also hides the Load databases action. Declared in manifest.schema.json and in the Tabularium extension schema. - `connection-modal.extra_fields` slot: optional `secretFields` (metadata only: value typed in this session, hasStoredValue, dirty) and `setSecretField(key, value)`; an empty value clears the secret. - Secrets are stored under `:plugin::` and only their names are persisted (`plugin_secret_keys`). They are resolved into `params.extra[key]` in memory for every driver call, Test Connection, Load Databases and the MCP server, and never written to connections.json or returned to the frontend. - Lifecycle: save, update (driver switch drops the old secrets), duplicate, delete, group delete, export with credentials and backups, import (a metadata-only re-import keeps the stored secrets). - Disconnect and export no longer fail when a stored plugin secret cannot be read. - PLUGIN_GUIDE documents both contracts; @tabularis/plugin-api exposes the typed slot context and `PluginSecretFieldState`. Existing manifests, saved connections and UI extensions keep their behaviour: every new field is optional. --- packages/plugin-api/src/index.ts | 1 + packages/plugin-api/src/slots.ts | 26 ++ plugins/PLUGIN_GUIDE.md | 53 ++- plugins/manifest.schema.json | 28 ++ plugins/tabularium-extensions.schema.json | 117 +++++++ src-tauri/src/commands.rs | 259 ++++++++++++-- src-tauri/src/connection_cache_tests.rs | 1 + src-tauri/src/connection_import/analyzer.rs | 1 + src-tauri/src/connection_import/convert.rs | 4 + src-tauri/src/connection_import/tabularis.rs | 8 + src-tauri/src/connection_migrations_tests.rs | 1 + src-tauri/src/connection_tags.rs | 1 + src-tauri/src/credential_cache.rs | 65 ++++ src-tauri/src/drivers/driver_trait.rs | 32 ++ src-tauri/src/drivers/mysql/mod.rs | 1 + src-tauri/src/drivers/postgres/mod.rs | 1 + src-tauri/src/drivers/registry/tests.rs | 1 + src-tauri/src/drivers/sqlite/mod.rs | 1 + src-tauri/src/export_import_tests.rs | 3 + src-tauri/src/keychain_utils.rs | 50 +++ src-tauri/src/lib.rs | 1 + src-tauri/src/mcp/mod.rs | 20 ++ src-tauri/src/models.rs | 14 + src-tauri/src/models_tests.rs | 15 + src-tauri/src/persistence.rs | 3 + src-tauri/src/persistence_tests.rs | 40 +++ src-tauri/src/plugin_secrets.rs | 319 ++++++++++++++++++ src-tauri/src/plugin_secrets_tests.rs | 202 +++++++++++ src-tauri/src/plugins/commands.rs | 1 + src-tauri/src/plugins/driver.rs | 1 + src-tauri/src/plugins/manager.rs | 5 +- src-tauri/src/plugins/tests.rs | 22 ++ src-tauri/src/sqlite_database.rs | 2 +- .../tests/postgres_integration/parity.rs | 1 + src/components/modals/NewConnectionModal.tsx | 205 +++++++---- src/contexts/DatabaseContext.ts | 1 + src/types/pluginSlots.ts | 9 + src/types/plugins.ts | 16 + src/utils/credentials.ts | 1 + .../modals/NewConnectionModal.test.tsx | 181 ++++++++++ tests/utils/connectionFieldsSchema.test.ts | 33 ++ 41 files changed, 1661 insertions(+), 85 deletions(-) create mode 100644 src-tauri/src/plugin_secrets.rs create mode 100644 src-tauri/src/plugin_secrets_tests.rs create mode 100644 tests/utils/connectionFieldsSchema.test.ts diff --git a/packages/plugin-api/src/index.ts b/packages/plugin-api/src/index.ts index 4727f6142..3e982490b 100644 --- a/packages/plugin-api/src/index.ts +++ b/packages/plugin-api/src/index.ts @@ -28,6 +28,7 @@ export type { SlotContext, SlotComponentProps, SlotContextMap, + PluginSecretFieldState, TypedSlotProps, UIExtensionDeclaration, } from "./slots"; diff --git a/packages/plugin-api/src/slots.ts b/packages/plugin-api/src/slots.ts index 3e079a546..18b8a261f 100644 --- a/packages/plugin-api/src/slots.ts +++ b/packages/plugin-api/src/slots.ts @@ -1,5 +1,18 @@ import type { ComponentType } from "react"; +/** + * State of one plugin-owned secret in the connection modal + * (`connection-modal.extra_fields` slot). + */ +export interface PluginSecretFieldState { + /** Value typed during this modal session. Stored values are never exposed. */ + value: string; + /** Whether the connection had a value in the OS keychain when editing began. */ + hasStoredValue: boolean; + /** Whether this modal will set (non-empty value) or clear (empty value) it on save. */ + dirty: boolean; +} + /** * Typed context shape per slot. * @@ -91,6 +104,19 @@ export type SlotContextMap = { * connection. The host resets the flag whenever the driver changes. */ setCredentialFieldsHidden: (hidden: boolean) => void; + /** + * Plugin-owned secrets of this connection, keyed by field name. A value + * already stored in the OS keychain is never exposed: it is reported only + * through `hasStoredValue`. Optional so bundles can detect older hosts. + */ + secretFields?: Record; + /** + * Set a plugin-owned secret for Test Connection and Save. The value is + * stored in the OS keychain, never in the connections file, and reaches + * the driver as `params.extra[key]` at runtime. An empty value explicitly + * clears the stored secret on save. + */ + setSecretField?: (key: string, value: string) => void; }; }; diff --git a/plugins/PLUGIN_GUIDE.md b/plugins/PLUGIN_GUIDE.md index 9caefb755..2570f2d82 100644 --- a/plugins/PLUGIN_GUIDE.md +++ b/plugins/PLUGIN_GUIDE.md @@ -121,6 +121,7 @@ One manifest tells Tabularis everything about your plugin — and, when you publ | `capabilities` | object | Feature flags (see below). | | `data_types` | array | List of supported data types (see below). | | `type_mappings` | object \| null | Optional map of generic inferred type names to driver-specific types. Used during paste/import to map generic types (e.g. `DATETIME`) to driver-native equivalents (e.g. `TIMESTAMP`). See [Type Mappings](#type-mappings) below. | +| `connection_fields` | object | Optional hide, label, and placeholder overrides for the common `host`, `port`, `username`, `password`, and `database` fields. Omitted fields preserve the standard UI. See [Connection Fields](#connection-fields) below. | ### Migrating display names @@ -219,6 +220,38 @@ Since `map_inferred_type()` is a synchronous trait method that cannot issue an R --- +### Connection Fields + +The optional `connection_fields` object adapts the host-owned inputs of the connection modal to a driver that does not use them as-is. Each of `host`, `port`, `username`, `password` and `database` accepts: + +| Field | Type | Description | +|-------|------|-------------| +| `hidden` | boolean | Hide the input for this driver. Defaults to `false`. | +| `label` | string | Driver-specific label (max 80 characters). | +| `placeholder` | string | Driver-specific placeholder (max 200 characters). | + +**Example for a cloud warehouse addressed by project id:** + +```json +{ + "connection_fields": { + "host": { "hidden": true }, + "port": { "hidden": true }, + "username": { "hidden": true }, + "password": { "hidden": true }, + "database": { "label": "Project ID", "placeholder": "billing-project" } + } +} +``` + +**Rules:** +- Overrides are presentation only: a hidden input is not rendered, but the value already saved for it is left untouched and still sent to the driver. +- A hidden `database` input is not required on Test Connection or Save. +- Hiding `host` also hides the "Load databases" action next to the database input. +- Use the `connection-modal.extra_fields` slot for the inputs the driver needs instead, and `setSecretField` for credentials (see [SlotContext](#slotcontext)). + +--- + ## 3. Plugin Settings Plugins can declare custom configuration fields that Tabularis renders in the **Settings → gear icon** modal for that plugin. Users fill in the values, Tabularis persists them in `config.json`, and passes them to the plugin at startup via an `initialize` RPC call. @@ -387,7 +420,7 @@ Add an optional `ui_extensions` array to your manifest: | `settings.plugin.actions` | Per-plugin actions in Settings modal | `targetPluginId` | Diagnostics, re-auth buttons | | `settings.plugin.before_settings` | Content above plugin settings form | `targetPluginId` | OAuth panels, status banners | | `connection-modal.connection_content` | Inside the connection form | `driver` | Custom connection fields | -| `connection-modal.extra_fields` | Below host/port in the connection form | `driver`, `extra`, `setExtraField`, `credentialFieldsHidden`, `setCredentialFieldsHidden` | Plugin-specific connection fields (e.g. AWS region). `setCredentialFieldsHidden(true)` hides and clears the host username/password inputs for drivers that authenticate without a login; while hidden the host also ignores the login of an imported connection string and drops the stored password on save | +| `connection-modal.extra_fields` | Below host/port in the connection form | `driver`, `extra`, `setExtraField`, `credentialFieldsHidden`, `setCredentialFieldsHidden`, `secretFields`, `setSecretField` | Plugin-specific connection fields (e.g. AWS region), including values stored per connection in the OS keychain. `setCredentialFieldsHidden(true)` hides and clears the host username/password inputs for drivers that authenticate without a login; while hidden the host also ignores the login of an imported connection string and drops the stored password on save | ### SlotContext @@ -407,6 +440,24 @@ interface SlotContext { } ``` +For `connection-modal.extra_fields`, `setSecretField(key, value)` keeps the +value out of `connections.json` and stores it under the saved connection in the +OS keychain. Passing an empty value explicitly clears the entry. On edit, +`secretFields[key]` exposes only `{ value, hasStoredValue, dirty }`: an existing +secret is represented by `hasStoredValue` and is never returned to plugin UI. +Keys are 1-64 ASCII letters, digits, `.`, `_` or `-`. + +At runtime the host resolves stored values into `params.extra[key]` before +calling the driver (every RPC method, Test Connection, Load Databases and the +MCP server), so the JSON-RPC method shapes remain backward compatible. Export +with credentials and backups carry the values; duplicating a connection copies +them; deleting it removes them; switching the connection to another driver +drops them. + +Both properties are optional in the public TypeScript contract. A plugin that +also supports older hosts must guard their presence (for example by falling +back to the host `password` input when `setSecretField` is undefined). + ### Building UI Extension Bundles Plugin UI components must be pre-built as **IIFE bundles** (Immediately Invoked Function Expression). The host provides `React`, `ReactJSXRuntime`, and the plugin API as globals — your bundle must **not** bundle its own copies of these. diff --git a/plugins/manifest.schema.json b/plugins/manifest.schema.json index 9050a97a9..6886b6c74 100644 --- a/plugins/manifest.schema.json +++ b/plugins/manifest.schema.json @@ -223,6 +223,34 @@ } } }, + "connection_fields": { + "type": "object", + "description": "Optional presentation overrides for host-owned connection fields. Omitted fields keep the standard Tabularis behavior.", + "additionalProperties": false, + "patternProperties": { + "^(host|port|username|password|database)$": { + "type": "object", + "additionalProperties": false, + "properties": { + "hidden": { + "type": "boolean", + "default": false, + "description": "Hide this field for the driver." + }, + "label": { + "type": "string", + "maxLength": 80, + "description": "Driver-specific label." + }, + "placeholder": { + "type": "string", + "maxLength": 200, + "description": "Driver-specific input placeholder." + } + } + } + } + }, "interpreter": { "type": "string", "description": "Optional interpreter for script-based plugins (e.g. python3)." diff --git a/plugins/tabularium-extensions.schema.json b/plugins/tabularium-extensions.schema.json index a44d638ab..140895453 100644 --- a/plugins/tabularium-extensions.schema.json +++ b/plugins/tabularium-extensions.schema.json @@ -273,6 +273,123 @@ } } } + }, + "connection_fields": { + "type": "object", + "additionalProperties": false, + "description": "Optional presentation overrides for the host-owned connection inputs. Omitted fields keep the standard Tabularis behaviour.", + "properties": { + "host": { + "type": "object", + "additionalProperties": false, + "description": "Overrides for the `host` input.", + "properties": { + "hidden": { + "type": "boolean", + "default": false, + "description": "Hide this input for the driver." + }, + "label": { + "type": "string", + "maxLength": 80, + "description": "Driver-specific label." + }, + "placeholder": { + "type": "string", + "maxLength": 200, + "description": "Driver-specific input placeholder." + } + } + }, + "port": { + "type": "object", + "additionalProperties": false, + "description": "Overrides for the `port` input.", + "properties": { + "hidden": { + "type": "boolean", + "default": false, + "description": "Hide this input for the driver." + }, + "label": { + "type": "string", + "maxLength": 80, + "description": "Driver-specific label." + }, + "placeholder": { + "type": "string", + "maxLength": 200, + "description": "Driver-specific input placeholder." + } + } + }, + "username": { + "type": "object", + "additionalProperties": false, + "description": "Overrides for the `username` input.", + "properties": { + "hidden": { + "type": "boolean", + "default": false, + "description": "Hide this input for the driver." + }, + "label": { + "type": "string", + "maxLength": 80, + "description": "Driver-specific label." + }, + "placeholder": { + "type": "string", + "maxLength": 200, + "description": "Driver-specific input placeholder." + } + } + }, + "password": { + "type": "object", + "additionalProperties": false, + "description": "Overrides for the `password` input.", + "properties": { + "hidden": { + "type": "boolean", + "default": false, + "description": "Hide this input for the driver." + }, + "label": { + "type": "string", + "maxLength": 80, + "description": "Driver-specific label." + }, + "placeholder": { + "type": "string", + "maxLength": 200, + "description": "Driver-specific input placeholder." + } + } + }, + "database": { + "type": "object", + "additionalProperties": false, + "description": "Overrides for the `database` input.", + "properties": { + "hidden": { + "type": "boolean", + "default": false, + "description": "Hide this input for the driver." + }, + "label": { + "type": "string", + "maxLength": 80, + "description": "Driver-specific label." + }, + "placeholder": { + "type": "string", + "maxLength": 200, + "description": "Driver-specific input placeholder." + } + } + } + } } } } diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index 5feb4ec48..dbec5c01c 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -910,7 +910,11 @@ fn restore_runtime_connection_uri( } } -pub fn find_connection_by_id( +/// Load a saved connection with its host-owned credentials (keychain password, +/// SSH secrets, connection URI) but without plugin-owned secrets. Used by the +/// edit modal and by the Test Connection / Load Databases flows, which resolve +/// plugin secrets themselves once the pending changes are known. +pub fn find_connection_metadata_by_id( app: &AppHandle, id: &str, ) -> Result { @@ -967,6 +971,18 @@ pub fn find_connection_by_id( Ok(conn) } +/// Load a connection for driver execution. Unlike the metadata/edit path, +/// this resolves plugin-owned secrets into `params.extra` only in memory. +pub fn find_connection_by_id( + app: &AppHandle, + id: &str, +) -> Result { + let mut conn = find_connection_metadata_by_id(app, id)?; + let cache = app.state::>(); + crate::plugin_secrets::hydrate_connection(&cache, &mut conn)?; + Ok(conn) +} + /// Merge a list of incoming groups into an existing list, preserving hierarchy /// and repairing any `parent_id` that points to a group id not present in the /// union (i.e. neither in the existing list nor in the incoming batch). @@ -1031,7 +1047,7 @@ pub async fn get_connection_by_id( app: AppHandle, id: String, ) -> Result { - find_connection_by_id(&app, &id) + find_connection_metadata_by_id(&app, &id) } #[tauri::command] @@ -1400,6 +1416,7 @@ pub async fn save_connection( app: AppHandle, name: String, params: ConnectionParams, + plugin_secret_changes: Option, detect_json_in_text_columns: Option, environment: Option, ) -> Result { @@ -1411,9 +1428,15 @@ pub async fn save_connection( let id = Uuid::new_v4().to_string(); let cache = app.state::>(); + let plugin_secret_changes = plugin_secret_changes.unwrap_or_default(); + let plugin_secret_keys = + crate::plugin_secrets::next_secret_keys(&[], true, &plugin_secret_changes)?; let connection_uri = runtime_connection_uri(¶ms).map(str::to_owned); let mut params_to_save = params_for_persistence(¶ms, connection_uri.is_some()); let mut keychain_writes = KeychainSecretWrites::default(); + for key in plugin_secret_changes.keys() { + params_to_save.extra.remove(key); + } if params.save_in_keychain.unwrap_or(false) { log::debug!("Storing passwords in keychain for connection: {}", name); @@ -1487,6 +1510,7 @@ pub async fn save_connection( id: id.clone(), name: name.clone(), params: params_to_save, + plugin_secret_keys, group_id: None, sort_order: None, detect_json_in_text_columns, @@ -1495,12 +1519,22 @@ pub async fn save_connection( environment: validate_environment(environment)?, }; conn_file.connections.push(new_conn.clone()); - if let Err(err) = persist_connection_uri_change( + if let Err(err) = crate::plugin_secrets::persist_secret_changes( &cache, &id, - false, - connection_uri.as_deref().map(Some), - || save_connections_and_invalidate(&app, &path, &conn_file), + None, + &[], + ¶ms.driver, + &plugin_secret_changes, + || { + persist_connection_uri_change( + &cache, + &id, + false, + connection_uri.as_deref().map(Some), + || save_connections_and_invalidate(&app, &path, &conn_file), + ) + }, ) { keychain_writes.roll_back(&cache, &id); return Err(err); @@ -1540,6 +1574,11 @@ pub async fn delete_connection(app: AppHandle, id: String) -> Res .find(|c| c.id == id) .and_then(|c| c.params.connection_uri_in_keychain) .unwrap_or(false); + let plugin_secret_metadata = conn_file + .connections + .iter() + .find(|c| c.id == id) + .map(|c| (c.params.driver.clone(), c.plugin_secret_keys.clone())); // Capture the appearance before retain so we can cascade-delete the icon file. let appearance_to_delete = conn_file @@ -1559,6 +1598,9 @@ pub async fn delete_connection(app: AppHandle, id: String) -> Res persist_connection_uri_change(&cache, &id, uri_stored_in_keychain, Some(None), || { save_connections_and_invalidate(&app, &path, &conn_file) })?; + if let Some((driver, keys)) = plugin_secret_metadata { + crate::plugin_secrets::delete_connection_secrets(&cache, &id, &driver, &keys); + } // Invalidate the in-memory cache for this connection credential_cache::invalidate_all_for_connection(&cache, &id); @@ -1595,6 +1637,7 @@ pub async fn update_connection( id: String, name: String, params: ConnectionParams, + plugin_secret_changes: Option, detect_json_in_text_columns: Option, environment: Option, ) -> Result { @@ -1616,6 +1659,13 @@ pub async fn update_connection( // must drop it rather than hand one driver's credentials to another. let previous_driver_id = conn_file.connections[conn_idx].params.driver.clone(); let same_driver = previous_driver_id == params.driver; + let original_plugin_secret_keys = conn_file.connections[conn_idx].plugin_secret_keys.clone(); + let plugin_secret_changes = plugin_secret_changes.unwrap_or_default(); + let plugin_secret_keys = crate::plugin_secrets::next_secret_keys( + &original_plugin_secret_keys, + same_driver, + &plugin_secret_changes, + )?; let connection_uri = runtime_connection_uri(¶ms).map(str::to_owned); // The frontend sends the URI back only when the user retyped it. An edit // that leaves the field untouched must keep the stored secret; an edit that @@ -1632,6 +1682,9 @@ pub async fn update_connection( }; let mut params_to_save = params_for_persistence(¶ms, connection_uri.is_some() || preserve_stored_uri); + for key in plugin_secret_changes.keys() { + params_to_save.extra.remove(key); + } let cache = app.state::>(); let mut keychain_writes = KeychainSecretWrites::default(); @@ -1738,6 +1791,7 @@ pub async fn update_connection( id: id.clone(), name, params: params_to_save, + plugin_secret_keys, group_id: original_group_id, sort_order: original_sort_order, detect_json_in_text_columns, @@ -1748,11 +1802,19 @@ pub async fn update_connection( conn_file.connections[conn_idx] = updated.clone(); - if let Err(err) = - persist_connection_uri_change(&cache, &id, existing_uri_in_keychain, uri_change, || { - save_connections_and_invalidate(&app, &path, &conn_file) - }) - { + if let Err(err) = crate::plugin_secrets::persist_secret_changes( + &cache, + &id, + Some(&previous_driver_id), + &original_plugin_secret_keys, + ¶ms.driver, + &plugin_secret_changes, + || { + persist_connection_uri_change(&cache, &id, existing_uri_in_keychain, uri_change, || { + save_connections_and_invalidate(&app, &path, &conn_file) + }) + }, + ) { keychain_writes.roll_back(&cache, &id); return Err(err); } @@ -1854,6 +1916,16 @@ pub async fn duplicate_connection( let mut original = conn_file.connections[original_idx].clone(); let cache = app.state::>(); + let plugin_secret_values = crate::plugin_secrets::load_secret_values( + &cache, + &original.id, + &original.params.driver, + &original.plugin_secret_keys, + )?; + let plugin_secret_changes: crate::plugin_secrets::PluginSecretChanges = plugin_secret_values + .into_iter() + .map(|(key, value)| (key, Some(value))) + .collect(); // Same IAM-auth guard as `find_connection_by_id`: never copy a stale RDS // auth token into a duplicated connection. @@ -1956,9 +2028,10 @@ pub async fn duplicate_connection( }; let new_conn = SavedConnection { - id: new_id, + id: new_id.clone(), name: format!("{} (Copy)", original.name), params: new_params, + plugin_secret_keys: original.plugin_secret_keys.clone(), group_id: original.group_id.clone(), // Copy to same group as original sort_order: None, // Will be placed at end of group detect_json_in_text_columns: original.detect_json_in_text_columns, @@ -1971,7 +2044,15 @@ pub async fn duplicate_connection( conn_file.connections.push(new_conn.clone()); - save_connections_and_invalidate(&app, &path, &conn_file)?; + crate::plugin_secrets::persist_secret_changes( + &cache, + &new_id, + None, + &[], + &original.params.driver, + &plugin_secret_changes, + || save_connections_and_invalidate(&app, &path, &conn_file), + )?; let mut returned_conn = new_conn; // Return with passwords for frontend consistency @@ -2873,6 +2954,12 @@ pub async fn test_connection( request.params.database ); let progress_id = request.progress_id.as_deref(); + // The saved connection, without plugin secrets: they are resolved below, + // after the pending changes from the modal are known. + let saved_conn = match &request.connection_id { + Some(id) => find_connection_metadata_by_id(&app, id).ok(), + None => None, + }; let mut expanded_params = expand_ssh_connection_params(&app, &request.params) .await @@ -2898,16 +2985,25 @@ pub async fn test_connection( .map_err(|e| emit_test_failure(&app, progress_id, "resolve", e))?; if !iam_auth && request.params.password.is_none() && expanded_params.password.is_none() { - let saved_conn = match &request.connection_id { - Some(id) => find_connection_by_id(&app, id).ok(), - None => None, - }; expanded_params.password = resolve_test_connection_password(&request.params, saved_conn.as_ref(), |conn_id| { keychain_utils::get_db_password(conn_id, "") }); } + // Plugin-owned secrets: stored values of the saved connection, overridden + // by whatever the modal is about to set or clear. + { + let cache = app.state::>(); + crate::plugin_secrets::apply_request_secrets( + &cache, + saved_conn.as_ref(), + &mut expanded_params, + &request.plugin_secret_changes, + ) + .map_err(|e| emit_test_failure(&app, progress_id, "resolve", e))?; + } + // Reconnecting to a saved connection sends the on-disk params, which never // carry the URI — restore it the same way the password is restored above. // An inline URI (the ephemeral Test Connection flow) always wins. @@ -3286,6 +3382,7 @@ mod tests { save_in_keychain: Some(save_in_keychain), ..base_params() }, + plugin_secret_keys: Vec::new(), group_id: None, sort_order: None, detect_json_in_text_columns: None, @@ -3314,6 +3411,7 @@ mod tests { id: "conn-1".to_string(), name: "Old Name".to_string(), params: base_params(), + plugin_secret_keys: Vec::new(), group_id: Some("group-a".to_string()), sort_order: Some(3), detect_json_in_text_columns: None, @@ -3334,6 +3432,7 @@ mod tests { id: existing.id.clone(), name: "New Name".to_string(), params: base_params(), + plugin_secret_keys: Vec::new(), group_id: existing.group_id.clone(), sort_order: existing.sort_order, detect_json_in_text_columns: None, @@ -3359,6 +3458,7 @@ mod tests { id: id.to_string(), name: "Test".to_string(), params: base_params(), + plugin_secret_keys: Vec::new(), group_id: None, sort_order: None, detect_json_in_text_columns: None, @@ -4392,11 +4492,11 @@ pub async fn list_databases( expanded_params.password.as_deref(), )?; + let saved_conn = match &request.connection_id { + Some(id) => find_connection_metadata_by_id(&app, id).ok(), + None => None, + }; if !iam_auth && request.params.password.is_none() && expanded_params.password.is_none() { - let saved_conn = match &request.connection_id { - Some(id) => find_connection_by_id(&app, id).ok(), - None => None, - }; expanded_params.password = resolve_test_connection_password(&request.params, saved_conn.as_ref(), |conn_id| { keychain_utils::get_db_password(conn_id, "") @@ -4413,6 +4513,17 @@ pub async fn list_databases( } } + // Plugin-owned secrets, resolved the same way as in `test_connection`. + { + let cache = app.state::>(); + crate::plugin_secrets::apply_request_secrets( + &cache, + saved_conn.as_ref(), + &mut expanded_params, + &request.plugin_secret_changes, + )?; + } + let resolved_params = if let Some(conn_id) = &request.connection_id { resolve_connection_params_with_id(&expanded_params, conn_id)? } else { @@ -6388,7 +6499,15 @@ pub async fn disconnect_connection( // Unregister from health check before closing the pool. crate::health_check::unregister_connection(&connection_id).await; - let saved_conn = find_connection_by_id(&app, &connection_id)?; + // A plugin secret that can no longer be read must not prevent closing the + // pools: fall back to the connection without plugin secrets. + let saved_conn = match find_connection_by_id(&app, &connection_id) { + Ok(conn) => conn, + Err(e) => { + log::warn!("Disconnecting {connection_id} without plugin secrets: {e}"); + find_connection_metadata_by_id(&app, &connection_id)? + } + }; if let Some(driver) = crate::drivers::registry::get_driver(&saved_conn.params.driver).await { driver .invalidate_connection_metadata(Some(&connection_id)) @@ -6915,6 +7034,23 @@ pub async fn delete_connection_group( // deleted along with it. Connections belonging to any group in the // subtree are removed as well. let to_delete = crate::models::collect_group_subtree(&file.groups, &id); + let deleted_plugin_secrets: Vec<(String, String, Vec)> = file + .connections + .iter() + .filter(|connection| { + connection + .group_id + .as_ref() + .is_some_and(|group_id| to_delete.contains(group_id)) + }) + .map(|connection| { + ( + connection.id.clone(), + connection.params.driver.clone(), + connection.plugin_secret_keys.clone(), + ) + }) + .collect(); file.groups.retain(|g| !to_delete.contains(&g.id)); file.connections.retain(|c| { @@ -6924,6 +7060,10 @@ pub async fn delete_connection_group( }); save_connections_and_invalidate(&app, &path, &file)?; + let cache = app.state::>(); + for (connection_id, driver, keys) in deleted_plugin_secrets { + crate::plugin_secrets::delete_connection_secrets(&cache, &connection_id, &driver, &keys); + } Ok(()) } @@ -7072,6 +7212,7 @@ pub async fn export_connections_payload( .state::>() .inner() .clone(); + let mut plugin_secrets = HashMap::new(); // Resolve passwords for database connections for conn in &mut conn_file.connections { @@ -7081,8 +7222,20 @@ pub async fn export_connections_payload( conn.params.ssh_password = None; conn.params.ssh_key_passphrase = None; conn.params.connection_uri = None; + conn.plugin_secret_keys.clear(); continue; } + // A missing keychain entry must not abort the whole export (or a + // scheduled backup): the connection is exported without that value. + let resolved_plugin_secrets = crate::plugin_secrets::load_available_secret_values( + &cache, + &conn.id, + &conn.params.driver, + &conn.plugin_secret_keys, + ); + if !resolved_plugin_secrets.is_empty() { + plugin_secrets.insert(conn.id.clone(), resolved_plugin_secrets); + } if conn.params.save_in_keychain.unwrap_or(false) { // Without this the export carries the marker but not the URI, and // restoring elsewhere yields a connection that cannot resolve it. @@ -7132,6 +7285,7 @@ pub async fn export_connections_payload( ssh_connections, k8s_connections, tags: conn_file.tags, + plugin_secrets, }) } @@ -7166,7 +7320,7 @@ pub async fn import_connections_payload( /// import command above and the foreign-app import flow. pub async fn apply_export_payload( app: AppHandle, - payload: ExportPayload, + mut payload: ExportPayload, ) -> Result<(), String> { let conn_path = get_config_path(&app)?; let ssh_path = get_ssh_config_path(&app)?; @@ -7183,6 +7337,7 @@ pub async fn apply_export_payload( .state::>() .inner() .clone(); + let mut imported_plugin_secrets = std::mem::take(&mut payload.plugin_secrets); // Merge groups (preserves hierarchy; demotes orphaned parent_ids to root) merge_groups(&mut current_file.groups, payload.groups); @@ -7222,6 +7377,64 @@ pub async fn apply_export_payload( // An imported payload is untrusted input and may carry an inline URI. // Hold it to the same rule as a save: keychain or nothing. validate_connection_uri_persistence(&new_conn.params)?; + let declared_plugin_secret_keys = std::mem::take(&mut new_conn.plugin_secret_keys); + for key in &declared_plugin_secret_keys { + crate::plugin_secrets::validate_secret_key(key)?; + new_conn.params.extra.remove(key); + } + let secrets_for_connection = imported_plugin_secrets + .remove(&new_conn.id) + .unwrap_or_default(); + let previous_plugin_metadata = current_file + .connections + .iter() + .find(|connection| connection.id == new_conn.id) + .map(|connection| { + ( + connection.params.driver.clone(), + connection.plugin_secret_keys.clone(), + ) + }); + // Imported values are "set" changes: keys the payload does not carry + // keep their stored value (a metadata-only export re-imported over the + // same connection must not wipe its secrets), unless the driver + // changed, in which case the old driver's secrets are dropped. + let kept_keys: Vec = match &previous_plugin_metadata { + Some((old_driver, old_keys)) if *old_driver == new_conn.params.driver => { + old_keys.clone() + } + _ => Vec::new(), + }; + let imported_keys = crate::plugin_secrets::next_secret_keys( + &kept_keys, + true, + &secrets_for_connection + .iter() + .map(|(key, value)| (key.clone(), Some(value.clone()))) + .collect(), + )?; + for (key, value) in &secrets_for_connection { + new_conn.params.extra.remove(key); + keychain_utils::set_plugin_secret(&new_conn.id, &new_conn.params.driver, key, value)?; + credential_cache::set_plugin_secret_cached( + &cache, + &new_conn.id, + &new_conn.params.driver, + key, + value, + ); + } + if let Some((old_driver, old_keys)) = previous_plugin_metadata { + if old_driver != new_conn.params.driver { + crate::plugin_secrets::delete_connection_secrets( + &cache, + &new_conn.id, + &old_driver, + &old_keys, + ); + } + } + new_conn.plugin_secret_keys = imported_keys; // Handle passwords in keychain if new_conn.params.save_in_keychain.unwrap_or(false) { diff --git a/src-tauri/src/connection_cache_tests.rs b/src-tauri/src/connection_cache_tests.rs index 77bf18a8e..198f577d9 100644 --- a/src-tauri/src/connection_cache_tests.rs +++ b/src-tauri/src/connection_cache_tests.rs @@ -8,6 +8,7 @@ mod tests { id: id.to_string(), name: name.to_string(), params: ConnectionParams::default(), + plugin_secret_keys: Vec::new(), group_id: None, sort_order: None, detect_json_in_text_columns: None, diff --git a/src-tauri/src/connection_import/analyzer.rs b/src-tauri/src/connection_import/analyzer.rs index b3c777940..2899290b6 100644 --- a/src-tauri/src/connection_import/analyzer.rs +++ b/src-tauri/src/connection_import/analyzer.rs @@ -216,6 +216,7 @@ mod tests { database: DatabaseSelection::Single(db.to_string()), ..Default::default() }, + plugin_secret_keys: Vec::new(), group_id: None, sort_order: None, detect_json_in_text_columns: None, diff --git a/src-tauri/src/connection_import/convert.rs b/src-tauri/src/connection_import/convert.rs index ade1e30e1..adad43d28 100644 --- a/src-tauri/src/connection_import/convert.rs +++ b/src-tauri/src/connection_import/convert.rs @@ -4,6 +4,8 @@ //! `SshConnection` records linked by `ssh_connection_id`; groups are matched to //! existing groups by name or created fresh. +use std::collections::HashMap; + use serde::Deserialize; use super::driver_map; @@ -53,6 +55,7 @@ pub fn build_payload( ssh_connections: Vec::new(), k8s_connections: Vec::new(), tags: Vec::new(), + plugin_secrets: HashMap::new(), }; // Resolve group name -> group id, reusing an existing group when the name @@ -219,6 +222,7 @@ fn build_connection( id: conn_id.to_string(), name: conn.name.clone(), params, + plugin_secret_keys: Vec::new(), group_id, sort_order: None, detect_json_in_text_columns: None, diff --git a/src-tauri/src/connection_import/tabularis.rs b/src-tauri/src/connection_import/tabularis.rs index f404f02f0..ae70b70ee 100644 --- a/src-tauri/src/connection_import/tabularis.rs +++ b/src-tauri/src/connection_import/tabularis.rs @@ -105,6 +105,7 @@ pub fn apply( // Carried through wholesale so any tag_ids on imported connections // keep resolving; the merge in apply_export_payload dedups by id. tags: payload.tags.clone(), + plugin_secrets: HashMap::new(), }; let mut group_ids: HashMap = HashMap::new(); // Original payload group ids to preserve verbatim (with their ancestor @@ -161,6 +162,7 @@ pub fn apply( original_group.clone() }; + let source_connection_id = conn.id.clone(); let mut new_conn = conn; new_conn.id = if res.action == "replace" { res.replace_existing_id.clone().unwrap_or_else(new_id) @@ -169,6 +171,10 @@ pub fn apply( }; new_conn.group_id = group_id; new_conn.sort_order = None; + if let Some(secrets) = payload.plugin_secrets.get(&source_connection_id) { + out.plugin_secrets + .insert(new_conn.id.clone(), secrets.clone()); + } // Remap the linked SSH record to a fresh id so the copy doesn't share // (and later clobber) the original's keychain entry. @@ -240,6 +246,7 @@ mod tests { database: DatabaseSelection::Single(db.into()), ..Default::default() }, + plugin_secret_keys: Vec::new(), group_id: group_id.map(str::to_string), sort_order: Some(3), detect_json_in_text_columns: None, @@ -268,6 +275,7 @@ mod tests { ssh_connections: Vec::new(), k8s_connections: Vec::new(), tags: Vec::new(), + plugin_secrets: HashMap::new(), } } diff --git a/src-tauri/src/connection_migrations_tests.rs b/src-tauri/src/connection_migrations_tests.rs index 0893ab79d..4f5dca845 100644 --- a/src-tauri/src/connection_migrations_tests.rs +++ b/src-tauri/src/connection_migrations_tests.rs @@ -32,6 +32,7 @@ mod tests { ssl_mode: ssl_mode.map(str::to_string), ..base_params() }, + plugin_secret_keys: Vec::new(), group_id: None, sort_order: None, detect_json_in_text_columns: None, diff --git a/src-tauri/src/connection_tags.rs b/src-tauri/src/connection_tags.rs index 5b25c47d8..2c0757e4d 100644 --- a/src-tauri/src/connection_tags.rs +++ b/src-tauri/src/connection_tags.rs @@ -239,6 +239,7 @@ mod tests { id: id.to_string(), name: id.to_string(), params: ConnectionParams::default(), + plugin_secret_keys: Vec::new(), group_id: None, sort_order: None, detect_json_in_text_columns: None, diff --git a/src-tauri/src/credential_cache.rs b/src-tauri/src/credential_cache.rs index ed2c0613e..2a0e4c748 100644 --- a/src-tauri/src/credential_cache.rs +++ b/src-tauri/src/credential_cache.rs @@ -20,6 +20,7 @@ pub enum CacheEntry { pub struct CredentialCache { pub db_passwords: Mutex>, pub connection_uris: Mutex>, + pub plugin_secrets: Mutex>, pub ssh_passwords: Mutex>, pub ssh_passphrases: Mutex>, pub ai_keys: Mutex>, @@ -30,6 +31,7 @@ impl Default for CredentialCache { Self { db_passwords: Mutex::new(HashMap::new()), connection_uris: Mutex::new(HashMap::new()), + plugin_secrets: Mutex::new(HashMap::new()), ssh_passwords: Mutex::new(HashMap::new()), ssh_passphrases: Mutex::new(HashMap::new()), ai_keys: Mutex::new(HashMap::new()), @@ -37,6 +39,10 @@ impl Default for CredentialCache { } } +fn plugin_secret_cache_key(connection_id: &str, driver: &str, key: &str) -> String { + format!("{}\0{}\0{}", connection_id, driver, key) +} + // ─── Read-through helpers ───────────────────────────────────────────────────── // These functions are synchronous and intended to be called from inside // `tokio::task::spawn_blocking` when used in async contexts. @@ -95,6 +101,33 @@ pub fn get_connection_uri_cached( Ok(Some(value)) } +pub fn get_plugin_secret_cached( + cache: &CredentialCache, + connection_id: &str, + driver: &str, + key: &str, +) -> Result, String> { + let cache_key = plugin_secret_cache_key(connection_id, driver, key); + { + let guard = cache.plugin_secrets.lock().unwrap(); + match guard.get(&cache_key) { + Some(CacheEntry::Present(value)) => return Ok(Some(value.clone())), + Some(CacheEntry::Absent) => return Ok(None), + None => {} + } + } + + let value = crate::keychain_utils::get_plugin_secret(connection_id, driver, key)?; + cache.plugin_secrets.lock().unwrap().insert( + cache_key, + match &value { + Some(value) => CacheEntry::Present(value.clone()), + None => CacheEntry::Absent, + }, + ); + Ok(value) +} + /// Get SSH password: check cache first, fall through to keychain on miss. pub fn get_ssh_password_cached( cache: &CredentialCache, @@ -198,6 +231,19 @@ pub fn set_connection_uri_cached( ); } +pub fn set_plugin_secret_cached( + cache: &CredentialCache, + connection_id: &str, + driver: &str, + key: &str, + value: &str, +) { + cache.plugin_secrets.lock().unwrap().insert( + plugin_secret_cache_key(connection_id, driver, key), + CacheEntry::Present(value.to_string()), + ); +} + pub fn set_ssh_password_cached(cache: &CredentialCache, connection_id: &str, password: &str) { cache.ssh_passwords.lock().unwrap().insert( connection_id.to_string(), @@ -237,6 +283,19 @@ pub fn invalidate_connection_uri(cache: &CredentialCache, connection_id: &str) { cache.connection_uris.lock().unwrap().remove(connection_id); } +pub fn invalidate_plugin_secret( + cache: &CredentialCache, + connection_id: &str, + driver: &str, + key: &str, +) { + cache + .plugin_secrets + .lock() + .unwrap() + .remove(&plugin_secret_cache_key(connection_id, driver, key)); +} + pub fn invalidate_ssh_password(cache: &CredentialCache, connection_id: &str) { cache.ssh_passwords.lock().unwrap().remove(connection_id); } @@ -255,4 +314,10 @@ pub fn invalidate_all_for_connection(cache: &CredentialCache, connection_id: &st cache.connection_uris.lock().unwrap().remove(connection_id); cache.ssh_passwords.lock().unwrap().remove(connection_id); cache.ssh_passphrases.lock().unwrap().remove(connection_id); + let prefix = format!("{}\0", connection_id); + cache + .plugin_secrets + .lock() + .unwrap() + .retain(|key, _| !key.starts_with(&prefix)); } diff --git a/src-tauri/src/drivers/driver_trait.rs b/src-tauri/src/drivers/driver_trait.rs index e75d6da36..9986b4f44 100644 --- a/src-tauri/src/drivers/driver_trait.rs +++ b/src-tauri/src/drivers/driver_trait.rs @@ -279,6 +279,34 @@ pub fn deprecation_for_builtin(builtin_id: &str) -> Option { } } +/// Optional presentation overrides for one of the host-owned connection +/// fields. Omitted values preserve the host defaults so manifests written +/// before this contract continue to render exactly as they do today. +#[derive(Debug, Serialize, Deserialize, Clone, Default)] +pub struct ConnectionFieldOverride { + #[serde(default)] + pub hidden: bool, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub label: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub placeholder: Option, +} + +/// Driver-specific overrides for the common network connection fields. +#[derive(Debug, Serialize, Deserialize, Clone, Default)] +pub struct ConnectionFieldOverrides { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub host: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub port: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub username: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub password: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub database: Option, +} + /// Metadata describing a registered driver plugin. #[derive(Debug, Serialize, Deserialize, Clone)] pub struct PluginManifest { @@ -327,6 +355,10 @@ pub struct PluginManifest { /// Plugin-owned EXPLAIN parser bundles. Absent for built-in drivers. #[serde(default, skip_serializing_if = "Option::is_none")] pub explain_parsers: Option>, + /// Optional hide/relabel/placeholder overrides for the common connection + /// fields. Absent for built-ins and legacy plugins. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub connection_fields: Option, /// Static type mappings applied by `map_inferred_type`. Keys are generic /// inferred types (uppercase, e.g. `"DATETIME"`), values are driver-specific /// types (e.g. `"TIMESTAMP"`). Empty for built-in drivers which override the diff --git a/src-tauri/src/drivers/mysql/mod.rs b/src-tauri/src/drivers/mysql/mod.rs index ce09d93b9..b156a7aaf 100644 --- a/src-tauri/src/drivers/mysql/mod.rs +++ b/src-tauri/src/drivers/mysql/mod.rs @@ -2030,6 +2030,7 @@ impl MysqlDriver { ], ui_extensions: None, explain_parsers: None, + connection_fields: None, type_mappings: std::collections::HashMap::new(), deprecated: None, }, diff --git a/src-tauri/src/drivers/postgres/mod.rs b/src-tauri/src/drivers/postgres/mod.rs index a7d34c628..7e6ef4984 100644 --- a/src-tauri/src/drivers/postgres/mod.rs +++ b/src-tauri/src/drivers/postgres/mod.rs @@ -2174,6 +2174,7 @@ impl PostgresDriver { }], ui_extensions: None, explain_parsers: None, + connection_fields: None, type_mappings: std::collections::HashMap::new(), // The built-in postgres driver is deprecated in favour of the // standalone `postgresql` plugin; the decision lives in the diff --git a/src-tauri/src/drivers/registry/tests.rs b/src-tauri/src/drivers/registry/tests.rs index 22db651c2..d9222c0c7 100644 --- a/src-tauri/src/drivers/registry/tests.rs +++ b/src-tauri/src/drivers/registry/tests.rs @@ -18,6 +18,7 @@ fn fake_manifest(id: &str) -> PluginManifest { settings: Vec::new(), ui_extensions: None, explain_parsers: None, + connection_fields: None, type_mappings: HashMap::new(), deprecated: None, } diff --git a/src-tauri/src/drivers/sqlite/mod.rs b/src-tauri/src/drivers/sqlite/mod.rs index 03b9b0146..595f1ff24 100644 --- a/src-tauri/src/drivers/sqlite/mod.rs +++ b/src-tauri/src/drivers/sqlite/mod.rs @@ -1082,6 +1082,7 @@ impl SqliteDriver { settings: vec![], ui_extensions: None, explain_parsers: None, + connection_fields: None, type_mappings: std::collections::HashMap::new(), deprecated: None, }, diff --git a/src-tauri/src/export_import_tests.rs b/src-tauri/src/export_import_tests.rs index 3ed78b0d1..54b7f84dc 100644 --- a/src-tauri/src/export_import_tests.rs +++ b/src-tauri/src/export_import_tests.rs @@ -27,6 +27,7 @@ mod tests { save_in_keychain: Some(true), ..Default::default() }, + plugin_secret_keys: vec![], group_id: Some("group1".to_string()), sort_order: Some(0), detect_json_in_text_columns: None, @@ -49,6 +50,7 @@ mod tests { }], k8s_connections: vec![], tags: vec![], + plugin_secrets: std::collections::HashMap::new(), }; let json = serde_json::to_string(&payload).unwrap(); @@ -113,6 +115,7 @@ mod tests { ssh_connections: vec![], k8s_connections: vec![], tags: vec![], + plugin_secrets: std::collections::HashMap::new(), }; let json = serde_json::to_string(&payload).unwrap(); diff --git a/src-tauri/src/keychain_utils.rs b/src-tauri/src/keychain_utils.rs index 538b7fe1e..96d8436f7 100644 --- a/src-tauri/src/keychain_utils.rs +++ b/src-tauri/src/keychain_utils.rs @@ -140,6 +140,56 @@ pub fn delete_connection_uri(connection_id: &str) -> Result<(), String> { } } +/// Keychain account of a plugin-owned connection secret. The driver id is part +/// of the account so switching a connection to another driver never hands one +/// plugin's secret to another. +fn plugin_secret_account(connection_id: &str, driver: &str, key: &str) -> String { + format!("{}:plugin:{}:{}", connection_id, driver, key) +} + +pub fn set_plugin_secret( + connection_id: &str, + driver: &str, + key: &str, + value: &str, +) -> Result<(), String> { + let entry = Entry::new( + SERVICE_NAME, + &plugin_secret_account(connection_id, driver, key), + ) + .map_err(|e| e.to_string())?; + entry.set_password(value).map_err(keychain_error) +} + +pub fn get_plugin_secret( + connection_id: &str, + driver: &str, + key: &str, +) -> Result, String> { + let entry = Entry::new( + SERVICE_NAME, + &plugin_secret_account(connection_id, driver, key), + ) + .map_err(|e| e.to_string())?; + match entry.get_password() { + Ok(value) => Ok(Some(value)), + Err(keyring::Error::NoEntry) => Ok(None), + Err(error) => Err(keychain_error(error)), + } +} + +pub fn delete_plugin_secret(connection_id: &str, driver: &str, key: &str) -> Result<(), String> { + let entry = Entry::new( + SERVICE_NAME, + &plugin_secret_account(connection_id, driver, key), + ) + .map_err(|e| e.to_string())?; + match entry.delete_credential() { + Ok(()) | Err(keyring::Error::NoEntry) => Ok(()), + Err(error) => Err(keychain_error(error)), + } +} + pub fn set_ssh_password(connection_id: &str, password: &str) -> Result<(), String> { eprintln!("[Keychain] Setting SSH password for {}", connection_id); let entry = diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index edc4fc61e..6411390ac 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -81,6 +81,7 @@ pub mod storage_location_tests; pub mod persistence; #[cfg(test)] pub mod persistence_tests; +pub mod plugin_secrets; pub mod plugins; pub mod pool_manager; #[cfg(test)] diff --git a/src-tauri/src/mcp/mod.rs b/src-tauri/src/mcp/mod.rs index d255894ea..3c0cd285b 100644 --- a/src-tauri/src/mcp/mod.rs +++ b/src-tauri/src/mcp/mod.rs @@ -306,6 +306,26 @@ async fn resolve_db_params( } } + // Plugin-owned secrets live only in the keychain and must reach the + // plugin driver through `params.extra`, exactly as in the app. + if !conn.plugin_secret_keys.is_empty() { + conn = tokio::task::spawn_blocking(move || { + let cache = credential_cache::CredentialCache::default(); + crate::plugin_secrets::hydrate_connection(&cache, &mut conn).map(|()| conn) + }) + .await + .map_err(|e| JsonRpcError { + code: -32000, + message: e.to_string(), + data: None, + })? + .map_err(|message| JsonRpcError { + code: -32000, + message, + data: None, + })?; + } + let expanded = expand_ssh_params_for_mcp(&conn.params).await?; let expanded = expand_k8s_params_for_mcp(&expanded).await?; let db_params = commands::resolve_connection_params(&expanded).map_err(|e| JsonRpcError { diff --git a/src-tauri/src/models.rs b/src-tauri/src/models.rs index 9e2704174..17f761f6b 100644 --- a/src-tauri/src/models.rs +++ b/src-tauri/src/models.rs @@ -330,6 +330,11 @@ pub struct SavedConnection { pub id: String, pub name: String, pub params: ConnectionParams, + /// Names of plugin-owned `extra` values stored in the OS keychain. Only + /// the names are persisted; values are resolved into `params.extra` at + /// runtime and are never written to the connections file. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub plugin_secret_keys: Vec, #[serde(skip_serializing_if = "Option::is_none")] pub group_id: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -434,6 +439,11 @@ pub struct ExportPayload { pub k8s_connections: Vec, #[serde(default)] pub tags: Vec, + /// Export-only secret material keyed by connection id and plugin field. + /// This remains empty for metadata-only exports and never appears in + /// `connections.json`. + #[serde(default, skip_serializing_if = "HashMap::is_empty")] + pub plugin_secrets: HashMap>, } #[derive(Debug, Deserialize, Serialize, Clone)] @@ -441,6 +451,10 @@ pub struct TestConnectionRequest { pub params: ConnectionParams, #[serde(skip_serializing_if = "Option::is_none")] pub connection_id: Option, + /// Per-field secret mutations. Missing keys preserve stored values, + /// strings set values, and null explicitly clears values. + #[serde(default, skip_serializing_if = "HashMap::is_empty")] + pub plugin_secret_changes: HashMap>, /// When set, the test emits "connection-test-progress" events tagged with /// this id so the caller can render a live step log. #[serde(skip_serializing_if = "Option::is_none")] diff --git a/src-tauri/src/models_tests.rs b/src-tauri/src/models_tests.rs index 9eb669175..f1fbfc5d0 100644 --- a/src-tauri/src/models_tests.rs +++ b/src-tauri/src/models_tests.rs @@ -222,4 +222,19 @@ mod tests { let json = serde_json::to_string(¶ms).expect("serialize params"); assert!(!json.contains("extra")); } + + #[test] + fn saved_connection_defaults_plugin_secret_markers_for_legacy_json() { + let stored = r#"{ + "id": "legacy", + "name": "Legacy", + "params": { "driver": "mysql", "database": "app" } + }"#; + let connection: crate::models::SavedConnection = + serde_json::from_str(stored).expect("legacy connection deserializes"); + assert!(connection.plugin_secret_keys.is_empty()); + + let json = serde_json::to_string(&connection).expect("serialize connection"); + assert!(!json.contains("plugin_secret_keys")); + } } diff --git a/src-tauri/src/persistence.rs b/src-tauri/src/persistence.rs index 17ce233d7..14a6ba5ae 100644 --- a/src-tauri/src/persistence.rs +++ b/src-tauri/src/persistence.rs @@ -55,6 +55,9 @@ pub fn save_connections_file(path: &Path, file: &ConnectionsFile) -> Result<(), let mut connections_to_save = Vec::new(); for conn in &file.connections { let mut c = conn.clone(); + for key in &c.plugin_secret_keys { + c.params.extra.remove(key); + } if c.params.save_in_keychain.unwrap_or(false) { // Passwords are stored in keychain, remove from JSON c.params.password = None; diff --git a/src-tauri/src/persistence_tests.rs b/src-tauri/src/persistence_tests.rs index cb388246f..23c1c55fb 100644 --- a/src-tauri/src/persistence_tests.rs +++ b/src-tauri/src/persistence_tests.rs @@ -46,6 +46,7 @@ mod tests { save_in_keychain: Some(save_in_keychain), ..ConnectionParams::default() }, + plugin_secret_keys: Vec::new(), group_id: None, sort_order: None, detect_json_in_text_columns: None, @@ -178,4 +179,43 @@ mod tests { ); assert_eq!(std::fs::read_to_string(&path).unwrap(), "new-secret"); } + + #[test] + fn strips_plugin_secret_values_but_preserves_markers() { + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("connections.json"); + let connection = SavedConnection { + id: "connection-1".into(), + name: "Plugin".into(), + params: ConnectionParams { + driver: "bigquery".into(), + database: crate::models::DatabaseSelection::Single("project".into()), + extra: [ + ("location".into(), "EU".into()), + ("credential".into(), "private-value".into()), + ] + .into_iter() + .collect(), + ..Default::default() + }, + plugin_secret_keys: vec!["credential".into()], + group_id: None, + sort_order: None, + detect_json_in_text_columns: None, + appearance: None, + tag_ids: None, + environment: None, + }; + let file = ConnectionsFile { + connections: vec![connection], + ..Default::default() + }; + + save_connections_file(&path, &file).expect("save connections"); + let stored = std::fs::read_to_string(path).expect("read connections"); + assert!(!stored.contains("private-value")); + assert!(stored.contains("credential")); + assert!(stored.contains("location")); + assert!(stored.contains("EU")); + } } diff --git a/src-tauri/src/plugin_secrets.rs b/src-tauri/src/plugin_secrets.rs new file mode 100644 index 000000000..c2908b71b --- /dev/null +++ b/src-tauri/src/plugin_secrets.rs @@ -0,0 +1,319 @@ +//! Plugin-owned connection secrets. +//! +//! A driver plugin can ask the connection modal to store some of its `extra` +//! fields as secrets (`setSecretField` in the `connection-modal.extra_fields` +//! slot). Only the field names are persisted, as +//! [`SavedConnection::plugin_secret_keys`]; the values live in the OS keychain +//! under `:plugin::` and are resolved into +//! `params.extra` in memory, right before a driver call. They never reach +//! `connections.json` and are never returned to the frontend. + +use std::collections::{HashMap, HashSet}; + +use crate::credential_cache::{self, CredentialCache}; +use crate::models::{ConnectionParams, SavedConnection}; + +pub type PluginSecretChanges = HashMap>; + +const MAX_SECRET_KEY_LENGTH: usize = 64; +const MAX_SECRET_VALUE_LENGTH: usize = 1024 * 1024; + +pub fn validate_secret_changes(changes: &PluginSecretChanges) -> Result<(), String> { + for (key, value) in changes { + validate_secret_key(key)?; + if value + .as_ref() + .is_some_and(|value| value.len() > MAX_SECRET_VALUE_LENGTH) + { + return Err(format!( + "Plugin secret '{}' exceeds the maximum size of {} bytes", + key, MAX_SECRET_VALUE_LENGTH + )); + } + } + Ok(()) +} + +pub fn validate_secret_key(key: &str) -> Result<(), String> { + if key.is_empty() + || key.len() > MAX_SECRET_KEY_LENGTH + || !key + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'-' | b'.')) + { + return Err(format!( + "Invalid plugin secret key '{}': use 1-{} ASCII letters, digits, '.', '_' or '-'", + key, MAX_SECRET_KEY_LENGTH + )); + } + Ok(()) +} + +pub fn next_secret_keys( + existing: &[String], + same_driver: bool, + changes: &PluginSecretChanges, +) -> Result, String> { + validate_secret_changes(changes)?; + let mut keys: HashSet = if same_driver { + existing.iter().cloned().collect() + } else { + HashSet::new() + }; + for (key, value) in changes { + if value.is_some() { + keys.insert(key.clone()); + } else { + keys.remove(key); + } + } + let mut keys: Vec = keys.into_iter().collect(); + keys.sort(); + Ok(keys) +} + +pub fn hydrate_connection( + cache: &CredentialCache, + connection: &mut SavedConnection, +) -> Result<(), String> { + let values = load_secret_values( + cache, + &connection.id, + &connection.params.driver, + &connection.plugin_secret_keys, + )?; + connection.params.extra.extend(values); + Ok(()) +} + +pub fn load_secret_values( + cache: &CredentialCache, + connection_id: &str, + driver: &str, + keys: &[String], +) -> Result, String> { + let mut values = HashMap::new(); + for key in keys { + validate_secret_key(key)?; + let value = credential_cache::get_plugin_secret_cached(cache, connection_id, driver, key)? + .ok_or_else(|| format!("Stored plugin secret '{}' is unavailable", key))?; + values.insert(key.clone(), value); + } + Ok(values) +} + +/// Like [`load_secret_values`], but skips the keys whose value can no longer +/// be read instead of failing. Used by flows that must not abort because one +/// connection lost a keychain entry (export, scheduled backups). +pub fn load_available_secret_values( + cache: &CredentialCache, + connection_id: &str, + driver: &str, + keys: &[String], +) -> HashMap { + let mut values = HashMap::new(); + for key in keys { + if validate_secret_key(key).is_err() { + log::warn!( + "Skipping invalid plugin secret key for connection {}", + connection_id + ); + continue; + } + match credential_cache::get_plugin_secret_cached(cache, connection_id, driver, key) { + Ok(Some(value)) => { + values.insert(key.clone(), value); + } + Ok(None) => log::warn!( + "Plugin secret '{}' of connection {} is missing from the keychain", + key, + connection_id + ), + Err(error) => log::warn!( + "Failed to read plugin secret '{}' of connection {}: {}", + key, + connection_id, + error + ), + } + } + values +} + +/// Resolve the plugin secrets for an ad-hoc driver call made with params sent +/// by the connection modal (Test Connection, Load Databases). Stored values of +/// `saved` are loaded only while the driver is unchanged, and only for the +/// keys the pending `changes` do not already set or clear. +pub fn apply_request_secrets( + cache: &CredentialCache, + saved: Option<&SavedConnection>, + params: &mut ConnectionParams, + changes: &PluginSecretChanges, +) -> Result<(), String> { + let stored = match saved.filter(|saved| saved.params.driver == params.driver) { + Some(saved) => { + let unresolved: Vec = saved + .plugin_secret_keys + .iter() + .filter(|key| !changes.contains_key(*key)) + .cloned() + .collect(); + load_secret_values(cache, &saved.id, &saved.params.driver, &unresolved)? + } + None => HashMap::new(), + }; + apply_runtime_changes(params, stored, changes) +} + +pub fn apply_runtime_changes( + params: &mut ConnectionParams, + stored_values: HashMap, + changes: &PluginSecretChanges, +) -> Result<(), String> { + validate_secret_changes(changes)?; + params.extra.extend(stored_values); + for (key, value) in changes { + match value { + Some(value) => _ = params.extra.insert(key.clone(), value.clone()), + None => _ = params.extra.remove(key), + } + } + Ok(()) +} + +pub fn persist_secret_changes( + cache: &CredentialCache, + connection_id: &str, + old_driver: Option<&str>, + old_keys: &[String], + new_driver: &str, + changes: &PluginSecretChanges, + persist: impl FnOnce() -> Result<(), String>, +) -> Result<(), String> { + validate_secret_changes(changes)?; + let same_driver = match old_driver { + None => true, + Some(driver) => driver == new_driver, + }; + let mut affected: Vec<(String, String)> = if same_driver { + changes + .keys() + .map(|key| (new_driver.to_string(), key.clone())) + .collect() + } else { + old_keys + .iter() + .map(|key| (old_driver.unwrap_or_default().to_string(), key.clone())) + .chain( + changes + .keys() + .map(|key| (new_driver.to_string(), key.clone())), + ) + .collect() + }; + affected.sort(); + affected.dedup(); + + let mut snapshots = Vec::with_capacity(affected.len()); + for (driver, key) in &affected { + let previous = crate::keychain_utils::get_plugin_secret(connection_id, driver, key)?; + snapshots.push((driver.clone(), key.clone(), previous)); + } + + let apply_result = (|| { + if !same_driver { + let old_driver = old_driver.unwrap_or_default(); + for key in old_keys { + crate::keychain_utils::delete_plugin_secret(connection_id, old_driver, key)?; + credential_cache::invalidate_plugin_secret(cache, connection_id, old_driver, key); + } + } + for (key, value) in changes { + match value { + Some(value) => { + crate::keychain_utils::set_plugin_secret( + connection_id, + new_driver, + key, + value, + )?; + credential_cache::set_plugin_secret_cached( + cache, + connection_id, + new_driver, + key, + value, + ); + } + None => { + crate::keychain_utils::delete_plugin_secret(connection_id, new_driver, key)?; + credential_cache::invalidate_plugin_secret( + cache, + connection_id, + new_driver, + key, + ); + } + } + } + persist() + })(); + + if apply_result.is_ok() { + return Ok(()); + } + + let original_error = apply_result.unwrap_err(); + let mut rollback_errors = Vec::new(); + for (driver, key, previous) in snapshots { + let result = match previous { + Some(value) => { + crate::keychain_utils::set_plugin_secret(connection_id, &driver, &key, &value).map( + |()| { + credential_cache::set_plugin_secret_cached( + cache, + connection_id, + &driver, + &key, + &value, + ); + }, + ) + } + None => crate::keychain_utils::delete_plugin_secret(connection_id, &driver, &key).map( + |()| { + credential_cache::invalidate_plugin_secret(cache, connection_id, &driver, &key); + }, + ), + }; + if let Err(error) = result { + rollback_errors.push(error); + } + } + + if rollback_errors.is_empty() { + Err(original_error) + } else { + Err(format!( + "{} (failed to roll back plugin secrets: {})", + original_error, + rollback_errors.join("; ") + )) + } +} + +pub fn delete_connection_secrets( + cache: &CredentialCache, + connection_id: &str, + driver: &str, + keys: &[String], +) { + for key in keys { + let _ = crate::keychain_utils::delete_plugin_secret(connection_id, driver, key); + credential_cache::invalidate_plugin_secret(cache, connection_id, driver, key); + } +} + +#[cfg(test)] +#[path = "plugin_secrets_tests.rs"] +mod tests; diff --git a/src-tauri/src/plugin_secrets_tests.rs b/src-tauri/src/plugin_secrets_tests.rs new file mode 100644 index 000000000..6b73f2ed3 --- /dev/null +++ b/src-tauri/src/plugin_secrets_tests.rs @@ -0,0 +1,202 @@ +use super::*; + +#[test] +fn preserves_existing_keys_and_applies_tri_state_changes() { + let changes = HashMap::from([ + ("replace".to_string(), Some("new".to_string())), + ("remove".to_string(), None), + ("add".to_string(), Some("value".to_string())), + ]); + let keys = next_secret_keys( + &["keep".into(), "replace".into(), "remove".into()], + true, + &changes, + ) + .unwrap(); + assert_eq!(keys, vec!["add", "keep", "replace"]); +} + +#[test] +fn changing_driver_drops_old_keys() { + let changes = HashMap::from([("new-token".to_string(), Some("x".to_string()))]); + let keys = next_secret_keys(&["old-token".into()], false, &changes).unwrap(); + assert_eq!(keys, vec!["new-token"]); +} + +#[test] +fn runtime_changes_merge_stored_values_and_apply_replacements_and_clears() { + let mut params = ConnectionParams { + extra: HashMap::from([ + ("region".to_string(), "EU".to_string()), + ("remove".to_string(), "stale".to_string()), + ]), + ..Default::default() + }; + let stored = HashMap::from([ + ("keep".to_string(), "stored".to_string()), + ("replace".to_string(), "old".to_string()), + ]); + let changes = HashMap::from([ + ("replace".to_string(), Some("new".to_string())), + ("remove".to_string(), None), + ]); + + apply_runtime_changes(&mut params, stored, &changes).unwrap(); + + assert_eq!(params.extra.get("region").map(String::as_str), Some("EU")); + assert_eq!(params.extra.get("keep").map(String::as_str), Some("stored")); + assert_eq!(params.extra.get("replace").map(String::as_str), Some("new")); + assert!(!params.extra.contains_key("remove")); +} + +#[test] +fn rejects_keys_that_could_escape_the_keychain_namespace() { + assert!(validate_secret_key("credential").is_ok()); + assert!(validate_secret_key("oauth.refresh_token").is_ok()); + assert!(validate_secret_key("bad:key").is_err()); + assert!(validate_secret_key("").is_err()); +} + +fn saved_connection(driver: &str, keys: &[&str]) -> SavedConnection { + SavedConnection { + id: "conn-1".to_string(), + name: "Plugin".to_string(), + params: ConnectionParams { + driver: driver.to_string(), + ..Default::default() + }, + plugin_secret_keys: keys.iter().map(|key| key.to_string()).collect(), + group_id: None, + sort_order: None, + detect_json_in_text_columns: None, + appearance: None, + tag_ids: None, + environment: None, + } +} + +#[test] +fn request_secrets_resolve_stored_values_and_apply_pending_changes() { + let cache = CredentialCache::default(); + credential_cache::set_plugin_secret_cached( + &cache, + "conn-1", + "bigquery", + "credential", + "stored", + ); + credential_cache::set_plugin_secret_cached(&cache, "conn-1", "bigquery", "token", "old"); + let saved = saved_connection("bigquery", &["credential", "token"]); + let mut params = ConnectionParams { + driver: "bigquery".to_string(), + extra: HashMap::from([("location".to_string(), "EU".to_string())]), + ..Default::default() + }; + let changes = HashMap::from([ + ("token".to_string(), None), + ("refresh".to_string(), Some("typed".to_string())), + ]); + + apply_request_secrets(&cache, Some(&saved), &mut params, &changes).unwrap(); + + assert_eq!(params.extra.get("location").map(String::as_str), Some("EU")); + assert_eq!( + params.extra.get("credential").map(String::as_str), + Some("stored") + ); + assert_eq!( + params.extra.get("refresh").map(String::as_str), + Some("typed") + ); + assert!(!params.extra.contains_key("token")); +} + +#[test] +fn request_secrets_ignore_stored_values_of_another_driver() { + let cache = CredentialCache::default(); + credential_cache::set_plugin_secret_cached( + &cache, + "conn-1", + "bigquery", + "credential", + "stored", + ); + let saved = saved_connection("bigquery", &["credential"]); + let mut params = ConnectionParams { + driver: "postgresql".to_string(), + ..Default::default() + }; + + apply_request_secrets(&cache, Some(&saved), &mut params, &HashMap::new()).unwrap(); + + assert!(params.extra.is_empty()); +} + +#[test] +fn request_secrets_without_saved_connection_only_apply_changes() { + let cache = CredentialCache::default(); + let mut params = ConnectionParams::default(); + let changes = HashMap::from([("credential".to_string(), Some("typed".to_string()))]); + + apply_request_secrets(&cache, None, &mut params, &changes).unwrap(); + + assert_eq!( + params.extra.get("credential").map(String::as_str), + Some("typed") + ); +} + +#[test] +fn request_secrets_reject_invalid_change_keys() { + let cache = CredentialCache::default(); + let mut params = ConnectionParams::default(); + let changes = HashMap::from([("bad:key".to_string(), Some("x".to_string()))]); + + assert!(apply_request_secrets(&cache, None, &mut params, &changes).is_err()); +} + +#[test] +fn available_secret_values_skip_invalid_keys() { + let cache = CredentialCache::default(); + credential_cache::set_plugin_secret_cached( + &cache, + "conn-1", + "bigquery", + "credential", + "stored", + ); + + let values = load_available_secret_values( + &cache, + "conn-1", + "bigquery", + &["credential".to_string(), "bad:key".to_string()], + ); + + assert_eq!(values.len(), 1); + assert_eq!(values.get("credential").map(String::as_str), Some("stored")); +} + +#[test] +fn hydrate_connection_merges_stored_values_into_extra() { + let cache = CredentialCache::default(); + credential_cache::set_plugin_secret_cached( + &cache, + "conn-1", + "bigquery", + "credential", + "stored", + ); + let mut connection = saved_connection("bigquery", &["credential"]); + + hydrate_connection(&cache, &mut connection).unwrap(); + + assert_eq!( + connection + .params + .extra + .get("credential") + .map(String::as_str), + Some("stored") + ); +} diff --git a/src-tauri/src/plugins/commands.rs b/src-tauri/src/plugins/commands.rs index 8cdc61a76..66d993b7f 100644 --- a/src-tauri/src/plugins/commands.rs +++ b/src-tauri/src/plugins/commands.rs @@ -349,6 +349,7 @@ pub async fn get_plugin_manifest(plugin_id: String) -> Result PluginManifest { settings: Vec::new(), ui_extensions: None, explain_parsers: None, + connection_fields: None, type_mappings: HashMap::new(), deprecated: None, } diff --git a/src-tauri/src/plugins/manager.rs b/src-tauri/src/plugins/manager.rs index 94dd940d7..9fc2a39c7 100644 --- a/src-tauri/src/plugins/manager.rs +++ b/src-tauri/src/plugins/manager.rs @@ -8,7 +8,8 @@ use tauri::AppHandle; use crate::config::PluginConfig; use crate::drivers::driver_trait::{ - DriverCapabilities, ExplainParserManifestEntry, PluginManifest, PluginSettingDefinition, + ConnectionFieldOverrides, DriverCapabilities, ExplainParserManifestEntry, PluginManifest, + PluginSettingDefinition, }; use crate::models::DataTypeInfo; use crate::plugins::driver::RpcDriver; @@ -91,6 +92,7 @@ pub struct ConfigManifest { pub ui_extensions: Option>, #[serde(default)] pub explain_parsers: Option>, + pub connection_fields: Option, /// Static type mappings for `map_inferred_type`. Keys are generic inferred /// types (e.g. `"DATETIME"`), values are driver-specific types (e.g. `"TIMESTAMP"`). #[serde(default)] @@ -269,6 +271,7 @@ pub async fn load_plugin_from_dir( settings: config.settings, ui_extensions: config.ui_extensions, explain_parsers: config.explain_parsers, + connection_fields: config.connection_fields, type_mappings: config.type_mappings, // External plugins are not deprecated in favour of another plugin; // `deprecated` is a built-in-only concept stamped at registration. diff --git a/src-tauri/src/plugins/tests.rs b/src-tauri/src/plugins/tests.rs index 8c5f40bd2..fdbf88f8a 100644 --- a/src-tauri/src/plugins/tests.rs +++ b/src-tauri/src/plugins/tests.rs @@ -164,6 +164,28 @@ fn preserves_explain_parser_declarations_from_manifest() { assert_eq!(entries[0].module, "explain/dist/index.iife.js"); } +#[test] +fn parses_optional_connection_field_overrides() { + let manifest: ConfigManifest = serde_json::from_str( + r#"{ + "name": "bigquery", + "version": "1.0.0", + "description": "BigQuery driver", + "connection_fields": { + "host": { "hidden": true }, + "database": { "label": "GCP Project ID", "placeholder": "billing-project" } + } +}"#, + ) + .expect("parse manifest"); + + let fields = manifest.connection_fields.expect("connection fields"); + assert!(fields.host.expect("host override").hidden); + let database = fields.database.expect("database override"); + assert_eq!(database.label.as_deref(), Some("GCP Project ID")); + assert_eq!(database.placeholder.as_deref(), Some("billing-project")); +} + #[test] fn returns_error_for_invalid_manifest() { let dir = tempdir().expect("temp dir"); diff --git a/src-tauri/src/sqlite_database.rs b/src-tauri/src/sqlite_database.rs index a323e915b..2121dd58d 100644 --- a/src-tauri/src/sqlite_database.rs +++ b/src-tauri/src/sqlite_database.rs @@ -125,7 +125,7 @@ pub async fn create_sqlite_database( ..ConnectionParams::default() }; - match crate::commands::save_connection(app, name, params, None, None).await { + match crate::commands::save_connection(app, name, params, None, None, None).await { Ok(connection) => Ok(connection), Err(error) => { if let Err(cleanup_error) = fs::remove_file(&path) { diff --git a/src-tauri/tests/postgres_integration/parity.rs b/src-tauri/tests/postgres_integration/parity.rs index aff5283c2..e0d040795 100644 --- a/src-tauri/tests/postgres_integration/parity.rs +++ b/src-tauri/tests/postgres_integration/parity.rs @@ -307,6 +307,7 @@ fn plugin_manifest() -> PluginManifest { settings: vec![], ui_extensions: None, explain_parsers: None, + connection_fields: None, type_mappings: { let mut m = HashMap::new(); m.insert("DATETIME".to_string(), "TIMESTAMP".to_string()); diff --git a/src/components/modals/NewConnectionModal.tsx b/src/components/modals/NewConnectionModal.tsx index 6bb35dc76..f3fa207c8 100644 --- a/src/components/modals/NewConnectionModal.tsx +++ b/src/components/modals/NewConnectionModal.tsx @@ -41,6 +41,7 @@ import { SshConnectionsModal } from "./SshConnectionsModal"; import { K8sConnectionsModal } from "./K8sConnectionsModal"; import { Select } from "../ui/Select"; import { SlotAnchor } from "../ui/SlotAnchor"; +import type { PluginSecretFieldState } from "../../types/pluginSlots"; import { useAlert } from "../../hooks/useAlert"; import { useDrivers } from "../../hooks/useDrivers"; import { useSettings } from "../../hooks/useSettings"; @@ -186,6 +187,7 @@ interface SavedConnection { id: string; name: string; params: ConnectionParams; + plugin_secret_keys?: string[]; detect_json_in_text_columns?: boolean; appearance?: ConnectionAppearance; tag_ids?: string[]; @@ -407,6 +409,9 @@ export const NewConnectionModal = ({ const [detectJsonInTextColumns, setDetectJsonInTextColumns] = useState(false); const [passwordDirty, setPasswordDirty] = useState(false); const [sshPasswordDirty, setSshPasswordDirty] = useState(false); + const [pluginSecretChanges, setPluginSecretChanges] = useState< + Record + >({}); const [connectionString, setConnectionString] = useState(""); const [connectionStringError, setConnectionStringError] = useState< string | null @@ -718,6 +723,11 @@ export const NewConnectionModal = ({ // Flat single-database store (e.g. Meilisearch): no database to select or name. const singleDatabase = activeDriver?.capabilities?.single_database === true; + const hostField = activeDriver?.connection_fields?.host; + const portField = activeDriver?.connection_fields?.port; + const usernameField = activeDriver?.connection_fields?.username; + const passwordField = activeDriver?.connection_fields?.password; + const databaseField = activeDriver?.connection_fields?.database; // ── plugin slot: connection-modal.connection_content ── const slotRegistry = usePluginSlotRegistry(); @@ -762,6 +772,34 @@ export const NewConnectionModal = ({ setFormData((prev) => ({ ...prev, username: "", password: "" })); } }, []); + const setSecretField = useCallback((key: string, value: string) => { + const normalizedKey = key.trim(); + if (!normalizedKey) return; + setPluginSecretChanges((previous) => ({ + ...previous, + [normalizedKey]: value === "" ? null : value, + })); + }, []); + const secretFields = useMemo(() => { + const storedKeys = + initialConnection?.params.driver === driver + ? (initialConnection.plugin_secret_keys ?? []) + : []; + const keys = new Set([...storedKeys, ...Object.keys(pluginSecretChanges)]); + return Object.fromEntries( + Array.from(keys).map((key): [string, PluginSecretFieldState] => { + const pendingValue = pluginSecretChanges[key]; + return [ + key, + { + value: typeof pendingValue === "string" ? pendingValue : "", + hasStoredValue: storedKeys.includes(key), + dirty: Object.hasOwn(pluginSecretChanges, key), + }, + ]; + }), + ); + }, [driver, initialConnection, pluginSecretChanges]); const extraFieldsSlotContext = useMemo( () => ({ driver, @@ -769,6 +807,8 @@ export const NewConnectionModal = ({ setExtraField, credentialFieldsHidden, setCredentialFieldsHidden, + secretFields, + setSecretField, }), [ driver, @@ -776,6 +816,8 @@ export const NewConnectionModal = ({ setExtraField, credentialFieldsHidden, setCredentialFieldsHidden, + secretFields, + setSecretField, ], ); @@ -1786,6 +1828,10 @@ export const NewConnectionModal = ({ request: { params: { ...listParams }, connection_id: initialConnection?.id, + // Pending plugin secrets belong to the driver being edited; stored + // ones are resolved by the backend from `connection_id`. + plugin_secret_changes: + effectiveDriver === driver ? pluginSecretChanges : {}, }, }); if (!shouldApply()) return; @@ -1832,6 +1878,7 @@ export const NewConnectionModal = ({ setPasswordDirty(false); setSshPasswordDirty(false); setCredentialFieldsHiddenState(false); + setPluginSecretChanges({}); setDbSearchQuery(""); setConnectionString(""); setConnectionStringError(null); @@ -2007,6 +2054,7 @@ export const NewConnectionModal = ({ const handleDriverChange = (newDriver: string) => { setDriver(newDriver); setCredentialFieldsHiddenState(false); + setPluginSecretChanges({}); setFormData({ driver: newDriver, host: "", @@ -2176,6 +2224,7 @@ export const NewConnectionModal = ({ params: { ...testParams }, connection_id: initialConnection?.id, progress_id: progressId, + plugin_secret_changes: pluginSecretChanges, }, }); @@ -2347,6 +2396,7 @@ export const NewConnectionModal = ({ } else if ( !noConnectionRequired && !singleDatabase && + !databaseField?.hidden && !hasConnectionUri && (!formData.database || (typeof formData.database === "string" && !formData.database.trim())) @@ -2433,6 +2483,7 @@ export const NewConnectionModal = ({ id: initialConnection.id, name, params, + pluginSecretChanges, detectJsonInTextColumns: detectJsonInTextColumns ? true : null, environment: environment || null, }); @@ -2449,6 +2500,7 @@ export const NewConnectionModal = ({ }>("save_connection", { name, params, + pluginSecretChanges, detectJsonInTextColumns: detectJsonInTextColumns ? true : null, environment: environment || null, }); @@ -2648,6 +2700,7 @@ export const NewConnectionModal = ({ if (driverChanged) { setDriver(newDriver); setCredentialFieldsHiddenState(false); + setPluginSecretChanges({}); } setFormData((prev) => ({ @@ -2870,29 +2923,41 @@ export const NewConnectionModal = ({ )} - {!isUriPassthrough && ( + {!isUriPassthrough && + (!hostField?.hidden || !portField?.hidden) && (
- updateField("host", v)} - placeholder="localhost" - /> - - updateField("port", v === "" ? undefined : Number(v)) - } - type="number" - placeholder={driver === "mysql" ? "3306" : "5432"} - /> + {!hostField?.hidden && ( + updateField("host", v)} + placeholder={hostField?.placeholder ?? "localhost"} + /> + )} + {!portField?.hidden && ( + + updateField("port", v === "" ? undefined : Number(v)) + } + type="number" + placeholder={ + portField?.placeholder ?? + (driver === "mysql" ? "3306" : "5432") + } + /> + )}
)} @@ -2903,64 +2968,83 @@ export const NewConnectionModal = ({ className="flex flex-col gap-3" /> - {/* User + Password (a plugin may hide them via the extra_fields slot) */} - {!credentialFieldsHidden && ( + {/* User + Password (a plugin may hide them via the extra_fields + slot or per field through its manifest `connection_fields`) */} + {!credentialFieldsHidden && + (!passwordField?.hidden || + (!isUriPassthrough && !usernameField?.hidden)) && (
- {!isUriPassthrough && ( + {!isUriPassthrough && !usernameField?.hidden && ( updateField("username", v)} - placeholder={t("newConnection.usernamePlaceholder")} + placeholder={ + usernameField?.placeholder ?? + t("newConnection.usernamePlaceholder") + } + /> + )} + {!passwordField?.hidden && ( + { + setPasswordDirty(true); + updateField("password", v); + }} + type="password" + placeholder={ + initialConnection && !passwordDirty && !formData.password + ? "••••••••" + : passwordField?.placeholder ?? + t("newConnection.passwordPlaceholder") + } /> )} - { - setPasswordDirty(true); - updateField("password", v); - }} - type="password" - placeholder={ - initialConnection && !passwordDirty && !formData.password - ? "••••••••" - : t("newConnection.passwordPlaceholder") - } - />
)} {/* Database (single) — shown unless the connection has opted into - multi-database selection (flat drivers are always opted in) */} - {!isUriPassthrough && !isOptedInMultiDb && !singleDatabase && ( + multi-database selection (flat drivers are always opted in) or + the plugin hides it through its manifest `connection_fields` */} + {!isUriPassthrough && + !isOptedInMultiDb && + !singleDatabase && + !databaseField?.hidden && (
- + {!hostField?.hidden && ( + + )}
{availableDatabases.length > 0 ? ( + c.setSecretField?.("credential", event.target.value) + } + /> + + {JSON.stringify(state ?? null)} +
+ ); +} + +function renderModalWithSecretField(initialConnection?: InitialConnection) { + const registry = registryWith({ + pluginId: "warehouse", + slot: "connection-modal.extra_fields", + component: PluginSecretField, + }); + return render( + + + , + ); +} + +function invokeArgs(command: string): Record | undefined { + const call = vi + .mocked(invoke) + .mock.calls.find(([name]) => name === command); + return call?.[1] as Record | undefined; +} + +describe("NewConnectionModal extra_fields slot plugin secrets", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(invoke).mockImplementation(async (command) => + command === "list_databases" ? [] : "ok", + ); + sshMocks.loadSshConnections.mockResolvedValue([]); + k8sMocks.loadK8sConnections.mockResolvedValue([]); + eventMocks.listen.mockResolvedValue(eventMocks.unlisten); + }); + + function connectionWithStoredSecret(): InitialConnection { + const connection = createInitialConnection({ driver: "mysql" }); + connection.plugin_secret_keys = ["credential"]; + return connection; + } + + it("exposes a stored secret only as metadata", async () => { + renderModalWithSecretField(connectionWithStoredSecret()); + + const input = await screen.findByLabelText("plugin-credential"); + expect(input).toHaveValue(""); + expect(input).toHaveAttribute("placeholder", "stored-secret"); + expect(screen.getByTestId("plugin-secret-state")).toHaveTextContent( + '{"value":"","hasStoredValue":true,"dirty":false}', + ); + }); + + it("sends a replaced secret to Test Connection as a pending change", async () => { + renderModalWithSecretField(connectionWithStoredSecret()); + + fireEvent.change(await screen.findByLabelText("plugin-credential"), { + target: { value: "replacement" }, + }); + fireEvent.click(screen.getByText("newConnection.testConnection")); + + await waitFor(() => { + expect(invokeArgs("test_connection")).toBeDefined(); + }); + const request = invokeArgs("test_connection")?.request as Record< + string, + unknown + >; + expect(request.plugin_secret_changes).toEqual({ credential: "replacement" }); + expect( + (request.params as { extra?: Record }).extra?.credential, + ).toBeUndefined(); + }); + + it("clears a stored secret with an explicit null on save", async () => { + renderModalWithSecretField(connectionWithStoredSecret()); + + fireEvent.click(await screen.findByText("plugin-clear-credential")); + expect(screen.getByTestId("plugin-secret-state")).toHaveTextContent( + '{"value":"","hasStoredValue":true,"dirty":true}', + ); + fireEvent.click(screen.getByText("newConnection.save")); + + await waitFor(() => { + expect(invokeArgs("update_connection")).toBeDefined(); + }); + expect(invokeArgs("update_connection")?.pluginSecretChanges).toEqual({ + credential: null, + }); + }); + + it("sends no secret changes when the plugin leaves the field untouched", async () => { + renderModalWithSecretField(connectionWithStoredSecret()); + + await screen.findByLabelText("plugin-credential"); + fireEvent.click(screen.getByText("newConnection.save")); + + await waitFor(() => { + expect(invokeArgs("update_connection")).toBeDefined(); + }); + expect(invokeArgs("update_connection")?.pluginSecretChanges).toEqual({}); + }); +}); diff --git a/tests/utils/connectionFieldsSchema.test.ts b/tests/utils/connectionFieldsSchema.test.ts new file mode 100644 index 000000000..348499287 --- /dev/null +++ b/tests/utils/connectionFieldsSchema.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, it } from "vitest"; +import manifestSchema from "../../plugins/manifest.schema.json"; +import extensionsSchema from "../../plugins/tabularium-extensions.schema.json"; + +const FIELDS = ["host", "port", "username", "password", "database"]; + +describe("connection_fields manifest contract", () => { + it("declares optional per-field overrides in the runtime manifest schema", () => { + const connectionFields = manifestSchema.properties.connection_fields; + expect(connectionFields.type).toBe("object"); + expect(connectionFields.additionalProperties).toBe(false); + const [pattern, override] = Object.entries(connectionFields.patternProperties)[0]; + for (const field of FIELDS) { + expect(new RegExp(pattern).test(field)).toBe(true); + } + expect(new RegExp(pattern).test("ssh_host")).toBe(false); + expect(Object.keys(override.properties)).toEqual(["hidden", "label", "placeholder"]); + expect(override.properties.hidden.default).toBe(false); + expect(manifestSchema.required).not.toContain("connection_fields"); + }); + + it("declares the same overrides in the Tabularium extension schema", () => { + const connectionFields = extensionsSchema.properties.connection_fields; + expect(connectionFields.additionalProperties).toBe(false); + expect(Object.keys(connectionFields.properties)).toEqual(FIELDS); + for (const field of FIELDS) { + const override = + connectionFields.properties[field as keyof typeof connectionFields.properties]; + expect(override.additionalProperties).toBe(false); + expect(Object.keys(override.properties)).toEqual(["hidden", "label", "placeholder"]); + } + }); +}); From 02d65fbe924b9e17cb2b2f25226f1978a112dec3 Mon Sep 17 00:00:00 2001 From: Andrea Debernardi Date: Sun, 18 Oct 2026 12:13:24 +0200 Subject: [PATCH 2/2] fix(plugins): tolerate missing plugin secrets A plugin secret listed in `plugin_secret_keys` but missing from the OS keychain (synced connections file, keychain reset, downgrade) made every driver call of the connection fail, along with DDL, dump, MCP and duplicate. - Hydration is lenient everywhere: an invalid, missing or unreadable key (including a locked keychain or denied access) is logged by name, never by value, and left unset so the plugin reports the missing credential. Driver calls, Test Connection, Load Databases, export, backups and duplicate share one read path. - Duplicate copies only the values that can be read and lists only those keys on the copy. - Import skips invalid secret keys and oversized values with a warning instead of rejecting the payload, and writes plugin secrets to the keychain only after connections.json has been saved, so a failed save leaves no orphaned entries. - PLUGIN_GUIDE documents the unset-secret behaviour and what happens after a downgrade drops `plugin_secret_keys`. --- plugins/PLUGIN_GUIDE.md | 16 ++ src-tauri/src/commands.rs | 93 +++----- src-tauri/src/credential_cache.rs | 2 +- src-tauri/src/mcp/mod.rs | 8 +- src-tauri/src/plugin_secrets.rs | 306 ++++++++++++++++++++++---- src-tauri/src/plugin_secrets_tests.rs | 305 ++++++++++++++++++++++++- 6 files changed, 625 insertions(+), 105 deletions(-) diff --git a/plugins/PLUGIN_GUIDE.md b/plugins/PLUGIN_GUIDE.md index 2570f2d82..935323854 100644 --- a/plugins/PLUGIN_GUIDE.md +++ b/plugins/PLUGIN_GUIDE.md @@ -454,6 +454,22 @@ with credentials and backups carry the values; duplicating a connection copies them; deleting it removes them; switching the connection to another driver drops them. +A stored secret can be unavailable at runtime: the connections file was synced +to a machine whose keychain does not have the entry, the keychain was reset, or +it is locked or denies access. The host never fails the call for that. It logs +a warning naming the key (never the value) and calls the driver with +`params.extra[key]` unset, so the plugin must check for the value and return a +clear error asking the user to set the credential again. Duplicating such a +connection copies only the values that can be read, and an import skips secret +keys with an invalid name instead of rejecting the whole file. Imported values +are written to the keychain only after the connections file has been saved. + +Older Tabularis versions do not know `plugin_secret_keys` and drop it when they +rewrite `connections.json`. After a downgrade (or with an older install sharing +the same file) the keychain entries stay but are no longer referenced, so the +plugin sees the secret as unset. Setting it again from the connection modal +restores it. + Both properties are optional in the public TypeScript contract. A plugin that also supports older hosts must guard their presence (for example by falling back to the host `password` input when `setSecretField` is undefined). diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index dbec5c01c..3d29ea23b 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -979,7 +979,7 @@ pub fn find_connection_by_id( ) -> Result { let mut conn = find_connection_metadata_by_id(app, id)?; let cache = app.state::>(); - crate::plugin_secrets::hydrate_connection(&cache, &mut conn)?; + crate::plugin_secrets::hydrate_connection(&cache, &mut conn); Ok(conn) } @@ -1916,16 +1916,22 @@ pub async fn duplicate_connection( let mut original = conn_file.connections[original_idx].clone(); let cache = app.state::>(); - let plugin_secret_values = crate::plugin_secrets::load_secret_values( - &cache, + // Same tolerant read as a driver call: a secret missing from the keychain + // is not copied, and the duplicate does not list it. + let plugin_secret_changes = crate::plugin_secrets::duplicate_secret_changes( &original.id, - &original.params.driver, &original.plugin_secret_keys, - )?; - let plugin_secret_changes: crate::plugin_secrets::PluginSecretChanges = plugin_secret_values - .into_iter() - .map(|(key, value)| (key, Some(value))) - .collect(); + |key| { + credential_cache::get_plugin_secret_cached( + &cache, + &original.id, + &original.params.driver, + key, + ) + }, + ); + let duplicate_plugin_secret_keys = + crate::plugin_secrets::next_secret_keys(&[], true, &plugin_secret_changes)?; // Same IAM-auth guard as `find_connection_by_id`: never copy a stale RDS // auth token into a duplicated connection. @@ -2031,7 +2037,7 @@ pub async fn duplicate_connection( id: new_id.clone(), name: format!("{} (Copy)", original.name), params: new_params, - plugin_secret_keys: original.plugin_secret_keys.clone(), + plugin_secret_keys: duplicate_plugin_secret_keys, group_id: original.group_id.clone(), // Copy to same group as original sort_order: None, // Will be placed at end of group detect_json_in_text_columns: original.detect_json_in_text_columns, @@ -7227,7 +7233,7 @@ pub async fn export_connections_payload( } // A missing keychain entry must not abort the whole export (or a // scheduled backup): the connection is exported without that value. - let resolved_plugin_secrets = crate::plugin_secrets::load_available_secret_values( + let resolved_plugin_secrets = crate::plugin_secrets::load_secret_values( &cache, &conn.id, &conn.params.driver, @@ -7338,6 +7344,7 @@ pub async fn apply_export_payload( .inner() .clone(); let mut imported_plugin_secrets = std::mem::take(&mut payload.plugin_secrets); + let mut plugin_secret_writes = Vec::new(); // Merge groups (preserves hierarchy; demotes orphaned parent_ids to root) merge_groups(&mut current_file.groups, payload.groups); @@ -7378,10 +7385,6 @@ pub async fn apply_export_payload( // Hold it to the same rule as a save: keychain or nothing. validate_connection_uri_persistence(&new_conn.params)?; let declared_plugin_secret_keys = std::mem::take(&mut new_conn.plugin_secret_keys); - for key in &declared_plugin_secret_keys { - crate::plugin_secrets::validate_secret_key(key)?; - new_conn.params.extra.remove(key); - } let secrets_for_connection = imported_plugin_secrets .remove(&new_conn.id) .unwrap_or_default(); @@ -7395,46 +7398,16 @@ pub async fn apply_export_payload( connection.plugin_secret_keys.clone(), ) }); - // Imported values are "set" changes: keys the payload does not carry - // keep their stored value (a metadata-only export re-imported over the - // same connection must not wipe its secrets), unless the driver - // changed, in which case the old driver's secrets are dropped. - let kept_keys: Vec = match &previous_plugin_metadata { - Some((old_driver, old_keys)) if *old_driver == new_conn.params.driver => { - old_keys.clone() - } - _ => Vec::new(), - }; - let imported_keys = crate::plugin_secrets::next_secret_keys( - &kept_keys, - true, - &secrets_for_connection - .iter() - .map(|(key, value)| (key.clone(), Some(value.clone()))) - .collect(), - )?; - for (key, value) in &secrets_for_connection { - new_conn.params.extra.remove(key); - keychain_utils::set_plugin_secret(&new_conn.id, &new_conn.params.driver, key, value)?; - credential_cache::set_plugin_secret_cached( - &cache, - &new_conn.id, - &new_conn.params.driver, - key, - value, - ); - } - if let Some((old_driver, old_keys)) = previous_plugin_metadata { - if old_driver != new_conn.params.driver { - crate::plugin_secrets::delete_connection_secrets( - &cache, - &new_conn.id, - &old_driver, - &old_keys, - ); - } - } - new_conn.plugin_secret_keys = imported_keys; + // Keychain writes are deferred until the connections file is saved, + // so a failed import never leaves orphaned plugin secrets behind. + plugin_secret_writes.extend(crate::plugin_secrets::plan_imported_secrets( + &mut new_conn, + &declared_plugin_secret_keys, + secrets_for_connection, + previous_plugin_metadata + .as_ref() + .map(|(driver, keys)| (driver.as_str(), keys.as_slice())), + )); // Handle passwords in keychain if new_conn.params.save_in_keychain.unwrap_or(false) { @@ -7506,8 +7479,14 @@ pub async fn apply_export_payload( } } - // Save files - save_connections_and_invalidate(&app, &conn_path, ¤t_file)?; + // Save files. Plugin secrets reach the keychain only once the connections + // file that lists them has landed. + crate::plugin_secrets::commit_secret_writes( + &crate::plugin_secrets::KeychainSecretStore, + &cache, + plugin_secret_writes, + || save_connections_and_invalidate(&app, &conn_path, ¤t_file), + )?; let ssh_json = serde_json::to_string_pretty(¤t_ssh).map_err(|e| e.to_string())?; persistence::write_private_file(&ssh_path, ssh_json)?; diff --git a/src-tauri/src/credential_cache.rs b/src-tauri/src/credential_cache.rs index 2a0e4c748..294d0b37b 100644 --- a/src-tauri/src/credential_cache.rs +++ b/src-tauri/src/credential_cache.rs @@ -39,7 +39,7 @@ impl Default for CredentialCache { } } -fn plugin_secret_cache_key(connection_id: &str, driver: &str, key: &str) -> String { +pub(crate) fn plugin_secret_cache_key(connection_id: &str, driver: &str, key: &str) -> String { format!("{}\0{}\0{}", connection_id, driver, key) } diff --git a/src-tauri/src/mcp/mod.rs b/src-tauri/src/mcp/mod.rs index 3c0cd285b..37844dc8b 100644 --- a/src-tauri/src/mcp/mod.rs +++ b/src-tauri/src/mcp/mod.rs @@ -311,18 +311,14 @@ async fn resolve_db_params( if !conn.plugin_secret_keys.is_empty() { conn = tokio::task::spawn_blocking(move || { let cache = credential_cache::CredentialCache::default(); - crate::plugin_secrets::hydrate_connection(&cache, &mut conn).map(|()| conn) + crate::plugin_secrets::hydrate_connection(&cache, &mut conn); + conn }) .await .map_err(|e| JsonRpcError { code: -32000, message: e.to_string(), data: None, - })? - .map_err(|message| JsonRpcError { - code: -32000, - message, - data: None, })?; } diff --git a/src-tauri/src/plugin_secrets.rs b/src-tauri/src/plugin_secrets.rs index c2908b71b..612b7f030 100644 --- a/src-tauri/src/plugin_secrets.rs +++ b/src-tauri/src/plugin_secrets.rs @@ -72,72 +72,105 @@ pub fn next_secret_keys( Ok(keys) } -pub fn hydrate_connection( - cache: &CredentialCache, - connection: &mut SavedConnection, -) -> Result<(), String> { +/// Resolve the stored plugin secrets of `connection` into `params.extra`. +/// +/// Lenient by design: a key whose keychain entry is missing (connections file +/// synced to another machine, keychain reset, entry deleted by hand) or cannot +/// be read (locked keychain, access denied) is logged and left unset, and the +/// call goes on. The plugin then reports the missing credential itself, which +/// is a far better failure than every query, dump and MCP call of the +/// connection erroring out on the host side. +pub fn hydrate_connection(cache: &CredentialCache, connection: &mut SavedConnection) { let values = load_secret_values( cache, &connection.id, &connection.params.driver, &connection.plugin_secret_keys, - )?; + ); connection.params.extra.extend(values); - Ok(()) } +/// Read the stored values of `keys` through the credential cache. Keys that +/// are invalid, missing or unreadable are skipped with a warning (see +/// [`resolve_secret_values`]). This is the single read path shared by driver +/// calls, Test Connection, export, backups and duplicate. pub fn load_secret_values( cache: &CredentialCache, connection_id: &str, driver: &str, keys: &[String], -) -> Result, String> { - let mut values = HashMap::new(); - for key in keys { - validate_secret_key(key)?; - let value = credential_cache::get_plugin_secret_cached(cache, connection_id, driver, key)? - .ok_or_else(|| format!("Stored plugin secret '{}' is unavailable", key))?; - values.insert(key.clone(), value); - } - Ok(values) +) -> HashMap { + resolve_secret_values(connection_id, keys, |key| { + credential_cache::get_plugin_secret_cached(cache, connection_id, driver, key) + }) + .values } -/// Like [`load_secret_values`], but skips the keys whose value can no longer -/// be read instead of failing. Used by flows that must not abort because one -/// connection lost a keychain entry (export, scheduled backups). -pub fn load_available_secret_values( - cache: &CredentialCache, +/// Plugin secret values read for one connection, plus the keys that could not +/// be resolved. +#[derive(Debug, Default)] +pub struct ResolvedSecrets { + pub values: HashMap, + pub unavailable: Vec, +} + +/// Read `keys` with `read`, never failing. Invalid key names, missing entries +/// (`Ok(None)`) and read errors are logged by key name (never by value) and +/// reported in [`ResolvedSecrets::unavailable`]. +pub fn resolve_secret_values( connection_id: &str, - driver: &str, keys: &[String], -) -> HashMap { - let mut values = HashMap::new(); + mut read: impl FnMut(&str) -> Result, String>, +) -> ResolvedSecrets { + let mut resolved = ResolvedSecrets::default(); for key in keys { if validate_secret_key(key).is_err() { log::warn!( - "Skipping invalid plugin secret key for connection {}", + "Skipping invalid plugin secret key of connection {}", connection_id ); + resolved.unavailable.push(key.clone()); continue; } - match credential_cache::get_plugin_secret_cached(cache, connection_id, driver, key) { + match read(key) { Ok(Some(value)) => { - values.insert(key.clone(), value); + resolved.values.insert(key.clone(), value); + } + Ok(None) => { + log::warn!( + "Plugin secret '{}' of connection {} is missing from the keychain; leaving it unset", + key, + connection_id + ); + resolved.unavailable.push(key.clone()); + } + Err(error) => { + log::warn!( + "Failed to read plugin secret '{}' of connection {}; leaving it unset: {}", + key, + connection_id, + error + ); + resolved.unavailable.push(key.clone()); } - Ok(None) => log::warn!( - "Plugin secret '{}' of connection {} is missing from the keychain", - key, - connection_id - ), - Err(error) => log::warn!( - "Failed to read plugin secret '{}' of connection {}: {}", - key, - connection_id, - error - ), } } - values + resolved +} + +/// Secret changes that give a duplicate of a connection its own copy of every +/// value that can still be read. Unavailable keys are not carried over, so the +/// copy never lists a secret it does not have. +pub fn duplicate_secret_changes( + connection_id: &str, + keys: &[String], + read: impl FnMut(&str) -> Result, String>, +) -> PluginSecretChanges { + resolve_secret_values(connection_id, keys, read) + .values + .into_iter() + .map(|(key, value)| (key, Some(value))) + .collect() } /// Resolve the plugin secrets for an ad-hoc driver call made with params sent @@ -158,7 +191,7 @@ pub fn apply_request_secrets( .filter(|key| !changes.contains_key(*key)) .cloned() .collect(); - load_secret_values(cache, &saved.id, &saved.params.driver, &unresolved)? + load_secret_values(cache, &saved.id, &saved.params.driver, &unresolved) } None => HashMap::new(), }; @@ -302,6 +335,201 @@ pub fn persist_secret_changes( } } +/// A plugin secret write deferred until the connections file is saved. +#[derive(Clone, PartialEq, Eq)] +pub enum SecretWrite { + Set { + connection_id: String, + driver: String, + key: String, + value: String, + }, + Delete { + connection_id: String, + driver: String, + key: String, + }, +} + +// Hand-written so a secret value never ends up in a log or a test failure. +impl std::fmt::Debug for SecretWrite { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Set { + connection_id, + driver, + key, + .. + } => f + .debug_struct("Set") + .field("connection_id", connection_id) + .field("driver", driver) + .field("key", key) + .finish_non_exhaustive(), + Self::Delete { + connection_id, + driver, + key, + } => f + .debug_struct("Delete") + .field("connection_id", connection_id) + .field("driver", driver) + .field("key", key) + .finish(), + } + } +} + +/// Keychain operations on plugin secrets. The production implementation is +/// [`KeychainSecretStore`]; tests use an in-memory store. +pub trait SecretStore { + fn set(&self, connection_id: &str, driver: &str, key: &str, value: &str) -> Result<(), String>; + fn delete(&self, connection_id: &str, driver: &str, key: &str) -> Result<(), String>; +} + +pub struct KeychainSecretStore; + +impl SecretStore for KeychainSecretStore { + fn set(&self, connection_id: &str, driver: &str, key: &str, value: &str) -> Result<(), String> { + crate::keychain_utils::set_plugin_secret(connection_id, driver, key, value) + } + + fn delete(&self, connection_id: &str, driver: &str, key: &str) -> Result<(), String> { + crate::keychain_utils::delete_plugin_secret(connection_id, driver, key) + } +} + +/// Work out the plugin secret keys and deferred keychain writes of one +/// imported connection. The payload is untrusted: invalid key names and +/// oversized values are skipped with a warning instead of aborting the import. +/// +/// Every declared or imported secret key is removed from `params.extra`, so a +/// value never lands in `connections.json`. Imported values are "set" changes: +/// keys the payload does not carry keep their stored value while the driver is +/// unchanged; when it changed, the previous driver's secrets are deleted. +pub fn plan_imported_secrets( + connection: &mut SavedConnection, + declared_keys: &[String], + imported_values: HashMap, + previous: Option<(&str, &[String])>, +) -> Vec { + for key in declared_keys { + connection.params.extra.remove(key); + } + let mut changes = PluginSecretChanges::new(); + for (key, value) in imported_values { + connection.params.extra.remove(&key); + let change = HashMap::from([(key.clone(), Some(value))]); + if validate_secret_changes(&change).is_err() { + log::warn!( + "Skipping an invalid plugin secret of imported connection {}", + connection.id + ); + continue; + } + changes.extend(change); + } + + let driver = connection.params.driver.clone(); + let kept_keys: Vec = match previous { + Some((old_driver, old_keys)) if old_driver == driver => old_keys + .iter() + .filter(|key| validate_secret_key(key).is_ok()) + .cloned() + .collect(), + _ => Vec::new(), + }; + connection.plugin_secret_keys = + next_secret_keys(&kept_keys, true, &changes).unwrap_or(kept_keys); + + let mut writes: Vec = Vec::new(); + if let Some((old_driver, old_keys)) = previous { + if old_driver != driver { + let valid_old_keys = old_keys + .iter() + .filter(|key| validate_secret_key(key).is_ok()); + writes.extend(valid_old_keys.map(|key| SecretWrite::Delete { + connection_id: connection.id.clone(), + driver: old_driver.to_string(), + key: key.clone(), + })); + } + } + let mut changes: Vec<(String, Option)> = changes.into_iter().collect(); + changes.sort_by(|a, b| a.0.cmp(&b.0)); + writes.extend(changes.into_iter().filter_map(|(key, value)| { + value.map(|value| SecretWrite::Set { + connection_id: connection.id.clone(), + driver: driver.clone(), + key, + value, + }) + })); + writes +} + +/// Run `save` (the connections file write) and, only once it succeeded, apply +/// the deferred plugin secret `writes`. A failed save therefore never leaves +/// orphaned secrets in the keychain. A keychain write that fails after the +/// save is logged and skipped: the connection lists the key, and runtime +/// hydration leaves it unset so the plugin reports the missing credential. +pub fn commit_secret_writes( + store: &impl SecretStore, + cache: &CredentialCache, + writes: Vec, + save: impl FnOnce() -> Result<(), String>, +) -> Result<(), String> { + save()?; + for write in writes { + match write { + SecretWrite::Set { + connection_id, + driver, + key, + value, + } => match store.set(&connection_id, &driver, &key, &value) { + Ok(()) => credential_cache::set_plugin_secret_cached( + cache, + &connection_id, + &driver, + &key, + &value, + ), + Err(error) => { + credential_cache::invalidate_plugin_secret( + cache, + &connection_id, + &driver, + &key, + ); + log::warn!( + "Failed to store imported plugin secret '{}' of connection {}: {}", + key, + connection_id, + error + ); + } + }, + SecretWrite::Delete { + connection_id, + driver, + key, + } => { + if let Err(error) = store.delete(&connection_id, &driver, &key) { + log::warn!( + "Failed to delete plugin secret '{}' of connection {}: {}", + key, + connection_id, + error + ); + } + credential_cache::invalidate_plugin_secret(cache, &connection_id, &driver, &key); + } + } + } + Ok(()) +} + pub fn delete_connection_secrets( cache: &CredentialCache, connection_id: &str, diff --git a/src-tauri/src/plugin_secrets_tests.rs b/src-tauri/src/plugin_secrets_tests.rs index 6b73f2ed3..1e0259ca5 100644 --- a/src-tauri/src/plugin_secrets_tests.rs +++ b/src-tauri/src/plugin_secrets_tests.rs @@ -1,3 +1,5 @@ +use std::cell::RefCell; + use super::*; #[test] @@ -166,7 +168,7 @@ fn available_secret_values_skip_invalid_keys() { "stored", ); - let values = load_available_secret_values( + let values = load_secret_values( &cache, "conn-1", "bigquery", @@ -189,7 +191,7 @@ fn hydrate_connection_merges_stored_values_into_extra() { ); let mut connection = saved_connection("bigquery", &["credential"]); - hydrate_connection(&cache, &mut connection).unwrap(); + hydrate_connection(&cache, &mut connection); assert_eq!( connection @@ -200,3 +202,302 @@ fn hydrate_connection_merges_stored_values_into_extra() { Some("stored") ); } + +fn missing_entry_reader( + stored: &[(&str, &str)], +) -> impl FnMut(&str) -> Result, String> { + let stored: HashMap = stored + .iter() + .map(|(key, value)| (key.to_string(), value.to_string())) + .collect(); + move |key: &str| match key { + "locked" => Err("Keychain access denied".to_string()), + _ => Ok(stored.get(key).cloned()), + } +} + +#[test] +fn resolve_leaves_missing_and_unreadable_keys_unset() { + let keys = vec![ + "credential".to_string(), + "token".to_string(), + "locked".to_string(), + "bad:key".to_string(), + ]; + + let resolved = resolve_secret_values( + "conn-1", + &keys, + missing_entry_reader(&[("credential", "stored")]), + ); + + assert_eq!(resolved.values.len(), 1); + assert_eq!( + resolved.values.get("credential").map(String::as_str), + Some("stored") + ); + assert_eq!(resolved.unavailable, vec!["token", "locked", "bad:key"]); +} + +#[test] +fn hydrate_keeps_connection_usable_when_a_keychain_entry_is_missing() { + let cache = CredentialCache::default(); + credential_cache::set_plugin_secret_cached( + &cache, + "conn-1", + "bigquery", + "credential", + "stored", + ); + // A cached miss stands in for a keychain without the entry. + cache.plugin_secrets.lock().unwrap().insert( + credential_cache::plugin_secret_cache_key("conn-1", "bigquery", "token"), + credential_cache::CacheEntry::Absent, + ); + let mut connection = saved_connection("bigquery", &["credential", "token"]); + connection + .params + .extra + .insert("location".to_string(), "EU".to_string()); + + hydrate_connection(&cache, &mut connection); + + assert_eq!( + connection + .params + .extra + .get("credential") + .map(String::as_str), + Some("stored") + ); + assert_eq!( + connection.params.extra.get("location").map(String::as_str), + Some("EU") + ); + assert!(!connection.params.extra.contains_key("token")); + assert_eq!(connection.plugin_secret_keys, vec!["credential", "token"]); +} + +#[test] +fn request_secrets_tolerate_a_missing_stored_entry() { + let cache = CredentialCache::default(); + cache.plugin_secrets.lock().unwrap().insert( + credential_cache::plugin_secret_cache_key("conn-1", "bigquery", "credential"), + credential_cache::CacheEntry::Absent, + ); + let saved = saved_connection("bigquery", &["credential"]); + let mut params = ConnectionParams { + driver: "bigquery".to_string(), + ..Default::default() + }; + + apply_request_secrets(&cache, Some(&saved), &mut params, &HashMap::new()).unwrap(); + + assert!(params.extra.is_empty()); +} + +#[test] +fn duplicate_copies_only_the_secrets_that_can_be_read() { + let keys = vec![ + "credential".to_string(), + "token".to_string(), + "locked".to_string(), + ]; + + let changes = duplicate_secret_changes( + "conn-1", + &keys, + missing_entry_reader(&[("credential", "stored")]), + ); + + assert_eq!( + changes, + HashMap::from([("credential".to_string(), Some("stored".to_string()))]) + ); + assert_eq!( + next_secret_keys(&[], true, &changes).unwrap(), + vec!["credential"] + ); +} + +#[test] +fn import_skips_invalid_keys_and_keeps_the_rest() { + let mut connection = saved_connection("bigquery", &[]); + connection.params.extra = HashMap::from([ + ("credential".to_string(), "inline".to_string()), + ("bad:key".to_string(), "inline".to_string()), + ("location".to_string(), "EU".to_string()), + ]); + let imported = HashMap::from([ + ("credential".to_string(), "secret".to_string()), + ("bad:key".to_string(), "secret".to_string()), + ("".to_string(), "secret".to_string()), + ("huge".to_string(), "x".repeat(MAX_SECRET_VALUE_LENGTH + 1)), + ]); + + let writes = plan_imported_secrets( + &mut connection, + &["credential".to_string(), "bad:key".to_string()], + imported, + None, + ); + + assert_eq!(connection.plugin_secret_keys, vec!["credential"]); + assert_eq!( + writes, + vec![SecretWrite::Set { + connection_id: "conn-1".to_string(), + driver: "bigquery".to_string(), + key: "credential".to_string(), + value: "secret".to_string(), + }] + ); + // No secret value, valid or not, is left in the persisted params. + assert_eq!( + connection.params.extra, + HashMap::from([("location".to_string(), "EU".to_string())]) + ); +} + +#[test] +fn import_keeps_stored_keys_of_the_same_driver_and_drops_another_drivers() { + let mut same = saved_connection("bigquery", &[]); + let old_keys = vec!["token".to_string()]; + let writes = plan_imported_secrets( + &mut same, + &[], + HashMap::new(), + Some(("bigquery", &old_keys)), + ); + assert!(writes.is_empty()); + assert_eq!(same.plugin_secret_keys, vec!["token"]); + + let mut switched = saved_connection("bigquery", &[]); + let writes = plan_imported_secrets( + &mut switched, + &[], + HashMap::new(), + Some(("snowflake", &old_keys)), + ); + assert!(switched.plugin_secret_keys.is_empty()); + assert_eq!( + writes, + vec![SecretWrite::Delete { + connection_id: "conn-1".to_string(), + driver: "snowflake".to_string(), + key: "token".to_string(), + }] + ); +} + +#[derive(Default)] +struct MemoryStore { + entries: RefCell>, + failing_key: Option, +} + +impl SecretStore for MemoryStore { + fn set(&self, connection_id: &str, driver: &str, key: &str, value: &str) -> Result<(), String> { + if self.failing_key.as_deref() == Some(key) { + return Err("Keychain locked".to_string()); + } + self.entries.borrow_mut().insert( + (connection_id.into(), driver.into(), key.into()), + value.into(), + ); + Ok(()) + } + + fn delete(&self, connection_id: &str, driver: &str, key: &str) -> Result<(), String> { + self.entries + .borrow_mut() + .remove(&(connection_id.into(), driver.into(), key.into())); + Ok(()) + } +} + +fn set_write(key: &str, value: &str) -> SecretWrite { + SecretWrite::Set { + connection_id: "conn-1".to_string(), + driver: "bigquery".to_string(), + key: key.to_string(), + value: value.to_string(), + } +} + +#[test] +fn failed_save_leaves_no_orphaned_secrets() { + let store = MemoryStore::default(); + store.entries.borrow_mut().insert( + ("conn-1".into(), "snowflake".into(), "token".into()), + "old".into(), + ); + let cache = CredentialCache::default(); + let writes = vec![ + SecretWrite::Delete { + connection_id: "conn-1".to_string(), + driver: "snowflake".to_string(), + key: "token".to_string(), + }, + set_write("credential", "secret"), + ]; + + let result = commit_secret_writes(&store, &cache, writes, || Err("disk full".to_string())); + + assert_eq!(result, Err("disk full".to_string())); + assert_eq!( + *store.entries.borrow(), + HashMap::from([( + ( + "conn-1".to_string(), + "snowflake".to_string(), + "token".to_string() + ), + "old".to_string() + )]) + ); + assert!(cache.plugin_secrets.lock().unwrap().is_empty()); +} + +#[test] +fn successful_save_applies_writes_and_tolerates_a_failing_one() { + let store = MemoryStore { + failing_key: Some("locked".to_string()), + ..Default::default() + }; + let cache = CredentialCache::default(); + let saved = RefCell::new(false); + + commit_secret_writes( + &store, + &cache, + vec![set_write("locked", "x"), set_write("credential", "secret")], + || { + *saved.borrow_mut() = true; + Ok(()) + }, + ) + .unwrap(); + + assert!(*saved.borrow()); + assert_eq!( + store + .entries + .borrow() + .get(&("conn-1".into(), "bigquery".into(), "credential".into())) + .map(String::as_str), + Some("secret") + ); + assert_eq!(store.entries.borrow().len(), 1); + assert_eq!( + credential_cache::get_plugin_secret_cached(&cache, "conn-1", "bigquery", "credential"), + Ok(Some("secret".to_string())) + ); +} + +#[test] +fn secret_write_debug_never_prints_the_value() { + let rendered = format!("{:?}", set_write("credential", "super-secret")); + assert!(rendered.contains("credential")); + assert!(!rendered.contains("super-secret")); +}