From a7b953b3a5498dac73d7268e25375cb872cf443f Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 21 Jul 2026 23:19:13 -0500 Subject: [PATCH 1/9] fix(server): reject non-ASCII characters in Kubernetes label validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validate_label_key and validate_label_value used Unicode-aware char::is_alphanumeric(), so values like 'café' or '日本語' passed gateway validation even though the Kubernetes label spec (([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])? is ASCII-only and the API server rejects such labels later. The same functions already validate the key prefix with ASCII-only checks. Use is_ascii_alphanumeric() and add regression tests for Unicode keys and values. Signed-off-by: Andrew White --- .../openshell-server/src/grpc/validation.rs | 31 +++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index 1f1dfd257e..f8b0c0bdcc 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -513,7 +513,7 @@ pub(super) fn validate_label_key(key: &str) -> Result<(), Status> { // Name must contain only alphanumeric, hyphens, underscores, and dots if !name .chars() - .all(|c| c.is_alphanumeric() || c == '-' || c == '_' || c == '.') + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') { return Err(Status::invalid_argument(format!( "label key name segment contains invalid characters (must be alphanumeric, '-', '_', or '.'): '{key}'" @@ -523,12 +523,12 @@ pub(super) fn validate_label_key(key: &str) -> Result<(), Status> { // Name must start and end with alphanumeric let first = name.chars().next().unwrap(); // safe: we checked !is_empty() let last = name.chars().last().unwrap(); - if !first.is_alphanumeric() { + if !first.is_ascii_alphanumeric() { return Err(Status::invalid_argument(format!( "label key name segment must start with alphanumeric character: '{key}'" ))); } - if !last.is_alphanumeric() { + if !last.is_ascii_alphanumeric() { return Err(Status::invalid_argument(format!( "label key name segment must end with alphanumeric character: '{key}'" ))); @@ -604,7 +604,7 @@ pub(super) fn validate_label_value(value: &str) -> Result<(), Status> { // Must contain only alphanumeric, hyphens, underscores, and dots if !value .chars() - .all(|c| c.is_alphanumeric() || c == '-' || c == '_' || c == '.') + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_' || c == '.') { return Err(Status::invalid_argument(format!( "label value contains invalid characters (must be alphanumeric, '-', '_', or '.'): '{value}'" @@ -614,12 +614,12 @@ pub(super) fn validate_label_value(value: &str) -> Result<(), Status> { // Must start and end with alphanumeric let first = value.chars().next().unwrap(); // safe: we checked !is_empty() let last = value.chars().last().unwrap(); - if !first.is_alphanumeric() { + if !first.is_ascii_alphanumeric() { return Err(Status::invalid_argument(format!( "label value must start with alphanumeric character: '{value}'" ))); } - if !last.is_alphanumeric() { + if !last.is_ascii_alphanumeric() { return Err(Status::invalid_argument(format!( "label value must end with alphanumeric character: '{value}'" ))); @@ -1506,6 +1506,17 @@ mod tests { assert!(err.message().contains("invalid characters")); } + #[test] + fn validate_label_key_rejects_unicode_characters() { + // The Kubernetes label spec is ASCII-only; Unicode alphanumerics + // must not pass gateway validation. + let err = validate_label_key("日本語").unwrap_err(); + assert_eq!(err.code(), Code::InvalidArgument); + + let err = validate_label_key("café").unwrap_err(); + assert_eq!(err.code(), Code::InvalidArgument); + } + // ---- Label value validation ---- #[test] @@ -1573,6 +1584,14 @@ mod tests { assert!(err.message().contains("invalid characters")); } + #[test] + fn validate_label_value_rejects_unicode_characters() { + // The Kubernetes label spec is ASCII-only; Unicode alphanumerics + // must not pass gateway validation. + let err = validate_label_value("café").unwrap_err(); + assert_eq!(err.code(), Code::InvalidArgument); + } + // ---- Label selector validation ---- #[test] From 97c3f7b1ef2ba767ebc5f6a602bb357e4c4aa25a Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 21 Jul 2026 23:19:13 -0500 Subject: [PATCH 2/9] fix(cli): guard server-supplied string truncation against panics Two display paths in sandbox policy commands sliced server-supplied strings without guarding: - sandbox_policy_set used &resp.policy_hash[..12] unconditionally; an empty or short proto3 hash field would panic. Use the existing short_hash() helper, as nearby call sites already do. - The policy revision table truncated load_error at a fixed byte index (&rev.load_error[..40]), panicking on multi-byte UTF-8 in server error messages. Back off to a char boundary instead. Signed-off-by: Andrew White --- crates/openshell-cli/src/run.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 64fd550852..13c73b0dd2 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -5805,7 +5805,7 @@ pub async fn sandbox_policy_set( "{} Policy unchanged (version {}, hash: {})", "·".dimmed(), resp.version, - &resp.policy_hash[..12] + short_hash(&resp.policy_hash) ); return Ok(()); } @@ -5814,7 +5814,7 @@ pub async fn sandbox_policy_set( "{} Policy version {} submitted (hash: {})", "✓".green().bold(), resp.version, - &resp.policy_hash[..12] + short_hash(&resp.policy_hash) ); if !wait { @@ -6513,7 +6513,13 @@ fn print_policy_revision_table(revisions: &[openshell_core::proto::SandboxPolicy &rev.policy_hash }; let error_short = if rev.load_error.len() > 40 { - format!("{}...", &rev.load_error[..40]) + // Back off to a char boundary: byte-index slicing panics on + // multi-byte UTF-8 in server-supplied error messages. + let mut end = 40; + while !rev.load_error.is_char_boundary(end) { + end -= 1; + } + format!("{}...", &rev.load_error[..end]) } else { rev.load_error.clone() }; From 0372ce928e5c80a6860a35635753b4f484882658 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 21 Jul 2026 23:19:13 -0500 Subject: [PATCH 3/9] fix(sandbox): include port 49152 in ephemeral range note The IANA dynamic/private (ephemeral) port range is 49152-65535 inclusive, but the check used port > 49152, silently omitting the advisory note for port 49152 itself. Signed-off-by: Andrew White --- crates/openshell-sandbox/src/mechanistic_mapper.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/openshell-sandbox/src/mechanistic_mapper.rs b/crates/openshell-sandbox/src/mechanistic_mapper.rs index 8ee2fc37f9..f49004d895 100644 --- a/crates/openshell-sandbox/src/mechanistic_mapper.rs +++ b/crates/openshell-sandbox/src/mechanistic_mapper.rs @@ -308,7 +308,8 @@ fn generate_security_notes(host: &str, port: u16, is_ssrf: bool) -> String { } // High port numbers may indicate ephemeral services. - if port > 49152 { + // The IANA dynamic/private (ephemeral) range is 49152-65535 inclusive. + if port >= 49152 { notes.push(format!( "Port {port} is in the ephemeral range — \ this may be a temporary service." From 9df28ace4dccc2abe873c9ccab51b9fb4302edf2 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 28 Jul 2026 15:09:18 -0500 Subject: [PATCH 4/9] fix(cli): guard UTF-8 hash slicing and add boundary tests Addresses gator-agent review feedback on #2450: - Make short_hash() slice at the 13th character boundary so multi-byte UTF-8 cannot panic. - Use short_hash() in the policy revision table instead of byte-index slicing. - Extract error-message truncation into truncate_error_message() and keep char-boundary backoff. - Add regression tests for short_hash, truncate_error_message, and the ephemeral-port inclusive boundary (49152). Signed-off-by: Andrew White --- crates/openshell-cli/src/commands/common.rs | 32 ++++++++++- crates/openshell-cli/src/run.rs | 55 +++++++++++++------ .../src/mechanistic_mapper.rs | 9 +++ 3 files changed, 79 insertions(+), 17 deletions(-) diff --git a/crates/openshell-cli/src/commands/common.rs b/crates/openshell-cli/src/commands/common.rs index e6edb4d33a..b876af0d2a 100644 --- a/crates/openshell-cli/src/commands/common.rs +++ b/crates/openshell-cli/src/commands/common.rs @@ -174,7 +174,14 @@ pub fn truncate_display(value: &str, max_width: usize) -> String { } pub fn short_hash(hash: &str) -> &str { - if hash.len() >= 12 { &hash[..12] } else { hash } + if hash.chars().count() <= 12 { + return hash; + } + // Slice at the 13th character boundary so multi-byte UTF-8 cannot panic. + hash.char_indices() + .nth(12) + .map(|(idx, _)| &hash[..idx]) + .unwrap_or(hash) } pub fn non_empty_or<'a>(value: &'a str, fallback: &'a str) -> &'a str { @@ -975,4 +982,27 @@ mod tests { let err = parse_duration_to_ms("\u{20ac}").expect_err("missing number should error"); assert!(err.to_string().contains("invalid duration")); } + + #[test] + fn short_hash_returns_first_12_characters() { + assert_eq!(short_hash("abcdefghijkl"), "abcdefghijkl"); + assert_eq!(short_hash("abcdefghijklm"), "abcdefghijkl"); + } + + #[test] + fn short_hash_leaves_short_input_unchanged() { + assert_eq!(short_hash("abc"), "abc"); + assert_eq!(short_hash(""), ""); + } + + #[test] + fn short_hash_handles_multibyte_characters() { + // "aaaaaaaaaaaaé" is 14 bytes (12 'a' + one 2-byte 'é') and 13 chars. + // The old byte-slice at 12 would split 'é' and panic. + assert_eq!(short_hash("aaaaaaaaaaaaé"), "aaaaaaaaaaaa"); + // A 12-char hash ending in a multi-byte char is returned unchanged. + assert_eq!(short_hash("aaaaaaaaaaaé"), "aaaaaaaaaaaé"); + // All-multi-byte input still slices on a character boundary. + assert_eq!(short_hash("ééééééééééééé"), "éééééééééééé"); + } } diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 13c73b0dd2..54c2a8921b 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -6500,6 +6500,19 @@ pub async fn sandbox_policy_list_global( Ok(()) } +fn truncate_error_message(error: &str, max_bytes: usize) -> String { + if error.len() <= max_bytes { + return error.to_string(); + } + // Back off to a char boundary: byte-index slicing panics on multi-byte + // UTF-8 in server-supplied error messages. + let mut end = max_bytes; + while !error.is_char_boundary(end) { + end -= 1; + } + format!("{}...", &error[..end]) +} + fn print_policy_revision_table(revisions: &[openshell_core::proto::SandboxPolicyRevision]) { println!( "{:<8} {:<14} {:<12} {:<24} ERROR", @@ -6507,22 +6520,8 @@ fn print_policy_revision_table(revisions: &[openshell_core::proto::SandboxPolicy ); for rev in revisions { let status = PolicyStatus::try_from(rev.status).unwrap_or(PolicyStatus::Unspecified); - let hash_short = if rev.policy_hash.len() >= 12 { - &rev.policy_hash[..12] - } else { - &rev.policy_hash - }; - let error_short = if rev.load_error.len() > 40 { - // Back off to a char boundary: byte-index slicing panics on - // multi-byte UTF-8 in server-supplied error messages. - let mut end = 40; - while !rev.load_error.is_char_boundary(end) { - end -= 1; - } - format!("{}...", &rev.load_error[..end]) - } else { - rev.load_error.clone() - }; + let hash_short = short_hash(&rev.policy_hash); + let error_short = truncate_error_message(&rev.load_error, 40); println!( "{:<8} {:<14} {:<12} {:<24} {}", rev.version, @@ -8392,4 +8391,28 @@ mod tests { assert!(json["revision"].is_null()); assert!(json["policy"].is_null()); } + + #[test] + fn truncate_error_message_leaves_short_input_unchanged() { + assert_eq!( + super::truncate_error_message("short error", 40), + "short error" + ); + } + + #[test] + fn truncate_error_message_backs_off_to_char_boundary() { + // 39 'a' + one 2-byte 'é' means byte 40 splits the multibyte char. + let error = "a".repeat(39) + "é"; + let truncated = super::truncate_error_message(&error, 40); + assert!( + truncated.ends_with("..."), + "expected ellipsis, got {truncated:?}" + ); + assert!( + !truncated.contains("é"), + "expected char boundary backoff, got {truncated:?}" + ); + assert_eq!(truncated.len(), 39 + 3); // 39 ASCII chars + "..." + } } diff --git a/crates/openshell-sandbox/src/mechanistic_mapper.rs b/crates/openshell-sandbox/src/mechanistic_mapper.rs index f49004d895..b1b3fbea7f 100644 --- a/crates/openshell-sandbox/src/mechanistic_mapper.rs +++ b/crates/openshell-sandbox/src/mechanistic_mapper.rs @@ -473,6 +473,15 @@ mod tests { assert!(notes.contains("SSRF")); } + #[test] + fn test_security_notes_ephemeral_range_inclusive_boundary() { + // 49151 is just below the IANA ephemeral range; 49152 is the first + // ephemeral port and must be flagged. + assert!(!generate_security_notes("api.example.com", 49151, false).contains("ephemeral")); + assert!(generate_security_notes("api.example.com", 49152, false).contains("ephemeral")); + assert!(generate_security_notes("api.example.com", 65535, false).contains("ephemeral")); + } + #[test] fn test_security_notes_internal_ip_uses_canonical_classifier() { // RFC 1918 is 172.16.0.0/12 only: the old starts_with("172.") prefix From 877aee9151b0f62d042d37eeffa4d5008c881ddd Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 28 Jul 2026 16:53:57 -0500 Subject: [PATCH 5/9] fix(cli): use short_hash in remaining policy hash display paths Replaces the two remaining byte-slice policy_hash renderings in sandbox_policy_set_global and sandbox_draft_approve with the existing short_hash helper, avoiding UTF-8 panic paths on multibyte hashes. Also fixes a clippy map_unwrap_or warning in short_hash itself. Addresses review feedback in NVIDIA/OpenShell#2450. Signed-off-by: Andrew White --- crates/openshell-cli/src/commands/common.rs | 3 +-- crates/openshell-cli/src/run.rs | 8 ++------ 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/crates/openshell-cli/src/commands/common.rs b/crates/openshell-cli/src/commands/common.rs index b876af0d2a..6518964bf6 100644 --- a/crates/openshell-cli/src/commands/common.rs +++ b/crates/openshell-cli/src/commands/common.rs @@ -180,8 +180,7 @@ pub fn short_hash(hash: &str) -> &str { // Slice at the 13th character boundary so multi-byte UTF-8 cannot panic. hash.char_indices() .nth(12) - .map(|(idx, _)| &hash[..idx]) - .unwrap_or(hash) + .map_or(hash, |(idx, _)| &hash[..idx]) } pub fn non_empty_or<'a>(value: &'a str, fallback: &'a str) -> &'a str { diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 54c2a8921b..6b4a70e2b6 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -5443,11 +5443,7 @@ pub async fn sandbox_policy_set_global( eprintln!( "{} Global policy configured (hash: {}, settings revision: {})", "✓".green().bold(), - if response.policy_hash.len() >= 12 { - &response.policy_hash[..12] - } else { - &response.policy_hash - }, + short_hash(&response.policy_hash), response.settings_revision, ); Ok(()) @@ -6796,7 +6792,7 @@ pub async fn sandbox_draft_approve( "{} Chunk approved. Policy version: {}, hash: {}", "OK".green().bold(), inner.policy_version, - &inner.policy_hash[..12.min(inner.policy_hash.len())] + short_hash(&inner.policy_hash) ); Ok(()) From 056e9348d658dd28752f54439556933120d20b87 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 28 Jul 2026 17:21:39 -0500 Subject: [PATCH 6/9] fix(server): include 49152 in gateway ephemeral-port security note The sandbox mechanistic_mapper already treated 49152 as ephemeral (port >= 49152), but the gateway's generate_security_notes used port > 49152, so the boundary port was inconsistently unflagged. Change the gateway check to port >= 49152 and add a regression test for the 49151/49152 boundary. Addresses review feedback in NVIDIA/OpenShell#2450. Signed-off-by: Andrew White --- crates/openshell-server/src/grpc/policy.rs | 26 +++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 1d942e4a3f..95f7ca4f2f 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -3881,7 +3881,7 @@ fn generate_security_notes(rule: &NetworkPolicyRule) -> String { }; for raw_port in ports { let port = u16::try_from(*raw_port).unwrap_or(0); - if port > 49152 { + if port >= 49152 { notes.push(format!( "Port {port} is in the ephemeral range — this may be a temporary service." )); @@ -4822,6 +4822,30 @@ mod tests { assert!(!notes.contains("Port 3306")); } + #[test] + fn security_notes_ephemeral_range_boundary_49152_inclusive() { + // IANA dynamic/private range is 49152-65535 inclusive. + let below = generate_security_notes(&NetworkPolicyRule { + endpoints: vec![NetworkEndpoint { + host: "api.example.com".to_string(), + port: 49151, + ..Default::default() + }], + ..Default::default() + }); + assert!(!below.contains("ephemeral"), "49151 should not be flagged: {below}"); + + let at = generate_security_notes(&NetworkPolicyRule { + endpoints: vec![NetworkEndpoint { + host: "api.example.com".to_string(), + port: 49152, + ..Default::default() + }], + ..Default::default() + }); + assert!(at.contains("Port 49152 is in the ephemeral range"), "49152 should be flagged: {at}"); + } + #[test] fn sandbox_caller_update_validation_allows_sandbox_policy_sync() { let req = UpdateConfigRequest { From 352a57433626a5e710ca91649c7648b03eb2b477 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 28 Jul 2026 19:11:58 -0500 Subject: [PATCH 7/9] ci: trigger fresh CI run From a72bed2e29ec1b39666bb5300a8bbb09ea146eee Mon Sep 17 00:00:00 2001 From: Andrew White Date: Wed, 29 Jul 2026 19:29:23 -0500 Subject: [PATCH 8/9] test(cli): use a genuinely UTF-8-splitting case in short_hash test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit aaaaaaaaaaaaé has a valid boundary at byte 12, so the old byte-slice would not panic for that input; correct the comment and add aaaaaaaaaaaéx (11 'a' + 2-byte 'é' + 'x'), where a byte-slice at 12 would split 'é', expecting aaaaaaaaaaaé. Verified with cargo test -p openshell-cli --lib short_hash (3 passed). Signed-off-by: Andrew White --- crates/openshell-cli/src/commands/common.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/openshell-cli/src/commands/common.rs b/crates/openshell-cli/src/commands/common.rs index 6518964bf6..9335aa5c1e 100644 --- a/crates/openshell-cli/src/commands/common.rs +++ b/crates/openshell-cli/src/commands/common.rs @@ -996,9 +996,12 @@ mod tests { #[test] fn short_hash_handles_multibyte_characters() { - // "aaaaaaaaaaaaé" is 14 bytes (12 'a' + one 2-byte 'é') and 13 chars. - // The old byte-slice at 12 would split 'é' and panic. + // 12 'a' + one 2-byte 'é' is 14 bytes and 13 chars; byte 12 is exactly + // the 'é' boundary, so even the old byte-slice would not panic here. assert_eq!(short_hash("aaaaaaaaaaaaé"), "aaaaaaaaaaaa"); + // 11 'a' + 2-byte 'é' + 'x': a byte-slice at 12 would split 'é' and + // panic; slicing at the 13th character boundary keeps 'é' intact. + assert_eq!(short_hash("aaaaaaaaaaaéx"), "aaaaaaaaaaaé"); // A 12-char hash ending in a multi-byte char is returned unchanged. assert_eq!(short_hash("aaaaaaaaaaaé"), "aaaaaaaaaaaé"); // All-multi-byte input still slices on a character boundary. From 5d14ccd3bb21b198539050fef43936093738e96b Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 31 Jul 2026 09:49:43 -0500 Subject: [PATCH 9/9] style(server): apply rustfmt to policy.rs test asserts --- crates/openshell-server/src/grpc/policy.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 95f7ca4f2f..05840284ab 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -4833,7 +4833,10 @@ mod tests { }], ..Default::default() }); - assert!(!below.contains("ephemeral"), "49151 should not be flagged: {below}"); + assert!( + !below.contains("ephemeral"), + "49151 should not be flagged: {below}" + ); let at = generate_security_notes(&NetworkPolicyRule { endpoints: vec![NetworkEndpoint { @@ -4843,7 +4846,10 @@ mod tests { }], ..Default::default() }); - assert!(at.contains("Port 49152 is in the ephemeral range"), "49152 should be flagged: {at}"); + assert!( + at.contains("Port 49152 is in the ephemeral range"), + "49152 should be flagged: {at}" + ); } #[test]