Mask userinfo wherever this player names a server

README.md

@@ -343,6 +343,29 @@ mode has already ruled out.
 ./build/sendspin-cli -s "mdns:Music Assistant"  # ...or one by its advertised name
 ```
 
+**A `-s` URL may carry userinfo, and this player does not send it.** `-s
+ws://user:token@host:8927/sendspin` is accepted and dials `host:8927/sendspin` — the
+credentials are dropped before the handshake, because the spec authenticates in the
+handshake (pairing and a PSK) and neither sendspin-cpp nor the IXWebSocket transport under
+it turns URL userinfo into an `Authorization` header. Verified on the wire: the upgrade
+request carries `Host`, `Upgrade`, `Sec-WebSocket-*` and `Origin`, and nothing else. So if
+something in front of your server wants HTTP Basic, a `-s` URL is not how to give it to it —
+and accepting a credential it cannot send is a wrong the player owes a fix, tracked in
+[docs/ROADMAP.md](docs/ROADMAP.md#6-daemonization-and-logging--shipped).
+
+What userinfo does do is get written down, so **the log masks it**: `Connecting to
+ws://user:***@host:8927/sendspin`, and the same in any complaint about a URL that did not
+parse. Two limits on that are worth knowing. Lines tagged `sendspin.*` are the library's own
+and print the URL in full — it logs what it dials through macros with no sink hook, so
+nothing here can reach them. And masking reads the URL the way a URL parser does, so a
+userinfo field containing an unencoded `/`, `?` or `#` — a raw base64 secret, say — ends the
+authority early and is *not* masked; percent-encode those (`%2F`, `%3F`, `%23`). Both are in
+[docs/ROADMAP.md](docs/ROADMAP.md#6-daemonization-and-logging--shipped).
+
+Either way `argv` is not something the player controls: `ps` shows a running process's
+command line to every local user on the box, so a URL you would rather not publish belongs
+in the config file, where the file's own `0600` is the protection.
+
 `mdns:` is reserved **before the first colon only**, the same way `-o` reads
 `<backend>:<device>`, so every address form still works — `hifi:8927` is a host
 and a port, and a bare `-s mdns` is still a host called `mdns`. Discovery is not a

docs/ROADMAP.md

@@ -550,6 +550,66 @@ And "the WebSocket port is already taken" turns out **not** to be a post-fork fa
 all: two instances on one `--port` both report listening, which is pre-existing
 sendspin-cpp/IXWebSocket behaviour and is not addressed here.
 
+**Userinfo in a `-s` URL is masked wherever this player names a server.** A `-s` value may
+carry userinfo — `ws://user:token@host:8927/sendspin` — and `parse_server_url()` passes a
+full URL through verbatim, so the whole of it reached `Connecting to %s` at `info` on every
+dial, repeating under the retry pacer's backoff. `redact_url_userinfo()` in
+`src/cli.{h,cpp}` is what names a server now: it masks the secret half of the userinfo and
+keeps the rest, so the line still says which endpoint was dialled —
+`ws://user:***@host:8927/sendspin`, or `***@host` where a single userinfo field could be a
+bearer token and there is nothing to tell it from one. A fixed `***` rather than one `*` per
+character, a secret's length being worth nothing to a reader. The dial itself is handed the
+real URL, and nothing credential-bearing reaches disk: `last-server` holds the server *id*,
+not a URL. Every rejection `parse_server_url()` writes goes through the same helper, a URL
+that does not parse being the one most likely to have been mistyped around a password. The
+helper is unit-tested on its own; that the dial sink actually calls it is a
+`scripts/smoke_test.sh` check, `src/main.cpp` not being linked by the test binary.
+
+**Userinfo is the boundary, and it is the boundary because of where the spec puts
+authentication.** The spec authenticates in the handshake — pairing and a PSK — so a credential
+in a URL is never something this protocol asked for; it is something *in front of* the server
+asking, a reverse proxy wanting HTTP Basic being the case this came from. And it does not reach
+that proxy either: the 6-argument `UrlParser::parse()` IXWebSocket's transport calls copies out
+scheme, host, path, query and port and **discards** the user and password it just parsed
+(`IXUrlParser.cpp`), and nothing in either library builds an `Authorization` header. Confirmed
+against a listening socket rather than by reading: the upgrade request carries `Host`,
+`Upgrade`, `Connection`, `Sec-WebSocket-*` and `Origin`, and no credential in any form. So
+userinfo here is *tolerated and dropped*, and the reason to mask it is that an operator will
+still type it, not that it does anything.
+
+**Two limits on the masking, both of them real.** A credential written into a *query* string is
+not masked: `?token=` is not a shape this player can tell from any other query, and guessing at
+parameter names would mask what is not secret while missing what is. And the mask reads the URL
+the way a URL parser does — the authority ends at the first `/`, `?` or `#` — so a userinfo
+field containing one of those *unencoded* ends the authority early, leaves no `@` inside it, and
+comes back unmasked. A raw base64 secret is the case that bites: `/` is in the alphabet, so a
+32-character token carries one about two times in five. RFC 3986 requires those percent-encoded
+in userinfo and IXWebSocket's own parser says the same, and such a URL does not resolve to the
+endpoint the operator meant in the first place — the host reads as everything up to that `/` —
+so this is a malformed value being printed faithfully rather than a parse to be repaired by
+guesswork. `README.md` and the wiki both say to percent-encode.
+
+**What is owed, and it is the honest answer to all of the above: stop accepting a credential
+this player cannot send.** Refusing a `-s` value that carries userinfo closes every leak by
+construction — a URL that is refused reaches neither `Connecting to %s` nor `connect_to()`, so
+it never reaches the `sendspin.*` lines either — and it closes the unencoded-delimiter shape
+with it, since that shape cannot reach the endpoint it appears to name in any case. It also
+stops the worse failure this item only documents: an operator running a player they believe is
+authenticated. It is a breaking change for anyone passing such a URL today, which is why it is
+owed here rather than done here. Masking is what this item ships; refusing is the fix.
+
+**A third thing this item does not claim: the library's own dial lines.** sendspin-cpp v0.7.0
+logs the URL it is dialling at `info` from `ConnectionManager::connect_to()` and again at
+`error` from `SendspinClientConnection`, through the same sink-less `SS_LOG*` macros that put
+timestamps and per-tag filtering out of reach above — so those lines still carry whatever the
+URL carries, at the default level, and no call this layer can make will change it.
+`connect_to()` takes a URL and nothing else, so the credentials cannot be moved off it either.
+A pipe over fd 2 would catch them and is rejected here for the reason it is rejected above.
+This one is sendspin-cpp's to fix, in the same place a log sink hook belongs; per
+`AI_POLICY.md` no upstream issue was opened here. Until it is, the boundary is exactly the tag
+prefix: every line tagged `sendspin.*` is the library's, and every line tagged otherwise is
+this player's and is held to the paragraph above.
+
 ### 7. Local control channel — *shipped*
 
 The player could only be driven by a remote controller, and `CMakeLists.txt` pinned

docs/wiki/Configuration.md

@@ -71,6 +71,21 @@ Booleans take `true`/`yes`/`on`/`1` or `false`/`no`/`off`/`0`. A line whose firs
 character is `#` is a comment; a `#` anywhere else is not, so a name or a path is free to
 contain one. Where a key appears twice, the last one wins.
 
+**A `server` URL that carries userinfo belongs here rather than on the command line — and it
+does not authenticate anything.** `ws://user:token@host:8927/sendspin` is accepted, and the
+`user:token` is dropped before the handshake: Sendspin authenticates in the handshake, not in
+the URL, so nothing turns userinfo into an `Authorization` header. If a proxy in front of
+your server wants HTTP Basic, this is not the way to give it to it — and the player accepting
+a credential it cannot send is a wrong it owes a fix, not a feature to configure around.
+
+Where it still matters is what gets written down. A URL typed at `-s` is in the process's
+`argv`, which `ps` shows to **every** local user for as long as the player runs; in this file
+it is protected by the file's own permissions, so `chmod 0600` and an owner is the whole of
+the fix. The player also masks userinfo out of every line *it* writes — `Connecting to
+ws://user:***@host:8927/sendspin` — but every line tagged `sendspin.*` is the library's own
+and prints the URL in full, at the default log level. Treat the log of a `-s` run with
+userinfo in it as sensitive either way.
+
 **Five things cannot come from a file**: `-l`, `-z`, `--config`, `--help` and `--version`.
 Run shape stays on the command line, and a config naming one is refused as an unknown key.
 Excluding them is reversible; debugging a `daemonize` that came out of a file under systemd

scripts/smoke_test.sh

@@ -44,6 +44,11 @@ readonly PORT_CONTROL=39285
 readonly PORT_CONTROL_SECOND=39286
 readonly PORT_CONFIG=39287
 readonly PORT_DELAY=39288
+readonly PORT_REDACTION=39289
+
+# Not a phase port: the address the redaction check *dials*, chosen so that nothing answers it.
+# The check is about what gets written down on the way to a dial, so the dial has to fail.
+readonly PORT_NO_SERVER=39290
 
 readonly MDNS_INSTANCE="sendspin-cli-smoke"
 
@@ -787,6 +792,47 @@ check_static_delay() {
     await_child "$pid" "$EXIT_TIMEOUT_S" >/dev/null 2>&1 || true
 }
 
+# A -s URL carrying credentials is logged with them masked, and no line of ours prints them.
+#
+# Not reachable from tests/: src/main.cpp is not in the sendspin-cli-tests target, and the claim
+# is about a line a *running* player writes on its way out to a server. redact_url_userinfo() is
+# unit-tested on its own; what this adds is that the dial sink actually calls it.
+#
+# The library's own lines are excluded, and naming that exclusion is the point of doing it this
+# way rather than grepping the whole file: sendspin-cpp v0.7.0 logs the URL it dials at info
+# (`conn_mgr`) and again at error (`client_connection`) through a bare fprintf with no sink hook,
+# so nothing here can redact them -- see docs/ROADMAP.md. Those lines are tagged `sendspin.*`;
+# every line tagged otherwise is this player's own, and is what this check holds to account.
+check_credential_redaction() {
+    local log="$WORK_DIR/redaction.log"
+    local out="$WORK_DIR/redaction.out"
+    # Not a real credential anywhere, and it never leaves this host: PORT_NO_SERVER answers nothing,
+    # so the dial fails before a byte is sent. It is on the command line because that is the shape
+    # of the leak being tested -- which is also why the docs tell operators to use the config file.
+    local secret="s3cr3t-not-a-real-password"
+
+    "$BIN" --no-mdns --no-control -o null --port "$PORT_REDACTION" "${NO_CONFIG[@]}" \
+        -f "$log" -s "ws://smoke:$secret@127.0.0.1:$PORT_NO_SERVER/sendspin" >"$out" 2>&1 &
+    local pid=$!
+    STARTED_PIDS+=("$pid")
+
+    wait_for_line "$log" "Connecting to" "$BOOT_TIMEOUT_S" ||
+        fail "no dial line within ${BOOT_TIMEOUT_S}s. Log: $(cat "$log")"
+
+    # -F, because the mask and the address are both regex metacharacters written literally.
+    grep -qF "Connecting to ws://smoke:***@127.0.0.1:$PORT_NO_SERVER/sendspin" "$log" ||
+        fail "the dial line did not mask the -s userinfo: $(grep 'Connecting to' "$log")"
+
+    local leaked
+    leaked="$(grep -hv ' sendspin\.[^ :]*:' "$log" "$out" | grep -F "$secret" || true)"
+    [ -z "$leaked" ] ||
+        fail "a sendspin-cli log line printed the -s password: $leaked"
+    pass "the dial line masks a -s URL's userinfo, and no line of ours written by then holds it"
+
+    kill -TERM "$pid" 2>/dev/null || true
+    await_child "$pid" "$EXIT_TIMEOUT_S" >/dev/null 2>&1 || true
+}
+
 main() {
     [ -x "$BIN" ] ||
         fail "no executable at '$BIN' -- pass the path to sendspin-cli as the first argument"
@@ -803,6 +849,7 @@ main() {
     check_missing_runtime_dir
     check_static_delay
     check_config_file
+    check_credential_redaction
     printf 'smoke: every check passed\n'
 }
 

src/cli.cpp

@@ -49,6 +49,12 @@ constexpr const char* FALLBACK_NAME = "sendspin-cli";
 /// `ws://server.local:8927/sendspin` (include/sendspin/client.h, src/esp/client_connection.h).
 constexpr uint16_t DEFAULT_REMOTE_SERVER_PORT = 8927U;
 
+/// What stands in for a redacted secret in a logged URL, at a fixed width.
+///
+/// Fixed rather than one '*' per character: the length of a password is worth nothing to a
+/// reader of the log and something to whoever else ends up holding it.
+constexpr const char* USERINFO_MASK = "***";
+
 /// Long-only option values, picked outside the short-option alphabet so `-V`/`-p` stay
 /// unclaimed for squeezelite's own meanings.
 enum LongOnly {
@@ -780,8 +786,12 @@ bool parse_options(int argc, char* argv[], Options& out, std::FILE* err) {
             // build lacks: a flag that parses and then quietly discovers nothing is worse
             // than one that says the build cannot do it.
             out.discover = false;
+            // Redacted like every other message that quotes a -s value. A discovery spec is a
+            // service instance name and has no userinfo to carry, so the helper is a no-op on
+            // any sane one -- but `mdns:` is only reserved before the first colon, so what
+            // follows it is whatever was typed, and this is the one message that prints it.
             fail_for(Opt::Server,
-                     "-s '" + out.server +
+                     "-s '" + redact_url_userinfo(out.server) +
                          "': this build has no mDNS support, so it cannot discover a server. "
                          "Rebuild with dns_sd.h available (libavahi-compat-libdnssd-dev on "
                          "Debian/Ubuntu, avahi-compat-libdns_sd-devel on Fedora), or give -s an "
@@ -1104,20 +1114,25 @@ bool parse_server_url(const std::string& server, std::string& url, std::string&
         return false;
     }
 
+    // Every message below quotes the value the operator typed back at them, and that value may
+    // carry credentials -- so none of them quotes it raw, nor any fragment of it. A value with
+    // no userinfo, which is every well-formed one, comes back exactly as written.
+    const std::string shown = redact_url_userinfo(server);
+
     // A scheme means the caller is spelling out the whole URL -- path, port and all -- so
     // the only thing to check is that it is one we can actually speak.
     const size_t scheme_end = server.find("://");
     if (scheme_end != std::string::npos) {
         const std::string scheme = server.substr(0, scheme_end);
         if (scheme != "ws" && scheme != "wss") {
-            error = "-s '" + server + "': Sendspin runs over WebSocket, so the scheme must be " +
+            error = "-s '" + shown + "': Sendspin runs over WebSocket, so the scheme must be " +
                     "ws:// or wss://, not " + scheme + "://";
             return false;
         }
         // The rest is the caller's to get right -- port, path and all -- but a scheme with
         // nothing after it names no server at all, and would fail far from here.
         if (scheme_end + 3 == server.size()) {
-            error = "-s '" + server + "': a scheme but no host";
+            error = "-s '" + shown + "': a scheme but no host";
             return false;
         }
         url = server;
@@ -1133,14 +1148,15 @@ bool parse_server_url(const std::string& server, std::string& url, std::string&
         // closing bracket can be a port.
         const size_t bracket = server.find(']');
         if (bracket == std::string::npos) {
-            error = "-s '" + server + "': unterminated '[' -- an IPv6 literal reads [::1]:8927";
+            error = "-s '" + shown + "': unterminated '[' -- an IPv6 literal reads [::1]:8927";
             return false;
         }
         host = server.substr(0, bracket + 1);
         const std::string rest = server.substr(bracket + 1);
         if (!rest.empty()) {
             if (rest.front() != ':') {
-                error = "-s '" + server + "': expected ':<port>' after ']', got '" + rest + "'";
+                error = "-s '" + shown + "': expected ':<port>' after ']', got '" +
+                        redact_url_userinfo(rest) + "'";
                 return false;
             }
             port_text = rest.substr(1);
@@ -1153,8 +1169,8 @@ bool parse_server_url(const std::string& server, std::string& url, std::string&
         } else if (server.find(':', colon + 1) != std::string::npos) {
             // More than one colon and no brackets: an IPv6 literal written bare, where
             // there is no way to tell the address's colons from a port separator.
-            error = "-s '" + server + "': an IPv6 literal must be bracketed -- try '[" + server +
-                    "]' or '[" + server + "]:<port>'";
+            error = "-s '" + shown + "': an IPv6 literal must be bracketed -- try '[" + shown +
+                    "]' or '[" + shown + "]:<port>'";
             return false;
         } else {
             host = server.substr(0, colon);
@@ -1165,22 +1181,62 @@ bool parse_server_url(const std::string& server, std::string& url, std::string&
 
     // "[]" is as empty a host as "".
     if (host.empty() || host == "[]") {
-        error = "-s '" + server + "': no host before the port";
+        error = "-s '" + shown + "': no host before the port";
         return false;
     }
 
     // Only when no ':' was written at all does the server's own default apply. A written
     // but empty port is a truncated line, not a request for the default.
     uint16_t port = DEFAULT_REMOTE_SERVER_PORT;
     if (has_port && !parse_port(port_text, port)) {
-        error = "-s '" + server + "': '" + port_text + "' is not a port number (expected 1-65535)";
+        error = "-s '" + shown + "': '" + redact_url_userinfo(port_text) +
+                "' is not a port number (expected 1-65535)";
         return false;
     }
 
     url = "ws://" + host + ":" + std::to_string(port) + SENDSPIN_PATH;
     return true;
 }
 
+std::string redact_url_userinfo(const std::string& url) {
+    // What can carry userinfo is the authority, and only the authority: it starts after the
+    // scheme when there is one and at the front when there is not -- a rejected -s value is a
+    // bare authority -- and ends at the first delimiter that closes it. Ending it there is what
+    // keeps an '@' further along, in a path or a query, from reading as a credential separator.
+    const size_t scheme_end = url.find("://");
+    const size_t begin = scheme_end == std::string::npos ? 0 : scheme_end + 3;
+    const size_t end = url.find_first_of("/?#", begin);
+    const std::string authority =
+        url.substr(begin, end == std::string::npos ? std::string::npos : end - begin);
+
+    // The *last* '@' in it: a host cannot contain one and a userinfo field can, so anything
+    // before the final '@' is userinfo however many it holds.
+    const size_t at = authority.rfind('@');
+    if (at == std::string::npos) {
+        return url;
+    }
+    const std::string userinfo = authority.substr(0, at);
+
+    // The *first* ':': everything after it is one password, colons and all, so splitting on the
+    // last would print a prefix of the secret.
+    const size_t colon = userinfo.find(':');
+    std::string masked;
+    if (colon == std::string::npos) {
+        // Nothing to hide at all, or one field that could be a bearer token and so goes whole.
+        if (userinfo.empty()) {
+            return url;
+        }
+        masked = USERINFO_MASK;
+    } else if (colon + 1 == userinfo.size()) {
+        // A username and an empty password: masking would invent a secret that is not there.
+        return url;
+    } else {
+        masked = userinfo.substr(0, colon + 1) + USERINFO_MASK;
+    }
+
+    return url.substr(0, begin) + masked + url.substr(begin + at);
+}
+
 std::string default_client_name() {
     char hostname[256];
     if (gethostname(hostname, sizeof(hostname)) != 0) {