diff --git a/src/rag/mod.rs b/src/rag/mod.rs index f1e1255..fc6f306 100644 --- a/src/rag/mod.rs +++ b/src/rag/mod.rs @@ -1582,18 +1582,6 @@ impl RagData { } } - if let Some(api_key) = self.driver_config.get("api_key") - && placeholder_secret_name(api_key).is_none() - { - bail!( - "driver_config.api_key must be a secret placeholder of the form \ - '{{{{NAME}}}}', not a literal key. Store the credential with \ - `coyote --add-secret ` and reference it by name; a literal \ - key would be written to this RAG's YAML in plaintext and cannot \ - be provisioned into the sandbox." - ); - } - match (self.driver.as_str(), self.attached) { ("yaml", false) => Ok(()), ("duckdb", false) => Ok(()), @@ -2203,21 +2191,6 @@ fn resolve_driver_config( }) } -/// The secret NAME inside a `{{NAME}}` placeholder, or `None` for anything else. -/// -/// Deliberately strict, and shared with sandbox provisioning so both agree on -/// what a placeholder is. A RAG's `driver_config.api_key` is supposed to hold a -/// placeholder, never a credential, but nothing stops a hand-edited or older -/// config from holding the literal key. Consumers report failures *by name*, so -/// treating a literal value as a name leaks the credential into stderr and logs. -pub(crate) fn placeholder_secret_name(value: &str) -> Option<&str> { - let inner = value.trim().strip_prefix("{{")?.strip_suffix("}}")?.trim(); - if inner.is_empty() || inner.contains(['{', '}']) { - return None; - } - Some(inner) -} - /// Interpolation core, taking the resolver as an argument so it can be exercised /// without a vault. Mirrors `interpolate_secrets` / `interpolate_secrets_with`. fn resolve_driver_config_with( @@ -3280,64 +3253,6 @@ vectors: {} assert!(data.validate().is_ok()); } - #[test] - fn ragdata_validate_rejects_a_literal_api_key() { - let mut data = RagData::new( - "m".into(), - 1024, - 50, - None, - 5, - None, - GraphRagConfig::default(), - ); - data.driver = "qdrant".to_string(); - data.attached = true; - data.driver_config - .insert("api_key".to_string(), "sk-a-real-looking-key".to_string()); - - let err = data.validate().unwrap_err().to_string(); - - assert!(err.contains("must be a secret placeholder"), "got: {err}"); - assert!( - !err.contains("sk-a-real-looking-key"), - "the error must never echo the credential back: {err}" - ); - } - - #[test] - fn ragdata_validate_accepts_a_placeholder_api_key() { - let mut data = RagData::new( - "m".into(), - 1024, - 50, - None, - 5, - None, - GraphRagConfig::default(), - ); - data.driver = "qdrant".to_string(); - data.attached = true; - data.driver_config - .insert("api_key".to_string(), "{{QDRANT_KEY}}".to_string()); - - assert!(data.validate().is_ok()); - } - - #[test] - fn placeholder_secret_name_accepts_only_well_formed_placeholders() { - assert_eq!(placeholder_secret_name("{{NAME}}"), Some("NAME")); - assert_eq!(placeholder_secret_name(" {{ NAME }} "), Some("NAME")); - assert_eq!(placeholder_secret_name("sk-literal-key"), None); - assert_eq!(placeholder_secret_name(""), None); - assert_eq!(placeholder_secret_name("{{}}"), None); - assert_eq!(placeholder_secret_name("{{ }}"), None); - assert_eq!(placeholder_secret_name("{{A}}{{B}}"), None); - assert_eq!(placeholder_secret_name("prefix{{NAME}}"), None); - assert_eq!(placeholder_secret_name("{{NAME"), None); - assert_eq!(placeholder_secret_name("NAME}}"), None); - } - #[test] fn ragdata_validate_rejects_zero_top_k_from_a_truncated_yaml() { let yaml = " diff --git a/src/sandbox/mod.rs b/src/sandbox/mod.rs index 39f362b..b75ce09 100644 --- a/src/sandbox/mod.rs +++ b/src/sandbox/mod.rs @@ -19,7 +19,7 @@ use crate::config::AppConfig; use crate::config::Config; use crate::config::VAULT_DATA_FILE_NAME; use crate::config::paths; -use crate::rag::{RagData, placeholder_secret_name}; +use crate::rag::RagData; use crate::sandbox::mcp_credentials::MCP_MIXIN_NAME; use crate::sandbox::mixins::DiscoveredMixin; use crate::utils::run_command_with_output; @@ -337,35 +337,25 @@ fn inject_rag_secrets(vault: &Vault, registered: &HashSet) -> Result<()> if !data.attached { continue; } - let Some(placeholder) = data.driver_config.get("api_key") else { - continue; - }; - let service_id = mcp_credentials::secret_service_id(&stem); - if service_id.is_empty() || registered.contains(&service_id) { - continue; - } - let Some(secret_name) = placeholder_secret_name(placeholder) else { - eprintln!( - "Warning: RAG '{stem}' has a driver_config.api_key that is not a \ - secret placeholder, so no credential can be provisioned to the \ - sandbox and queries to this RAG will fail inside it. Store the \ - key with `coyote --add-secret `, then set api_key to the \ - matching placeholder in the RAG YAML." - ); + let secret_names = driver_config_secret_names(&data); + let Some((primary, extra)) = secret_names.split_first() else { continue; }; - match vault.get_secret(secret_name, false) { - Ok(secret_value) => { - sbx_secret_set(&service_id, &secret_value) - .context("Failed to register RAG secret with sbx")?; - } - Err(e) => { - eprintln!( - "Warning: could not load secret '{secret_name}' for RAG '{stem}': {e}. \ - Queries to this RAG will fail inside the sandbox. \ - Run `coyote --add-secret {secret_name}` to fix." - ); + // The generated mixin declares one credential per RAG, keyed on the RAG's + // own service id, so that is where the first one binds. + let service_id = mcp_credentials::secret_service_id(&stem); + if !service_id.is_empty() && !registered.contains(&service_id) { + bind_rag_secret(vault, &service_id, primary, &stem)?; + } + + // Anything beyond the first is registered under its own name, the way MCP + // secrets are, so a hand-written mixin can reference it. The generated + // mixin cannot yet: it carries a single credential entry. + for name in extra { + let id = mcp_credentials::secret_service_id(name); + if !id.is_empty() && !registered.contains(&id) { + bind_rag_secret(vault, &id, name, &stem)?; } } } @@ -373,6 +363,53 @@ fn inject_rag_secrets(vault: &Vault, registered: &HashSet) -> Result<()> Ok(()) } +/// Every distinct vault secret referenced by a RAG's `driver_config`. +/// +/// Deliberately keyed on the placeholder grammar rather than on field names: a +/// driver may call its credential `api_key`, `token` or anything else, and this +/// path should not have to learn each one. Plain values such as `host` and +/// `collection` never match, so they are skipped. +/// +/// A value only counts when it is a placeholder and *nothing else*. That is what +/// keeps a literal credential from being read as a secret NAME — the caller +/// reports failures by name, so a literal would otherwise be printed to stderr. +fn driver_config_secret_names(data: &RagData) -> Vec { + let mut names: Vec = Vec::new(); + for value in data.driver_config.values() { + let trimmed = value.trim(); + let Ok(Some(caps)) = SECRET_RE.captures(trimmed) else { + continue; + }; + if caps.get(0).map(|m| m.as_str()) != Some(trimmed) { + continue; + } + let Some(name) = caps.get(1).map(|m| m.as_str().trim()) else { + continue; + }; + if !name.is_empty() && !names.iter().any(|n| n == name) { + names.push(name.to_string()); + } + } + names +} + +fn bind_rag_secret(vault: &Vault, service_id: &str, secret_name: &str, stem: &str) -> Result<()> { + match vault.get_secret(secret_name, false) { + Ok(secret_value) => { + sbx_secret_set(service_id, &secret_value) + .context("Failed to register RAG secret with sbx")?; + } + Err(e) => { + eprintln!( + "Warning: could not load secret '{secret_name}' for RAG '{stem}': {e}. \ + Queries to this RAG will fail inside the sandbox. \ + Run `coyote --add-secret {secret_name}` to fix." + ); + } + } + Ok(()) +} + fn provider_to_sbx_service(provider_type: &str, client_name: Option<&str>) -> String { match provider_type { "claude" => "anthropic".to_string(), @@ -652,6 +689,70 @@ fn chown_agent_recursive(sandbox: &str, path: &str) -> Result<()> { #[cfg(test)] mod tests { use super::*; + + fn rag_with(driver_config: &[(&str, &str)]) -> RagData { + let mut data = RagData::new("m".into(), 1024, 50, None, 5, None, Default::default()); + data.driver = "qdrant".to_string(); + data.attached = true; + for (k, v) in driver_config { + data.driver_config.insert(k.to_string(), v.to_string()); + } + data + } + + /// The whole point of keying on the placeholder grammar: a driver may call + /// its credential anything, and this path must not have to know the name. + #[test] + fn secret_names_are_found_whatever_the_field_is_called() { + let data = rag_with(&[ + ("host", "qdrant.example.com:6333"), + ("collection", "docs"), + ("token", "{{SOME_TOKEN}}"), + ]); + + assert_eq!(driver_config_secret_names(&data), vec!["SOME_TOKEN"]); + } + + /// A literal credential must never be read as a secret NAME. Callers report + /// failures by name, so doing so prints the credential to stderr. + #[test] + fn a_literal_credential_is_not_treated_as_a_secret_name() { + let data = rag_with(&[("api_key", "sk-a-real-looking-key")]); + + assert!(driver_config_secret_names(&data).is_empty()); + } + + #[test] + fn plain_values_are_never_mistaken_for_secrets() { + let data = rag_with(&[("host", "localhost:6333"), ("collection", "docs")]); + + assert!(driver_config_secret_names(&data).is_empty()); + } + + /// A value that merely *contains* a placeholder is not the credential: the + /// sbx proxy injects the whole secret as the header value. + #[test] + fn a_partial_placeholder_is_not_a_credential() { + let data = rag_with(&[("api_key", "Bearer {{KEY}}")]); + + assert!(driver_config_secret_names(&data).is_empty()); + } + + #[test] + fn several_secrets_are_all_found_and_deduped() { + let data = rag_with(&[ + ("api_key", "{{QDRANT_KEY}}"), + ("host", "localhost:6333"), + ("token", "{{ OTHER_TOKEN }}"), + ("fallback_key", "{{QDRANT_KEY}}"), + ]); + + assert_eq!( + driver_config_secret_names(&data), + vec!["QDRANT_KEY", "OTHER_TOKEN"], + "order follows driver_config, and a repeat is not registered twice" + ); + } use std::time::{SystemTime, UNIX_EPOCH}; #[test]