fix: keep the streaming OAuth callback alive through browser noise - #423
Merged
Merged
Conversation
The streaming login hands port 8989 to librespot, whose callback server accepted exactly one connection and gave up if it was not the redirect. 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 dropped the listener, so the redirect carrying the code hit a closed 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 (#414, upstream librespot#1705). 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. Only the librespot half was fragile, which is why the web API login worked and the streaming one did not. Fixed in the fork rather than here: cherry-picked upstream PR #1706 and hardened it further, since skipping to the next connection on a blank line only works when the blank line arrives on its own connection. Bumps the patch rev to pick it up. Also names the actual error in the status message, which previously said only that authentication failed.
setup_logging hard-coded "/tmp/spotatui_logs/", and the help screen and docs repeated that literal string. On Windows it is a drive-relative path, so the app printed a location the user's own shell could not resolve, directly above the line inviting them to report bugs. Anyone asked for a log file on Windows had to guess where it went. Resolved through std::env::temp_dir now, so Windows lands in %TEMP% and the POSIX platforms keep /tmp. One helper owns the path and both the writing side and the two reporting sides call it, so what is shown is always what is written. Kept in the temp directory rather than moved to the state dir: a file is written per process id, and temp is the one location the platform clears on its own.
run_auto_update ran concurrently with authentication in a tokio::join!, and on a successful install it re-exec'd immediately. That deadlocks startup. The re-exec blocks the task in Command::status(), so the joined authentication future stops being polled while it still owns the OAuth callback port, and the child, which repeats startup from scratch, cannot bind that port because the parent is frozen holding it. The parent waits on the child, the child waits on a port the parent will never release, and both fight over the same terminal. The update check still runs concurrently, since that is a network round trip worth overlapping. Only the restart moves, to after the join. Authentication persists its token before returning, so the child reuses it instead of opening a second browser login. The restart runs before the auth error is propagated, so a broken auth state can still restart into the newer build that may fix it.
wait_for_oauth_callback_port probes by binding and releasing, so between its release and librespot's own bind the port is unowned. That race cannot be closed from here, because librespot binds the port itself. Refusing to start the login when the probe times out was the worst reading of it: we never even attempted, and reported a message far vaguer than the bind error librespot would have produced. It now reports rather than decides. A busy port is logged and the login goes ahead, leaving librespot's bind as the authority on whether the port is usable.
Picks up the callback listener fixes made during review: the captured code is no longer discarded when the success page fails to write, the accept loop has an overall deadline that also covers reading, and each connection handles a single request so no request framing has to be guessed at. Also corrects the log location wording, since temp_dir honours TMPDIR where it is set rather than always being /tmp.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR centralizes temporary log paths, changes OAuth callback probing to a boolean fallback, updates the librespot patch revision, and defers automatic-update restarts until authentication completes. ChangesRuntime flow updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Startup
participant AutoUpdate
participant Authentication
participant restart_after_update
participant UpdatedBinary
Startup->>AutoUpdate: run_auto_update()
AutoUpdate-->>Startup: installed version or None
Startup->>Authentication: complete startup authentication
Authentication-->>Startup: success or error
Startup->>restart_after_update: restart after authentication
restart_after_update->>UpdatedBinary: re-execute installed binary
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/paths.rs`:
- Around line 54-64: Update the runtime log-directory setup around the
create_dir_all call to use ensure_private_dir or equivalent private-directory
creation, then verify the resulting permissions before opening the log file.
Apply this to the path returned by app_log_dir so Unix users cannot read logs
through the shared temporary directory.
In `@src/runtime.rs`:
- Line 643: Update restart_after_update to return anyhow::Result<()> and replace
the current_exe expect call with ? so the executable-path error propagates;
update its call site to use restart_after_update(installed_update)?. Preserve
the existing restart behavior on success.
In `@src/tui/ui/home.rs`:
- Around line 584-585: Update App initialization to resolve and store the
application log path in App state, then change build_changelog_lines and its
callers to render that stored value instead of calling
crate::core::paths::app_log_path() during UI rendering. Ensure UI code in
src/tui/ui/**/*.rs reads the path exclusively from App state.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25bcd8c4-f517-4c29-a329-69f2fa5642a0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (6)
Cargo.tomldocs/configuration.mdsrc/core/paths.rssrc/infra/player/streaming.rssrc/runtime.rssrc/tui/ui/home.rs
The flake pins a fixed-output hash for the librespot git dependency, so bumping the [patch.crates-io] rev fails the Nix build with a mismatch until this moves with it. Notes the coupling next to the constant so the next bump does not rediscover it from a red CI run.
Three findings from review. The log directory sits in the shared OS temp dir under a predictable name, and create_dir_all left it at the default mode, so on Unix any other local user could read the logs. It goes through ensure_private_dir now, the same 0700 helper the credential directories use. The help screen called app_log_path() during rendering, which reads the environment on every frame and breaks the rule that draw code renders from App state and performs no I/O. The path is resolved once into App and threaded through to the renderer. std::env::current_exe can fail, and expect turned that into a panic at the worst moment, just after an update had installed. restart_after_update returns Result now so the caller reports it like any other startup failure.
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.
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_credentialsit 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 (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
AuthCodeListenerParsein their log who confirmed Chrome works). The common variable is the browser.spotatui's own callback server never had this problem:
extract_callback_urlrunssplit_whitespace()over the whole buffer, which steps over a leading CRLF, and it answers unrelated requests with a 400 and keeps waiting.read_linestops 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 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:
run_auto_updateran concurrently with authentication in atokio::join!and re-exec'd immediately on a successful install. The re-exec blocks the task inCommand::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.setup_logginghard-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 throughstd::env::temp_dir.wait_for_oauth_callback_portrefused 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) andcargo 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.icorequest 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.ps1all build with[patch]active. The one gap iscargo install spotatuifrom 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: falseinclient.yml. That stops the every-launch browser flow and leaves Spotify Connect working.Summary by CodeRabbit
New Features
Bug Fixes
Documentation