Fix: Skip empty lines when listening to OAuth callback - #1706
Open
spacid wants to merge 1 commit into
Open
Conversation
It is possible that a browser, after login, sends out empty lines through the socket before sending the auth code. This fix will skip the empty lines and will parse the first non-empty line.
|
cargo clippy failure is unrelated — pre-existing lint in |
LargeModGames
added a commit
to LargeModGames/spotatui-librespot
that referenced
this pull request
Aug 6, 2026
Builds on the cherry-picked librespot-org#1706. That fix moves to the next connection when a request line is blank, which is right only if the blank line and the redirect arrive on separate connections. A leading CRLF on the same connection is what RFC 9112 section 2.2 tells servers to tolerate, and skipping the connection there discards the code and then blocks forever in accept(). Read past blank lines within a connection instead, and fall through to the next connection only when this one produces no request line at all, so both shapes work. Also stop treating a parseable but non-callback request as fatal. A GET /favicon.ico still returned Err from get_code, dropped the listener and closed the port, so the redirect carrying the code hit a dead port and the user was stranded on a browser "unable to connect" page. Answer those with a 400 and keep waiting. A per-connection read timeout stops a socket that is opened but never written from parking the login, and bounded skip counts keep a misbehaving client from holding the server open indefinitely. accept_authcode is split out so tests can bind port 0 instead of racing for a fixed port. Covers both librespot#1705 shapes plus the favicon case.
LargeModGames
added a commit
to LargeModGames/spotatui
that referenced
this pull request
Aug 6, 2026
) # Summary Fixes #414. The streaming login hands port 8989 to librespot, whose callback server accepted exactly one connection and gave up if it was not the redirect, dropping the listener and closing the port. Some browsers, LibreWolf in particular, send a bare CRLF or open a connection without writing to it before the real callback arrives. librespot consumed that, failed to parse it, and closed the port, so the redirect carrying the code hit a dead port. The user is left on a browser "unable to connect" page, and because the flow errors before `save_credentials` it repeats on every launch, which is the "streaming cookie does not get cached" the reporter described. Not Windows specific. Three independent reports line up: upstream [librespot#1705](librespot-org/librespot#1705) (LibreWolf + ncspot on Fedora, with a debug trace showing the empty line and vanilla Firefox working as a control), #234 (LibreWolf on Gentoo, same "web API login works, streaming fails" split), and #364 (a commenter with `AuthCodeListenerParse` in their log who confirmed Chrome works). The common variable is the browser. spotatui's own callback server never had this problem: `extract_callback_url` runs `split_whitespace()` over the whole buffer, which steps over a leading CRLF, and it answers unrelated requests with a 400 and keeps waiting. `read_line` stops at the first `\n`. Only the librespot half was fragile, which is why the web API login succeeded and the streaming one did not. Fixed in our librespot fork rather than here, since that is where the defective listener lives. The fork cherry-picks upstream PR [#1706](librespot-org/librespot#1706) with its original attribution, then hardens it: #1706 skips to the next *connection* on a blank line, which is correct only if the blank line arrives on its own connection and hangs if it is a leading CRLF on the same one. This PR bumps the `[patch.crates-io]` rev to pick that up. Three unrelated startup bugs found while tracing this are fixed in their own commits: - **Auto-update deadlock.** `run_auto_update` ran concurrently with authentication in a `tokio::join!` and re-exec'd immediately on a successful install. The re-exec blocks the task in `Command::status()`, so the joined authentication future stops being polled while it still owns the callback port, and the child, which repeats startup from scratch, cannot bind that port. The parent waits on the child, the child waits on a port the parent will never release. The check still runs concurrently; only the restart moves to after the join. - **Log path.** `setup_logging` hard-coded `/tmp/spotatui_logs/`, which on Windows is drive-relative, so the app printed a location the user's shell could not resolve directly above the line inviting them to report bugs. Now resolved through `std::env::temp_dir`. - **OAuth port probe.** `wait_for_oauth_callback_port` refused to start the login when its probe timed out, so we never even attempted and reported something vaguer than the bind error librespot would have produced. It now warns and proceeds. # Testing - `cargo fmt --all` (and `--check`, clean) - `cargo clippy --no-default-features --features telemetry -- -D warnings` (clean) - `cargo clippy -- -D warnings` (clean, default features) - `cargo test --no-default-features --features telemetry` (544 passed) - `cargo test` (809 passed) In the fork, `cargo test -p librespot-oauth` (14 passed) and `cargo clippy -p librespot-oauth --all-targets -- -D warnings` (clean). The listener tests cover both shapes of librespot#1705 (blank line on the same connection, and on a separate one), a `/favicon.ico` request before the callback, a code surviving a browser that hangs up before the success page is written, and a peer dribbling bytes being cut off at the deadline. They run behind a watchdog so a regression fails the suite instead of hanging it. Reviewed across five rounds with the Codex CLI, which caught four real defects: a captured authorization code being discarded when the success page failed to write, the accept loop having no overall deadline, HTTP header lines being read as if each were a request, and the deadline not covering an in-flight read. # Additional notes Not verified against a live LibreWolf on Windows. I am on Linux and cannot reproduce the reporter's setup, so this rests on code reading plus the three corroborating reports above rather than an observed repro. A fork fix reaches everyone who installs today: GitHub releases, winget, Homebrew, both AUR packages, and `install.sh`/`install.ps1` all build with `[patch]` active. The one gap is `cargo install spotatui` from crates.io, which is stuck at 0.40.2 because the v0.40.3 publish job fails to compile against upstream librespot (`unresolved import librespot_connect::SavedPlaybackState`). Worth fixing separately; those users cannot get 0.40.3+ by any route today. Workaround for anyone hitting this before the next release: set `enable_streaming: false` in `client.yml`. That stops the every-launch browser flow and leaves Spotify Connect working. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Log files now use a platform-appropriate temporary directory and include the process ID. - The changelog displays the actual log-file location. - Automatic updates restart after authentication resources are released. - **Bug Fixes** - Improved OAuth callback handling when the callback port is unavailable. - Streaming authentication errors now include more actionable details. - Updated playback and connection reliability fixes. - **Documentation** - Updated safe-by-default guidance to explain how to find the log-file location. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
Verified this against current |
Member
|
It needs a changelog entry. Or it could be closed because librewolf fixed their bug. Either would be OK by me. |
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
4 participants
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.
It is possible that a browser, after login, sends out empty lines through the socket before sending the auth code. This fix will skip the empty lines and will parse the first non-empty line.
This fixes 1705.
I am not sure if this fix is significant enough to be worth mentioning in
CHANGELOG.md, so I omitted it.