From d6a88e32d738bb7c183d4eb999551f720f210b1c Mon Sep 17 00:00:00 2001 From: ilbertt Date: Tue, 29 Sep 2026 18:03:21 +0200 Subject: [PATCH] fix: require complete braced environment references --- crates/guest-contract/src/instance_env.rs | 147 ++++++++++++++------- crates/protocol/schema/desired.schema.json | 2 +- crates/protocol/src/domain.rs | 52 +++----- crates/protocol/src/tests.rs | 34 ++++- 4 files changed, 143 insertions(+), 92 deletions(-) diff --git a/crates/guest-contract/src/instance_env.rs b/crates/guest-contract/src/instance_env.rs index c3ee047f..67aefe50 100644 --- a/crates/guest-contract/src/instance_env.rs +++ b/crates/guest-contract/src/instance_env.rs @@ -1,6 +1,6 @@ use protocol::{ - AppHostname, AppHostnameKind, GuestPath, Hostname, HttpPort, RestartPolicy, TenantArguments, - TenantEnvironment, + runtime_references, AppHostname, AppHostnameKind, GuestPath, Hostname, HttpPort, RestartPolicy, + TenantArguments, TenantEnvironment, RUNTIME_VALUE_NAMES, }; pub const INSTANCE_ENV_FILENAME: &str = "instance.env"; @@ -132,50 +132,23 @@ pub struct InstanceConfig { } fn expand(key: &str, value: &str, runtime: &[(String, String)]) -> Result { - if !value.contains(&format!("${RUNTIME_PREFIX}")) && !value.contains(&format!("${{{RUNTIME_PREFIX}")) { - return Ok(value.to_string()); - } let mut expanded = String::with_capacity(value.len()); - let bytes = value.as_bytes(); let mut at = 0; - while at < bytes.len() { - if bytes[at] != b'$' { - expanded.push(value[at..].chars().next().unwrap_or('$')); - at += value[at..].chars().next().map_or(1, char::len_utf8); - continue; - } - let braced = value[at + 1..].starts_with('{'); - let names_at = at + 1 + usize::from(braced); - let rest = &value[names_at..]; - if !rest.starts_with(RUNTIME_PREFIX) { - expanded.push('$'); - at += 1; - continue; - } - let end = if braced { - match rest.find('}') { - Some(end) => end, - None => { - return Err(InstanceEnvError::Malformed { - key: key.to_string(), - rule: "a closed reference", - }) - } - } - } else { - rest.find(|c: char| !c.is_ascii_alphanumeric() && c != '_') - .unwrap_or(rest.len()) - }; - let reference = &rest[..end]; - let Some((_, found)) = runtime.iter().find(|(name, _)| name == reference) else { + for (range, reference) in runtime_references(value) { + let found = runtime + .iter() + .find(|(name, _)| name == reference && RUNTIME_VALUE_NAMES.contains(&reference)); + let Some((_, found)) = found else { return Err(InstanceEnvError::UnknownReference { key: key.to_string(), reference: reference.to_string(), }); }; + expanded.push_str(&value[at..range.start]); expanded.push_str(found); - at = names_at + end + usize::from(braced); + at = range.end; } + expanded.push_str(&value[at..]); Ok(expanded) } @@ -559,13 +532,14 @@ mod both_ends { } #[test] - fn only_a_reference_to_a_runtime_value_expands() { + fn only_a_complete_reference_to_a_runtime_value_expands() { let config = parse_instance_env(&written( &[ ("BARE", "port $NIBRUN_HTTP_PORT here"), ("BRACED", "port ${NIBRUN_HTTP_PORT} here"), ("SHELL_LIKE", "$HOME and $$ and a bare $"), - ("BOTH", "$NIBRUN_HOSTNAME:$NIBRUN_HTTP_PORT"), + ("BOTH", "${NIBRUN_HOSTNAME}:${NIBRUN_HTTP_PORT}"), + ("TWICE", "${NIBRUN_HTTP_PORT}${NIBRUN_HTTP_PORT}0"), ], &[], )) @@ -579,26 +553,97 @@ mod both_ends { .unwrap() }; let port = DEFAULT_HTTP_PORT.to_string(); - assert_eq!(value("BARE"), format!("port {port} here")); + assert_eq!(value("BARE"), "port $NIBRUN_HTTP_PORT here"); assert_eq!(value("BRACED"), format!("port {port} here")); assert_eq!(value("SHELL_LIKE"), "$HOME and $$ and a bare $"); assert_eq!(value("BOTH"), format!("my-app.nibrun.app:{port}")); + assert_eq!(value("TWICE"), format!("{port}{port}0")); } #[test] - fn a_reference_this_runtime_does_not_offer_fails_the_boot_at_both_ends() { - assert!( - TenantValue::parse("$NIBRUN_NO_SUCH_THING").is_err(), - "the host would have written it" - ); + fn secrets_with_bare_names_and_incomplete_references_reach_the_app_unchanged() { + for literal in [ + "$", + "{", + "}", + "${", + "${}", + "$NIBRUN_HTTP_PORT", + "$NIBRUN_NOPE", + "$NIBRUN_PUBLIC_IPV4", + "$NIBRUN_EXTRA_PUBLIC_PORT", + "${NIBRUN_HTTP_PORT", + "${NIBRUN_NOPE", + "{NIBRUN_HTTP_PORT}", + "${NIBRUN_HTTP_PORT!}", + "${NIBRUN_HTTP_PORT with spaces}", + "secret$NIBRUN_HTTP_PORT}suffix", + "$2y$10$K3JqBQ8Rt7uVwXyZaBcDeF", + "🔑${NIBRUN_HTTP_PORTé}尾", + ] { + let config = parse_instance_env(&written(&[("SECRET", literal)], &[])).unwrap(); + assert!(config + .tenant_environment() + .contains(&("SECRET".to_string(), literal.to_string()))); + } + } - let handed = format!("{}ENV_BAD=$NIBRUN_NO_SUCH_THING\n", written(&[], &[])); - let error = parse_instance_env(&handed).unwrap_err(); - assert!( - matches!(&error, InstanceEnvError::UnknownReference { reference, .. } if reference == "NIBRUN_NO_SUCH_THING"), - "{error}" + #[test] + fn complete_references_expand_among_literal_characters() { + let config = parse_instance_env(&written( + &[( + "MIXED", + "🔑$${NIBRUN_HTTP_PORT}|{${NIBRUN_HOSTNAME}}|${NIBRUN_BROKEN:${NIBRUN_HTTP_PORT}|$尾", + )], + &[], + )) + .unwrap(); + assert_eq!( + config.environment, + vec![( + "MIXED".to_string(), + format!( + "🔑${DEFAULT_HTTP_PORT}|{{my-app.nibrun.app}}|${{NIBRUN_BROKEN:{DEFAULT_HTTP_PORT}|$尾" + ), + )] ); - assert!(error.to_string().contains("BAD"), "{error}"); + } + + #[test] + fn a_reference_this_runtime_does_not_offer_fails_the_boot_at_both_ends() { + for (value, name) in [ + ("${NIBRUN_NO_SUCH_THING}", "NIBRUN_NO_SUCH_THING"), + ("${NIBRUN_HTTP_PORT0}", "NIBRUN_HTTP_PORT0"), + ("${NIBRUN_MAX_RESTARTS}", "NIBRUN_MAX_RESTARTS"), + ("${NIBRUN_}", "NIBRUN_"), + ("${NIBRUN_BROKEN:${NIBRUN_NO_SUCH_THING}", "NIBRUN_NO_SUCH_THING"), + ] { + assert!( + TenantValue::parse(value).is_err(), + "the host would have written it" + ); + + let handed = format!("{}ENV_BAD={value}\n", written(&[], &[])); + let error = parse_instance_env(&handed).unwrap_err(); + assert!( + matches!(&error, InstanceEnvError::UnknownReference { reference, .. } if reference == name), + "{error}" + ); + assert!(error.to_string().contains("BAD"), "{error}"); + } + } + + #[test] + fn a_complete_reference_to_a_missing_hostname_fails_the_boot() { + let handed = written(&[("URL", "https://${NIBRUN_HOSTNAME}")], &[]) + .lines() + .filter(|line| !line.starts_with("NIBRUN_HOSTNAME=")) + .collect::>() + .join("\n"); + assert!(matches!( + parse_instance_env(&handed), + Err(InstanceEnvError::UnknownReference { reference, .. }) if reference == "NIBRUN_HOSTNAME" + )); } #[test] diff --git a/crates/protocol/schema/desired.schema.json b/crates/protocol/schema/desired.schema.json index 2973fea4..ff200951 100644 --- a/crates/protocol/schema/desired.schema.json +++ b/crates/protocol/schema/desired.schema.json @@ -774,7 +774,7 @@ } }, "TenantValue": { - "description": "Handed to the app as is, except that `${NAME}` and `$NAME` are filled in for NAME in NIBRUN_HOSTNAME, NIBRUN_HTTP_PORT; any other `$NIBRUN_` reference is refused.", + "description": "Only complete `${NAME}` references are filled in for NAME in NIBRUN_HOSTNAME, NIBRUN_HTTP_PORT. Bare names, incomplete references and other dollar signs remain literal; complete references to unknown `NIBRUN_` names are refused.", "type": "string", "maxLength": 32768 }, diff --git a/crates/protocol/src/domain.rs b/crates/protocol/src/domain.rs index 1a1ad4a2..5350454d 100644 --- a/crates/protocol/src/domain.rs +++ b/crates/protocol/src/domain.rs @@ -1,6 +1,7 @@ #[cfg(feature = "schema")] use std::borrow::Cow; use std::collections::BTreeMap; +use std::ops::Range; #[cfg(feature = "schema")] use schemars::{JsonSchema, Schema, SchemaGenerator}; @@ -26,46 +27,22 @@ fn is_name_character(c: char) -> bool { c.is_ascii_alphanumeric() || c == '_' } -fn runtime_references(value: &str) -> Vec<(String, bool)> { - let mut found = Vec::new(); - let bytes = value.as_bytes(); - let mut index = 0; - while index < bytes.len() { - if bytes[index] != b'$' { - index += 1; - continue; +pub fn runtime_references(value: &str) -> impl Iterator, &str)> { + value.match_indices("${").filter_map(|(start, opening)| { + let rest = &value[start + opening.len()..]; + if !rest.starts_with(RUNTIME_VALUE_PREFIX) { + return None; } - let mut cursor = index + 1; - let braced = bytes.get(cursor) == Some(&b'{'); - if braced { - cursor += 1; + let end = rest.find(|c| !is_name_character(c))?; + if rest.as_bytes()[end] != b'}' { + return None; } - if !value[cursor..].starts_with(RUNTIME_VALUE_PREFIX) { - index += 1; - continue; - } - let start = cursor; - while cursor < bytes.len() && is_name_character(bytes[cursor] as char) { - cursor += 1; - } - let name = &value[start..cursor]; - let closed = if braced { - let closed = bytes.get(cursor) == Some(&b'}'); - if closed { - cursor += 1; - } - closed - } else { - true - }; - found.push((name.to_string(), closed && RUNTIME_VALUE_NAMES.contains(&name))); - index = cursor.max(index + 1); - } - found + Some((start..start + opening.len() + end + 1, &rest[..end])) + }) } pub fn names_offered_runtime_values(value: &str) -> bool { - runtime_references(value).iter().all(|(_, allowed)| *allowed) + runtime_references(value).all(|(_, name)| RUNTIME_VALUE_NAMES.contains(&name)) } #[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -122,8 +99,9 @@ impl JsonSchema for TenantValue { schema.insert( "description".into(), format!( - "Handed to the app as is, except that `${{NAME}}` and `$NAME` are filled in for NAME \ - in {}; any other `$NIBRUN_` reference is refused.", + "Only complete `${{NAME}}` references are filled in for NAME in {}. \ + Bare names, incomplete references and other dollar signs remain literal; \ + complete references to unknown `NIBRUN_` names are refused.", RUNTIME_VALUE_NAMES.join(", ") ) .into(), diff --git a/crates/protocol/src/tests.rs b/crates/protocol/src/tests.rs index a8342745..ed6487d3 100644 --- a/crates/protocol/src/tests.rs +++ b/crates/protocol/src/tests.rs @@ -127,10 +127,38 @@ fn a_secret_never_prints_itself() { fn a_tenant_value_may_name_only_offered_runtime_values() { assert!(TenantValue::parse("$HOME and $$ and a bcrypt $2b$10$abc").is_ok()); assert!(TenantValue::parse("http://x:${NIBRUN_HTTP_PORT}/").is_ok()); - assert!(TenantValue::parse("$NIBRUN_HTTP_PORT").is_ok()); - assert!(TenantValue::parse("$NIBRUN_HTTP_PORTS").is_err()); + assert!(TenantValue::parse("${NIBRUN_HTTP_PORTS}").is_err()); assert!(TenantValue::parse("${NIBRUN_NOPE}").is_err()); - assert!(TenantValue::parse("${NIBRUN_HTTP_PORT").is_err()); + assert!(TenantValue::parse("${NIBRUN_}").is_err()); + assert!(TenantValue::parse("${NIBRUN_BROKEN:${NIBRUN_NOPE}").is_err()); +} + +#[test] +fn a_tenant_value_preserves_everything_except_complete_runtime_references() { + for literal in [ + "$", + "{", + "}", + "${", + "${}", + "$NIBRUN_HTTP_PORT", + "$NIBRUN_NOPE", + "${NIBRUN_HTTP_PORT", + "${NIBRUN_NOPE", + "{NIBRUN_HTTP_PORT}", + "${NIBRUN_HTTP_PORT!}", + "${NIBRUN_HTTP_PORT with spaces}", + "secret$NIBRUN_HTTP_PORT}suffix", + "🔑${NIBRUN_HTTP_PORTé}尾", + ] { + assert_eq!(TenantValue::parse(literal).unwrap().expose(), literal); + let decoded: TenantValue = serde_json::from_value(serde_json::json!(literal)).unwrap(); + assert_eq!(decoded.expose(), literal); + } + assert!(TenantValue::parse( + "$${NIBRUN_HTTP_PORT}|{${NIBRUN_HOSTNAME}}|${NIBRUN_BROKEN:${NIBRUN_HTTP_PORT}|$" + ) + .is_ok()); } #[test]