feat(mcp-oauth): confidential pre-registered clients (client_secret_env) — REQ-044 Phase 2

crates/animus-mcp-oauth/src/config.rs

@@ -32,6 +32,10 @@ pub enum ServerResolutionError {
     WorkflowConfig(String),
     #[error("failed to load project config: {0}")]
     ProjectConfig(String),
+    /// Resolving the pinned confidential client secret (`client_secret_env`)
+    /// from the process env / project keychain failed.
+    #[error("failed to resolve OAuth client secret for MCP server `{0}`: {1}")]
+    ClientSecret(String, String),
 }
 
 /// Outcome of resolving a server's OAuth shape from config.
@@ -45,6 +49,10 @@ pub struct ServerResolution {
     pub scopes: Vec<String>,
     /// Pre-registered client id, if the config pins one (skips DCR).
     pub client_id: Option<String>,
+    /// Resolved confidential client secret for a pinned pre-registered app,
+    /// when the `authorization_code` oauth block sets `client_secret_env`.
+    /// `None` for public (PKCE-only) pinned clients and for DCR.
+    pub client_secret: Option<String>,
     /// True when the resolved server's oauth block is an
     /// `authorization_code` flow (vs absent / a machine-to-machine flow).
     pub is_authorization_code: bool,
@@ -130,15 +138,15 @@ pub fn resolve_server_url(
         }
     };
     if let Some(def) = loaded.as_ref().and_then(|loaded| loaded.config.mcp_servers.get(server)) {
-        return finalize(server, url_override, def.url.clone(), def.oauth.clone());
+        return finalize(project_root, server, url_override, def.url.clone(), def.oauth.clone());
     }
 
     // Project config mcp_servers next.
     let project_config = protocol::Config::load_or_default(&project_root.display().to_string())
         .map_err(|err| ServerResolutionError::ProjectConfig(err.to_string()))?;
     if let Some(entry) = project_config.mcp_servers.get(server) {
         let oauth = entry.oauth.as_ref().and_then(|value| serde_json::from_value::<OauthConfig>(value.clone()).ok());
-        return finalize(server, url_override, entry.url.clone(), oauth);
+        return finalize(project_root, server, url_override, entry.url.clone(), oauth);
     }
 
     // Not in config: only proceed if the user passed an explicit URL.
@@ -147,6 +155,7 @@ pub fn resolve_server_url(
             url: url.to_string(),
             scopes: Vec::new(),
             client_id: None,
+            client_secret: None,
             is_authorization_code: true,
             broker_oauth: None,
         }),
@@ -155,6 +164,7 @@ pub fn resolve_server_url(
 }
 
 fn finalize(
+    project_root: &Path,
     server: &str,
     url_override: Option<&str>,
     config_url: Option<String>,
@@ -165,30 +175,62 @@ fn finalize(
         .or(config_url)
         .ok_or_else(|| ServerResolutionError::MissingUrl(server.to_string()))?;
     match oauth {
-        Some(cfg) if cfg.flow == OauthFlow::AuthorizationCode => Ok(ServerResolution {
-            url,
-            scopes: cfg.scopes,
-            client_id: cfg.client_id,
-            is_authorization_code: true,
-            broker_oauth: None,
-        }),
+        Some(cfg) if cfg.flow == OauthFlow::AuthorizationCode => {
+            let client_secret = resolve_client_secret(project_root, server, &cfg)?;
+            Ok(ServerResolution {
+                url,
+                scopes: cfg.scopes,
+                client_id: cfg.client_id,
+                client_secret,
+                is_authorization_code: true,
+                broker_oauth: None,
+            })
+        }
         Some(cfg) => Ok(ServerResolution {
             url,
             scopes: Vec::new(),
             client_id: None,
+            client_secret: None,
             is_authorization_code: false,
             broker_oauth: Some(cfg),
         }),
         None => Ok(ServerResolution {
             url,
             scopes: Vec::new(),
             client_id: None,
+            client_secret: None,
             is_authorization_code: false,
             broker_oauth: None,
         }),
     }
 }
 
+/// Resolve the confidential pre-registered client secret for an
+/// `authorization_code` server, if its oauth block pins one via
+/// `client_secret_env`. The name is looked up in the process environment first
+/// (explicit parent env wins, matching the `${VAR}`/keychain precedence used
+/// elsewhere), then the project keychain the `animus secret` surface writes to.
+/// Returns `None` when no `client_secret_env` is set or it resolves to empty —
+/// a public (PKCE-only) pinned client or DCR.
+fn resolve_client_secret(
+    project_root: &Path,
+    server: &str,
+    cfg: &OauthConfig,
+) -> Result<Option<String>, ServerResolutionError> {
+    let Some(name) = cfg.client_secret_env.as_deref().map(str::trim).filter(|s| !s.is_empty()) else {
+        return Ok(None);
+    };
+    if let Ok(value) = std::env::var(name) {
+        if !value.is_empty() {
+            return Ok(Some(value));
+        }
+    }
+    let store = build_secret_store(project_root)?;
+    let value =
+        store.get(name).map_err(|err| ServerResolutionError::ClientSecret(server.to_string(), err.to_string()))?;
+    Ok(value.filter(|v| !v.is_empty()))
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/animus-mcp-oauth/src/flow.rs

@@ -448,7 +448,14 @@ pub async fn run_auth(project_root: &Path, server: &str, opts: RunAuthOptions<'_
     //   pinned id, failing on servers that don't support DCR.
     // - No pinned id: `AuthorizationSession::new` performs DCR.
     let (auth_url, csrf_state, exchange) = if let Some(client_id) = pinned_client_id {
-        let config = OAuthClientConfig::new(client_id.to_string(), redirect_uri.clone()).with_scopes(scopes.clone());
+        let mut config =
+            OAuthClientConfig::new(client_id.to_string(), redirect_uri.clone()).with_scopes(scopes.clone());
+        // Confidential pre-registered app: attach the resolved client_secret so
+        // the token exchange authenticates as that app (public/PKCE clients
+        // leave this unset).
+        if let Some(secret) = resolution.client_secret.as_deref() {
+            config = config.with_client_secret(secret.to_string());
+        }
         manager
             .configure_client(config)
             .map_err(|err| anyhow!("failed to configure pinned client_id for `{server}`: {err}"))?;
@@ -608,11 +615,18 @@ pub async fn begin_auth(project_root: &Path, server: &str, opts: BeginOptions<'_
     // still return a confidential client; carry the secret so completion can
     // authenticate the exchange with the same client begin registered.
     let (client_id, client_secret) = if let Some(id) = pinned_client_id {
-        let config = OAuthClientConfig::new(id.to_string(), redirect_uri.clone()).with_scopes(scopes.clone());
+        let mut config = OAuthClientConfig::new(id.to_string(), redirect_uri.clone()).with_scopes(scopes.clone());
+        // Confidential pre-registered app: carry the resolved client_secret into
+        // the pending record so `complete_auth` authenticates the exchange as the
+        // same app. Public/PKCE pinned clients leave it `None`.
+        let pinned_secret = resolution.client_secret.clone();
+        if let Some(secret) = pinned_secret.as_deref() {
+            config = config.with_client_secret(secret.to_string());
+        }
         manager
             .configure_client(config)
             .map_err(|err| anyhow!("failed to configure pinned client_id for `{server}`: {err}"))?;
-        (id.to_string(), None)
+        (id.to_string(), pinned_secret)
     } else {
         // register_client runs DCR and internally calls configure_client.
         let config = manager

crates/orchestrator-config/src/workflow_config/tests.rs

@@ -3411,6 +3411,80 @@ fn validation_rejects_authorization_code_with_blank_client_id() {
     assert!(err.to_string().contains("client_id"), "should reject blank client_id: {err}");
 }
 
+#[test]
+fn validation_accepts_authorization_code_with_pinned_client_and_secret_env() {
+    // A confidential pre-registered app (REQ-044 Phase 2): a pinned client_id
+    // plus a client_secret_env is the supported confidential shape.
+    let mut config = builtin_workflow_config();
+    config.mcp_servers.insert(
+        "hubspot".to_string(),
+        http_oauth_server(OauthConfig {
+            flow: OauthFlow::AuthorizationCode,
+            token_url: None,
+            client_id_env: None,
+            client_secret_env: Some("HUBSPOT_CLIENT_SECRET".to_string()),
+            refresh_token_env: None,
+            bearer_env: None,
+            scopes: vec![],
+            audience: None,
+            cache: true,
+            client_id: Some("app-123".to_string()),
+        }),
+    );
+    validate_workflow_config(&config)
+        .expect("authorization_code with a pinned client_id + client_secret_env should validate");
+}
+
+#[test]
+fn validation_rejects_authorization_code_secret_env_without_client_id() {
+    // A client_secret_env with no pinned client_id makes no sense: DCR mints the
+    // client, so there is nothing to attach the secret to.
+    let mut config = builtin_workflow_config();
+    config.mcp_servers.insert(
+        "hubspot".to_string(),
+        http_oauth_server(OauthConfig {
+            flow: OauthFlow::AuthorizationCode,
+            token_url: None,
+            client_id_env: None,
+            client_secret_env: Some("HUBSPOT_CLIENT_SECRET".to_string()),
+            refresh_token_env: None,
+            bearer_env: None,
+            scopes: vec![],
+            audience: None,
+            cache: true,
+            client_id: None,
+        }),
+    );
+    let err = validate_workflow_config(&config)
+        .expect_err("client_secret_env without a pinned client_id must fail validation");
+    let msg = err.to_string();
+    assert!(msg.contains("client_secret_env"), "should name client_secret_env: {msg}");
+    assert!(msg.contains("client_id"), "should require a pinned client_id: {msg}");
+}
+
+#[test]
+fn validation_rejects_authorization_code_blank_secret_env() {
+    // A blank client_secret_env name is a typo — reject it even with a client_id.
+    let mut config = builtin_workflow_config();
+    config.mcp_servers.insert(
+        "hubspot".to_string(),
+        http_oauth_server(OauthConfig {
+            flow: OauthFlow::AuthorizationCode,
+            token_url: None,
+            client_id_env: None,
+            client_secret_env: Some("   ".to_string()),
+            refresh_token_env: None,
+            bearer_env: None,
+            scopes: vec![],
+            audience: None,
+            cache: true,
+            client_id: Some("app-123".to_string()),
+        }),
+    );
+    let err = validate_workflow_config(&config).expect_err("blank client_secret_env must fail validation");
+    assert!(err.to_string().contains("client_secret_env"), "should reject blank client_secret_env: {err}");
+}
+
 #[test]
 fn validation_accepts_oauth_client_credentials() {
     let mut config = builtin_workflow_config();

crates/orchestrator-config/src/workflow_config/validation.rs

@@ -427,13 +427,13 @@ fn validate_oauth_config(server_name: &str, transport: Option<&str>, oauth: &Oau
             }
         }
         OauthFlow::AuthorizationCode => {
-            // Discovery + DCR fill in the token/authorization endpoints, so
-            // none of the machine-to-machine `*_env` fields apply. Reject
-            // them so a misplaced credential pointer surfaces at validation
-            // time rather than being silently ignored.
+            // Discovery + DCR fill in the token/authorization endpoints, so the
+            // machine-to-machine `*_env` fields don't apply — EXCEPT
+            // `client_secret_env`, which pins the secret of a CONFIDENTIAL
+            // pre-registered app (handled below). Reject the rest so a misplaced
+            // credential pointer surfaces at validation time.
             for (value, name) in [
                 (&oauth.client_id_env, "client_id_env"),
-                (&oauth.client_secret_env, "client_secret_env"),
                 (&oauth.refresh_token_env, "refresh_token_env"),
                 (&oauth.bearer_env, "bearer_env"),
                 (&oauth.token_url, "token_url"),
@@ -447,9 +447,30 @@ fn validate_oauth_config(server_name: &str, transport: Option<&str>, oauth: &Oau
             // skip Dynamic Client Registration with an empty id, failing later
             // at the browser/token-exchange step. Reject it here. Omit the key
             // entirely to use DCR.
+            let has_pinned_client_id = oauth.client_id.as_ref().is_some_and(|id| !id.trim().is_empty());
             if oauth.client_id.as_ref().is_some_and(|id| id.trim().is_empty()) {
                 errors.push(format!("{} must not be blank for flow=\"authorization_code\"", field("client_id")));
             }
+            // `client_secret_env` names the keychain/env entry holding the secret
+            // of a CONFIDENTIAL pre-registered app (providers like HubSpot that
+            // require a `client_secret` at token exchange). It only makes sense
+            // alongside a pinned `client_id` — there is no client to attach a
+            // secret to under DCR — and the name itself must be non-blank.
+            if let Some(secret_env) = oauth.client_secret_env.as_ref() {
+                if secret_env.trim().is_empty() {
+                    errors.push(format!(
+                        "{} must not be blank for flow=\"authorization_code\"",
+                        field("client_secret_env")
+                    ));
+                }
+                if !has_pinned_client_id {
+                    errors.push(format!(
+                        "{} requires a pinned {} for flow=\"authorization_code\"",
+                        field("client_secret_env"),
+                        field("client_id")
+                    ));
+                }
+            }
         }
     }
 }

docs/reference/mcp-oauth.md

@@ -52,6 +52,8 @@ mcp_servers:
         - repo
         - read:user
       client_id: my-pre-registered-client      # skip Dynamic Client Registration
+      # optional — only for a CONFIDENTIAL pre-registered app (requires client_id):
+      client_secret_env: MY_APP_CLIENT_SECRET   # names a keychain/env secret
 ```
 
 Fields:
@@ -62,11 +64,14 @@ Fields:
 | `url` | yes | Upstream HTTP MCP endpoint; also the OAuth `resource` indicator (RFC 8707), the discovery seed (RFC 9728 protected-resource metadata → authorization server), and the proxy target |
 | `scopes` | no | Requested at authorization. **When omitted, Animus auto-detects the server's advertised `scopes_supported`** from discovery metadata and requests those (see "Scope posture" below); if the server advertises none, nothing is requested and it applies its own minimal default. Set this to pin a specific (narrower) scope set, or `[none]` to force no scopes (opt out of auto-detection). |
 | `client_id` | no | Pre-registered client id; when omitted, Dynamic Client Registration (RFC 7591) is used |
-
-The machine-to-machine credential pointers (`token_url`, `client_id_env`,
-`client_secret_env`, `refresh_token_env`, `bearer_env`) must **not** be set on
-an `authorization_code` server — discovery fills those endpoints in, and
-validation rejects them if present.
+| `client_secret_env` | no | For a **confidential** pre-registered app (e.g. HubSpot) that requires a `client_secret` at token exchange. Names the env var / keychain entry (`animus secret set <NAME>`) holding the secret — resolved from the process env first, then the project keychain. Requires a pinned `client_id`; leave unset for public (PKCE-only) clients and DCR. |
+
+The remaining machine-to-machine credential pointers (`token_url`,
+`client_id_env`, `refresh_token_env`, `bearer_env`) must **not** be set on an
+`authorization_code` server — discovery fills those endpoints in, and validation
+rejects them if present. `client_secret_env` is the one exception (see above):
+it pins the secret of a confidential pre-registered app and is only valid
+alongside a `client_id`.
 
 ## CLI