From d0de30e70954375c8818f216f55ac55d94e4f360 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Thu, 23 Jul 2026 22:06:36 -0500 Subject: [PATCH 1/7] fix(core): avoid i64 underflow on expired GCP token When a cached GCP access token was already expired (expires_at_ms < now), the remaining lifetime calculation used plain subtraction: (expires_at_ms - now) / 1000. This underflows in debug builds and wraps in release builds. Use saturating_sub so an expired token simply yields a non-positive remaining lifetime and is skipped. Add a regression test. Signed-off-by: Andrew White --- .../src/provider_credentials.rs | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/crates/openshell-core/src/provider_credentials.rs b/crates/openshell-core/src/provider_credentials.rs index 65310b3a43..d0b7b38ad5 100644 --- a/crates/openshell-core/src/provider_credentials.rs +++ b/crates/openshell-core/src/provider_credentials.rs @@ -243,7 +243,7 @@ impl ProviderCredentialState { DEFAULT_EXPIRES_IN } else { let now = crate::time::now_ms(); - (expires_at_ms - now) / 1000 + expires_at_ms.saturating_sub(now) / 1000 } }, ); @@ -587,6 +587,27 @@ mod tests { ); } + #[test] + fn gcp_token_response_handles_already_expired_token_without_panic() { + let now_ms = i64::try_from( + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_millis(), + ) + .unwrap(); + let state = ProviderCredentialState::from_environment( + 1, + HashMap::from([("GCP_ADC_ACCESS_TOKEN".to_string(), "adc-tok".to_string())]), + HashMap::from([("GCP_ADC_ACCESS_TOKEN".to_string(), now_ms - 1_000)]), + HashMap::new(), + ); + assert!( + state.gcp_token_response().is_none(), + "expired token should be skipped rather than panic" + ); + } + #[test] fn child_env_with_gcp_resolved_resolves_vertex_vars_without_metadata_host() { let state = ProviderCredentialState::from_environment( From 758e969673ee4c2096cf24fc934638c8ce6ad4d3 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Thu, 23 Jul 2026 22:06:36 -0500 Subject: [PATCH 2/7] fix(server): avoid i64 underflow in lease age calculation The lease steal path computed age as now_ms() - record.updated_at_ms. If the stored updated_at timestamp is in the future (e.g. clock skew), the subtraction underflows. Use saturating_sub to treat a future timestamp as age 0, keeping the lease considered fresh. Signed-off-by: Andrew White --- crates/openshell-server/src/compute/lease.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/openshell-server/src/compute/lease.rs b/crates/openshell-server/src/compute/lease.rs index 1de4de4734..e51ee67b22 100644 --- a/crates/openshell-server/src/compute/lease.rs +++ b/crates/openshell-server/src/compute/lease.rs @@ -121,7 +121,7 @@ impl ReconcilerLease { pub async fn try_steal_expired(&self) -> Result { let record = self.read().await?.ok_or(LeaseError::NotFound)?; - let age_ms = now_ms() - record.updated_at_ms; + let age_ms = now_ms().saturating_sub(record.updated_at_ms); let ttl_ms = i64::try_from(self.ttl.as_millis()).unwrap_or(i64::MAX); if age_ms < ttl_ms { return Err(LeaseError::AlreadyHeld); From b3c0572ebfbd69e5621fe4c6726b0742a92af16c Mon Sep 17 00:00:00 2001 From: Andrew White Date: Thu, 23 Jul 2026 22:06:36 -0500 Subject: [PATCH 3/7] fix(server): avoid i64 overflow on large credential max lifetime max_lifetime_seconds is only validated as >= 0. The code multiplied the raw i64 value by 1000 to compute max_expires, which overflows for values above i64::MAX / 1000. The capped i32 value used for the STS request was already computed but not reused. Compute max_lifetime_ms from the capped value with saturating_mul and use it for both expires_at fallback and max_expires. Signed-off-by: Andrew White --- crates/openshell-server/src/provider_refresh.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/openshell-server/src/provider_refresh.rs b/crates/openshell-server/src/provider_refresh.rs index f8fa867c1b..b51ea3b17e 100644 --- a/crates/openshell-server/src/provider_refresh.rs +++ b/crates/openshell-server/src/provider_refresh.rs @@ -769,6 +769,7 @@ async fn mint_aws_sts_assume_role( DEFAULT_MAX_LIFETIME_SECONDS }; let max_lifetime = i32::try_from(max_lifetime_i64.min(i64::from(i32::MAX))).unwrap_or(i32::MAX); + let max_lifetime_ms = i64::from(max_lifetime).saturating_mul(1000); let mut req = client .assume_role() @@ -797,8 +798,8 @@ async fn mint_aws_sts_assume_role( let expires_at_ms = creds .expiration() .to_millis() - .unwrap_or_else(|_| now_ms + max_lifetime_i64 * 1000); - let max_expires = now_ms + max_lifetime_i64 * 1000; + .unwrap_or_else(|_| now_ms + max_lifetime_ms); + let max_expires = now_ms + max_lifetime_ms; let expires_at_ms = expires_at_ms.min(max_expires); // Map STS response fields to the env keys the profile bound to each semantic From 9fa1cda4cd5f87bcbcd3feb01d725efd4a26a614 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Thu, 23 Jul 2026 22:06:36 -0500 Subject: [PATCH 4/7] fix(server): avoid hardcoded indexing into base_url_config_keys resolve_vertex_ai_route assumed every inference provider profile has at least two base URL config keys by indexing [0] and [1]. Only the Vertex profile satisfies that today; other profiles would panic here. Iterate base_url_config_keys with find_map instead, and add a test with an empty key list to ensure no panic. Signed-off-by: Andrew White --- crates/openshell-server/src/inference.rs | 26 +++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/crates/openshell-server/src/inference.rs b/crates/openshell-server/src/inference.rs index 97217c2565..94dff620b7 100644 --- a/crates/openshell-server/src/inference.rs +++ b/crates/openshell-server/src/inference.rs @@ -627,9 +627,10 @@ fn resolve_vertex_ai_route( // protocol and path contract, but only for the OpenAI-compatible Vertex surface. // Anthropic-on-Vertex needs model-path shaping and body adaptation that a fully // caller-controlled URL cannot safely preserve. - if let Some(base_url) = config - .get(profile.base_url_config_keys[0]) - .or_else(|| config.get(profile.base_url_config_keys[1])) + if let Some(base_url) = profile + .base_url_config_keys + .iter() + .find_map(|key| config.get(*key)) .map(String::as_str) .filter(|v| !v.trim().is_empty()) { @@ -1133,6 +1134,25 @@ mod tests { } } + #[test] + fn resolve_vertex_ai_route_handles_empty_base_url_keys() { + let config = HashMap::new(); + let profile = openshell_core::inference::InferenceProviderProfile { + provider_type: "google_vertex_ai", + default_base_url: "https://example.com", + protocols: &[], + credential_key_names: &[], + base_url_config_keys: &[], + auth: openshell_core::inference::AuthHeader::Bearer, + default_headers: &[], + passthrough_headers: &[], + }; + let result = resolve_vertex_ai_route(&config, "model-id", "route", "api-key", &profile); + // Empty base_url_config_keys must not panic. The route still errors because + // the minimal Vertex config is missing, so we only assert reachability. + assert!(result.is_err(), "expected missing config to error, got {result:?}"); + } + #[test] fn inference_bundle_requires_sandbox_principal() { let sandbox = test_sandbox_principal(); From 04804deff8df2e506c4f8371664ce953fce9d0a3 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Sat, 25 Jul 2026 18:55:29 -0500 Subject: [PATCH 5/7] fix(server): clamp lease age at zero for future timestamps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback: add regression coverage for future-timestamp lease clock skew. Extract lease_is_expired() and clamp the age at zero — i64::saturating_sub saturates at i64::MIN, not zero, so a future updated_at_ms must be clamped explicitly to be treated as age zero and remain unstealable with a positive TTL. Signed-off-by: Andrew White --- crates/openshell-server/src/compute/lease.rs | 37 ++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/crates/openshell-server/src/compute/lease.rs b/crates/openshell-server/src/compute/lease.rs index e51ee67b22..bf58fae48b 100644 --- a/crates/openshell-server/src/compute/lease.rs +++ b/crates/openshell-server/src/compute/lease.rs @@ -121,9 +121,9 @@ impl ReconcilerLease { pub async fn try_steal_expired(&self) -> Result { let record = self.read().await?.ok_or(LeaseError::NotFound)?; - let age_ms = now_ms().saturating_sub(record.updated_at_ms); + let current_ms = now_ms(); let ttl_ms = i64::try_from(self.ttl.as_millis()).unwrap_or(i64::MAX); - if age_ms < ttl_ms { + if !lease_is_expired(current_ms, record.updated_at_ms, ttl_ms) { return Err(LeaseError::AlreadyHeld); } @@ -252,6 +252,17 @@ pub fn replica_id() -> String { .unwrap_or_else(|_| uuid::Uuid::new_v4().to_string()) } +/// True if a lease record's age exceeds its TTL. +/// +/// Clamps at zero so a stored `updated_at_ms` in the future (clock skew) +/// is treated as age zero and the lease is considered fresh. Note that +/// `i64::saturating_sub` alone is not sufficient here: it saturates at +/// `i64::MIN`, not zero. +fn lease_is_expired(now_ms: i64, updated_at_ms: i64, ttl_ms: i64) -> bool { + let age_ms = now_ms.saturating_sub(updated_at_ms).max(0); + age_ms >= ttl_ms +} + #[cfg(test)] mod tests { use super::*; @@ -351,6 +362,28 @@ mod tests { assert_eq!(record.resource_version, guard.resource_version); } + #[test] + fn future_updated_at_is_treated_as_age_zero() { + let now = 1_000_000; + let future_updated = now + 86_400_000; // 1 day in the future + assert!( + !lease_is_expired(now, future_updated, 1_000), + "future updated_at_ms should not be stealable with positive TTL" + ); + assert!( + lease_is_expired(now, future_updated, 0), + "TTL zero always expires regardless of timestamp" + ); + assert!( + !lease_is_expired(now, now, 1_000), + "fresh lease with positive TTL should not be expired" + ); + assert!( + lease_is_expired(now, now, 0), + "present timestamp with TTL zero should be expired" + ); + } + #[tokio::test] async fn release_allows_immediate_reacquire() { let store = test_store().await; From 26ba16ef33b6d9fe48b370a72e240f9d6f3fad0d Mon Sep 17 00:00:00 2001 From: Andrew White Date: Sat, 25 Jul 2026 18:55:29 -0500 Subject: [PATCH 6/7] fix(server): fall back past blank preferred Vertex base URL Address review feedback: find_map stopped at the first present key even when its value was blank, and the outer filter discarded it without considering lower-priority aliases. Filter blank values inside the closure so a blank preferred key falls back to a valid alias, matching the documented priority order. Add a regression test with a blank preferred key and a valid fallback. Signed-off-by: Andrew White --- crates/openshell-server/src/inference.rs | 45 ++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/crates/openshell-server/src/inference.rs b/crates/openshell-server/src/inference.rs index 94dff620b7..39d6afe2fd 100644 --- a/crates/openshell-server/src/inference.rs +++ b/crates/openshell-server/src/inference.rs @@ -630,9 +630,8 @@ fn resolve_vertex_ai_route( if let Some(base_url) = profile .base_url_config_keys .iter() - .find_map(|key| config.get(*key)) + .find_map(|key| config.get(*key).filter(|v| !v.trim().is_empty())) .map(String::as_str) - .filter(|v| !v.trim().is_empty()) { if is_anthropic { return Err(Status::invalid_argument( @@ -1150,7 +1149,47 @@ mod tests { let result = resolve_vertex_ai_route(&config, "model-id", "route", "api-key", &profile); // Empty base_url_config_keys must not panic. The route still errors because // the minimal Vertex config is missing, so we only assert reachability. - assert!(result.is_err(), "expected missing config to error, got {result:?}"); + assert!( + result.is_err(), + "expected missing config to error, got {result:?}" + ); + } + + #[test] + fn resolve_vertex_ai_route_skips_blank_preferred_for_fallback() { + let mut config = HashMap::new(); + config.insert("GOOGLE_VERTEX_AI_BASE_URL".to_string(), " ".to_string()); + config.insert( + "VERTEX_AI_BASE_URL".to_string(), + "https://us-central1-aiplatform.googleapis.com".to_string(), + ); + config.insert( + VERTEX_AI_PROJECT_ID_KEY.to_string(), + "my-project".to_string(), + ); + config.insert(VERTEX_AI_REGION_KEY.to_string(), "us-central1".to_string()); + + let profile = openshell_core::inference::InferenceProviderProfile { + provider_type: "google-vertex-ai", + default_base_url: "", + protocols: &[], + credential_key_names: &[], + base_url_config_keys: &["GOOGLE_VERTEX_AI_BASE_URL", "VERTEX_AI_BASE_URL"], + auth: openshell_core::inference::AuthHeader::Bearer, + default_headers: &[], + passthrough_headers: &[], + }; + + let result = resolve_vertex_ai_route(&config, "model-id", "route", "api-key", &profile); + assert!( + result.is_ok(), + "blank preferred key should fall back to valid alias: {result:?}" + ); + let route = result.unwrap(); + assert_eq!( + route.endpoint, + "https://us-central1-aiplatform.googleapis.com" + ); } #[test] From 296e1b5ddcc05ba22d97647eccfd01ab990cecdc Mon Sep 17 00:00:00 2001 From: Andrew White Date: Sat, 25 Jul 2026 18:55:29 -0500 Subject: [PATCH 7/7] fix(server): saturate AWS STS expiry cap on saturated clock Address review feedback: now_ms + max_lifetime_ms could still overflow when current_time_ms() saturates near i64::MAX. Compute max_expires = now_ms.saturating_add(max_lifetime_ms) once and use it for both the conversion fallback and the expiry cap. Add a boundary unit test covering a saturated clock. Signed-off-by: Andrew White --- crates/openshell-server/src/provider_refresh.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/crates/openshell-server/src/provider_refresh.rs b/crates/openshell-server/src/provider_refresh.rs index b51ea3b17e..a51ec53373 100644 --- a/crates/openshell-server/src/provider_refresh.rs +++ b/crates/openshell-server/src/provider_refresh.rs @@ -795,11 +795,8 @@ async fn mint_aws_sts_assume_role( let session_token = creds.session_token().to_string(); let now_ms = current_time_ms(); - let expires_at_ms = creds - .expiration() - .to_millis() - .unwrap_or_else(|_| now_ms + max_lifetime_ms); - let max_expires = now_ms + max_lifetime_ms; + let max_expires = now_ms.saturating_add(max_lifetime_ms); + let expires_at_ms = creds.expiration().to_millis().unwrap_or(max_expires); let expires_at_ms = expires_at_ms.min(max_expires); // Map STS response fields to the env keys the profile bound to each semantic @@ -1520,6 +1517,16 @@ mod tests { ); } + #[test] + fn aws_sts_max_expires_saturates_on_saturated_clock() { + assert_eq!(i64::MAX.saturating_add(3_600_000), i64::MAX); + let now_ms = i64::MAX - 1_000; + let max_lifetime_ms = 3_600_000; + let max_expires = now_ms.saturating_add(max_lifetime_ms); + assert_eq!(max_expires, i64::MAX); + assert!(max_expires >= now_ms); + } + #[tokio::test] async fn aws_sts_assume_role_mints_three_credentials_from_mock_endpoint() { let mock_server = MockServer::start().await;