Update with remarks Tess

Author: Gijsreyn

resources/sshdconfig/locales/en-us.toml

@@ -51,12 +51,12 @@ schema = "Schema command:"
 set = "Set command: '%{input}'"
 
 [parser]
+combinedMultiArgValue = "combined multiple arguments into a single value for keyword '%{keyword}'"
 failedToParse = "failed to parse: '%{input}'"
 failedToParseAsArray = "value is not an array"
 failedToParseNode = "failed to parse '%{input}'"
 failedToParseRoot = "failed to parse root: '%{input}'"
 invalidConfig = "invalid config: '%{input}'"
-invalidMultiArgNode = "multi-arg node '%{input}' is not valid"
 keyNotFound = "key '%{key}' not found"
 keyNotRepeatable = "key '%{key}' is not repeatable"
 missingCriteriaInMatch = "missing criteria field in match block: '%{input}'"

resources/sshdconfig/src/get.rs

@@ -17,7 +17,6 @@ use crate::canonical_properties::CanonicalProperty;
 use crate::error::SshdConfigError;
 use crate::inputs::{CommandInfo, SSHD_CONFIG_FILEPATH};
 use crate::parser::parse_text_to_map;
-use crate::repeat_keyword::MULTI_ARG_KEYWORDS_SPACE_SEP;
 use crate::util::{
     build_command_info,
     extract_sshd_defaults,
@@ -134,11 +133,11 @@ pub fn get_sshd_settings(cmd_info: &CommandInfo, is_get: bool) -> Result<Map<Str
         result.insert("match".to_string(), match_value.clone());
     }
 
-    // sshd -T normalizes space-separated list keywords by stripping the quotes that preserve
-    // values containing spaces (e.g. Windows group names like "openssh users"), splitting them
-    // into separate entries. Prefer the value parsed directly from the config file, which retains
-    // the quoting, for these keywords when they are explicitly set.
-    prefer_explicit_space_sep_lists(&mut result, &explicit_settings);
+    // sshd -T strips the quotes that preserve values containing spaces (e.g. Windows group names
+    // like "openssh users" or paths like "C:\Program Files\ssh\banner.txt") and normalizes their
+    // casing. Prefer the value parsed directly from the config file, which retains the quoting and
+    // the original casing, for any keyword whose explicit value contains whitespace.
+    prefer_explicit_values_with_spaces(&mut result, &explicit_settings);
 
     if cmd_info.include_defaults {
         // get default from SSHD -T with empty config
@@ -179,14 +178,24 @@ pub fn get_sshd_settings(cmd_info: &CommandInfo, is_get: bool) -> Result<Map<Str
     Ok(result)
 }
 
-fn prefer_explicit_space_sep_lists(result: &mut Map<String, Value>, explicit_settings: &Map<String, Value>) {
-    for keyword in MULTI_ARG_KEYWORDS_SPACE_SEP {
-        if let Some(value) = explicit_settings.get(keyword) {
-            result.insert((*keyword).to_string(), value.clone());
+fn prefer_explicit_values_with_spaces(result: &mut Map<String, Value>, explicit_settings: &Map<String, Value>) {
+    for (keyword, value) in explicit_settings {
+        if contains_whitespace(value) {
+            result.insert(keyword.clone(), value.clone());
         }
     }
 }
 
+/// Whether the value, or any value nested within it, is a string containing whitespace.
+fn contains_whitespace(value: &Value) -> bool {
+    match value {
+        Value::String(s) => s.contains(char::is_whitespace),
+        Value::Array(items) => items.iter().any(contains_whitespace),
+        Value::Object(map) => map.values().any(contains_whitespace),
+        _ => false
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -202,14 +211,46 @@ mod tests {
         let mut explicit_settings = Map::new();
         explicit_settings.insert("allowgroups".to_string(), json!(["administrators", "openssh users"]));
 
-        prefer_explicit_space_sep_lists(&mut result, &explicit_settings);
+        prefer_explicit_values_with_spaces(&mut result, &explicit_settings);
 
         assert_eq!(
             result.get("allowgroups").unwrap(),
             &json!(["administrators", "openssh users"])
         );
     }
 
+    #[test]
+    fn overrides_single_value_keyword_with_quoted_file_value() {
+        let mut result = Map::new();
+        result.insert("banner".to_string(), json!("c:\\program files\\ssh\\sample_banner.txt"));
+
+        let mut explicit_settings = Map::new();
+        explicit_settings.insert("banner".to_string(), json!("C:\\Program Files\\ssh\\sample_banner.txt"));
+
+        prefer_explicit_values_with_spaces(&mut result, &explicit_settings);
+
+        assert_eq!(
+            result.get("banner").unwrap(),
+            &json!("C:\\Program Files\\ssh\\sample_banner.txt")
+        );
+    }
+
+    #[test]
+    fn overrides_nested_value_with_spaces() {
+        let mut result = Map::new();
+        result.insert("subsystem".to_string(), json!([{"name": "sftp", "value": "c:/program files/openssh/sftp-server.exe"}]));
+
+        let mut explicit_settings = Map::new();
+        explicit_settings.insert("subsystem".to_string(), json!([{"name": "sftp", "value": "C:/Program Files/OpenSSH/sftp-server.exe"}]));
+
+        prefer_explicit_values_with_spaces(&mut result, &explicit_settings);
+
+        assert_eq!(
+            result.get("subsystem").unwrap(),
+            &json!([{"name": "sftp", "value": "C:/Program Files/OpenSSH/sftp-server.exe"}])
+        );
+    }
+
     #[test]
     fn leaves_keyword_absent_from_file_untouched() {
         // allowgroups is present in sshd -T output but not explicitly set in the config file.
@@ -218,7 +259,7 @@ mod tests {
 
         let explicit_settings = Map::new();
 
-        prefer_explicit_space_sep_lists(&mut result, &explicit_settings);
+        prefer_explicit_values_with_spaces(&mut result, &explicit_settings);
 
         assert_eq!(
             result.get("allowgroups").unwrap(),
@@ -227,16 +268,18 @@ mod tests {
     }
 
     #[test]
-    fn leaves_non_space_sep_keyword_untouched() {
-        // port is not a space-separated list keyword and must not be overridden.
+    fn leaves_value_without_spaces_untouched() {
         let mut result = Map::new();
         result.insert("port".to_string(), json!([22]));
+        result.insert("allowgroups".to_string(), json!(["administrators"]));
 
         let mut explicit_settings = Map::new();
         explicit_settings.insert("port".to_string(), json!([2222]));
+        explicit_settings.insert("allowgroups".to_string(), json!(["openssh"]));
 
-        prefer_explicit_space_sep_lists(&mut result, &explicit_settings);
+        prefer_explicit_values_with_spaces(&mut result, &explicit_settings);
 
         assert_eq!(result.get("port").unwrap(), &json!([22]));
+        assert_eq!(result.get("allowgroups").unwrap(), &json!(["administrators"]));
     }
 }

resources/sshdconfig/src/parser.rs

@@ -266,10 +266,11 @@ fn parse_arguments_node(arg_node: tree_sitter::Node, input: &str, input_bytes: &
     let mut vec: Vec<Value> = Vec::new();
     let is_vec = keyword_info.is_multi_arg();
 
-    // if there is more than one argument, but a vector is not expected for the keyword, throw an error
     let children: Vec<_> = arg_node.named_children(&mut cursor).collect();
+
     if children.len() > 1 && !is_vec {
-        return Err(SshdConfigError::ParserError(t!("parser.invalidMultiArgNode", input = input).to_string()));
+        debug!("{}", t!("parser.combinedMultiArgValue", keyword = &keyword_info.name).to_string());
+        return Ok(Value::String(combine_args(&children, input, input_bytes)?));
     }
 
     for node in &children {
@@ -333,6 +334,33 @@ fn parse_arguments_node(arg_node: tree_sitter::Node, input: &str, input_bytes: &
     }
 }
 
+/// Combine multiple argument nodes into a single string value.
+///
+/// The grammar cannot include spaces in a `string` token, so an unquoted value containing spaces is
+/// split into several argument nodes. The separator that appeared between two arguments in the
+/// source text is preserved, so a comma-separated value is not silently rewritten as a
+/// space-separated one. Quote characters sit between the argument nodes and are dropped, matching
+/// how a quoted value that parsed as a single argument is handled.
+fn combine_args(children: &[tree_sitter::Node], input: &str, input_bytes: &[u8]) -> Result<String, SshdConfigError> {
+    let mut combined = String::new();
+    let mut previous_end: Option<usize> = None;
+
+    for node in children {
+        if node.is_error() {
+            return Err(SshdConfigError::ParserError(t!("parser.failedToParseNode", input = input).to_string()));
+        }
+        if let Some(end) = previous_end {
+            let between = input.get(end..node.start_byte()).unwrap_or_default();
+            combined.push_str(if between.contains(',') { "," } else { " " });
+        }
+        combined.push_str(node.utf8_text(input_bytes)?.trim());
+        previous_end = Some(node.end_byte());
+    }
+
+    // Unescape backslashes (sshd -T escapes them on Windows)
+    Ok(unescape_backslashes(&combined))
+}
+
 /// Parse arguments node for match criteria, always returning an array.
 /// Match criteria are always comma-separated and should be arrays.
 fn parse_arguments_node_as_array(arg_node: tree_sitter::Node, input: &str, input_bytes: &[u8], _keyword_info: &KeywordInfo) -> Result<Value, SshdConfigError> {
@@ -487,6 +515,57 @@ mod tests {
         assert_eq!(allowgroups[1], Value::String("developers".to_string()));
     }
 
+    #[test]
+    fn single_value_keyword_with_spaces() {
+        // sshd -T prints Banner "C:\Program Files\ssh\sample_banner.txt" without the quotes.
+        let input = "banner c:\\program files\\ssh\\sample_banner.txt\r\n";
+        let result: Map<String, Value> = parse_text_to_map(input).unwrap();
+        assert_eq!(
+            result.get("banner").unwrap(),
+            &Value::String("c:\\program files\\ssh\\sample_banner.txt".to_string())
+        );
+    }
+
+    #[test]
+    fn single_value_keyword_with_quotes_matches_unquoted() {
+        let quoted = parse_text_to_map("banner \"c:\\program files\\ssh\\sample_banner.txt\"\r\n").unwrap();
+        let unquoted = parse_text_to_map("banner c:\\program files\\ssh\\sample_banner.txt\r\n").unwrap();
+        assert_eq!(quoted.get("banner").unwrap(), unquoted.get("banner").unwrap());
+    }
+
+    #[test]
+    fn single_value_keyword_preserves_comma_separator() {
+        // logverbose is not a known multi-arg keyword, but its value is comma-separated.
+        let input = "logverbose kex.c:*:1,monitor.c:*\r\n";
+        let result: Map<String, Value> = parse_text_to_map(input).unwrap();
+        assert_eq!(
+            result.get("logverbose").unwrap(),
+            &Value::String("kex.c:*:1,monitor.c:*".to_string())
+        );
+    }
+
+    #[test]
+    fn single_value_keyword_preserves_mixed_separators() {
+        let input = "forcecommand /usr/bin/cmd -o a,b -x\r\n";
+        let result: Map<String, Value> = parse_text_to_map(input).unwrap();
+        assert_eq!(
+            result.get("forcecommand").unwrap(),
+            &Value::String("/usr/bin/cmd -o a,b -x".to_string())
+        );
+    }
+
+    #[test]
+    fn single_value_keyword_with_spaces_roundtrip() {
+        use crate::formatter::write_config_map_to_text;
+
+        let input = "banner \"/etc/ssh/sample banner.txt\"\n";
+        let parsed = parse_text_to_map(input).unwrap();
+        let formatted = write_config_map_to_text(&parsed).unwrap();
+        let reparsed = parse_text_to_map(&formatted).unwrap();
+
+        assert_eq!(parsed.get("banner").unwrap(), reparsed.get("banner").unwrap());
+    }
+
     #[test]
     fn err_multiarg_repeated_keyword() {
         let input = "hostkeyalgorithms ssh-ed25519-cert-v01@openssh.com\r\n hostkeyalgorithms ecdsa-sha2-nistp256-cert-v01@openssh.com\r\n";

resources/sshdconfig/tests/sshdconfig.get.tests.ps1

@@ -47,6 +47,14 @@ AllowGroups administrators "openssh users"
 "@
         $TestConfigPathWithQuotedGroups = Join-Path $TestDrive 'test_sshd_config_quoted_groups'
         $configWithQuotedGroups | Set-Content -Path $TestConfigPathWithQuotedGroups
+        $BannerPath = Join-Path $TestDrive 'Sample Banner.txt'
+        'welcome' | Set-Content -Path $BannerPath
+        $configWithQuotedPath = @"
+PasswordAuthentication no
+Banner "$BannerPath"
+"@
+        $TestConfigPathWithQuotedPath = Join-Path $TestDrive 'test_sshd_config_quoted_path'
+        $configWithQuotedPath | Set-Content -Path $TestConfigPathWithQuotedPath
     }
 
     AfterAll {
@@ -62,6 +70,12 @@ AllowGroups administrators "openssh users"
         if (Test-Path $TestConfigPathWithQuotedGroups) {
             Remove-Item -Path $TestConfigPathWithQuotedGroups -Force -ErrorAction SilentlyContinue
         }
+        if (Test-Path $TestConfigPathWithQuotedPath) {
+            Remove-Item -Path $TestConfigPathWithQuotedPath -Force -ErrorAction SilentlyContinue
+        }
+        if (Test-Path $BannerPath) {
+            Remove-Item -Path $BannerPath -Force -ErrorAction SilentlyContinue
+        }
     }
 
     It '<Command> command <Description>' -TestCases @(
@@ -174,6 +188,27 @@ AllowGroups administrators "openssh users"
         $result.AllowGroups[1] | Should -Be "openssh users"
     }
 
+    It '<Command> command preserves a single-value keyword whose path contains spaces' -TestCases @(
+        @{ Command = 'get' }
+        @{ Command = 'export' }
+    ) {
+        param($Command)
+
+        $inputData = @{
+            sshd_config_filepath = $TestConfigPathWithQuotedPath
+        } | ConvertTo-Json
+
+        if ($Command -eq 'get') {
+            $result = sshdconfig $Command --input $inputData -s sshd-config 2>$null | ConvertFrom-Json
+        }
+        else {
+            $result = sshdconfig $Command --input $inputData 2>$null | ConvertFrom-Json
+        }
+
+        $LASTEXITCODE | Should -Be 0
+        $result.Banner | Should -Be $BannerPath
+    }
+
     It 'Should fail without creating target config when file does not exist' {
         $nonExistentPath = Join-Path $TestDrive 'nonexistent_sshd_config'