Skip to content

Commit

Permalink
Merge of #12020
Browse files Browse the repository at this point in the history
  • Loading branch information
mergify[bot] authored Dec 14, 2024
2 parents 77e391b + 3666113 commit d108bdc
Show file tree
Hide file tree
Showing 5 changed files with 248 additions and 4 deletions.
21 changes: 21 additions & 0 deletions doc/manual/rl-next/nix-sshopts-parsing.md
Original file line number Diff line number Diff line change
@@ -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.
13 changes: 11 additions & 2 deletions src/libstore/ssh.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,17 @@ void SSHMaster::addCommonSSHOpts(Strings & args)
{
auto state(state_.lock());

for (auto & i : tokenizeString<Strings>(getEnv("NIX_SSHOPTS").value_or("")))
args.push_back(i);
std::string sshOpts = getEnv("NIX_SSHOPTS").value_or("");

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()) {
Expand Down
107 changes: 105 additions & 2 deletions src/libutil-tests/strings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@
#include <rapidcheck/gtest.h>

#include "strings.hh"
#include "error.hh"

namespace nix {

using Strings = std::vector<std::string>;

/* ----------------------------------------------------------------------------
* concatStringsSep
* --------------------------------------------------------------------------*/
Expand Down Expand Up @@ -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
104 changes: 104 additions & 0 deletions src/libutil/strings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "strings-inline.hh"
#include "os-string.hh"
#include "error.hh"

namespace nix {

Expand Down Expand Up @@ -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
7 changes: 7 additions & 0 deletions src/libutil/strings.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

0 comments on commit d108bdc

Please sign in to comment.