Merge pull request #1404 from mickem/claude/nscp-issue-460-check-39fdt1

Settings: send query parameters on http(s) settings urls

Author: mickem

docs/docs/concepts/settings.md

@@ -144,6 +144,64 @@ Adding a script:
 scripts/myscript.bat = http://www.myserver.com/myscript.bat
 ```
 
+#### Query parameters
+
+The url may carry a query string, which is passed on to the server unchanged.
+This lets a script generate the configuration per host instead of serving a static file:
+
+```ini
+[settings]
+1 = http://nsclient.mydom.local/nsclient/nsclient.php?RootFolder=myhost/&Filename=nsclient.ini
+2 = ini://${shared-path}/nsclient.ini
+```
+
+Each distinct query gets its own file in the cache folder, so several urls pointing at the same
+script with different parameters do not overwrite each other's cached configuration. An existing
+cache file written by an older version is moved to the new name on first start, so a host that
+cannot reach its settings server during the upgrade still boots off its cached configuration.
+
+Characters that are not legal in a url query - a space, most notably - are percent-encoded before
+the request is sent. Anything already written as `%XX` is left as it is, so a query you encoded
+yourself is not encoded twice.
+
+#### Host name placeholders
+
+The url may contain the same host name placeholders the submit clients (NRDP, Graphite, Syslog and
+friends) accept, so a single `boot.ini` can be rolled out to an entire fleet and each agent asks for
+its own configuration:
+
+```ini
+[settings]
+1 = http://cfgsrv/nsclient.php?host=${hostname}
+```
+
+| Placeholder | Expands to |
+|---|---|
+| `${hostname}` | the system host name as reported, e.g. `srv01.example.com` |
+| `${host}` | the part before the first `.`, e.g. `srv01` |
+| `${domain}` | the part after the first `.`, e.g. `example.com` |
+
+Each of the three also has a `_lc` and a `_uc` variant (`${hostname_lc}`, `${host_uc}`, ...) that
+lower- or upper-cases the result.
+
+Placeholders are expanded before the url is parsed, so they may appear anywhere in it - in the
+query, in the path (`http://cfgsrv/hosts/${host}/nsclient.ini`) or even in the host name. They are
+expanded before percent-encoding, so a host name containing a character that needs escaping is
+escaped rather than corrupting the request. The cache file name is derived from the expanded url,
+so each host caches its own configuration.
+
+> **New in 0.17:** `${hostname}`, `${hostname_lc}` and `${hostname_uc}`. The other placeholders
+> already existed for the submit clients; this makes them available in settings urls too.
+
+If the query carries a credential (`?token=...`), note that it is still sent in clear text unless
+the url is `https://`. NSClient++ keeps query parameters out of its own log and out of the settings
+url it prints (`nscp settings --show`): both render a settings url as scheme, host and path only.
+Anything else that handles the url - a proxy, the settings server's own access log - is of course
+outside the agent's control.
+
+> **Changed in 0.17:** query parameters used to be silently dropped from the request, so the
+> server only ever saw the bare path.
+
 #### Using TLS
 
 You likely want to use TLS when using http settings.

include/net/net.hpp

@@ -14,6 +14,44 @@ struct string_traits {
   static std::string protocol_suffix() { return "://"; }
   static std::string port_prefix() { return ":"; }
 };
+
+// Percent-encode whatever is not legal in a url query, so the result can be put
+// on an HTTP request line verbatim. RFC 3986 allows query = *( pchar / "/" /
+// "?" ), i.e. unreserved / sub-delims / ":" / "@" / "/" / "?" / pct-encoded.
+// Everything else either makes the request line unparseable (a space turns
+// "GET /a?b=c d HTTP/1.0" into a malformed three-token line) or, for a stray CR
+// or LF, splits one request into two. The query only reaches the wire since
+// issue #460, so this guards a door that was previously closed by accident.
+//
+// An operator may well have written the query already encoded, so an existing
+// "%XX" pair is passed through untouched rather than turned into "%25XX". A '%'
+// that does not introduce a valid pair is not an escape and is encoded.
+inline std::string encode_query(const std::string &query) {
+  static const std::string sub_delims = "-._~!$&'()*+,;=:@/?";
+  static const char hex[] = "0123456789ABCDEF";
+  const auto is_hex = [](const char c) { return (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'); };
+
+  std::string out;
+  out.reserve(query.size());
+  for (std::string::size_type i = 0; i < query.size(); ++i) {
+    const char c = query[i];
+    if (c == '%' && i + 2 < query.size() && is_hex(query[i + 1]) && is_hex(query[i + 2])) {
+      out.append(query, i, 3);
+      i += 2;
+      continue;
+    }
+    const auto uc = static_cast<unsigned char>(c);
+    if ((uc >= 'A' && uc <= 'Z') || (uc >= 'a' && uc <= 'z') || (uc >= '0' && uc <= '9') || sub_delims.find(c) != std::string::npos) {
+      out.push_back(c);
+      continue;
+    }
+    out.push_back('%');
+    out.push_back(hex[(uc >> 4) & 0xf]);
+    out.push_back(hex[uc & 0xf]);
+  }
+  return out;
+}
+
 struct url {
   std::string protocol;
   std::string host;
@@ -22,14 +60,38 @@ struct url {
   unsigned int port;
   url() : port(0) {}
 
-  std::string to_string() const {
+  // The full url, query string included. Prefer to_log_safe_string() for
+  // anything that ends up in a log or an error message.
+  std::string to_string() const { return get_baseurl() + get_request_path(); }
+
+  // The url without the query string. A settings url is free to carry
+  // credentials in its parameters (".../cfg.php?token=..."), and the settings
+  // layer logs the url it is fetching on every boot - at warning level when TLS
+  // verification is off or the CA bundle is missing. Identifying the source
+  // does not need the parameters, so they are left out rather than written to
+  // disk in clear text.
+  std::string to_log_safe_string() const { return get_baseurl() + get_path(); }
+
+  // Scheme and authority: "http://host:8080".
+  std::string get_baseurl() const {
     std::stringstream ss;
     ss << protocol << string_traits::protocol_suffix() << host;
     if (port != 0) ss << string_traits::port_prefix() << port;
-    ss << path;
     return ss.str();
   }
 
+  // The document path, without the query string.
+  std::string get_path() const { return path; }
+
+  // The resource as it has to appear on the HTTP request line: everything
+  // after the authority, query string included. `path` on its own stops at
+  // the '?', so a caller that hands it straight to a downloader silently
+  // drops every parameter the user wrote (issue #460).
+  std::string get_request_path() const {
+    if (query.empty()) return path;
+    return path + "?" + encode_query(query);
+  }
+
   unsigned int get_port() const { return port; }
   unsigned int get_port(unsigned int default_port) const {
     if (port == 0) return default_port;

include/net/net_test.cpp

@@ -0,0 +1,148 @@
+// SPDX-FileCopyrightText: 2004-2026 Michael Medin
+// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-only
+
+#include <gtest/gtest.h>
+
+#include <net/net.hpp>
+
+TEST(net_url, parse_splits_protocol_host_port_and_path) {
+  const net::url u = net::parse("http://example.com:8080/dir/file.ini");
+  EXPECT_EQ(u.protocol, "http");
+  EXPECT_EQ(u.host, "example.com");
+  EXPECT_EQ(u.port, 8080u);
+  EXPECT_EQ(u.path, "/dir/file.ini");
+  EXPECT_TRUE(u.query.empty());
+}
+
+TEST(net_url, parse_extracts_the_query_string) {
+  const net::url u = net::parse("http://nsclient.mydom.local/nsclient/nsclient.php?RootFolder=myhost/&Filename=nsclient.ini");
+  EXPECT_EQ(u.protocol, "http");
+  EXPECT_EQ(u.host, "nsclient.mydom.local");
+  // The path stops at the '?' ...
+  EXPECT_EQ(u.path, "/nsclient/nsclient.php");
+  // ... and the parameters land in query, '&' and '/' included.
+  EXPECT_EQ(u.query, "RootFolder=myhost/&Filename=nsclient.ini");
+}
+
+TEST(net_url, get_request_path_reassembles_path_and_query) {
+  // What a downloader has to put on the request line (issue #460).
+  const net::url u = net::parse("https://host/cfg.php?a=1&b=2");
+  EXPECT_EQ(u.get_request_path(), "/cfg.php?a=1&b=2");
+}
+
+TEST(net_url, get_request_path_without_a_query_is_just_the_path) {
+  const net::url u = net::parse("https://host/settings.ini");
+  EXPECT_EQ(u.get_request_path(), "/settings.ini");
+  // No trailing '?' when there is nothing to pass.
+  EXPECT_EQ(u.get_request_path().find('?'), std::string::npos);
+}
+
+TEST(net_url, get_request_path_keeps_an_empty_query_marker_out) {
+  // "?" with nothing after it parses to an empty query and must not be
+  // re-emitted, otherwise every plain url would grow a stray '?'.
+  const net::url u = net::parse("http://host/f.ini?");
+  EXPECT_EQ(u.path, "/f.ini");
+  EXPECT_TRUE(u.query.empty());
+  EXPECT_EQ(u.get_request_path(), "/f.ini");
+}
+
+TEST(net_url, to_string_round_trips_a_url_with_parameters) {
+  const std::string raw = "http://example.com:8080/cfg.php?host=a&mode=b";
+  EXPECT_EQ(net::parse(raw).to_string(), raw);
+}
+
+TEST(net_url, to_string_includes_port_only_when_set) {
+  EXPECT_EQ(net::parse("http://example.com/x.ini").to_string(), "http://example.com/x.ini");
+  EXPECT_EQ(net::parse("http://example.com:81/x.ini").to_string(), "http://example.com:81/x.ini");
+}
+
+TEST(net_url, ini_paths_keep_their_windows_drive_letter) {
+  // "ini://C:/foo" must not read "C" as a host and ":/foo" as a port; the
+  // parser skips port handling for the ini and registry protocols.
+  const net::url u = net::parse("ini://${shared-path}/nsclient.ini");
+  EXPECT_EQ(u.protocol, "ini");
+  EXPECT_TRUE(u.query.empty());
+  EXPECT_EQ(u.get_request_path(), u.path);
+}
+
+// --- request-line safety ----------------------------------------------------
+
+TEST(net_url, encode_query_leaves_legal_characters_alone) {
+  // Everything RFC 3986 permits in a query has to survive verbatim, or the
+  // parameters stop meaning what the operator wrote.
+  const std::string legal = "RootFolder=myhost/&Filename=nsclient.ini";
+  EXPECT_EQ(net::encode_query(legal), legal);
+  EXPECT_EQ(net::encode_query("a=1&b=2;c=3,d=4+5:6@7?8*9!$'()~-._"), "a=1&b=2;c=3,d=4+5:6@7?8*9!$'()~-._");
+}
+
+TEST(net_url, encode_query_escapes_a_space) {
+  EXPECT_EQ(net::encode_query("Folder=my host"), "Folder=my%20host");
+}
+
+TEST(net_url, encode_query_escapes_crlf) {
+  // The interesting one: unescaped, this splits the request line in two.
+  EXPECT_EQ(net::encode_query("a=1\r\nX-Evil: yes"), "a=1%0D%0AX-Evil:%20yes");
+}
+
+TEST(net_url, encode_query_does_not_double_encode) {
+  // A query written already-encoded must not turn "%20" into "%2520".
+  EXPECT_EQ(net::encode_query("Folder=my%20host"), "Folder=my%20host");
+  EXPECT_EQ(net::encode_query("a=%2F%2f"), "a=%2F%2f");
+}
+
+TEST(net_url, encode_query_escapes_a_stray_percent) {
+  // A '%' that introduces no valid pair is not an escape.
+  EXPECT_EQ(net::encode_query("a=100%"), "a=100%25");
+  EXPECT_EQ(net::encode_query("a=%zz"), "a=%25zz");
+  EXPECT_EQ(net::encode_query("a=%2"), "a=%252");
+}
+
+TEST(net_url, encode_query_escapes_high_bytes_and_controls) {
+  EXPECT_EQ(net::encode_query(std::string("a=\x01")), "a=%01");
+  EXPECT_EQ(net::encode_query(std::string("a=\xc3\xa5")), "a=%C3%A5");
+}
+
+TEST(net_url, get_request_path_encodes_the_query) {
+  const net::url u = net::parse("http://host/cfg.php?Folder=my host");
+  EXPECT_EQ(u.get_request_path(), "/cfg.php?Folder=my%20host");
+}
+
+// --- log-safe rendering (keeps parameters out of the log) -------------------
+
+TEST(net_url, to_log_safe_string_drops_the_query) {
+  const net::url u = net::parse("https://cfgsrv:8443/nsclient.php?token=s3cret&host=a");
+  EXPECT_EQ(u.to_log_safe_string(), "https://cfgsrv:8443/nsclient.php");
+  EXPECT_EQ(u.to_log_safe_string().find("s3cret"), std::string::npos);
+  // to_string() is still the faithful rendering.
+  EXPECT_NE(u.to_string().find("token=s3cret"), std::string::npos);
+}
+
+TEST(net_url, to_log_safe_string_equals_to_string_without_a_query) {
+  const net::url u = net::parse("http://cfgsrv/settings.ini");
+  EXPECT_EQ(u.to_log_safe_string(), u.to_string());
+}
+
+TEST(net_url, get_baseurl_and_get_path_split_the_url) {
+  const net::url u = net::parse("https://cfgsrv:8443/dir/nsclient.php?token=s3cret");
+  EXPECT_EQ(u.get_baseurl(), "https://cfgsrv:8443");
+  EXPECT_EQ(u.get_path(), "/dir/nsclient.php");
+  EXPECT_EQ(u.get_baseurl() + u.get_path(), u.to_log_safe_string());
+}
+
+TEST(net_url, get_baseurl_omits_an_unset_port) {
+  EXPECT_EQ(net::parse("http://cfgsrv/x.ini").get_baseurl(), "http://cfgsrv");
+}
+
+TEST(net_url, apply_and_import_carry_the_query) {
+  net::url base = net::parse("http://host/a.ini");
+  const net::url with_query = net::parse("http://host/b.ini?k=v");
+
+  net::url applied = base;
+  applied.apply(with_query);
+  EXPECT_EQ(applied.get_request_path(), "/b.ini?k=v");
+
+  net::url imported = net::parse("http://host/");
+  imported.path.clear();
+  imported.import(with_query);
+  EXPECT_EQ(imported.get_request_path(), "/b.ini?k=v");
+}

include/net/socket/socket_helpers.cpp

@@ -35,6 +35,13 @@ std::string socket_helpers::expand_hostname(std::string spec) {
   if (spec == "auto-lc") return boost::algorithm::to_lower_copy(host_name);
   if (spec == "auto-uc") return boost::algorithm::to_upper_copy(host_name);
 
+  // The full name exactly as the system reports it. ${host} stops at the first
+  // '.', so without this there is no way to get the fqdn from inside a template
+  // - only by setting the whole spec to "auto", which a template cannot do.
+  str::utils::replace(spec, "${hostname_uc}", boost::algorithm::to_upper_copy(host_name));
+  str::utils::replace(spec, "${hostname_lc}", boost::algorithm::to_lower_copy(host_name));
+  str::utils::replace(spec, "${hostname}", host_name);
+
   const str::utils::token dn = str::utils::getToken(host_name, '.');
   str::utils::replace(spec, "${host}", dn.first);
   str::utils::replace(spec, "${domain}", dn.second);

include/net/socket/socket_helpers.hpp

@@ -70,9 +70,10 @@ void validate_certificate(const std::string& certificate, std::list<std::string>
 //   "auto"     -> system host name as-is
 //   "auto-lc"  -> system host name, lower-cased
 //   "auto-uc"  -> system host name, upper-cased
-//   anything else: ${host}, ${domain}, ${host_lc}, ${host_uc}, ${domain_lc}
-//   and ${domain_uc} are substituted from the system host name (split on the
-//   first '.' into host and domain). Other text is preserved.
+//   anything else: ${hostname}, ${hostname_lc} and ${hostname_uc} are the
+//   system host name as reported; ${host}, ${domain}, ${host_lc}, ${host_uc},
+//   ${domain_lc} and ${domain_uc} are substituted from it after splitting on
+//   the first '.' into host and domain. Other text is preserved.
 std::string expand_hostname(std::string spec);
 
 class socket_exception : public std::exception {

include/net/socket/socket_helpers_test.cpp

@@ -291,6 +291,30 @@ TEST(ExpandHostname, HostPlaceholderIsExpanded) {
   EXPECT_NE(out.find("-suffix"), std::string::npos);
 }
 
+TEST(ExpandHostname, HostnamePlaceholderIsTheFullSystemName) {
+  // ${host} stops at the first '.', ${hostname} does not - that is the whole
+  // point of having both.
+  const std::string host = boost::asio::ip::host_name();
+  EXPECT_EQ(socket_helpers::expand_hostname("${hostname}"), host);
+  EXPECT_EQ(socket_helpers::expand_hostname("a=${hostname}&b=1"), "a=" + host + "&b=1");
+}
+
+TEST(ExpandHostname, HostnameCasePlaceholders) {
+  const std::string host = boost::asio::ip::host_name();
+  EXPECT_EQ(socket_helpers::expand_hostname("${hostname_lc}"), boost::algorithm::to_lower_copy(host));
+  EXPECT_EQ(socket_helpers::expand_hostname("${hostname_uc}"), boost::algorithm::to_upper_copy(host));
+}
+
+TEST(ExpandHostname, HostnameAndHostPlaceholdersDoNotCollide) {
+  // "${host}" is a character-wise prefix of "${hostname}" up to the brace, so a
+  // careless replace order would rewrite "${hostname}" into "<host>name}".
+  const std::string host = boost::asio::ip::host_name();
+  const std::string out = socket_helpers::expand_hostname("${hostname}|${host}|${hostname_lc}|${host_lc}");
+  EXPECT_EQ(out.find("${"), std::string::npos) << out;
+  EXPECT_EQ(out.find("name}"), std::string::npos) << out;
+  EXPECT_EQ(out.substr(0, host.size()), host) << out;
+}
+
 TEST(ExpandHostname, CasePlaceholdersAreExpanded) {
   // No assertion on the exact host name (varies per machine), only that all
   // placeholders are substituted away.