Conversation
There was a problem hiding this comment.
Changes requested:
- Do not store the proxy password in readable
settings.json. Use owner-only credential state or keep it session-only. - System proxy mode must also route librespot when an HTTP proxy can be resolved, or document the platforms where it cannot.
- Resolve the advisories in the activated
hyper-proxy2TLS path. - Verify both Web API and local playback through an actual HTTP proxy.
The manual HTTP routing approach is sound.
|
Addressed the four review points in 5ac6e85: Password. System → librespot. System mode now resolves an HTTP proxy with the same hyper-proxy2 TLS. The published 0.1.0 crate is patched in-tree onto rustls 0.23 / hyper-rustls 0.27. Verification. The Nix |
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a couple of security-relevant issues (proxy username logging and private-file permission tightening) plus a PR-description mismatch about where the proxy password is stored.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds first-class HTTP/SOCKS5 proxy support to Fastpotify by introducing a shared HTTP client that can be rebuilt at runtime from Settings (and from the login screen), while also routing librespot local playback through an HTTP CONNECT proxy when applicable. The PR stays within the product boundary of “native client talking only to Spotify” and includes docs/tests for the new behavior.
Changes:
- Add proxy settings UI (Settings → Proxy and a “Proxy Settings” expander on the login screen) and plumb a new
Action::ApplyProxythrough app/backend. - Introduce
ProxyMode/ProxyConfigand a sharedHttphandle to rebuild thereqwestclient on proxy changes without restarting the app. - Vendor/patch
hyper-proxy2to align TLS dependencies with rustls 0.23, and update docs for new settings + secret file location.
File summaries
| File | Description |
|---|---|
| vendor/hyper-proxy2/src/tunnel.rs | Vendored CONNECT tunnel implementation used by patched hyper-proxy2. |
| vendor/hyper-proxy2/src/stream.rs | Vendored proxy stream wrapper implementing Hyper I/O traits. |
| vendor/hyper-proxy2/src/rt.rs | Vendored runtime helpers for buffered read/write futures. |
| vendor/hyper-proxy2/src/lib.rs | Vendored hyper-proxy2 connector patched to rustls 0.23-compatible deps. |
| vendor/hyper-proxy2/README.md | Notes why hyper-proxy2 is vendored/patched and when to drop it. |
| vendor/hyper-proxy2/LICENSE-MIT.md | Adds vendored MIT license file. |
| vendor/hyper-proxy2/Cargo.toml | Defines vendored crate deps/features for rustls 0.23/hyper-rustls 0.27. |
| src/ui/widgets.rs | Adds shared “labeled field” widget and a manual proxy form UI helper. |
| src/ui/settings.rs | Adds Settings → Proxy section, dirty tracking, and Apply behavior. |
| src/ui/login.rs | Adds login-screen proxy expander and applies proxy before first sign-in. |
| src/settings.rs | Adds proxy modes/config parsing/validation + secret-file persistence hooks. |
| src/player.rs | Threads ProxyConfig into engine config and librespot session config. |
| src/paths.rs | Adds path for proxy secret file in the state directory. |
| src/model.rs | Adds Action::ApplyProxy for rebuilding HTTP client / restarting playback conditionally. |
| src/milkdrop.rs | Routes MilkDrop preset downloads through the shared proxy-aware HTTP client. |
| src/main.rs | Loads the proxy secret file at startup. |
| src/lib.rs | Exposes new http module. |
| src/images.rs | Moves artwork loader to use shared Http handle rather than a fixed reqwest::Client. |
| src/http.rs | New shared Http wrapper and builders for async/blocking reqwest clients w/ proxy config. |
| src/backend.rs | Uses shared Http, adds proxy apply command path, and refines reconnect limiter logic. |
| src/auth.rs | Exposes write_private for writing the proxy secret file (owner-only). |
| src/app.rs | Saves proxy secret alongside settings; wires ApplyProxy and sign-in proxy validation. |
| src/api/gateway.rs | Accepts shared Http handle so API sessions pick up proxy changes. |
| src/api/client.rs | Uses shared Http for Web API calls and token refresh so proxy changes take effect. |
| README.md | Documents proxy settings and that the proxy password is stored outside settings.json. |
| docs/_reference/settings-and-files.md | Documents proxy password file and proxy settings fields. |
| docs/_reference/how-it-connects.md | Documents proxy behavior and impact on Web API vs local playback. |
| docs/_guide/getting-started.md | Mentions configuring proxy before first sign-in and secret storage location. |
| Cargo.toml | Enables reqwest socks/system-proxy; adds hyper-util proxy matcher; patches hyper-proxy2. |
| Cargo.lock | Updates lockfile for patched hyper-proxy2 + dependency graph changes. |
Review details
- Files reviewed: 22/30 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Still needed:
- Do not pass credential-bearing proxy URLs to librespot. It does not apply proxy authentication and its info log writes the full URL, including the password. Until upstream fixes both, the UI and docs must say proxy login covers Web API traffic only.
- Keep LAN receiver requests direct.
- Make proxy secret replacement work on Windows and tighten existing Unix secret files to mode 0600.
- Do not log proxy usernames from Fastpotify either.
- Update the PR description, remove the em dashes, refresh the Nix hash, and test sign-in plus local playback through a real HTTP proxy.
e809e13 to
837839e
Compare
837839e to
eb40b98
Compare
|
Addressed the remaining review points in
All CI jobs now pass on Ubuntu, macOS, Windows, and Nix. I have now verified the supported HTTP and SOCKS5 paths against real proxy servers. They work as expected within the documented limitations. macOS System mode needs more work. Cocoa applications delegate proxy selection to the system URL loading stack, whose behavior includes per-URL bypass rules, SOCKS, PAC, and ordered fallback. The current reqwest-based implementation does not fully match that behavior, so I need to refactor the macOS HTTP transport before this is ready. |
|
After digging further, I do not think the current implementation is the right final architecture for proxy support. Two design issues remain:
Would this split make sense to you: implement the |
|
Please keep this to the transports we already support directly. Do not add URLSession or a loopback playback bridge. Web requests may use the configured proxy; local playback should use only the unauthenticated HTTP proxy librespot supports, with that limitation stated plainly. Please rebase onto main. Also, Http::from_proxy must report a client-build failure instead of silently switching the configured proxy off. |
|
Another real-world data point for the scoped-down design in this PR, from a corporate network where this is currently a hard blocker. Environment: Spotifast 0.7.1 (Flatpak, latest release), Linux. This network has no direct egress at all — not merely blocked DNS:
The behaviour matches the analysis in this thread exactly. The Web API works — which lines up with The part that seems useful for this PR: the unauthenticated-
So passing an unauthenticated |
Add centered sign-in and Settings controls for System, Off, HTTP and SOCKS5. Apply accepted changes through the shared HTTP client, keep local receiver traffic direct, and route supported playback connections through HTTP CONNECT. Store endpoint-bound proxy passwords in the platform credential store and preserve draft, migration and stale engine-connection ordering. Use the signed hyper-proxy2 fork with its upstream provider-compatibility contribution instead of a vendored TLS patch. Cover real proxy transports, credential storage and narrow-window forms on native platforms. Co-authored-by: Carmine Paolino <carmine@paolino.me>
|
Implemented on main in a50347b, with follow-up ec1d934. Proxy settings are available before sign-in, and local playback now uses the configured unauthenticated HTTP proxy, including System mode. Authenticated HTTP and SOCKS5 apply to web requests only. All checks pass. This is unreleased, after 0.8.0. Thanks @we11adam for the contribution and @aaronrancsik for confirming the corporate-network case. |
Why
Spotify is sometimes reachable only through an HTTP or SOCKS5 proxy.
Fastpotify had no proxy setting, so browser grants, Web API requests, and
local playback connections could not follow that network policy.
What changed
Settings > Proxy and the sign-in screen now offer four modes: Off,
System, HTTP, and SOCKS5. Manual modes take a host, port, and
optional login. Off and System apply immediately. Manual settings wait for
Apply settings and validate the host and port first.
The Web API, token refresh, artwork, lyrics, update checks, and MilkDrop
preset downloads use one replaceable proxy-aware HTTP client. SOCKS5 resolves
Spotify hostnames at the proxy. Spotify Connect receiver activation stays
direct because those endpoints are on the LAN.
Local playback follows only an unauthenticated
http://proxy. This is thesubset librespot currently supports safely. SOCKS5, authenticated HTTP,
authenticated System proxies, and
https://proxy endpoints still cover Webtraffic, while librespot connects directly. The UI and documentation state
that boundary. Proxy changes discard any in-flight stale engine connection
and reconnect only when the proxy librespot can actually use changes.
The optional proxy password is not written to
settings.json. It is storedin the state directory through the owner-only secret path. Older settings
files containing the field migrate on their next save. Existing Unix secret
files are tightened to mode 0600, and atomic replacement uses the Windows
replace-file API where
std::fs::renamecannot overwrite a destination.Proxy usernames and passwords are excluded from Fastpotify and librespot
logs.
The vendored
hyper-proxy2patch moves its active CONNECT TLS path to rustls0.23 and removes the older advised
rustls-webpkicopy. The Nix vendor hashis updated from the CI-produced value for the current
mainmerge result.Verification
Run locally on macOS aarch64 after rebasing onto current
main:Focused tests exercise exact opaque proxy authentication, HTTP forwarding,
librespot CONNECT routing, credential-free librespot configuration, System
proxy restrictions, atomic secret replacement, Unix permission tightening,
and proxy changes during an in-flight engine connection.
Local Clippy currently stops on the unchanged
src/mac_links.rs:41type_complexitylint introduced on currentmain. I did not suppress orfold that unrelated fix into this PR. The previous PR CI passed quality,
Linux, macOS, Windows, and docs; the Nix job supplied the corrected hash used
here. The new CI run will compile and execute the Windows replacement path.
I could not run
bundle exec jekyll buildlocally because the machine lacksthe locked Bundler 4.0.16. I also have not claimed a signed-in playback test
through a real external HTTP proxy: no proxy service is available on this
machine. The tests use real local TCP proxy listeners, but they do not replace
that final live-network check.
Platform coverage
Runtime testing was on macOS aarch64. CI covers Linux, macOS, and Windows.
CONTRIBUTING.mdand kept this pull request to one concern.and network access.
including AI-assisted code.