diff --git a/crates/openshell-cli/src/commands/common.rs b/crates/openshell-cli/src/commands/common.rs index e6edb4d33a..9335aa5c1e 100644 --- a/crates/openshell-cli/src/commands/common.rs +++ b/crates/openshell-cli/src/commands/common.rs @@ -174,7 +174,13 @@ 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_or(hash, |(idx, _)| &hash[..idx]) } pub fn non_empty_or<'a>(value: &'a str, fallback: &'a str) -> &'a str { @@ -975,4 +981,30 @@ 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() { + // 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. + assert_eq!(short_hash("ééééééééééééé"), "éééééééééééé"); + } } diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 64fd550852..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(()) @@ -5805,7 +5801,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 +5810,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 { @@ -6500,6 +6496,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,16 +6516,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 { - format!("{}...", &rev.load_error[..40]) - } 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, @@ -6791,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(()) @@ -8386,4 +8387,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 8ee2fc37f9..b1b3fbea7f 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." @@ -472,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 diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 1d942e4a3f..05840284ab 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,36 @@ 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 { 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]