From 775a3e7a3778957f12d3c95000c414a2d6887de8 Mon Sep 17 00:00:00 2001 From: latenighthackathon Date: Tue, 12 May 2026 20:34:31 -0500 Subject: [PATCH] fix(grpc): allow credential rotation when legacy provider.type exceeds current limit Closes #1347. Reported by @KodeDaemon in the NVIDIA Developer Discord after upgrading NemoClaw 0.0.38 -> 0.0.39 left the inference provider stranded with `provider.type exceeds maximum length (79 > 64)` on every credential update. `update_provider_record` rebuilt the merged Provider from `existing.r#type` and ran `validate_provider_fields` over it. The CLI's `provider update` sends `r#type: ""`, so the existing value is preserved without modification, but the validator still measured it. Any record whose stored type predates current MAX_PROVIDER_TYPE_LEN (or was written by a path that bypassed validation, e.g. the TUI form which forwards r#type to the request without going through normalize_provider_type) became unupdateable: the only escape was `provider delete` + recreate, which loses any provider.config entries the caller never sees. Split the validator into create-time (validate_provider_fields, full check including immutable name/type) and update-time (validate_provider_mutable_fields, checks only credentials/config maps). The update path validates only what the caller is mutating; immutable fields carried forward from existing are trusted because they passed validation when stored. Tests: new regression in `update_provider_allows_credential_rotation_on_legacy_oversized_type` inserts a Provider with a 79-char type directly via `store.put_message` (bypassing gRPC validation), updates a credential, asserts the update succeeds and the original type is preserved. Existing `update_provider_validates_merged_result` still covers the case where incoming credential entries push the merged map past limits. Signed-off-by: latenighthackathon --- crates/openshell-server/src/grpc/provider.rs | 59 +++++++++++++++++-- .../openshell-server/src/grpc/validation.rs | 13 +++- 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index 3ddaae037..ba8471dcf 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -13,7 +13,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_MAP_KEY_LEN, MAX_MAP_VALUE_LEN, MAX_PAGE_SIZE, MAX_PROVIDER_CONFIG_ENTRIES, clamp_limit, }; @@ -215,9 +215,14 @@ pub(super) async fn update_provider_record( provider.credential_expires_at_ms, ); - // Validate BEFORE writing to prevent persisting invalid state + // Validate BEFORE writing to prevent persisting invalid state. + // Validate only the mutable fields (credentials/config) plus metadata and + // attached-sandbox invariants. The immutable name/type are carried forward + // from `existing` and re-running full `validate_provider_fields` here would + // strand legacy records whose stored type predates current limits. See + // #1347. super::validation::validate_object_metadata(candidate.metadata.as_ref(), "provider")?; - validate_provider_fields(&candidate)?; + validate_provider_mutable_fields(&candidate)?; validate_provider_update_against_attached_sandboxes(store, &candidate).await?; // Serialize labels for storage @@ -1443,8 +1448,8 @@ pub(super) async fn handle_delete_provider( #[cfg(test)] mod tests { use super::*; - use crate::grpc::MAX_MAP_KEY_LEN; use crate::grpc::test_support::test_server_state; + use crate::grpc::{MAX_MAP_KEY_LEN, MAX_PROVIDER_TYPE_LEN}; async fn test_store() -> Store { Store::connect("sqlite::memory:?cache=shared") @@ -3267,6 +3272,52 @@ 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 = test_store().await; + + 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(), + resource_version: 0, + }), + r#type: oversized_type.clone(), + credentials: std::iter::once(("API_TOKEN".to_string(), "old".to_string())).collect(), + config: HashMap::new(), + credential_expires_at_ms: 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(), + resource_version: 0, + }), + r#type: String::new(), + credentials: std::iter::once(("API_TOKEN".to_string(), "new".to_string())) + .collect(), + config: HashMap::new(), + credential_expires_at_ms: HashMap::new(), + }, + ) + .await + .unwrap(); + + assert_eq!(updated.r#type, oversized_type); + } + #[tokio::test] async fn resolve_provider_env_empty_list_returns_empty() { let store = test_store().await; diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index 53f292053..83658ef7b 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,