Mask userinfo wherever this player names a server - #21
Merged
chrisuthe merged 1 commit intoAug 20, 2026
Merged
Conversation
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.
chrisuthe
marked this pull request as ready for review
August 20, 2026 17:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
-s ws://user:token@host:8927/sendspinreachedConnecting to %sat info in full, repeating on every retry-pacer backoff. Reported from the field: a container wrapping this binary redacts credentials in its own log line, and sendspin-cli's line then printed them anyway.redact_url_userinfo()(src/cli.{h,cpp}) masks the secret half and keeps the rest, so the line still names the endpoint —ws://user:***@host:8927/sendspin, or***@hostwhere a lone userinfo field could be a bearer token. Fixed-width mask; authority-scoped, so an@in a path or query is untouched and a password containing colons is not printed as a prefix of itself.Applied at both
Connecting tosinks and at every message that quotes a-svalue — 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 missed (-s 'user:tok@host'puts the token inport_text) and the no-mDNS refusal. The dial gets the real URL;last-serverholds the server id, so nothing credential-bearing reaches disk.Three limits, recorded in docs/ROADMAP.md rather than left to be found
sendspin.*are the library's: sendspin-cpp v0.7.0 logs the URL it dials at info and error through macros with no sink hook — the same missing hook that already put timestamps and per-tag filtering out of reach.?token=is indistinguishable from any other query./,?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. The docs say to percent-encode.Worth knowing before review
Userinfo never reaches the wire. 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; neither library builds anAuthorizationheader. Confirmed against a listening socket, not inferred — the upgrade request carriesHost,Upgrade,Connection,Sec-WebSocket-*andOriginand no credential in any form.So the player accepts a credential it cannot send. That is the worse bug, it is a breaking change to fix, and it is tracked separately rather than smuggled in here; the ROADMAP entry names refusal as the fix this change only documents. README and the wiki now say a credentialed
-sURL authenticates nothing, instead of implying it works.Verification
347 → 350 tests, none opening 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, so a rejection added later is covered). That the dial sink calls the helper is a
scripts/smoke_test.shcheck,src/main.cppnot being linked by the test binary.Clean with
-DSENDSPIN_CLI_WERROR=ONboth with and without mDNS — the no-mDNS build matters, since the refusal edited there sits inside#ifndef SENDSPIN_CLI_HAVE_MDNS.shellcheck scripts/*.shclean, smoke suite green.ctestis 349/350; the one failure,ConfigMerge.AConfiguredControlSocketIsAbsolutizedUnderDaemonize, is a pre-existing baseline artifact of this worktree's long absolute path (107-byte socket path against macOS's 103-byte limit) and fails identically onmain.Beyond the unit cases, the helper was also checked exhaustively over every string of length ≤ 4 built from the parse-steering characters (
@ : / ? # [ ] w s A . 1) plus targeted shapes with an embedded secret: no throw, no leak where the secret sat in userinfo, byte-identical passthrough wherever it did not.