diff --git a/README.md b/README.md index 999b8e2..e016afc 100644 --- a/README.md +++ b/README.md @@ -54,8 +54,8 @@ sendspin-cli -n living-room -o hw:1,0 # Prefer formats, in order -- the server uses the first one it can encode sendspin-cli -n living-room --audio-format flac:48000:24:2,pcm:48000:24:2 -# Discover a server by its advertised name and connect to it, instead of waiting to be found -sendspin-cli -n living-room -s "mdns:Music Assistant" +# Connect out to a specific server, instead of waiting to be found +sendspin-cli -n living-room -s music.local ``` | Option | What it does | @@ -63,7 +63,7 @@ sendspin-cli -n living-room -s "mdns:Music Assistant" | `-n, --name ` | The friendly name a server displays. Defaults to this host's name. | | `-o, --output ` | Which sound card to play through. `-l` lists this host's devices and what they accept. | | `--audio-format [,...]` | Formats to offer first, in priority order, e.g. `flac:48000:24:2,pcm:48000:24:2`. Everything else the player normally offers still follows, and a server uses the first format it can encode, so it can still fall back to a later one. This sets a preference, not a restriction. | -| `-s, --server mdns:[]` | Discover a server over mDNS and connect to it, rather than waiting to be discovered. `mdns:` takes any server. Turns off the mDNS advertisement. | +| `-s, --server ` | Connect out to a server rather than waiting to be discovered, or `mdns:[]` to discover one over mDNS. Turns off the mDNS advertisement. | Any of these can go in a config file instead of on the command line — see [Configuration](https://github.com/Sendspin/sendspin-cpp-cli/wiki/Configuration). diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index d5eec84..6f7cf0c 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -14,9 +14,9 @@ daemonization. - Boots a `SendspinClient` with the `player`, `metadata` and `controller` roles, starts its WebSocket server, pumps `client.loop()`, and shuts down cleanly on `SIGINT`/`SIGTERM`. - Speaks both of the protocol's connection modes, and keeps them exclusive as the spec - requires: it advertises `_sendspin._tcp` over mDNS by default, and `-s mdns:` instead - discovers a server on `_sendspin-server._tcp` and dials it — retrying with a backoff - until it answers (item 5). A typed-in address is no longer accepted (item 26). + requires: it advertises `_sendspin._tcp` over mDNS by default, and `-s` instead dials out — + either a typed-in address or a `mdns:` server it discovers on `_sendspin-server._tcp` — + retrying with a backoff until it answers (item 5, item 26). - Defines the `AudioSink` seam (`src/audio_sink.h`) and plays real audio through it: auto-detected ALSA (item 2), PortAudio (item 3), PulseAudio (item 18) and PipeWire (item 19) backends, with the device-less null/stdout sink as the fallback, so the @@ -2460,71 +2460,33 @@ CLI can reach around it — so the flag cannot be expressed by any other route. on its own. **Not expected to move soon.** -### 26. `-s` takes no address — *shipped* +### 26. Direct-address dialling, removed then restored alongside `mdns:` — *shipped* -`-s` used to take a typed-in address — `[:]` or a `ws://` URL — beside its -`mdns:` discovery form. The spec has exactly two ways to connect: the server discovers the +`-s` briefly took only its `mdns:` discovery form: a typed-in `[:]` or `ws://` +URL was removed because the spec has exactly two ways to connect — the server discovers the client through its `_sendspin._tcp` advertisement, or the client discovers the server on -`_sendspin-server._tcp` and connects "using the advertised address and path". Dialling a -hand-entered address is neither, so it went. +`_sendspin-server._tcp` and connects "using the advertised address and path" — and a +hand-entered address is neither. + +That removal is **reversed by choice**: direct-address dialling is restored beside discovery, +because pointing a player straight at a known server is worth more here than matching the +spec's connection model exactly. This is a deliberate divergence, not a spec-conformance gap. **Shipped** in `src/cli.{h,cpp}`, `src/main.cpp`, `src/outbound.{h,cpp}`, `tests/cli_test.cpp`, `tests/config_file_test.cpp`, `tests/discovery_test.cpp` and `scripts/smoke_test.sh`: -- **`-s mdns:[]` is the only form**, typed or as `server =` in a config file, and it - behaves as it did: browse, pick with the last-server tie-break, dial on the retry pacer's - backoff, advertisement suppressed. -- **Anything else is a hard error at parse time**, not a warning, so an install still - configured with an address fails loudly rather than quietly changing how it connects. From a - config file the error names the file and line, and a bare `mdns` is asked whether it meant - `mdns:`. -- **The error does not quote the value.** An address was where credentials got typed, and - `redact_url_userinfo()` went with `parse_server_url()`: a discovered URL is built from a - resolved address, a port and a TXT `path`, and has no userinfo to mask. The smoke test's - credential-redaction check went with them. -- **`SENDSPIN_SERVER_URL` is answered only for the server_id the dial chose.** `LastDial` has - no "literal URL, taken at its word" case left; a dial with no id answers nothing. - -### 28. A second device outage in one stream never recovers — *shipped (hardware pass still owed)* - -Found by item 27's CoreAudio hardware pass — two unplugs in one run, the first back in 190 ms, -the second never — but it belongs to item 14 and sits in the shared `SinkRecovery`, so every -device-backed sink had it. **A rescan that succeeded retired the budget instead of restoring -it**: `rescan_done()` sent a recovered rescan and an exhausted one down the same branch, and -`reopen_spent_` was still set from the first outage. On the second, `reopen_due()` declined, -`escalate_()` found the rescan spent, and the sink discarded until the next track with no log -line at all. - -**Shipped** in `src/sink_recovery.{h,cpp}`, its callers (`src/alsa_sink.cpp`, -`src/pulse_sink.cpp`, `src/pipewire_sink.cpp`, `src/portaudio_sink.cpp`) and -`tests/sink_recovery_test.cpp`: - -- **A recovered rescan refills the budget** — the in-place reopen and the whole rescan ladder, - back to `SINK_RESCAN_DELAY_MS` — the same intent `reopen_done(true)` already had for its own - path. A recovered reopen still refills nothing, so the outage after it goes straight to the - rescan, and that rescan's recovery refills both. -- **The outage gap survives the refill.** Only the attempt bookkeeping refills; the - discarded-frame count is not `reset()`'s to drop here, since the first timed write still owes - the player that gap, and a device that dies again before one adds to it. -- **A device that never comes back still gives up.** Within one outage the budget is unchanged — - one reopen, then `SINK_RESCAN_ATTEMPTS` rescans on the doubling delay — and only a real - recovery refills it. -- **`rescan_abandoned()` for an attempt that did not recover anything.** The shutdown paths in - ALSA, PulseAudio and PipeWire used to report `rescan_done(true)` when `stop()` landed - mid-attempt, which would now refill; they abandon instead. PortAudio used to report `true` up - front whatever happened; it now abandons on each failure and reports `true` only when the - stream is really back, so its rebuild stays one-shot per outage. - -**A flapping device is not given a floor of its own.** One that recovers and dies again, -repeatedly, now keeps recovering — which is what it is doing — and the loop is already paced: -each outage spends the in-place reopen at most once, the next one has to wait out -`SINK_RESCAN_DELAY_MS` before its rescan, and a refill never shortens that below the base delay. -So the main loop pays at most one rescan per two seconds whatever the device does. A cap on -recoveries per stream would turn a flaky cable back into the silence this item removes. - -The regression tests (`ASecondOutageInTheSameStreamRecovers`, -`EveryOutageAfterARecoveryStillGivesUp`, `TheDiscardedGapSurvivesTheRefillIntoTheNextOutage`) -fail against the old helper and pass against the new one. **Still owed:** two unplug/replug -cycles in one stream on real hardware — ALSA first, since it has the most users, then CoreAudio -with item 27's harness. +- **`-s
` dials directly again**, typed or as `server =` in a config file: a bare + host (server default port 8927), `host:port`, a full `ws://`/`wss://` URL, or a bracketed + IPv6 literal. `parse_server_url()` resolves it once at parse time, so a bad address fails + before the daemon starts. +- **`-s mdns:[]` still discovers**, unchanged: browse, pick with the last-server + tie-break, dial on the retry pacer's backoff. `mdns:` is reserved only before the first + colon, so `hifi:8927` is a host and a bare `-s mdns` is a host named `mdns`. +- **Any `-s` suppresses the advertisement**, address or discovery — the invariant + `advertises()` keeps, so no new code path advertises while an address is configured. +- **A parse error never quotes the credential.** An address can carry userinfo, so + `redact_url_userinfo()` masks it out of every message and every line the player logs. The + smoke test's credential-redaction check covers the dial line. +- **`SENDSPIN_SERVER_URL`** answers a discovery dial only for the `server_id` it chose, and a + literal-address dial (no id, unverifiable) at its word — `LastDial` carries both cases. diff --git a/docs/wiki/Advanced-Usage.md b/docs/wiki/Advanced-Usage.md index f0b104b..168950b 100644 --- a/docs/wiki/Advanced-Usage.md +++ b/docs/wiki/Advanced-Usage.md @@ -12,19 +12,18 @@ server to connect: sendspin-cli -n living-room ``` -To make the player find a server and connect to it instead, use `-s`/`--server` with -`mdns:`. The player browses `_sendspin-server._tcp` and dials what it finds. This disables +To make the player connect to a known server instead, use `-s`/`--server`. This disables mDNS advertisement because the Sendspin protocol does not allow both modes at once: ```bash +sendspin-cli --server music.local # the server port defaults to 8927 +sendspin-cli --server music.local:9000 +sendspin-cli --server ws://music.local:9000/sendspin +sendspin-cli --server "[2001:db8::1]:8927" # an IPv6 literal must be bracketed sendspin-cli --server mdns: # discover any server sendspin-cli --server "mdns:Music Assistant" # ...or one by its advertised name ``` -`-s` does not take an address. The spec only has a player connect to a server it has -discovered, so a host, `host:port` or `ws://` URL is refused at startup. Where mDNS cannot -reach between the two, leave `-s` off and point the server at the player instead. - An outbound connection retries until it answers, and `--mdns-name` is unused in this mode. `--no-mdns` turns the advertisement off without switching modes. diff --git a/docs/wiki/Configuration.md b/docs/wiki/Configuration.md index 3ea91cd..1ee2897 100644 --- a/docs/wiki/Configuration.md +++ b/docs/wiki/Configuration.md @@ -52,7 +52,7 @@ already does that. | `id` | `--id` | the stable client id a server files this player's settings under — two players on one host must not share it | derived from the interface MAC | | `manufacturer` | `--manufacturer` | the manufacturer `client/hello` reports | `sendspin-cpp-cli` | | `product-name` | `--product-name` | the product name `client/hello` reports | `sendspin-cli` | -| `server` | `-s`, `--server` | `mdns:` to discover any server, or `mdns:` for the one advertised under that name | none — wait to be discovered | +| `server` | `-s`, `--server` | `[:]`, a `ws://` URL, or `mdns:[]` | none — wait to be discovered | | `port` | `--port` | the port this player's own WebSocket server listens on | `8928` | | `buffer-ms` | `--buffer-ms` | audio the output backend keeps queued, 10–2000 | `100` | | `audio-format` | `--audio-format` | preferred formats, comma-separated in priority order: `codec:rate:depth:channels[,...]`, e.g. `flac:48000:24:2,pcm:48000:24:2`; offered first in that order, with the rest of the advertised list still behind them, so a server that cannot encode them falls back — a preference, not a restriction; refuses to start if the advertised list does not carry every one — it carries a single channel count — and an `opus` entry at anything but 48000/16 and at most 2 channels is refused outright | none — device-derived order | @@ -77,6 +77,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/docs/wiki/Controlling-the-Player.md b/docs/wiki/Controlling-the-Player.md index c16973d..1eed59f 100644 --- a/docs/wiki/Controlling-the-Player.md +++ b/docs/wiki/Controlling-the-Player.md @@ -211,10 +211,12 @@ with — gathered when the stream started, because a stream usually ends when it connection goes and there is nothing left to ask by then. `SENDSPIN_SERVER_URL` is the one to read carefully: it is the URL this run dialled, not -which server answered. A lost connection clears it, and it is exported only if the stream -arrived from the discovered server it dialled: `-s` leaves the inbound listener up, and the -library reports that a connection is up, not where it came from, so the server id is what -tells a server that dialled *in* from the player's own dial. Read `SENDSPIN_SERVER_ID` when a hook has to be certain which server it is acting +which server answered. A lost connection clears it, and when discovery chose the server it +is exported only if the stream arrived from the server it dialled. On a literal `-s URL` +run the rest is out of the player's reach: `-s` leaves the inbound listener up, and a +server that dials *in* while that attempt is outstanding or has failed is a connection the +player cannot tell from its own — the library reports that one is up, not where it came +from. Read `SENDSPIN_SERVER_ID` when a hook has to be certain which server it is acting on. The hook never blocks playback: it is spawned and reaped from the main loop, its output diff --git a/docs/wiki/Getting-Started-on-Linux.md b/docs/wiki/Getting-Started-on-Linux.md index 4472ec7..bb8712b 100644 --- a/docs/wiki/Getting-Started-on-Linux.md +++ b/docs/wiki/Getting-Started-on-Linux.md @@ -166,16 +166,14 @@ The player advertises `_sendspin._tcp` and waits. Open your Sendspin controller should appear under the name it logged — which is `-n`, falling back to this host's name. Nothing needs configuring on the server side. -To go the other way and have the player discover the server and dial it instead, set -`server` in the config: +To go the other way and have the player dial the server instead, set `server` in the config: ```ini -# The server advertised as "Music Assistant"; `server = mdns:` takes any -server = mdns:Music Assistant +# Pick one — a repeated key takes its last value, so keep a single `server` line: +server = 192.168.1.10 # a host, port 8927 assumed +#server = mdns:Music Assistant # or discover one by its advertised name ``` -There is no address form: the spec only has a player connect to a server it has discovered. - Any `server` value turns the mDNS advertisement off. That is the spec's rule rather than a preference here, and the two modes are mutually exclusive by design — see [Connection modes](Advanced-Usage#connection-modes). diff --git a/docs/wiki/Home.md b/docs/wiki/Home.md index fb7b646..bbf7241 100644 --- a/docs/wiki/Home.md +++ b/docs/wiki/Home.md @@ -26,8 +26,8 @@ I mdns: advertising _sendspin._tcp as "living-room" on port 8928 (path /sendspin ``` That is the whole of the usual setup: nothing to configure on either end. A Sendspin -server discovers the advertisement and dials in. `-s mdns:` inverts it: this player -discovers a server and dials it, which the protocol treats as the other of two mutually exclusive +server discovers the advertisement and dials in. `-s ` inverts it and makes this +player the one dialling, which the protocol treats as the other of two mutually exclusive modes — see [Connection modes](Advanced-Usage#connection-modes). Audio goes out through ALSA (the Linux default) or PortAudio (the cross-platform one, and diff --git a/docs/wiki/Troubleshooting.md b/docs/wiki/Troubleshooting.md index 0cb30cd..e57d7ee 100644 --- a/docs/wiki/Troubleshooting.md +++ b/docs/wiki/Troubleshooting.md @@ -209,8 +209,8 @@ sudo apt install avahi-daemon libavahi-compat-libdnssd1 quietly finding nothing: ``` -I mdns: This build has no mDNS support, so it can neither be discovered nor discover a -server: point a server at ws://:8928/sendspin. See docs/ROADMAP.md. +I mdns: This build has no mDNS support, so it cannot be discovered: point a server at +ws://:8928/sendspin, or dial one with -s. See docs/ROADMAP.md. ``` Rebuild with `libavahi-compat-libdnssd-dev` present, or point the server at the URL by hand. diff --git a/packaging/sendspin-cli.conf.example b/packaging/sendspin-cli.conf.example index 3e9f149..70a6df5 100644 --- a/packaging/sendspin-cli.conf.example +++ b/packaging/sendspin-cli.conf.example @@ -31,7 +31,8 @@ # The port this player's own WebSocket server listens on, for a server that connects in. #port = 8928 -# Discover a server and dial it: mdns: for any, mdns: for one. Disables advertising. +# Dial a server instead of waiting to be found: [:], a ws:// URL, or mdns: to +# discover one by its advertised name. Any value here disables advertising, as -s does. #server = mdns:Living Room # Latency added after the audio port, 0-5000 ms. A first-run default: a remembered delay wins. diff --git a/scripts/smoke_test.sh b/scripts/smoke_test.sh index b934591..50d1e6b 100755 --- a/scripts/smoke_test.sh +++ b/scripts/smoke_test.sh @@ -32,6 +32,10 @@ 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 nothing answers it. +readonly PORT_NO_SERVER=39290 readonly MDNS_INSTANCE="sendspin-cli-smoke" @@ -665,6 +669,38 @@ 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. +# The library's own `sendspin.*` lines are excluded: v0.7.0+ logs the dialled URL through a bare +# fprintf with no sink hook, so nothing here can redact them -- see docs/ROADMAP.md. +check_credential_redaction() { + local log="$WORK_DIR/redaction.log" + local out="$WORK_DIR/redaction.out" + # Not a real credential, and it never leaves this host: PORT_NO_SERVER answers nothing, so the + # dial fails before a byte is sent. On the command line because that is the leak being tested. + 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" @@ -681,6 +717,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 b28d02e..2b9445c 100644 --- a/src/cli.cpp +++ b/src/cli.cpp @@ -41,6 +41,13 @@ namespace { constexpr const char* FALLBACK_NAME = "sendspin-cli"; +/// The port a Sendspin *server* listens on, for -s to dial when none is given. +/// Not DEFAULT_SERVER_PORT (8928): that is the port *this* player serves on. +constexpr uint16_t DEFAULT_REMOTE_SERVER_PORT = 8927U; + +/// The fixed-width stand-in for a redacted secret in a logged URL. +constexpr const char* USERINFO_MASK = "***"; + /// Long-only option values, outside the short-option alphabet. enum LongOnly { OPT_VERSION = 0x100, @@ -129,6 +136,27 @@ bool parse_port(const std::string& str, uint16_t& port) { return true; } +/// True if `s` is a valid URI scheme: ALPHA *( ALPHA / DIGIT / "+" / "-" / "." ). +/// A "://" not preceded by one is not a scheme delimiter but sits inside a bare authority, so +/// treating that prefix as a scheme would echo userinfo (a ':' and an '@') into a diagnostic. +bool is_uri_scheme(const std::string& s) { + if (s.empty()) { + return false; + } + const char first = s.front(); + if (!((first >= 'a' && first <= 'z') || (first >= 'A' && first <= 'Z'))) { + return false; + } + for (const char c : s) { + const bool ok = (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || c == '+' || c == '-' || c == '.'; + if (!ok) { + return false; + } + } + return true; +} + /// Parses a buffer size in milliseconds: digits only, MIN_BUFFER_MS to MAX_BUFFER_MS. bool parse_buffer_ms(const std::string& str, uint32_t& buffer_ms) { if (!is_all_digits(str)) { @@ -254,7 +282,7 @@ bool apply_option(const SettableOption& option, const std::string& value, Option out.name = value; break; case Opt::Server: - // Emptiness is left to the -s resolution, which explains what -s takes. + // Emptiness is left to parse_server_url(), which explains what a server looks like. out.server = value; break; case Opt::Pidfile: @@ -697,23 +725,7 @@ bool parse_options(int argc, char* argv[], Options& out, std::FILE* err) { }; if (error.empty() && out.was_given(Opt::Server)) { - if (!parse_discovery_spec(out.server, out.discover_name)) { - // Hard error; the value is not quoted, since an address can carry credentials. - std::string message = "connecting to an address with -s was removed: the Sendspin " - "spec only has a player connect to a server it has discovered."; - if (out.server == "mdns") { - message += " Did you mean -s mdns:?"; - } -#ifdef SENDSPIN_CLI_HAVE_MDNS - message += " Use -s mdns: for any server or -s mdns: for one, or drop -s and " - "let a server discover this player."; -#else - message += " This build has no mDNS support, so it cannot discover one either: drop " - "-s and point a server at ws://:" + - std::to_string(out.port) + SENDSPIN_PATH + "."; -#endif - fail_for(Opt::Server, std::move(message)); - } else { + if (parse_discovery_spec(out.server, out.discover_name)) { out.discover = true; #ifndef SENDSPIN_CLI_HAVE_MDNS // Refused at parse time rather than quietly discovering nothing. @@ -722,10 +734,13 @@ bool parse_options(int argc, char* argv[], Options& out, std::FILE* err) { "-s mdns: needs mDNS, and 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 drop -s and point a server at " - "ws://:" + - std::to_string(out.port) + SENDSPIN_PATH + "."); + "avahi-compat-libdns_sd-devel on Fedora), or give -s an address."); #endif + } else { + std::string reason; + if (!parse_server_url(out.server, out.server_url, reason)) { + fail_for(Opt::Server, std::move(reason)); + } } } @@ -842,12 +857,7 @@ void print_usage(std::FILE* out, const char* prog) { std::fprintf(out, " %s [args] [--port ] [--control-socket ]\n\n", prog); std::fprintf(out, "A headless Sendspin audio player. Listens for a Sendspin server to\n"); -#ifdef SENDSPIN_CLI_HAVE_MDNS - std::fprintf(out, "connect to it, or discovers one with -s %s and connects to it.\n\n", - DISCOVERY_PREFIX); -#else - std::fprintf(out, "connect to it.\n\n"); -#endif + std::fprintf(out, "connect to it, or dials one with -s.\n\n"); std::fprintf(out, "With a subcommand, it instead talks to a player already running on this\n"); std::fprintf(out, "host over its control socket, and exits. The subcommand must come first.\n\n"); @@ -898,17 +908,25 @@ void print_usage(std::FILE* out, const char* prog) { std::fprintf(out, " network interface MAC, which two players on one host\n"); std::fprintf(out, " would share: give each its own --id (and its own\n"); std::fprintf(out, " --port and --state-dir)\n"); - std::fprintf(out, " -s, --server %s[]\n", DISCOVERY_PREFIX); - std::fprintf(out, " Discover a Sendspin server over mDNS and connect to it,\n"); - std::fprintf(out, " retrying until it answers: -s %s takes the one\n", + std::fprintf(out, " -s, --server \n"); + std::fprintf(out, " Connect out to [:] or a ws:// URL\n"); + std::fprintf(out, " (the server's port defaults to %u), retrying until it\n", + DEFAULT_REMOTE_SERVER_PORT); + std::fprintf(out, " answers. Any -s turns off the mDNS advertisement: the\n"); + std::fprintf(out, " spec forbids advertising %s while\n", MDNS_CLIENT_SERVICE); + std::fprintf(out, " the client is the one initiating the connection\n"); +#ifdef SENDSPIN_CLI_HAVE_MDNS + std::fprintf(out, " -s %s instead discovers a server over mDNS,\n", DISCOVERY_PREFIX); - std::fprintf(out, " advertised under , -s %s takes any. Turns off\n", + std::fprintf(out, " by its advertised name; -s %s takes any server.\n", DISCOVERY_PREFIX); - std::fprintf(out, " the mDNS advertisement: the spec forbids advertising\n"); - std::fprintf(out, " %s while the client initiates the connection\n", - MDNS_CLIENT_SERVICE); -#ifndef SENDSPIN_CLI_HAVE_MDNS - std::fprintf(out, " (needs mDNS, which this build does not have)\n"); + std::fprintf(out, " '%s' is reserved before the first colon only, so a\n", + DISCOVERY_PREFIX); + std::fprintf(out, " bare -s mdns is still a host called mdns\n"); +#else + std::fprintf(out, " (-s %s discovery needs mDNS, which this build\n", + DISCOVERY_PREFIX); + std::fprintf(out, " does not have)\n"); #endif std::fprintf(out, " -z Fork into the background and detach from the terminal.\n"); std::fprintf(out, " Refuses -o stdout, whose output would go to /dev/null;\n"); @@ -1029,6 +1047,137 @@ bool parse_discovery_spec(const std::string& server, std::string& name) { return true; } +bool parse_server_url(const std::string& server, std::string& url, std::string& error) { + if (server.empty()) { + error = "-s needs a server: [:], or a full ws:// URL"; + return false; + } + + // Every message quotes the value back, which may carry credentials, so it goes through here. + const std::string shown = redact_url_userinfo(server); + + // A scheme means the caller spelled out the whole URL; only the scheme is ours to check. + // A "://" whose prefix is not a real scheme is userinfo's own, so it falls through to be read + // (and rejected) as a bare authority rather than echoed as a scheme. + const size_t scheme_end = server.find("://"); + if (scheme_end != std::string::npos && is_uri_scheme(server.substr(0, scheme_end))) { + const std::string scheme = server.substr(0, scheme_end); + if (scheme != "ws" && scheme != "wss") { + error = "-s '" + shown + "': Sendspin runs over WebSocket, so the scheme must be " + + "ws:// or wss://, not " + scheme + "://"; + return false; + } + if (scheme_end + 3 == server.size()) { + error = "-s '" + shown + "': a scheme but no host"; + return false; + } + url = server; + return true; + } + + std::string host; + std::string port_text; + bool has_port = false; + + if (server.front() == '[') { + // A bracketed IPv6 literal keeps its brackets; only what follows ']' can be a port. + const size_t bracket = server.find(']'); + if (bracket == std::string::npos) { + 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 '" + shown + "': expected ':' after ']', got '" + + redact_url_userinfo(rest) + "'"; + return false; + } + port_text = rest.substr(1); + has_port = true; + } + } else { + // The bare form is [:]; a path or credentials need a full ws:// URL, not one + // guessed by pasting the default port after them. + if (server.find_first_of("/@?#") != std::string::npos) { + error = "-s '" + shown + + "': a bare host takes no path or credentials -- use a full ws:// or wss:// URL"; + return false; + } + const size_t colon = server.find(':'); + if (colon == std::string::npos) { + host = server; + } else if (server.find(':', colon + 1) != std::string::npos) { + // More than one colon and no brackets: an unbracketed IPv6 literal, which is ambiguous. + error = "-s '" + shown + "': an IPv6 literal must be bracketed -- try '[" + shown + + "]' or '[" + shown + "]:'"; + return false; + } else { + host = server.substr(0, colon); + port_text = server.substr(colon + 1); + has_port = true; + } + } + + // "[]" is as empty a host as "". + if (host.empty() || host == "[]") { + error = "-s '" + shown + "': no host before the port"; + return false; + } + + // The server default applies only when no ':' was written; a written empty port is truncated. + uint16_t port = DEFAULT_REMOTE_SERVER_PORT; + if (has_port && !parse_port(port_text, port)) { + 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) { + // The authority is the only place userinfo can live: after the scheme, or at the front when + // there is none, ending at the first delimiter so an '@' in a path is not read as a separator. + // A "://" not preceded by a real scheme is userinfo's own, so the authority starts at the + // front. + const size_t scheme_end = url.find("://"); + const size_t begin = + (scheme_end != std::string::npos && is_uri_scheme(url.substr(0, scheme_end))) + ? scheme_end + 3 + : 0; + 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* '@': a host holds none, so anything before the final one is userinfo. + 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 the password, colons and all. + const size_t colon = userinfo.find(':'); + std::string masked; + if (colon == std::string::npos) { + // Nothing to hide, or a lone 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 6e74433..dec5829 100644 --- a/src/cli.h +++ b/src/cli.h @@ -26,10 +26,11 @@ namespace sendspin_cli { -/// The WebSocket path this player serves and advertises. +/// The WebSocket path this player serves and advertises, and that -s fills into a bare host. inline constexpr const char* SENDSPIN_PATH = "/sendspin"; /// The `-s` prefix that asks for mDNS discovery; the name follows the first colon. +/// Reserved only before the first colon, so a bare `-s mdns` is still a host named mdns. inline constexpr const char* DISCOVERY_PREFIX = "mdns:"; /// The -o default: the most direct real backend this build has, else `null`. @@ -91,7 +92,7 @@ struct Options { /// --manufacturer / --product-name : device info sent in `client/hello`. std::string manufacturer{"sendspin-cpp-cli"}; std::string product_name{"sendspin-cli"}; - std::string server; ///< -s mdns:[]: discover a server and dial it + std::string server; ///< -s : dial this address, or mdns:[] to discover one bool daemonize{false}; ///< -z: detach and run in the background std::string pidfile; ///< -P : write our pid here std::string logfile; ///< -f : send log output to this file @@ -144,6 +145,9 @@ struct Options { /// The words after the subcommand, exactly its arity. std::vector subcommand_args; + /// The WebSocket URL `server` resolved to; empty for discovery and when -s was not given. + std::string server_url; + /// True when -s asked for discovery. bool discover{false}; @@ -184,6 +188,19 @@ void print_usage(std::FILE* out, const char* prog); /// Prints our version and the sendspin-cpp tag this binary was built against. void print_version(std::FILE* out); +/// Turns a -s address into a WebSocket URL, or explains why it cannot. +/// Accepts a full ws:// or wss:// URL as written, else `[:]`, filling the /sendspin +/// path and the server's default port (8927, not this player's 8928). IPv6 literals must be +/// bracketed. Rejects rather than guesses, so a bad address fails before the daemon dials. +/// @param error Set to a human-readable reason when the return value is false. +bool parse_server_url(const std::string& server, std::string& url, std::string& error); + +/// A server URL that is safe to log: its userinfo masked to a fixed `***`. +/// A username is kept (`ws://user:***@host`); a lone field, indistinguishable from a token, +/// goes whole (`ws://***@host`). The authority ends at the first `/`, `?` or `#`, so an `@` +/// in a path is left alone. A value with no userinfo comes back unchanged. +std::string redact_url_userinfo(const std::string& url); + /// Reads `mdns:` or a bare `mdns:` from a -s value. /// @param name Set to the TXT `name` filter, empty when none was given. bool parse_discovery_spec(const std::string& server, std::string& name); diff --git a/src/main.cpp b/src/main.cpp index 700f3ab..f864522 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -150,13 +150,14 @@ std::vector advertised_formats(const Audio return formats; } -/// The outbound (-s) mode: choose a discovered server, dial it, and keep redialling. +/// The outbound (-s) mode: dial the configured address or a discovered server, and keep redialling. class OutboundMode { public: /// `store` must outlive this mode, and is where the chosen server is remembered. OutboundMode(const Options& opts, MdnsService& mdns, StateStore& store) : opts_(opts), mdns_(mdns), store_(store), remembered_(store.last_server()) { - if (!this->remembered_.empty()) { + // Only meaningful when discovering: an address leaves nothing to choose between. + if (!this->remembered_.empty() && opts.discover) { log_line(LogLevel::INFO, LOG_TAG_OUTBOUND, "Last server used was \"%s\" -- it wins if it turns up among the candidates", this->remembered_.c_str()); @@ -184,8 +185,14 @@ class OutboundMode { std::string url; std::string server_id; - if (!this->choose(url, server_id)) { - return; + if (this->opts_.discover) { + if (!this->choose(url, server_id)) { + return; + } + } else { + url = this->opts_.server_url; + log_line(LogLevel::INFO, LOG_TAG_OUTBOUND, "Connecting to %s", + redact_url_userinfo(url).c_str()); } // Stamped before dialling, so the backoff measures from the attempt's start. @@ -214,9 +221,10 @@ class OutboundMode { return false; } server_id = chosen->instance; + // Redacted for one spelling with the address log line, though a discovered URL has none. 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; } @@ -433,8 +441,8 @@ void start_advertising(MdnsService& mdns, const Options& opts) { if (!mdns_available()) { log_line(LogLevel::INFO, LOG_TAG_MDNS, - "This build has no mDNS support, so it can neither be discovered nor discover a " - "server: point a server at ws://:%u%s. See docs/ROADMAP.md.", + "This build has no mDNS support, so it cannot be discovered: point a server at " + "ws://:%u%s, or dial one with -s. See docs/ROADMAP.md.", opts.port, SENDSPIN_PATH); return; } @@ -673,14 +681,17 @@ int main(int argc, char* argv[]) { player_listener, player, *sink); std::unique_ptr outbound; - if (opts.discover) { - std::string error; - if (!mdns.browse(error)) { - log_line(LogLevel::WARN, LOG_TAG_DISCOVERY, "%s -- retrying", error.c_str()); + if (opts.was_given(Opt::Server)) { + // Discovery browses mDNS; an address dials without it. + if (opts.discover) { + std::string error; + if (!mdns.browse(error)) { + log_line(LogLevel::WARN, LOG_TAG_DISCOVERY, "%s -- retrying", error.c_str()); + } + log_line(LogLevel::INFO, LOG_TAG_DISCOVERY, "Looking for a Sendspin server on %s%s%s%s", + MDNS_SERVER_SERVICE, opts.discover_name.empty() ? "" : " named \"", + opts.discover_name.c_str(), opts.discover_name.empty() ? "" : "\""); } - log_line(LogLevel::INFO, LOG_TAG_DISCOVERY, "Looking for a Sendspin server on %s%s%s%s", - MDNS_SERVER_SERVICE, opts.discover_name.empty() ? "" : " named \"", - opts.discover_name.c_str(), opts.discover_name.empty() ? "" : "\""); outbound = std::make_unique(opts, mdns, state_store); } diff --git a/src/outbound.cpp b/src/outbound.cpp index 5506bce..1212972 100644 --- a/src/outbound.cpp +++ b/src/outbound.cpp @@ -77,7 +77,8 @@ void LastDial::note_lost() { } std::string LastDial::url_for(const std::string& connected_server_id) const { - if (this->server_id_.empty() || this->server_id_ != connected_server_id) { + // A literal dial has no id to check, so it is taken at its word; a discovery dial must match. + if (!this->server_id_.empty() && this->server_id_ != connected_server_id) { return {}; } return this->url_; diff --git a/src/outbound.h b/src/outbound.h index f1f2e98..c02d61a 100644 --- a/src/outbound.h +++ b/src/outbound.h @@ -59,18 +59,18 @@ class RetryPacer { /// The last dial, and what SENDSPIN_SERVER_URL may honestly claim of it. class LastDial { public: - /// Records that connect_to() has just been called with `url` for `server_id`. + /// Records connect_to() with `url`; `server_id` empty for a literal -s dial, unverifiable. void note_dial(const std::string& url, const std::string& server_id); /// Forgets the dial: the connection it could have described is gone. void note_lost(); - /// The dialled URL, or empty unless a live dial named `connected_server_id`. + /// The dialled URL: a literal dial answers always, a discovery dial only for its own id. std::string url_for(const std::string& connected_server_id) const; private: std::string url_; - std::string server_id_; ///< who url_ was expected to reach + std::string server_id_; ///< who url_ was expected to reach; empty means a literal dial }; } // namespace sendspin_cli diff --git a/tests/cli_test.cpp b/tests/cli_test.cpp index c8d60d9..7da50dd 100644 --- a/tests/cli_test.cpp +++ b/tests/cli_test.cpp @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -/// parse_options(): what the flag surface accepts and rejects. +/// parse_options() and parse_server_url(): what the flag surface accepts and rejects. #include "cli.h" @@ -36,21 +36,13 @@ using sendspin::LogLevel; // Every flag reaches its field TEST(ParseOptions, EachFlagSetsItsField) { - std::vector args = { - "-o", "null", "-n", "kitchen", "-z", "-P", "/run/x.pid", - "-d", "debug", "-f", "/var/log/x.log", "--port", "9000"}; -#ifdef SENDSPIN_CLI_HAVE_MDNS - // -s only parses in a build that can discover a server. - args.insert(args.end(), {"-s", "mdns:hifi"}); -#endif - Parse parse(args); + Parse parse({"-o", "null", "-n", "kitchen", "-s", "192.168.12.2", "-z", "-P", "/run/x.pid", + "-d", "debug", "-f", "/var/log/x.log", "--port", "9000"}); ASSERT_TRUE(parse.ok()) << parse.diagnostics(); EXPECT_EQ(parse.options().device, "null"); EXPECT_EQ(parse.options().name, "kitchen"); -#ifdef SENDSPIN_CLI_HAVE_MDNS - EXPECT_EQ(parse.options().server, "mdns:hifi"); -#endif + EXPECT_EQ(parse.options().server, "192.168.12.2"); EXPECT_TRUE(parse.options().daemonize); EXPECT_EQ(parse.options().pidfile, "/run/x.pid"); EXPECT_EQ(parse.options().log_level, LogLevel::DEBUG); @@ -75,7 +67,7 @@ TEST(ParseOptions, DefaultsWhenNothingIsGiven) { EXPECT_FALSE(parse.options().daemonize); EXPECT_FALSE(parse.options().list_devices); EXPECT_TRUE(parse.options().server.empty()); - EXPECT_FALSE(parse.options().discover); + EXPECT_TRUE(parse.options().server_url.empty()); // -n falls back to the hostname, so the one thing promised is that it is not empty. EXPECT_FALSE(parse.options().name.empty()); } @@ -612,78 +604,168 @@ TEST(ParseOptions, MissingValueNamesTheOptionNotTheCluster) { << long_form.diagnostics(); } -// -s takes no address +// -s, through the parser + +TEST(ParseOptions, ServerIsResolvedDuringParsing) { + Parse parse({"-s", "192.168.12.2"}); + + ASSERT_TRUE(parse.ok()) << parse.diagnostics(); + EXPECT_EQ(parse.options().server, "192.168.12.2"); + EXPECT_EQ(parse.options().server_url, "ws://192.168.12.2:8927/sendspin"); +} + +TEST(ParseOptions, BadServerFailsTheWholeParse) { + Parse parse({"-s", "host:abc"}); + + EXPECT_FALSE(parse.ok()); + EXPECT_NE(parse.diagnostics().find("error:"), std::string::npos); + EXPECT_NE(parse.diagnostics().find("host:abc"), std::string::npos); +} + +// -s, as a matrix over parse_server_url() + +TEST(ParseServerUrl, Accepted) { + const std::pair cases[] = { + // A bare host takes the server's port (8927), not this player's serve port (8928). + {"192.168.12.2", "ws://192.168.12.2:8927/sendspin"}, + {"192.168.12.2:8927", "ws://192.168.12.2:8927/sendspin"}, + {"music.local:9000", "ws://music.local:9000/sendspin"}, + // A full URL is the caller's to get right, path and all. + {"ws://host:9000/sendspin", "ws://host:9000/sendspin"}, + {"wss://host/sendspin", "wss://host/sendspin"}, + // A bracketed IPv6 literal keeps its brackets in the URL. + {"[::1]:8927", "ws://[::1]:8927/sendspin"}, + {"[::1]", "ws://[::1]:8927/sendspin"}, + {"[2001:db8::1]:9000", "ws://[2001:db8::1]:9000/sendspin"}, + }; -/// How many times `needle` occurs in `text`. -size_t occurrences(const std::string& text, const std::string& needle) { - size_t count = 0; - for (size_t at = text.find(needle); at != std::string::npos; at = text.find(needle, at + 1)) { - ++count; + for (const auto& [input, expected] : cases) { + std::string url; + std::string error; + + ASSERT_TRUE(parse_server_url(input, url, error)) << input << ": " << error; + EXPECT_EQ(url, expected) << input; } - return count; } -TEST(ParseOptions, AnAddressIsRefusedWithTheRemoval) { - const char* addresses[] = { - "music.local", - "music.local:8927", - "ws://host:8927/sendspin", - "wss://host/sendspin", - "192.168.12.2", - "[::1]:8927", - "hifi:8927", - "", +TEST(ParseServerUrl, Rejected) { + const char* cases[] = { + "", // nothing at all + "host:abc", // not a port + "host:", // a truncated line, not a request for the default + ":8927", // no host + "host:0", // ports are 1-65535 + "host:70000", // ditto + "::1", // an IPv6 literal must be bracketed to be told from host:port + "[::1", // unterminated bracket + "[::1]junk", // trailing text where a port belongs + "[]", // no host + "[]:8927", // ditto + "http://host", // Sendspin is WebSocket only + "https://host", // ditto + "ws://", // a scheme naming no server + "wss://", // ditto + "host/path", // a bare host takes no path + "token@host", // ...nor credentials + "host?x", // ...nor a query + "host#x", // ...nor a fragment + "alice:s3cr3t@host://x", // userinfo before a stray '://' is not a scheme }; - for (const char* address : addresses) { - for (const char* flag : {"-s", "--server"}) { - Parse parse({flag, address}); - - EXPECT_FALSE(parse.ok()) << flag << " '" << address << "' was accepted"; - const std::string diagnostics = parse.diagnostics(); - EXPECT_NE(diagnostics.find("error: connecting to an address with -s was removed"), - std::string::npos) - << flag << " '" << address << "': " << diagnostics; - EXPECT_EQ(occurrences(diagnostics, "error:"), 1U) << diagnostics; - EXPECT_EQ(occurrences(diagnostics, "\n"), 1U) << diagnostics; -#ifdef SENDSPIN_CLI_HAVE_MDNS - EXPECT_NE(diagnostics.find("-s mdns:"), std::string::npos) << diagnostics; - EXPECT_NE(diagnostics.find("let a server discover this player"), std::string::npos) - << diagnostics; -#else - EXPECT_NE(diagnostics.find("point a server at"), std::string::npos) << diagnostics; -#endif - } + for (const char* input : cases) { + std::string url; + std::string error; + + EXPECT_FALSE(parse_server_url(input, url, error)) << "accepted '" << input << "'"; + EXPECT_FALSE(error.empty()) << "no reason given for '" << input << "'"; } } -TEST(ParseOptions, ABareMdnsIsRefusedWithAHint) { - // The one address most likely to be the discovery form with its colon left off. - Parse parse({"-s", "mdns"}); +// redact_url_userinfo(): what a logged server URL is allowed to say - EXPECT_FALSE(parse.ok()); - EXPECT_NE(parse.diagnostics().find("connecting to an address with -s was removed"), - std::string::npos) - << parse.diagnostics(); - EXPECT_NE(parse.diagnostics().find("Did you mean -s mdns:?"), std::string::npos) - << parse.diagnostics(); +TEST(RedactUrlUserinfo, MasksTheSecretAndKeepsTheRest) { + const std::pair cases[] = { + // A user:password pair keeps its username: the line names which endpoint was dialled. + {"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 hold colons, so the split is on the *first* one. + {"ws://alice:s3c:r3t@host/sendspin", "ws://alice:***@host/sendspin"}, + // 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"}, + // A "://" after userinfo is not a scheme delimiter, so the authority still starts at front. + {"alice:s3cr3t@host://x", "alice:***@host://x"}, + }; + + 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(ParseOptions, TheRemovalNeverQuotesTheValue) { - // Addresses may carry credentials, so they are never quoted. - const char* addresses[] = { - "ws://u:s3cr3t@host/sendspin", - "u:s3cr3t@host:8927", - "s3cr3t@host", - "s3cr3t.local", +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. + "ws://@host:8927/sendspin", + "ws://alice:@host:8927/sendspin", + // The authority ends at the first '/', '?' or '#', so an '@' past it is not a separator. + "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", + // An unencoded '/', '?' or '#' inside userinfo ends the authority early, so the value + // comes back whole -- a malformed URL RFC 3986 requires percent-encoded, not masked. + "ws://alice:aGVsbG8/d29ybGQ=@host:8927/sendspin", + }; + + for (const char* input : unchanged) { + EXPECT_EQ(redact_url_userinfo(input), input); + } +} + +// A value that does not parse is the one most likely mistyped around a password, so assert over +// the reason rather than per message, covering a rejection added later without listing it 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 + "alice:s3cr3t@host://x", // userinfo before a stray '://', not a scheme }; - for (const char* address : addresses) { - Parse parse({"-s", address}); + for (const char* input : cases) { + std::string url; + std::string error; - ASSERT_FALSE(parse.ok()) << "accepted '" << address << "'"; - EXPECT_EQ(parse.diagnostics().find("s3cr3t"), std::string::npos) - << "'" << address << "' was refused with: " << parse.diagnostics(); + 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; } } @@ -706,7 +788,7 @@ TEST(ParseDiscoverySpec, SplitsOnTheFirstColonOnly) { } TEST(ParseDiscoverySpec, LeavesEveryOtherFormAlone) { - // Only the exact `mdns:` prefix is the discovery form. + // Only the exact `mdns:` prefix is the discovery form; a bare `mdns` is still an address. const char* addresses[] = { "hifi:8927", "mdns", "mdnsx:8927", "192.168.1.10", "ws://mdns:8927/sendspin", "", }; @@ -724,6 +806,8 @@ TEST(ParseOptions, DiscoveryReachesTheOptions) { ASSERT_TRUE(parse.ok()) << parse.diagnostics(); EXPECT_TRUE(parse.options().discover); EXPECT_EQ(parse.options().discover_name, "Living room"); + // There is no URL until a server has actually been found. + EXPECT_TRUE(parse.options().server_url.empty()); #else EXPECT_FALSE(parse.ok()); EXPECT_NE(parse.diagnostics().find("error:"), std::string::npos); @@ -743,6 +827,23 @@ TEST(ParseOptions, DiscoveryWithNoNameFilter) { #endif } +TEST(ParseOptions, AHostWithAColonIsStillAHost) { + // The reserved prefix must not regress this: `hifi:8927` is a host and a port. + Parse parse({"-s", "hifi:8927"}); + + ASSERT_TRUE(parse.ok()) << parse.diagnostics(); + EXPECT_FALSE(parse.options().discover); + EXPECT_EQ(parse.options().server_url, "ws://hifi:8927/sendspin"); +} + +TEST(ParseOptions, ABareMdnsIsStillAHost) { + Parse parse({"-s", "mdns"}); + + ASSERT_TRUE(parse.ok()) << parse.diagnostics(); + EXPECT_FALSE(parse.options().discover); + EXPECT_EQ(parse.options().server_url, "ws://mdns:8927/sendspin"); +} + // The two connection modes are exclusive TEST(ParseOptions, AdvertisesByDefault) { @@ -753,15 +854,26 @@ TEST(ParseOptions, AdvertisesByDefault) { } TEST(ParseOptions, AnyServerSuppressesTheAdvertisement) { - for (const char* server : {"mdns:", "mdns:Living room"}) { + // The spec's rule, so it holds for every -s form -- there is deliberately no flag that turns + // the advertisement back on alongside one. + const char* servers[] = {"192.168.1.10", "host:9000", "ws://host:9000/sendspin", "[::1]"}; + + for (const char* server : servers) { Parse parse({"-s", server}); -#ifdef SENDSPIN_CLI_HAVE_MDNS ASSERT_TRUE(parse.ok()) << server << ": " << parse.diagnostics(); -#endif EXPECT_FALSE(parse.options().advertises()) << server; } } +TEST(ParseOptions, DiscoverySuppressesTheAdvertisementToo) { + Parse parse({"-s", "mdns:"}); + +#ifdef SENDSPIN_CLI_HAVE_MDNS + ASSERT_TRUE(parse.ok()) << parse.diagnostics(); +#endif + EXPECT_FALSE(parse.options().advertises()); +} + TEST(ParseOptions, NoMdnsSuppressesTheAdvertisementWithoutAServer) { Parse parse({"--no-mdns"}); @@ -804,17 +916,15 @@ TEST(ParseOptions, MdnsNameNeedsAValue) { << parse.diagnostics(); } -#ifdef SENDSPIN_CLI_HAVE_MDNS TEST(ParseOptions, MdnsNameWithAServerWarnsButStillStarts) { // Inert with -s, so warned rather than refused. - Parse parse({"-s", "mdns:", "--mdns-name", "Kitchen"}); + Parse parse({"-s", "192.168.1.10", "--mdns-name", "Kitchen"}); ASSERT_TRUE(parse.ok()) << parse.diagnostics(); EXPECT_NE(parse.diagnostics().find("warning:"), std::string::npos) << parse.diagnostics(); EXPECT_NE(parse.diagnostics().find("--mdns-name is unused with -s"), std::string::npos) << parse.diagnostics(); } -#endif TEST(ParseOptions, MdnsNameAloneDoesNotWarn) { Parse parse({"--mdns-name", "Kitchen"}); @@ -1093,22 +1203,12 @@ TEST(ParseOptions, TracksWhichOptionsWereExplicitlyGiven) { } TEST(ParseOptions, ExplicitlyGivenTracksEveryOption) { - std::vector args = {"-o", "null", "-l", "-n", - "kitchen", "-z", "-P", "/run/x.pid", - "-d", "debug", "-f", "/var/log/x.log", - "--port", "9000", "--buffer-ms", "200"}; - std::vector given = {Opt::Device, Opt::ListDevices, Opt::Name, - Opt::Daemonize, Opt::Pidfile, Opt::Logfile, - Opt::LogLevel, Opt::Port, Opt::BufferMs}; -#ifdef SENDSPIN_CLI_HAVE_MDNS - // -s only parses in a build that can discover a server. - args.insert(args.end(), {"-s", "mdns:"}); - given.push_back(Opt::Server); -#endif - Parse parse(args); + Parse parse({"-o", "null", "-l", "-n", "kitchen", "-s", "host", "-z", "-P", "/run/x.pid", "-d", + "debug", "-f", "/var/log/x.log", "--port", "9000", "--buffer-ms", "200"}); ASSERT_TRUE(parse.ok()) << parse.diagnostics(); - for (const Opt opt : given) { + for (const Opt opt : {Opt::Device, Opt::ListDevices, Opt::Name, Opt::Server, Opt::Daemonize, + Opt::Pidfile, Opt::Logfile, Opt::LogLevel, Opt::Port, Opt::BufferMs}) { EXPECT_TRUE(parse.options().was_given(opt)) << "option " << static_cast(opt) << " not recorded as given"; } diff --git a/tests/config_file_test.cpp b/tests/config_file_test.cpp index 0bc1d0f..f56bdd4 100644 --- a/tests/config_file_test.cpp +++ b/tests/config_file_test.cpp @@ -306,26 +306,18 @@ TEST(ConfigPrecedence, AcceptsEveryBooleanSpelling) { // The long aliases, so every config key is a flag name TEST(LongAliases, EachBehavesExactlyLikeItsLetter) { - std::vector args = {"--output", "null", "--name", "kitchen", - "--pidfile", "/run/x.pid", "--logfile", "/var/log/x.log", - "--log-level", "debug"}; -#ifdef SENDSPIN_CLI_HAVE_MDNS - // --server only parses in a build that can discover a server. - args.insert(args.end(), {"--server", "mdns:Living Room"}); -#endif - Parse parse(args); + Parse parse({"--output", "null", "--name", "kitchen", "--server", "192.168.12.2", "--pidfile", + "/run/x.pid", "--logfile", "/var/log/x.log", "--log-level", "debug"}); ASSERT_TRUE(parse.ok()) << parse.diagnostics(); EXPECT_EQ(parse.options().device, "null"); EXPECT_EQ(parse.options().name, "kitchen"); + EXPECT_EQ(parse.options().server, "192.168.12.2"); EXPECT_EQ(parse.options().pidfile, "/run/x.pid"); EXPECT_EQ(parse.options().logfile, "/var/log/x.log"); EXPECT_EQ(parse.options().log_level, LogLevel::DEBUG); -#ifdef SENDSPIN_CLI_HAVE_MDNS - EXPECT_EQ(parse.options().server, "mdns:Living Room"); - EXPECT_TRUE(parse.options().discover); - EXPECT_EQ(parse.options().discover_name, "Living Room"); -#endif + // And the whole resolution downstream ran over them, exactly as for the letters. + EXPECT_EQ(parse.options().server_url, "ws://192.168.12.2:8927/sendspin"); } TEST(LongAliases, AreListedByHelpAlongsideTheConfigSearchPath) { @@ -364,7 +356,7 @@ TEST(ConfigRefusals, ABadValueGetsTheFlagsOwnMessagePrefixedWithTheLine) { {"buffer-ms = 0", "invalid --buffer-ms '0' -- expected 10-2000"}, {"static-delay = 5001", "invalid --static-delay '5001' -- expected 0-5000"}, {"port = 99999", "invalid --port '99999' -- expected 1-65535"}, - {"server = music.local", "connecting to an address with -s was removed"}, + {"server = music.local:abc", "'abc' is not a port number"}, {"log-level = shouty", "unknown log level 'shouty'"}, {"name =", "-n needs a non-empty value"}, {"no-mdns = perhaps", "invalid --no-mdns 'perhaps'"}, @@ -429,18 +421,30 @@ TEST(ConfigRefusals, TheRunShapeCannotComeFromAFile) { // A configured value reaches every resolution a typed one does +TEST(ConfigMerge, AConfiguredServerSuppressesTheAdvertisementAndResolves) { + ScratchDir scratch; + ASSERT_TRUE(scratch.created()); + const std::string config = scratch.write("config", "server = music.local\n"); + + Parse parse({}, config); + + ASSERT_TRUE(parse.ok()) << parse.diagnostics(); + // The reason the merge marks options as supplied rather than only setting them: left unmarked, + // this player would dial *and* advertise, and server_url would never have been filled. + EXPECT_FALSE(parse.options().advertises()); + EXPECT_TRUE(parse.options().was_given(Opt::Server)); + EXPECT_EQ(parse.options().server_url, "ws://music.local:8927/sendspin"); +} + TEST(ConfigMerge, AConfiguredAddressIsRefusedWithoutQuotingIt) { ScratchDir scratch; ASSERT_TRUE(scratch.created()); - const std::string config = - scratch.write("config", "server = ws://u:s3cr3t@music.local:8927/sendspin\n"); + const std::string config = scratch.write("config", "server = u:s3cr3t@host:notaport\n"); Parse parse({}, config); + // A bad address fails loudly and names the line, but never quotes the credential. EXPECT_FALSE(parse.ok()); - EXPECT_NE(parse.diagnostics().find("connecting to an address with -s was removed"), - std::string::npos) - << parse.diagnostics(); EXPECT_NE(parse.diagnostics().find(config + ":1:"), std::string::npos) << parse.diagnostics(); EXPECT_EQ(parse.diagnostics().find("s3cr3t"), std::string::npos) << parse.diagnostics(); } diff --git a/tests/discovery_test.cpp b/tests/discovery_test.cpp index 244d6bc..cff6571 100644 --- a/tests/discovery_test.cpp +++ b/tests/discovery_test.cpp @@ -346,13 +346,13 @@ TEST(LastDial, StartsWithNothingToExport) { EXPECT_EQ(dial.url_for("srv-1"), ""); } -TEST(LastDial, ADialWithNoServerIdAnswersNothing) { - // A dial with no server id has nothing to check against, so it exports nothing. +TEST(LastDial, ALiteralUrlIsTakenAtItsWord) { + // A literal -s dial has no id and promises nothing about who answers, so it exports as dialled. LastDial dial; dial.note_dial("ws://hifi:8927/sendspin", ""); - EXPECT_EQ(dial.url_for("srv-1"), ""); - EXPECT_EQ(dial.url_for(""), ""); + EXPECT_EQ(dial.url_for("srv-1"), "ws://hifi:8927/sendspin"); + EXPECT_EQ(dial.url_for(""), "ws://hifi:8927/sendspin"); } TEST(LastDial, ADiscoveryDialAnswersOnlyForTheServerItDialled) {