From aacffea417d9ed6d603594f25016c2f54614a496 Mon Sep 17 00:00:00 2001 From: Nathan DeMoss Date: Tue, 1 Sep 2026 16:12:38 -0400 Subject: [PATCH 1/2] fix(cli): preserve provider type on credential updates provider_update built every UpdateProviderRequest with an empty type and profile workspace. Policy interceptors evaluate the request before the gateway merges it with stored state, so they cannot tell the target is a profile-managed provider and deny credential-only updates. Creating a provider from an interceptor-vended profile works; rotating its credentials does not. Look up the stored provider and forward its type and profile workspace verbatim. The gateway treats profile_workspace as immutable, so the stored value is passed through rather than recomputed from the request workspace. The read is best-effort: a caller holding provider:write without provider:read must still be able to rotate credentials, so a denied read falls back to the previous empty metadata rather than failing the update. --from-existing and --from-oidc-token need the stored type, so they surface the error instead. This also removes a duplicate GetProvider call, since the oidc-token and from-existing paths each fetched the same provider separately. Signed-off-by: Nathan DeMoss --- crates/openshell-cli/src/run.rs | 76 +++++++++++-------- .../tests/provider_commands_integration.rs | 53 +++++++++++++ 2 files changed, 98 insertions(+), 31 deletions(-) diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index ea495d5c04..485848611a 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -4962,17 +4962,39 @@ pub async fn provider_update(options: ProviderUpdateOptions<'_>) -> Result<()> { } let mut client = grpc_client(server, tls).await?; + + // Look up the stored provider so the update can carry its type and profile + // workspace. Policy interceptors evaluate the request before the gateway + // merges it with stored state, so an update that omits them cannot be + // authorized against the profile that owns the provider. + // + // The read is best-effort. A caller holding `provider:write` without + // `provider:read` must still be able to rotate credentials, so a denied + // read keeps the previous behavior of sending empty metadata rather than + // failing the update. `--from-existing` and `--from-oidc-token` need the + // stored type, so they surface the error instead. + let existing = match client + .get_provider(GetProviderRequest { + name: name.to_string(), + workspace: workspace.to_string(), + }) + .await + { + Ok(response) => response.into_inner().provider, + Err(status) + if status.code() == Code::PermissionDenied && !from_existing && !from_oidc_token => + { + None + } + Err(status) => return Err(status).into_diagnostic(), + }; + + if existing.is_none() && (from_existing || from_oidc_token) { + return Err(miette::miette!("provider '{name}' not found")); + } + let oidc_profile = if from_oidc_token { - let existing = client - .get_provider(GetProviderRequest { - name: name.to_string(), - workspace: workspace.to_string(), - }) - .await - .into_diagnostic()? - .into_inner() - .provider - .ok_or_else(|| miette::miette!("provider '{name}' not found"))?; + let existing = existing.as_ref().expect("checked above"); Some( fetch_provider_profile(&mut client, &existing.r#type, &existing.profile_workspace) .await?, @@ -4991,25 +5013,11 @@ pub async fn provider_update(options: ProviderUpdateOptions<'_>) -> Result<()> { credential_expires_at_ms.extend(oidc_credential_expires_at_ms); if from_existing { - // Fetch the existing provider to discover its type for credential lookup. - let existing = client - .get_provider(GetProviderRequest { - name: name.to_string(), - workspace: workspace.to_string(), - }) - .await - .into_diagnostic()? - .into_inner() - .provider - .ok_or_else(|| miette::miette!("provider '{name}' not found"))?; - - let provider_type = existing.r#type; - let discovered = discover_existing_provider_data( - &mut client, - &provider_type, - &existing.profile_workspace, - ) - .await?; + let stored = existing.as_ref().expect("checked above"); + let provider_type = stored.r#type.clone(); + let discovered = + discover_existing_provider_data(&mut client, &provider_type, &stored.profile_workspace) + .await?; let Some(discovered) = discovered else { return Err(miette::miette!( "no existing local credentials/config found for provider type '{provider_type}'" @@ -5037,11 +5045,17 @@ pub async fn provider_update(options: ProviderUpdateOptions<'_>) -> Result<()> { workspace: workspace.to_string(), deletion_timestamp_ms: 0, }), - r#type: String::new(), + r#type: existing + .as_ref() + .map(|provider| provider.r#type.clone()) + .unwrap_or_default(), credentials: credential_map, config: config_map, credential_expires_at_ms: HashMap::new(), - profile_workspace: String::new(), + profile_workspace: existing + .as_ref() + .map(|provider| provider.profile_workspace.clone()) + .unwrap_or_default(), credential_handles: HashMap::new(), }), credential_expires_at_ms, diff --git a/crates/openshell-cli/tests/provider_commands_integration.rs b/crates/openshell-cli/tests/provider_commands_integration.rs index d198c1e37a..5396205a11 100644 --- a/crates/openshell-cli/tests/provider_commands_integration.rs +++ b/crates/openshell-cli/tests/provider_commands_integration.rs @@ -1202,6 +1202,59 @@ async fn install_test_profile(ts: &TestServer, id: &str, credential_key: &str) { ); } +/// A readable provider must carry its stored type and profile workspace into +/// the update request. Policy interceptors evaluate the request before the +/// gateway merges it with stored state, so an update that omits them cannot be +/// authorized against the profile that owns the provider. +/// +/// The stored `profile_workspace` is forwarded verbatim rather than recomputed +/// from the request workspace. The gateway treats it as immutable, so deriving +/// it here would look like a change and be rejected. +#[tokio::test] +async fn provider_update_preserves_stored_type_when_readable() { + let ts = run_server().await; + + run::provider_create( + &ts.endpoint, + "my-claude", + "claude", + false, + &["API_KEY=abc".to_string()], + false, + &[], + "default", + &ts.tls, + ) + .await + .expect("provider create"); + + run::provider_update(run::ProviderUpdateOptions { + server: &ts.endpoint, + name: "my-claude", + from_existing: false, + from_oidc_token: false, + credentials: &["API_KEY=rotated".to_string()], + config: &[], + credential_expires_at: &[], + workspace: "default", + tls: &ts.tls, + }) + .await + .expect("provider update"); + + let requests = ts.state.provider_update_requests.lock().await; + let request = requests.last().expect("provider update request"); + // `claude` normalizes to the canonical `claude-code` at creation, so the + // update carries the stored type rather than the alias the caller typed. + assert_eq!(request.r#type, "claude-code"); + // Forwarded verbatim rather than recomputed. The gateway treats + // profile_workspace as immutable, so any substitution here would look like + // a change and be rejected. + let stored = ts.state.providers.lock().await; + let stored = stored.get("my-claude").expect("stored provider"); + assert_eq!(request.profile_workspace, stored.profile_workspace); +} + #[tokio::test] async fn provider_cli_run_functions_support_full_crud_flow() { let ts = run_server().await; From 2ce827fbd99f2125d1fc1b272bca99abd2f6c5af Mon Sep 17 00:00:00 2001 From: natedemoss Date: Tue, 1 Sep 2026 16:47:24 -0400 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- crates/openshell-cli/tests/provider_commands_integration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/openshell-cli/tests/provider_commands_integration.rs b/crates/openshell-cli/tests/provider_commands_integration.rs index 5396205a11..3849df7a69 100644 --- a/crates/openshell-cli/tests/provider_commands_integration.rs +++ b/crates/openshell-cli/tests/provider_commands_integration.rs @@ -1211,7 +1211,7 @@ async fn install_test_profile(ts: &TestServer, id: &str, credential_key: &str) { /// from the request workspace. The gateway treats it as immutable, so deriving /// it here would look like a change and be rejected. #[tokio::test] -async fn provider_update_preserves_stored_type_when_readable() { +async fn provider_update_preserves_stored_type_and_profile_workspace_when_readable() { let ts = run_server().await; run::provider_create(