diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index 2ed4d439d..4aeb7091e 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -11,7 +11,7 @@ use prost::Message; use tonic::Status; use tracing::warn; -use super::validation::validate_provider_fields; +use super::validation::{validate_provider_fields, validate_provider_mutable_fields}; use super::{MAX_PAGE_SIZE, clamp_limit}; // --------------------------------------------------------------------------- @@ -160,7 +160,11 @@ pub(super) async fn update_provider_record( // Ensure metadata is valid (defense in depth - existing.metadata should always be valid) super::validation::validate_object_metadata(updated.metadata.as_ref(), "provider")?; - validate_provider_fields(&updated)?; + // Validate only the mutable fields (credentials/config). The immutable + // name/type are carried forward from `existing` and re-checking them would + // strand legacy records whose stored type predates current limits. See + // #1347. + validate_provider_mutable_fields(&updated)?; store .put_message(&updated) @@ -755,7 +759,7 @@ mod tests { use super::*; use crate::ServerState; use crate::compute::new_test_runtime; - use crate::grpc::MAX_MAP_KEY_LEN; + use crate::grpc::{MAX_MAP_KEY_LEN, MAX_PROVIDER_TYPE_LEN}; use crate::sandbox_index::SandboxIndex; use crate::sandbox_watch::SandboxWatchBus; use crate::supervisor_session::SupervisorSessionRegistry; @@ -1732,6 +1736,50 @@ mod tests { assert_eq!(err.code(), Code::InvalidArgument); } + /// Regression for #1347: a credential rotation must not fail just because the + /// existing record's `type` field exceeds the current `MAX_PROVIDER_TYPE_LEN`. + /// The caller never touches `type`; re-validating it strands legacy records. + #[tokio::test] + async fn update_provider_allows_credential_rotation_on_legacy_oversized_type() { + let store = Store::connect("sqlite::memory:?cache=shared") + .await + .unwrap(); + + let oversized_type = "x".repeat(MAX_PROVIDER_TYPE_LEN + 15); + let legacy = Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: uuid::Uuid::new_v4().to_string(), + name: "legacy-oversized-type".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + }), + r#type: oversized_type.clone(), + credentials: std::iter::once(("API_TOKEN".to_string(), "old".to_string())).collect(), + config: HashMap::new(), + }; + store.put_message(&legacy).await.unwrap(); + + let updated = update_provider_record( + &store, + Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "legacy-oversized-type".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + }), + r#type: String::new(), + credentials: std::iter::once(("API_TOKEN".to_string(), "new".to_string())) + .collect(), + config: HashMap::new(), + }, + ) + .await + .unwrap(); + + assert_eq!(updated.r#type, oversized_type); + } + #[tokio::test] async fn resolve_provider_env_empty_list_returns_empty() { let store = Store::connect("sqlite::memory:").await.unwrap(); diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index 160b7e031..e7af40d8c 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -239,7 +239,7 @@ pub(super) fn validate_string_map( // Provider field validation // --------------------------------------------------------------------------- -/// Validate field sizes on a `Provider` before persisting. +/// Validate field sizes on a `Provider` before persisting a new record. pub(super) fn validate_provider_fields(provider: &Provider) -> Result<(), Status> { let name_len = provider.metadata.as_ref().map_or(0, |m| m.name.len()); if name_len > MAX_NAME_LEN { @@ -253,6 +253,17 @@ pub(super) fn validate_provider_fields(provider: &Provider) -> Result<(), Status provider.r#type.len() ))); } + validate_provider_mutable_fields(provider) +} + +/// Validate field sizes on a `Provider` before persisting an update. +/// +/// Skips the immutable `name` and `type` fields, which are carried forward from +/// the existing record. Re-checking them would block credential rotation on any +/// legacy record whose stored `name`/`type` predates current limits (or was +/// written by a path that bypassed validation), even though the caller never +/// touches those fields. See #1347. +pub(super) fn validate_provider_mutable_fields(provider: &Provider) -> Result<(), Status> { validate_string_map( &provider.credentials, MAX_PROVIDER_CREDENTIALS_ENTRIES,