From 2e93709410b3fc7fb57773cc024ad437a9a2ca6c Mon Sep 17 00:00:00 2001 From: Chris Uthe Date: Thu, 20 Aug 2026 12:02:20 -0500 Subject: [PATCH] Mask userinfo wherever this player names a server A -s value may carry credentials -- ws://user:token@host:8927/sendspin -- and parse_server_url() passes a full URL through verbatim, deliberately: "the rest is the caller's to get right". So the whole of it reached "Connecting to %s" at info on every dial, and under the retry pacer that repeats the credential into the log on each backoff. Reported from the field, where a container wrapping this binary redacts credentials in its own log line and sendspin-cli's line then printed them anyway. redact_url_userinfo() in src/cli.{h,cpp} is what names a server now. It masks the secret half 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 and something to whoever else ends up holding it. It reads the authority the way a URL parser does -- ending at the first /, ? or #, splitting userinfo at the last @ inside it and the password at the first : -- so an @ in a path or a query is left alone, and a password containing colons is not printed as a prefix of itself. Every message that quotes a -s value goes through it, not only the dial line: a URL that does not parse is the one most likely to have been mistyped around a password. That includes two fragments the prefix alone would have missed -- -s 'user:tok@host' puts the token in port_text, and -s '[::1]:tok@host' does too -- and the no-mDNS refusal, which is the one message that prints whatever follows a reserved mdns: prefix. The dial itself is handed the real URL, and nothing credential-bearing reaches disk: last-server holds the server id. The discovery line goes through it as well, though a discovered URL has no userinfo to hide, so that both "Connecting to" lines have one spelling and one place to change. src/mdns_dnssd.cpp logs the same URL when it first resolves and deliberately does not: it depends on mdns.h and log.h and nothing else, and pulling cli.h into the mDNS backend to restate a guarantee discovered_server_url() already owns would cost more than it buys. Three limits, all recorded in docs/ROADMAP.md rather than left to be found. Lines tagged sendspin.* are the library's own: sendspin-cpp v0.7.0 logs the URL it dials at info and again at error through macros with no sink hook, so nothing here can reach them -- the same missing hook that already put timestamps and per-tag filtering out of reach. A credential in a query string is not masked, ?token= being indistinguishable from any other query. And a userinfo field holding an unencoded /, ? or # ends the authority early and comes back whole; RFC 3986 requires those percent-encoded and such a URL does not name the host it appears to, so it is a malformed value printed faithfully, and the docs say to percent-encode. Worth knowing, and the reason the ROADMAP entry names refusal as the fix this only documents: userinfo never reaches the wire at all. The six-argument UrlParser::parse() IXWebSocket's transport calls copies out scheme, host, path, query and port and discards the user and password it just parsed, and neither 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 the player accepts a credential it cannot send, which is the worse bug and a breaking change to fix; README and the wiki now say so instead of implying such a URL authenticates anything. Three tests in tests/cli_test.cpp, taking the suite from 347 to 350, none of which opens a socket: the masking matrix, the passthrough set with the unencoded delimiter pinned as a known boundary, and one asserting no rejection reason quotes a credential -- written over the reason rather than per message, so a rejection added later is covered without being listed. That the dial sink actually calls the helper is a scripts/smoke_test.sh check, src/main.cpp not being linked by the test binary; it holds every line not tagged sendspin.* to account and says in place why that boundary is where it is. --- README.md | 23 +++++++++ docs/ROADMAP.md | 60 +++++++++++++++++++++++ docs/wiki/Configuration.md | 15 ++++++ scripts/smoke_test.sh | 47 ++++++++++++++++++ src/cli.cpp | 74 ++++++++++++++++++++++++---- src/cli.h | 29 +++++++++++ src/main.cpp | 14 ++++-- tests/cli_test.cpp | 98 ++++++++++++++++++++++++++++++++++++++ 8 files changed, 348 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 1576636..2aab217 100644 --- a/README.md +++ b/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 `:`, 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 diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index e4ff3f4..e8ef5ed 100644 --- a/docs/ROADMAP.md +++ b/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 diff --git a/docs/wiki/Configuration.md b/docs/wiki/Configuration.md index 705a729..6195f31 100644 --- a/docs/wiki/Configuration.md +++ b/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 diff --git a/scripts/smoke_test.sh b/scripts/smoke_test.sh index 2790947..77d5467 100755 --- a/scripts/smoke_test.sh +++ b/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' } diff --git a/src/cli.cpp b/src/cli.cpp index 65b57f4..816e30c 100644 --- a/src/cli.cpp +++ b/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 ':' after ']', got '" + rest + "'"; + error = "-s '" + shown + "': expected ':' 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 + "]:'"; + error = "-s '" + shown + "': an IPv6 literal must be bracketed -- try '[" + shown + + "]' or '[" + shown + "]:'"; return false; } else { host = server.substr(0, colon); @@ -1165,7 +1181,7 @@ 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; } @@ -1173,7 +1189,8 @@ bool parse_server_url(const std::string& server, std::string& url, std::string& // 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; } @@ -1181,6 +1198,45 @@ bool parse_server_url(const std::string& server, std::string& url, std::string& 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) { diff --git a/src/cli.h b/src/cli.h index c9b5f1b..95bdbe9 100644 --- a/src/cli.h +++ b/src/cli.h @@ -294,6 +294,35 @@ void print_version(std::FILE* out); /// @return true if `server` resolved to a URL. bool parse_server_url(const std::string& server, std::string& url, std::string& error); +/// @brief The spelling of a server URL that is safe to log: its userinfo masked. +/// +/// A `-s` value may carry credentials -- `ws://user:token@host:8927/sendspin` -- and every +/// line that names a server is a line an operator pastes into an issue, so every message that +/// quotes one comes through here. The URL handed to the dial is untouched, which is also the +/// limit of what this buys: the library logs the URL it dials itself, and those lines are not +/// this layer's to redact. +/// +/// Masks only the secret half. A `user:password` pair keeps its username, which is what makes +/// the line still worth reading -- `ws://user:***@host:8927/sendspin` names the endpoint that +/// was dialled. A userinfo field with no colon is indistinguishable from a bearer token, so +/// the whole of it goes: `ws://***@host`. The mask is a fixed `***` either way, since the +/// length of a secret is itself worth nothing to a reader and something to an attacker. +/// +/// Reads the authority the way a URL parser would -- it ends at the first `/`, `?` or `#`, and +/// the userinfo at the last `@` inside it -- which is what makes this safe on strings that are +/// not URLs at all: an `@` in a path or a query, or a whole second URL smuggled into one, is +/// left alone. A value with no scheme is read as a bare authority, which is what a rejected +/// `-s` value is. Anything with no userinfo, an empty one (`ws://@host`) or an empty password +/// (`ws://user:@host`) comes back unchanged: there is nothing there to hide. +/// +/// Reading it that way is also the one thing a caller has to know the limit of: a userinfo +/// field holding an *unencoded* `/`, `?` or `#` ends the authority early, so there is no `@` +/// left inside it to split on and the value comes back whole. RFC 3986 requires those +/// percent-encoded there, and such a URL does not name the host it appears to, so it is a +/// malformed value rather than a parse to repair by guesswork -- but it is not masked. +/// @return `url` with its userinfo masked, or `url` itself when it carries none. +std::string redact_url_userinfo(const std::string& url); + /// @brief Reads a -s value as the reserved discovery form, if that is what it is. /// /// `mdns:` asks for a server whose TXT `name` is ``; a bare `mdns:` asks for diff --git a/src/main.cpp b/src/main.cpp index d6b8393..d26ef40 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -245,7 +245,8 @@ class OutboundMode { } } else { url = this->opts_.server_url; - log_line(LogLevel::INFO, LOG_TAG_OUTBOUND, "Connecting to %s", url.c_str()); + log_line(LogLevel::INFO, LOG_TAG_OUTBOUND, "Connecting to %s", + redact_url_userinfo(url).c_str()); } // Stamped before the dial rather than after, so the backoff measures from when the @@ -269,9 +270,16 @@ class OutboundMode { // Discovery already said why, at debug, when the instance first resolved. return false; } + // A discovered URL has no userinfo to hide -- discovered_server_url() builds it from a + // resolved address and a TXT path it requires to start with '/', so the authority is + // always just that address. It goes through the helper anyway so that both "Connecting + // to" lines have one spelling and one place to change it, not because this one is + // suspect. The mDNS backend logs the same URL when it first resolves and does *not* do + // this: src/mdns_dnssd.cpp depends on mdns.h and log.h and nothing else, and pulling + // cli.h into it to restate a guarantee it already owns would cost more than it buys. log_line(LogLevel::INFO, LOG_TAG_OUTBOUND, - "Connecting to %s (server \"%s\") -- chosen because %s", url.c_str(), - chosen->instance.c_str(), reason.c_str()); + "Connecting to %s (server \"%s\") -- chosen because %s", + redact_url_userinfo(url).c_str(), chosen->instance.c_str(), reason.c_str()); return true; } diff --git a/tests/cli_test.cpp b/tests/cli_test.cpp index ef1926d..8a15523 100644 --- a/tests/cli_test.cpp +++ b/tests/cli_test.cpp @@ -627,6 +627,104 @@ TEST(ParseServerUrl, Rejected) { } } +// --------------------------------------------------------------------------- +// redact_url_userinfo(): what a logged server URL is allowed to say +// --------------------------------------------------------------------------- + +TEST(RedactUrlUserinfo, MasksTheSecretAndKeepsTheRest) { + const std::pair cases[] = { + // A user:password pair keeps its username: the line exists to say which endpoint was + // dialled, and a username is not the secret. + {"ws://alice:s3cr3t@host:8927/sendspin", "ws://alice:***@host:8927/sendspin"}, + {"wss://alice:s3cr3t@host/sendspin", "wss://alice:***@host/sendspin"}, + // One field with no colon could be a bearer token, so the whole of it goes. + {"ws://s3cr3t@host:8927/sendspin", "ws://***@host:8927/sendspin"}, + // A password may contain colons, so the split is on the *first* one -- splitting on the + // last would print a prefix of the secret. + {"ws://alice:s3c:r3t@host/sendspin", "ws://alice:***@host/sendspin"}, + // And the authority splits on the *last* '@', a host being unable to contain one. + {"ws://alice:s3c@r3t@host/sendspin", "ws://alice:***@host/sendspin"}, + // A bracketed IPv6 host keeps its brackets and its own colons. + {"ws://alice:s3cr3t@[2001:db8::1]:8927/sendspin", + "ws://alice:***@[2001:db8::1]:8927/sendspin"}, + {"ws://s3cr3t@[::1]:8927/sendspin", "ws://***@[::1]:8927/sendspin"}, + // No username is still a secret to hide. + {"ws://:s3cr3t@host/sendspin", "ws://:***@host/sendspin"}, + // A rejected -s value never had a scheme, so a bare authority is read as one. + {"alice:s3cr3t@host", "alice:***@host"}, + {"s3cr3t@host", "***@host"}, + }; + + for (const auto& [input, expected] : cases) { + EXPECT_EQ(redact_url_userinfo(input), expected) << input; + EXPECT_EQ(redact_url_userinfo(input).find("s3cr3t"), std::string::npos) + << "the secret survived in '" << input << "'"; + } +} + +TEST(RedactUrlUserinfo, LeavesAloneWhatHoldsNoSecret) { + const char* unchanged[] = { + // Nothing to hide. + "", + "ws://host:8927/sendspin", + "wss://[2001:db8::1]:8927/sendspin", + "host:8927", + "mdns:Living room", + // A scheme naming nothing, and other fragments a rejected -s value arrives as. + "ws://", + "::1", + "[::1", + // An empty userinfo, and an empty password: masking either would invent a secret that is + // not there. + "ws://@host:8927/sendspin", + "ws://alice:@host:8927/sendspin", + // The authority ends at the first '/', '?' or '#', so an '@' past it is not a credential + // separator -- including one a server chose. A TXT `path` has to start with '/', which is + // what puts a hostile one here rather than in the authority. + "ws://host:8927/sendspin@1", + "ws://host:8927//alice:s3cr3t@evil/x", + "ws://host:8927/sendspin?token=a@b", + "ws://host:8927/sendspin#a@b", + // A known boundary, pinned rather than left to be discovered: an unencoded '/', '?' or + // '#' *inside* userinfo ends the authority early, so there is no '@' left in it to split + // on and the value comes back whole. RFC 3986 requires those percent-encoded there, and + // such a URL does not name the endpoint it looks like it names anyway -- the host reads + // as everything up to the '/'. README.md and the wiki say to percent-encode; the + // alternative, refusing the shape, would refuse a value -s accepts today. + "ws://alice:aGVsbG8/d29ybGQ=@host:8927/sendspin", + }; + + for (const char* input : unchanged) { + EXPECT_EQ(redact_url_userinfo(input), input); + } +} + +// Every rejection parse_server_url() writes quotes the -s value back at the operator, and a value +// that does not parse is exactly the one most likely to have been mistyped around a password. +// Asserted over the reason rather than per message, so a rejection added later is covered without +// being listed here. +TEST(RedactUrlUserinfo, NoRejectionReasonQuotesACredential) { + const char* cases[] = { + "alice:s3cr3t@host", // userinfo lands in the port field + "alice:s3cr3t@2001:db8::1", // ...and in the bracket-it-yourself advice + "http://alice:s3cr3t@host/sendspin", // the wrong scheme + "ws://", // a scheme and nothing else + "[::1]alice:s3cr3t@host", // a fragment after the closing bracket + "[::1]:s3cr3t@host", // ...and one that reads as a port + ":s3cr3t@host", // no host before the port + }; + + for (const char* input : cases) { + std::string url; + std::string error; + + ASSERT_FALSE(parse_server_url(input, url, error)) << "accepted '" << input << "'"; + ASSERT_FALSE(error.empty()) << "no reason given for '" << input << "'"; + EXPECT_EQ(error.find("s3cr3t"), std::string::npos) + << "'" << input << "' was refused with: " << error; + } +} + // --------------------------------------------------------------------------- // -s mdns:, the discovery form // ---------------------------------------------------------------------------