feat(providersv2): suppress v2-injected credentials from sandbox env and add E2E coverage

Signed-off-by: Calum Murray <cmurray@redhat.com>

Author: Cali0707

crates/openshell-core/src/provider_credentials.rs

@@ -37,11 +37,13 @@ impl ProviderCredentialState {
         credential_expires_at_ms: HashMap<String, i64>,
         dynamic_credentials: HashMap<String, crate::proto::ProviderProfileCredential>,
     ) -> Self {
+        let suppress = v2_injected_env_keys(&dynamic_credentials);
         let (child_env, generation_resolver, current_resolver) =
             SecretResolver::from_provider_env_for_current_revision(
                 env,
                 credential_expires_at_ms,
                 revision,
+                &suppress,
             );
         let snapshot = Arc::new(ProviderCredentialSnapshot {
             revision,
@@ -205,11 +207,13 @@ impl ProviderCredentialState {
         credential_expires_at_ms: HashMap<String, i64>,
         dynamic_credentials: HashMap<String, crate::proto::ProviderProfileCredential>,
     ) -> usize {
+        let suppress = v2_injected_env_keys(&dynamic_credentials);
         let (mut child_env, generation_resolver, current_resolver) =
             SecretResolver::from_provider_env_for_current_revision(
                 env,
                 credential_expires_at_ms,
                 revision,
+                &suppress,
             );
         let mut inner = self
             .inner
@@ -239,6 +243,29 @@ impl ProviderCredentialState {
     }
 }
 
+/// Collect env var keys that are covered by providers-v2 static proxy
+/// injection.  These credentials have no `token_grant` (token grants use
+/// the env var placeholder path) and will be injected by the proxy
+/// transparently — the sandbox process must not see them.
+///
+/// Credentials with a `token_grant` always pass through `can_inject_credential`
+/// regardless of `providers_v2_enabled`, so they are never suppressed here.
+/// Static credentials only enter `dynamic_credentials` when `providers_v2_enabled`
+/// is true on the server side, so this function does not need to re-check the
+/// flag.
+fn v2_injected_env_keys(
+    dynamic_credentials: &HashMap<String, crate::proto::ProviderProfileCredential>,
+) -> HashSet<String> {
+    let mut keys = HashSet::new();
+    for cred in dynamic_credentials.values() {
+        if cred.token_grant.is_some() {
+            continue;
+        }
+        keys.extend(cred.env_vars.iter().cloned());
+    }
+    keys
+}
+
 fn merge_resolvers(
     generations: &VecDeque<Arc<SecretResolver>>,
     current_resolver: Option<&Arc<SecretResolver>>,
@@ -593,4 +620,100 @@ mod tests {
             "suppressed key must not reappear after install_environment"
         );
     }
+
+    #[test]
+    fn v2_injected_env_keys_suppresses_static_but_not_token_grants() {
+        use crate::proto::{ProviderCredentialTokenGrant, ProviderProfileCredential};
+
+        let dynamic_credentials = HashMap::from([
+            // Static bearer credential — should be suppressed
+            (
+                "host\t443\t\tmy-provider:api_key".to_string(),
+                ProviderProfileCredential {
+                    name: "api_key".to_string(),
+                    env_vars: vec!["API_KEY".to_string()],
+                    auth_style: "bearer".to_string(),
+                    header_name: "Authorization".to_string(),
+                    ..Default::default()
+                },
+            ),
+            // Static header credential — should be suppressed
+            (
+                "host\t443\t\tmy-provider:custom_key".to_string(),
+                ProviderProfileCredential {
+                    name: "custom_key".to_string(),
+                    env_vars: vec!["CUSTOM_KEY".to_string()],
+                    auth_style: "header".to_string(),
+                    header_name: "X-Custom".to_string(),
+                    ..Default::default()
+                },
+            ),
+            // Token grant credential — must NOT be suppressed
+            (
+                "host\t443\t\tmy-provider:token".to_string(),
+                ProviderProfileCredential {
+                    name: "token".to_string(),
+                    env_vars: vec!["GRANT_TOKEN".to_string()],
+                    auth_style: "bearer".to_string(),
+                    header_name: "Authorization".to_string(),
+                    token_grant: Some(ProviderCredentialTokenGrant {
+                        token_endpoint: "https://auth.example.com/token".to_string(),
+                        ..Default::default()
+                    }),
+                    ..Default::default()
+                },
+            ),
+        ]);
+
+        let keys = v2_injected_env_keys(&dynamic_credentials);
+        assert!(keys.contains("API_KEY"), "static bearer should be suppressed");
+        assert!(keys.contains("CUSTOM_KEY"), "static header should be suppressed");
+        assert!(!keys.contains("GRANT_TOKEN"), "token grant must not be suppressed");
+    }
+
+    #[test]
+    fn v2_injected_credentials_hidden_from_child_env() {
+        use crate::proto::ProviderProfileCredential;
+
+        let env = HashMap::from([
+            ("API_KEY".to_string(), "secret-key".to_string()),
+            ("OTHER_VAR".to_string(), "visible-value".to_string()),
+        ]);
+        let dynamic_credentials = HashMap::from([(
+            "host\t443\t\tprov:api_key".to_string(),
+            ProviderProfileCredential {
+                name: "api_key".to_string(),
+                env_vars: vec!["API_KEY".to_string()],
+                auth_style: "bearer".to_string(),
+                ..Default::default()
+            },
+        )]);
+
+        let state = ProviderCredentialState::from_environment(
+            1,
+            env,
+            HashMap::new(),
+            dynamic_credentials,
+        );
+        let snapshot = state.snapshot();
+
+        assert!(
+            !snapshot.child_env.contains_key("API_KEY"),
+            "v2-injected key must not appear in child_env"
+        );
+        assert!(
+            snapshot.child_env.contains_key("OTHER_VAR"),
+            "non-injected key must still appear in child_env"
+        );
+
+        // The resolver must still be able to resolve the suppressed key
+        // so the proxy can inject it.
+        let resolver = state.resolver().expect("resolver should exist");
+        let resolved = resolver.resolve_placeholder("openshell:resolve:env:v1_API_KEY");
+        assert_eq!(
+            resolved,
+            Some("secret-key"),
+            "resolver must still resolve suppressed credential for proxy injection"
+        );
+    }
 }

crates/openshell-core/src/secrets.rs

@@ -3,7 +3,7 @@
 
 use crate::time::now_ms;
 use base64::Engine as _;
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::fmt;
 
 const PLACEHOLDER_PREFIX: &str = "openshell:resolve:env:";
@@ -127,13 +127,22 @@ impl SecretResolver {
             credential_expires_at_ms,
             revision,
             false,
+            &HashSet::new(),
         )
     }
 
+    /// Build a child environment and secret resolvers from provider credentials.
+    ///
+    /// Keys in `suppress_from_child_env` are stored in the resolver (so the
+    /// proxy can resolve them for injection) but omitted from `child_env` so
+    /// sandbox processes never see them — not even as placeholders.  This is
+    /// the mechanism that keeps providers-v2 proxy-injected credentials
+    /// invisible to the sandbox.
     pub fn from_provider_env_for_current_revision(
         provider_env: HashMap<String, String>,
         credential_expires_at_ms: HashMap<String, i64>,
         revision: u64,
+        suppress_from_child_env: &HashSet<String>,
     ) -> (HashMap<String, String>, Option<Self>, Option<Self>) {
         if revision == 0 {
             let (child_env, current_resolver) =
@@ -142,6 +151,7 @@ impl SecretResolver {
                     credential_expires_at_ms,
                     0,
                     true,
+                    suppress_from_child_env,
                 );
             return (child_env, None, current_resolver);
         }
@@ -153,12 +163,14 @@ impl SecretResolver {
                 credential_expires_at_ms,
                 revision,
                 false,
+                suppress_from_child_env,
             );
         let (_, current_resolver) = Self::from_provider_env_for_revision_with_current_aliases(
             provider_env_for_current,
             credential_expires_at_ms_for_current,
             revision,
             true,
+            suppress_from_child_env,
         );
         (child_env, revision_resolver, current_resolver)
     }
@@ -168,6 +180,7 @@ impl SecretResolver {
         credential_expires_at_ms: HashMap<String, i64>,
         revision: u64,
         include_current_aliases: bool,
+        suppress_from_child_env: &HashSet<String>,
     ) -> (HashMap<String, String>, Option<Self>) {
         if provider_env.is_empty() {
             return (HashMap::new(), None);
@@ -185,7 +198,9 @@ impl SecretResolver {
                     .copied()
                     .unwrap_or_default(),
             };
-            child_env.insert(key.clone(), placeholder.clone());
+            if !suppress_from_child_env.contains(&key) {
+                child_env.insert(key.clone(), placeholder.clone());
+            }
             by_placeholder.insert(placeholder, secret.clone());
             if include_current_aliases && revision != 0 {
                 by_placeholder.insert(placeholder_for_env_key(&key), secret.clone());