Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical issues leave ordinary HTTP connections unbounded, enabling resource exhaustion, and allow DNS rebinding to expose the pairing token because the Host header is not validated.
2 open issues
What changed in this PR
Adds an opt-in HTTP pairing-token endpoint so a server on the same host can pair with the player without manually copying a token.
Changes:
- Adds CLI and config options requiring an explicit, non-wildcard IP address.
- Binds the endpoint before client startup, advertises its role and port, and suppresses startup token logging when enabled.
- Updates the library pin and adds parser, HTTP, and address-binding tests plus security documentation.
| File | Description |
|---|---|
tests/pairing_endpoint_test.cpp |
Tests responses, HTTP serving, and address isolation. |
tests/config_file_test.cpp |
Tests endpoint configuration and wildcard rejection. |
tests/cli_test.cpp |
Tests defaults and address/port validation. |
src/pairing_endpoint.h |
Declares the endpoint interface and role constants. |
src/pairing_endpoint.cpp |
Implements parsing, HTTP responses, and listener lifecycle. |
src/main.cpp |
Integrates binding, advertisement, logging, and shutdown. |
src/cli.h |
Adds endpoint option fields. |
src/cli.cpp |
Adds flag/config parsing and help text. |
README.md |
Documents the new option. |
docs/wiki/Configuration.md |
Documents the configuration key. |
docs/wiki/Advanced-Usage.md |
Explains same-host pairing and its security model. |
CMakeLists.txt |
Updates the dependency pin and registers endpoint sources and tests. |
Review effort: Balanced
| this->server_ = std::make_unique<ix::HttpServer>(this->port_, this->host_, | ||
| ix::SocketServer::kDefaultTcpBacklog, | ||
| MAX_CONNECTIONS, family, TIMEOUT_SECS); |
There was a problem hiding this comment.
Confirmed: in v12.0.1 HttpServer inherits WebSocketServer::getConnectedClientsCount(), which is final and counts only upgraded clients, so the limit never applied to plain requests. (A stalled one was still dropped after the 5 s parseRequest deadline, so threads were bounded by connection rate rather than unbounded.) Fixed in edb2a9e: the endpoint now serves from an ix::SocketServer subclass whose count is every live connection, tracked from the accept loop until the worker is reaped. ClosesAConnectionBeyondItsLimit holds four stalled connections open, checks a fifth is closed unanswered, then that requests are served again once they close.
| const PairingReply reply = | ||
| pairing_token_reply(request->method, request->uri, client_id, token); |
There was a problem hiding this comment.
Fixed in edb2a9e: the token goes only to a request whose Host is the endpoint's own address:port (case-insensitive; the bare address only on port 80). Anything else gets 421 and no token. Covered by RefusesAHostThatDoesNotNameIt, MatchesHostLikeAnAddress, and RefusesARebindingHostOverHttp, which sends Host: evil.example over a raw connection.
3a3722a8 is sendspin-cpp#218 as merged, which adds application-specific roles. It brings #234, which removes ConnectionTrust and on_trust_changed() for SendspinClient::is_paired(). The main loop now polls it and logs "Connected server is paired/not paired" when a session is new or re-keyed, as the callback did. status reads it before is_connected(), as the library asks. Its trust line loses "unknown": a connected session is always paired or not.
--pairing-token-endpoint <address>:<port> answers GET /pairing-token with the client id and token on that one address, and advertises _ohf_local_pairing@v1 {"port":N} in client/hello. A wildcard or missing address is refused, so it never listens on every interface. Part of Sendspin/backlog#86.
Music Assistant fetches GET / and reads the body as the token, so answer that instead of a JSON object on /pairing-token. The role stays _ohf_local_pairing@v1. pair_with_token already refuses a token for another client id, so the reply no longer carries one.
71b0809 to
d60bf52
Compare
ix::HttpServer counts only upgraded WebSocket clients against its connection limit, so each plain HTTP connection got its own worker thread with no cap. Serve from an ix::SocketServer subclass that counts every accepted connection until its worker is reaped, and drop the WebSocket upgrade handling that is no longer reachable. Answer only a request whose Host names the endpoint's own address and port, so a page that rebinds its hostname to that address gets 421 and no token.

Adds
--pairing-token-endpoint <address>:<port>(config keypairing-token-endpoint). With it set, the player:GET /with the pairing token as plain text (text/plain,Cache-Control: no-store) on that one address only;_ohf_local_pairing@v1inclient/hellosupported_roleswith"_ohf_local_pairing@v1_support": {"port": N}.The address is required and must be a numeric IPv4 or bracketed IPv6 literal. A bare port, a hostname,
0.0.0.0,::or::ffff:0.0.0.0is refused at parse time, so it can never listen on every interface. If the address cannot be bound, the player exits before it advertises anything. A request whoseHostis not the endpoint's ownaddress:portgets 421 and no token, so a page that DNS-rebinds its hostname to the address can't read it. At most four connections are served at once; a fifth is closed as soon as it is accepted. Requests are logged at debug with the peer address and status only. With the endpoint on, the startup line that otherwise prints the token points atsendspin-cli pair-tokeninstead, because add-on logs are visible in the UI.Docs: the wiki Configuration key, and a new Advanced Usage section, "Pairing with a server on the same host", that covers the security model.
Decisions
GET /, matching Pair Sendspin players on the Home Assistant host with their local token music-assistant/server#7371's fetch. An earlier revision answeredGET /pairing-tokenwith JSON carrying the client id. MA'spair_with_tokenalready refuses a token issued for another client id, so the reply carries only the token.ix::SocketServersubclass, notix::HttpServer. In IXWebSocket v12.0.1,ix::HttpServercounts only upgraded WebSocket clients against its connection limit, and that count isfinal, so plain requests were never capped. The subclass counts every accepted connection, from the accept loop until its worker thread is reaped.client/hellogoes out, and the cli refuses to start if it can't bind. A wrapper servingpair-tokenoutput could advertise a port nobody is serving._ohf_local_pairing@v1, following theohf_prefix suggestion on backlog#86, instead of the brief's_ma_local_pairing@v1. Still provisional. Pair Sendspin players on the Home Assistant host with their local token music-assistant/server#7371 uses the same name.Requires upstream
Builds on #115, which re-pins sendspin-cpp to
3a3722a8(sendspin-cpp#218 as merged, addingSendspinClientConfig::application_roles) and moves tois_paired(). Until #115 merges, its commit shows here too; review the last three.Compatibility
Nothing breaks for CLI users: no existing flag, config key, control-socket message, state format or exit code changes, and the option is off by default.
application_rolesdefaults to empty, so hello is unchanged unless the new option is set.Testing
Hostmatching accepts the endpoint's own address and port (any case, the bare address only on port 80) and refuses a foreign name, a wrong port or none. Over a raw connection,Host: evil.examplegets 421 without the token, and with four stalled connections open a fifth is closed unanswered, then requests are served again once they close.ssshows the endpoint bound only to 127.0.0.1,curlreturns the token withCache-Control: no-store,0.0.0.0is refused, and an address the host doesn't have exits with a bind error.-DSENDSPIN_CLI_WERROR=ON): 566 of 569 pass, every endpoint test included; the live fetch checks the plain-text body and both headers. The three failures are theStateStoreread-only-directory tests, which cannot pass as root, as on Re-pin sendspin-cpp to 3a3722a8 and read pairing from is_paired() #115.scripts/smoke_test.shandscripts/format.sh --checkpass.Part of Sendspin/backlog#86.