Implement shellSplitString for proper handling of NIX_SSHOPTS with spaces and quotes

doc/manual/rl-next/nix-sshopts-parsing.md

@@ -0,0 +1,21 @@
+---
+synopsis: "Improved `NIX_SSHOPTS` parsing for better SSH option handling"
+issues: [5181]
+prs: [12020]
+---
+
+The parsing of the `NIX_SSHOPTS` environment variable has been improved to handle spaces and quotes correctly.
+Previously, incorrectly split SSH options could cause failures in CLIs like `nix-copy-closure`,
+especially when using complex ssh invocations such as `-o ProxyCommand="ssh -W %h:%p ..."`.
+
+This change introduces a `shellSplitString` function to ensure
+that `NIX_SSHOPTS` is parsed in a manner consistent with shell
+behavior, addressing common parsing errors.
+
+For example, the following now works as expected:
+
+```bash
+export NIX_SSHOPTS='-o ProxyCommand="ssh -W %h:%p ..."'
+```
+
+This update improves the reliability of SSH-related operations using `NIX_SSHOPTS` across Nix CLIs.

src/libstore/ssh.cc

@@ -41,8 +41,17 @@ void SSHMaster::addCommonSSHOpts(Strings & args)
 {
     auto state(state_.lock());
 
-    for (auto & i : tokenizeString<Strings>(getEnv("NIX_SSHOPTS").value_or("")))
+    std::string sshOpts = getEnv("NIX_SSHOPTS").value_or("");
-        args.push_back(i);
+
+    try {
+        std::list<std::string> opts = shellSplitString(sshOpts);
+        for (auto & i : opts)
+            args.push_back(i);
+    } catch (Error & e) {
+        e.addTrace({}, "while splitting NIX_SSHOPTS '%s'", sshOpts);
+        throw;
+    }
+
     if (!keyFile.empty())
         args.insert(args.end(), {"-i", keyFile});
     if (!sshPublicHostKey.empty()) {

src/libutil-tests/strings.cc

@@ -2,11 +2,10 @@
 #include <rapidcheck/gtest.h>
 
 #include "strings.hh"
+#include "error.hh"
 
 namespace nix {
 
-using Strings = std::vector<std::string>;
-
 /* ----------------------------------------------------------------------------
  * concatStringsSep
  * --------------------------------------------------------------------------*/
@@ -345,4 +344,108 @@ RC_GTEST_PROP(splitString, recoveredByConcatStringsSep, (const std::string & s))
     RC_ASSERT(concatStringsSep("a", splitString<Strings>(s, "a")) == s);
 }
 
+/* ----------------------------------------------------------------------------
+ * shellSplitString
+ * --------------------------------------------------------------------------*/
+
+TEST(shellSplitString, empty)
+{
+    std::list<std::string> expected = {};
+
+    ASSERT_EQ(shellSplitString(""), expected);
+}
+
+TEST(shellSplitString, oneWord)
+{
+    std::list<std::string> expected = {"foo"};
+
+    ASSERT_EQ(shellSplitString("foo"), expected);
+}
+
+TEST(shellSplitString, oneWordQuotedWithSpaces)
+{
+    std::list<std::string> expected = {"foo bar"};
+
+    ASSERT_EQ(shellSplitString("'foo bar'"), expected);
+}
+
+TEST(shellSplitString, oneWordQuotedWithSpacesAndDoubleQuoteInSingleQuote)
+{
+    std::list<std::string> expected = {"foo bar\""};
+
+    ASSERT_EQ(shellSplitString("'foo bar\"'"), expected);
+}
+
+TEST(shellSplitString, oneWordQuotedWithDoubleQuotes)
+{
+    std::list<std::string> expected = {"foo bar"};
+
+    ASSERT_EQ(shellSplitString("\"foo bar\""), expected);
+}
+
+TEST(shellSplitString, twoWords)
+{
+    std::list<std::string> expected = {"foo", "bar"};
+
+    ASSERT_EQ(shellSplitString("foo bar"), expected);
+}
+
+TEST(shellSplitString, twoWordsWithSpacesAndQuotesQuoted)
+{
+    std::list<std::string> expected = {"foo bar'", "baz\""};
+
+    ASSERT_EQ(shellSplitString("\"foo bar'\" 'baz\"'"), expected);
+}
+
+TEST(shellSplitString, emptyArgumentsAreAllowedSingleQuotes)
+{
+    std::list<std::string> expected = {"foo", "", "bar", "baz", ""};
+
+    ASSERT_EQ(shellSplitString("foo '' bar baz ''"), expected);
+}
+
+TEST(shellSplitString, emptyArgumentsAreAllowedDoubleQuotes)
+{
+    std::list<std::string> expected = {"foo", "", "bar", "baz", ""};
+
+    ASSERT_EQ(shellSplitString("foo \"\" bar baz \"\""), expected);
+}
+
+TEST(shellSplitString, singleQuoteDoesNotUseEscapes)
+{
+    std::list<std::string> expected = {"foo\\\"bar"};
+
+    ASSERT_EQ(shellSplitString("'foo\\\"bar'"), expected);
+}
+
+TEST(shellSplitString, doubleQuoteDoesUseEscapes)
+{
+    std::list<std::string> expected = {"foo\"bar"};
+
+    ASSERT_EQ(shellSplitString("\"foo\\\"bar\""), expected);
+}
+
+TEST(shellSplitString, backslashEscapesSpaces)
+{
+    std::list<std::string> expected = {"foo bar", "baz", "qux quux"};
+
+    ASSERT_EQ(shellSplitString("foo\\ bar baz qux\\ quux"), expected);
+}
+
+TEST(shellSplitString, backslashEscapesQuotes)
+{
+    std::list<std::string> expected = {"foo\"bar", "baz", "qux'quux"};
+
+    ASSERT_EQ(shellSplitString("foo\\\"bar baz qux\\'quux"), expected);
+}
+
+TEST(shellSplitString, testUnbalancedQuotes)
+{
+    ASSERT_THROW(shellSplitString("foo'"), Error);
+    ASSERT_THROW(shellSplitString("foo\""), Error);
+    ASSERT_THROW(shellSplitString("foo'bar"), Error);
+    ASSERT_THROW(shellSplitString("foo\"bar"), Error);
+    ASSERT_THROW(shellSplitString("foo\"bar\\\""), Error);
+}
+
 } // namespace nix

src/libutil/strings.cc

@@ -4,6 +4,7 @@
 
 #include "strings-inline.hh"
 #include "os-string.hh"
+#include "error.hh"
 
 namespace nix {
 
@@ -48,4 +49,107 @@ template std::string dropEmptyInitThenConcatStringsSep(std::string_view, const s
 template std::string dropEmptyInitThenConcatStringsSep(std::string_view, const std::set<std::string> &);
 template std::string dropEmptyInitThenConcatStringsSep(std::string_view, const std::vector<std::string> &);
 
+/**
+ * Shell split string: split a string into shell arguments, respecting quotes and backslashes.
+ *
+ * Used for NIX_SSHOPTS handling, which previously used `tokenizeString` and was broken by
+ * Arguments that need to be passed to ssh with spaces in them.
+ *
+ * Read https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html for the
+ * POSIX shell specification, which is technically what we are implementing here.
+ */
+std::list<std::string> shellSplitString(std::string_view s)
+{
+    std::list<std::string> result;
+    std::string current;
+    bool startedCurrent = false;
+    bool escaping = false;
+
+    auto pushCurrent = [&]() {
+        if (startedCurrent) {
+            result.push_back(current);
+            current.clear();
+            startedCurrent = false;
+        }
+    };
+
+    auto pushChar = [&](char c) {
+        current.push_back(c);
+        startedCurrent = true;
+    };
+
+    auto pop = [&]() {
+        auto c = s[0];
+        s.remove_prefix(1);
+        return c;
+    };
+
+    auto inDoubleQuotes = [&]() {
+        startedCurrent = true;
+        // in double quotes, escaping with backslash is only effective for $, `, ", and backslash
+        while (!s.empty()) {
+            auto c = pop();
+            if (escaping) {
+                switch (c) {
+                case '$':
+                case '`':
+                case '"':
+                case '\\':
+                    pushChar(c);
+                    break;
+                default:
+                    pushChar('\\');
+                    pushChar(c);
+                    break;
+                }
+                escaping = false;
+            } else if (c == '\\') {
+                escaping = true;
+            } else if (c == '"') {
+                return;
+            } else {
+                pushChar(c);
+            }
+        }
+        if (s.empty()) {
+            throw Error("unterminated double quote");
+        }
+    };
+
+    auto inSingleQuotes = [&]() {
+        startedCurrent = true;
+        while (!s.empty()) {
+            auto c = pop();
+            if (c == '\'') {
+                return;
+            }
+            pushChar(c);
+        }
+        if (s.empty()) {
+            throw Error("unterminated single quote");
+        }
+    };
+
+    while (!s.empty()) {
+        auto c = pop();
+        if (escaping) {
+            pushChar(c);
+            escaping = false;
+        } else if (c == '\\') {
+            escaping = true;
+        } else if (c == ' ' || c == '\t') {
+            pushCurrent();
+        } else if (c == '"') {
+            inDoubleQuotes();
+        } else if (c == '\'') {
+            inSingleQuotes();
+        } else {
+            pushChar(c);
+        }
+    }
+
+    pushCurrent();
+
+    return result;
+}
 } // namespace nix

src/libutil/strings.hh

@@ -71,4 +71,11 @@ extern template std::string dropEmptyInitThenConcatStringsSep(std::string_view,
 extern template std::string dropEmptyInitThenConcatStringsSep(std::string_view, const std::set<std::string> &);
 extern template std::string dropEmptyInitThenConcatStringsSep(std::string_view, const std::vector<std::string> &);
 
+/**
+ * Shell split string: split a string into shell arguments, respecting quotes and backslashes.
+ *
+ * Used for NIX_SSHOPTS handling, which previously used `tokenizeString` and was broken by
+ * Arguments that need to be passed to ssh with spaces in them.
+ */
+std::list<std::string> shellSplitString(std::string_view s);
 }