From d2fe81747c5e22299a59201e0b3f08a7bc943b19 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 | 54 +++++++++++++++++-- .../openshell-server/src/grpc/validation.rs | 13 ++++- 2 files changed, 63 insertions(+), 4 deletions(-) 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,