From 1982b65417eb260be540055421b4398fba60ffba Mon Sep 17 00:00:00 2001 From: postigodev Date: Tue, 6 Oct 2026 12:14:29 -0500 Subject: [PATCH 1/7] ci: validate integrated main without publishing releases (#32) --- .github/workflows/release.yml | 12 +++++++---- scripts/Test-ValidationWorkflow.ps1 | 32 +++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 scripts/Test-ValidationWorkflow.ps1 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b982db9..65ae505 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -3,6 +3,8 @@ name: Release on: pull_request: push: + branches: + - main tags: - "v*" @@ -29,7 +31,7 @@ jobs: name: Validate release identity shell: pwsh env: - EXPECTED_TAG: ${{ github.event_name == 'push' && github.ref_name || '' }} + EXPECTED_TAG: ${{ github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v') && github.ref_name || '' }} run: | $info = ./scripts/Test-ReleaseVersion.ps1 -ExpectedTag $env:EXPECTED_TAG -PassThru "version=$($info.Version)" >> $env:GITHUB_OUTPUT @@ -61,7 +63,8 @@ jobs: run: | cargo fmt --all -- --check cargo check --workspace --locked - cargo test --workspace --locked + # Binding persistence tests mutate process-wide HOME/APPDATA. + cargo test --workspace --locked -- --test-threads=1 - name: Parse release scripts shell: pwsh run: | @@ -76,6 +79,7 @@ jobs: } } if ($failed) { exit 1 } + ./scripts/Test-ValidationWorkflow.ps1 - name: Check frontend working-directory: apps/tauri shell: pwsh @@ -96,7 +100,7 @@ jobs: build-windows: name: Build Windows installers - if: github.event_name == 'push' + if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v') needs: [plan, validate] runs-on: windows-2022 steps: @@ -125,7 +129,7 @@ jobs: publish: name: Publish verified draft - if: github.event_name == 'push' + if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v') needs: [plan, build-windows] runs-on: windows-2022 permissions: diff --git a/scripts/Test-ValidationWorkflow.ps1 b/scripts/Test-ValidationWorkflow.ps1 new file mode 100644 index 0000000..b844f92 --- /dev/null +++ b/scripts/Test-ValidationWorkflow.ps1 @@ -0,0 +1,32 @@ +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +$workflow = Get-Content -Raw (Join-Path $PSScriptRoot '../.github/workflows/release.yml') +if ($workflow -notmatch '(?m)^ pull_request:\s*$' -or + $workflow -notmatch '(?m)^ push:\s*\r?\n branches:\s*\r?\n - main\s*\r?\n tags:\s*\r?\n - "v\*"') { + throw 'Validation must run for pull requests, main pushes, and version tags.' +} + +$tagGuard = "github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v')" +foreach ($job in @('build-windows', 'publish')) { + if (-not $workflow.Contains(" ${job}:`n") -and -not $workflow.Contains(" ${job}:`r`n")) { + throw "Missing release job: $job" + } + if ($workflow -notmatch ("(?m)^ " + $job + ":\s*\r?\n name:[^\r\n]*\r?\n if: " + [regex]::Escape($tagGuard) + '\s*$')) { + throw "$job must be restricted to version tag pushes." + } +} +if (-not $workflow.Contains("EXPECTED_TAG: " + '${{ ' + $tagGuard + " && github.ref_name || '' }}")) { + throw 'Ordinary main pushes must not be interpreted as release tags.' +} +foreach ($job in @('plan', 'validate')) { + $block = [regex]::Match($workflow, "(?ms)^ ${job}:.*?(?=^ [a-z-]+:|\z)").Value + if (-not $block -or $block -match '(?m)^ if:') { + throw "$job must run for every triggered validation event." + } +} +if (-not $workflow.Contains('cargo test --workspace --locked -- --test-threads=1')) { + throw 'Environment-mutating persistence tests must run serially.' +} +./scripts/Test-ReleaseVersion.ps1 -ExpectedTag '' +Write-Host 'Validation workflow regression checks passed.' From 4117fdbafed05c1502793bca0546979476e29ea1 Mon Sep 17 00:00:00 2001 From: postigodev Date: Tue, 6 Oct 2026 12:18:57 -0500 Subject: [PATCH 2/7] fix(spotify): ensure Start on TV leaves playback running (#31) --- crates/core/src/spotify/mod.rs | 165 ++++++++++++++++++++++++++++++++- 1 file changed, 164 insertions(+), 1 deletion(-) diff --git a/crates/core/src/spotify/mod.rs b/crates/core/src/spotify/mod.rs index 46608f9..544a4a4 100644 --- a/crates/core/src/spotify/mod.rs +++ b/crates/core/src/spotify/mod.rs @@ -447,11 +447,41 @@ pub async fn skip_previous(config: &AppConfig) -> Result { pub async fn start_on_tv(config: &AppConfig) -> Result { let firetv_result = crate::firetv::prepare_spotify_session(&config.firetv_ip)?; - let spotify_result = toggle_on_tv(config).await?; + if !spotify_configured(config) { + bail!("{}", spotify_config_error(config)); + } + let spotify = build_spotify(config)?; + ensure_token(&spotify).await?; + let target = resolve_control_target(config, &spotify).await?; + let spotify_result = ensure_target_playing(&spotify, &target).await?; Ok(format!("{}. {}", firetv_result.summary, spotify_result)) } +async fn ensure_target_playing(spotify: &AuthCodeSpotify, target: &TargetDevice) -> Result { + let playback = spotify + .current_playback(None, Some(&[AdditionalType::Episode])) + .await + .context("failed to fetch current Spotify playback")?; + let on_target = + playback.as_ref().and_then(|item| item.device.id.as_deref()) == Some(target.id.as_str()); + if on_target && playback.as_ref().is_some_and(|item| item.is_playing) { + return Ok(format!("Spotify is already playing on {}", target.name)); + } + if !on_target { + spotify + .transfer_playback(&target.id, Some(false)) + .await + .context("failed to transfer Spotify playback to TV")?; + sleep(Duration::from_millis(300)).await; + } + spotify + .resume_playback(Some(&target.id), None) + .await + .context("failed to resume playback on TV")?; + Ok(format!("Started Spotify on {}", target.name)) +} + fn spotify_configured(config: &AppConfig) -> bool { validate_spotify_config( &config.spotify_client_id, @@ -912,6 +942,139 @@ fn ensure_token_cache_dir() -> Result<()> { mod tests { use super::*; + async fn playback_client( + device_id: &str, + is_playing: bool, + write_statuses: &[u16], + ) -> (AuthCodeSpotify, tokio::task::JoinHandle>) { + use tokio::io::AsyncReadExt; + + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let body = serde_json::json!({ + "device": {"id": device_id, "name": "TV", "is_active": true, + "is_private_session": false, "is_restricted": false, + "type": "TV", "volume_percent": 50}, + "repeat_state": "off", "shuffle_state": false, + "timestamp": 0, "is_playing": is_playing, "item": null, + "currently_playing_type": "unknown", "actions": {"disallows": {}} + }) + .to_string(); + let mut responses = vec![(200, body)]; + responses.extend(write_statuses.iter().map(|status| (*status, String::new()))); + let server = tokio::spawn(async move { + let mut requests = Vec::new(); + for (status, body) in responses { + let (mut stream, _) = timeout(Duration::from_secs(5), listener.accept()) + .await + .unwrap() + .unwrap(); + let mut reader = BufReader::new(&mut stream); + let mut request = String::new(); + let mut length = 0; + loop { + let mut line = String::new(); + reader.read_line(&mut line).await.unwrap(); + if line == "\r\n" { + break; + } + if let Some(value) = line.to_ascii_lowercase().strip_prefix("content-length:") { + length = value.trim().parse::().unwrap(); + } + request.push_str(&line); + } + let mut payload = vec![0; length]; + reader.read_exact(&mut payload).await.unwrap(); + request.push_str(std::str::from_utf8(&payload).unwrap()); + requests.push(request); + stream.write_all(format!( + "HTTP/1.1 {status} Test\r\ncontent-type: application/json\r\ncontent-length: {}\r\nconnection: close\r\n\r\n{body}", body.len() + ).as_bytes()).await.unwrap(); + } + requests + }); + let spotify = AuthCodeSpotify::from_token_with_config( + rspotify::Token { + access_token: "test-token".into(), + ..Default::default() + }, + Credentials::default(), + OAuth::default(), + Config { + api_base_url: format!("http://{address}/"), + token_refreshing: false, + ..Default::default() + }, + ); + (spotify, server) + } + + #[tokio::test] + async fn start_leaves_already_playing_target_running_on_repeated_calls() { + let target = TargetDevice { + id: "tv".into(), + name: "TV".into(), + }; + for _ in 0..2 { + let (spotify, server) = playback_client("tv", true, &[]).await; + assert_eq!( + ensure_target_playing(&spotify, &target).await.unwrap(), + "Spotify is already playing on TV" + ); + let requests = server.await.unwrap(); + assert_eq!(requests.len(), 1); + assert!(requests[0].starts_with("GET /me/player?")); + } + } + + #[tokio::test] + async fn start_resumes_paused_target_and_routes_playback_elsewhere() { + let target = TargetDevice { + id: "tv".into(), + name: "TV".into(), + }; + let (spotify, server) = playback_client("tv", false, &[204]).await; + ensure_target_playing(&spotify, &target).await.unwrap(); + let requests = server.await.unwrap(); + assert_eq!(requests.len(), 2); + assert!(requests[1].starts_with("PUT /me/player/play?device_id=tv ")); + + let (spotify, server) = playback_client("phone", true, &[204, 204]).await; + ensure_target_playing(&spotify, &target).await.unwrap(); + let requests = server.await.unwrap(); + assert_eq!(requests.len(), 3); + assert!(requests[1].starts_with("PUT /me/player ")); + assert!(requests[1].contains("\"device_ids\":[\"tv\"]")); + assert!(requests[2].starts_with("PUT /me/player/play?device_id=tv ")); + } + + #[tokio::test] + async fn start_reports_transfer_and_resume_failures() { + let target = TargetDevice { + id: "tv".into(), + name: "TV".into(), + }; + for (device, statuses, expected) in [ + ( + "phone", + vec![500], + "failed to transfer Spotify playback to TV", + ), + ("phone", vec![204, 500], "failed to resume playback on TV"), + ("tv", vec![500], "failed to resume playback on TV"), + ] { + let (spotify, server) = playback_client(device, false, &statuses).await; + assert_eq!( + ensure_target_playing(&spotify, &target) + .await + .unwrap_err() + .to_string(), + expected + ); + server.await.unwrap(); + } + } + fn config(selected_device_id: &str) -> AppConfig { AppConfig { spotify_selected_device_id: selected_device_id.into(), From fc6135686971d10b98b38f5d4baf94267a1db40b Mon Sep 17 00:00:00 2001 From: postigodev Date: Tue, 6 Oct 2026 12:23:15 -0500 Subject: [PATCH 3/7] fix(bindings): persist complete favorite reorders atomically (#35) --- apps/tauri/src-tauri/src/commands.rs | 5 ++ apps/tauri/src-tauri/src/lib.rs | 1 + apps/tauri/src/api.ts | 1 + apps/tauri/src/main.ts | 8 +- crates/core/src/bindings.rs | 113 +++++++++++++++++++++++++++ 5 files changed, 121 insertions(+), 7 deletions(-) diff --git a/apps/tauri/src-tauri/src/commands.rs b/apps/tauri/src-tauri/src/commands.rs index d82ded9..50fee43 100644 --- a/apps/tauri/src-tauri/src/commands.rs +++ b/apps/tauri/src-tauri/src/commands.rs @@ -56,6 +56,11 @@ pub async fn bindings_save(binding: Binding) -> Result { run_blocking(move || bindings::save_binding(binding).map_err(|e| e.to_string())).await } +#[command] +pub async fn bindings_reorder_favorites(ids: Vec) -> Result { + run_blocking(move || bindings::reorder_favorites(&ids).map_err(|e| format!("{e:#}"))).await +} + #[command] pub async fn bindings_delete(id: String) -> Result { run_blocking(move || bindings::delete_binding(&id).map_err(|e| e.to_string())).await diff --git a/apps/tauri/src-tauri/src/lib.rs b/apps/tauri/src-tauri/src/lib.rs index c6f874f..aa1b13d 100644 --- a/apps/tauri/src-tauri/src/lib.rs +++ b/apps/tauri/src-tauri/src/lib.rs @@ -37,6 +37,7 @@ pub fn run() { commands::save_settings, commands::bindings_list, commands::bindings_save, + commands::bindings_reorder_favorites, commands::bindings_delete, commands::bindings_execute, commands::health_check, diff --git a/apps/tauri/src/api.ts b/apps/tauri/src/api.ts index 954dca6..32999be 100644 --- a/apps/tauri/src/api.ts +++ b/apps/tauri/src/api.ts @@ -21,6 +21,7 @@ export const api = { saveSettings: (config: AppConfig) => invoke("save_settings", { config }), bindingsList: () => invoke("bindings_list"), bindingsSave: (binding: Binding) => invoke("bindings_save", { binding }), + bindingsReorderFavorites: (ids: string[]) => invoke("bindings_reorder_favorites", { ids }), bindingsDelete: (id: string) => invoke("bindings_delete", { id }), bindingsExecute: (id: string) => invoke("bindings_execute", { id }), healthCheck: () => invoke("health_check"), diff --git a/apps/tauri/src/main.ts b/apps/tauri/src/main.ts index aba734b..f4b493f 100644 --- a/apps/tauri/src/main.ts +++ b/apps/tauri/src/main.ts @@ -1222,13 +1222,7 @@ async function reorderFavoriteBindings(sourceId: string, targetId: string) { render(); try { - let latestStore = { bindings: currentBindings }; - for (const binding of reorderedFavorites) { - latestStore = await api.bindingsSave({ - ...binding, - favorite_order: reorderedById.get(binding.id)!.favorite_order, - }); - } + const latestStore = await api.bindingsReorderFavorites(reorderedFavorites.map((binding) => binding.id)); currentBindings = latestStore.bindings; busy = false; flash("Quick Access order updated."); diff --git a/crates/core/src/bindings.rs b/crates/core/src/bindings.rs index 7e1a669..7d8ed6f 100644 --- a/crates/core/src/bindings.rs +++ b/crates/core/src/bindings.rs @@ -7,6 +7,7 @@ use anyhow::{bail, Context, Result}; use rand::{distr::Alphanumeric, Rng}; use serde::{Deserialize, Serialize}; use std::{ + collections::HashSet, fs::{self, OpenOptions}, io::Write, path::PathBuf, @@ -120,6 +121,45 @@ fn normalize_favorite_orders(store: &mut BindingStore) { } } +pub fn reorder_favorites(ids: &[String]) -> Result { + let mut store = list_bindings()?; + let favorite_count = store + .bindings + .iter() + .filter(|binding| binding.favorite) + .count(); + if ids.len() != favorite_count { + bail!("Favorite order must include every favorite exactly once. Reload Quick Access and try again."); + } + let mut seen = HashSet::new(); + for id in ids { + let binding = store + .bindings + .iter() + .find(|binding| &binding.id == id) + .with_context(|| { + format!("Binding not found: {id}. Reload Quick Access and try again.") + })?; + if !binding.favorite { + bail!("Binding is not a favorite: {id}"); + } + if !seen.insert(id) { + bail!("Duplicate binding in favorite order: {id}"); + } + } + for (index, id) in ids.iter().enumerate() { + store + .bindings + .iter_mut() + .find(|binding| &binding.id == id) + .expect("validated favorite") + .favorite_order = + u32::try_from(index + 1).context("too many favorites to persist their order")?; + } + write_store(&store)?; + Ok(store) +} + pub async fn execute_binding(id: &str, config: &AppConfig) -> Result { let store = list_bindings()?; let binding = store @@ -460,4 +500,77 @@ mod tests { assert!(fs::read_to_string(path).is_ok()); }); } + + #[test] + fn reorder_normalizes_all_favorites_and_preserves_other_binding_fields() { + with_temp_home(|| { + save_binding(binding("home", "Home", true, 7)).unwrap(); + save_binding(binding("spotify", "Spotify", true, 7)).unwrap(); + save_binding(binding("other", "Other", false, 12)).unwrap(); + let before = list_bindings().unwrap(); + let reordered = reorder_favorites(&["spotify".into(), "home".into()]).unwrap(); + assert_eq!(reordered.bindings[0].favorite_order, 2); + assert_eq!(reordered.bindings[1].favorite_order, 1); + for (old, new) in before.bindings.iter().zip(&reordered.bindings) { + let mut expected = serde_json::to_value(old).unwrap(); + if old.favorite { + expected["favorite_order"] = new.favorite_order.into(); + } + assert_eq!(expected, serde_json::to_value(new).unwrap()); + } + assert_eq!( + serde_json::to_value(list_bindings().unwrap()).unwrap(), + serde_json::to_value(reordered).unwrap() + ); + assert_eq!( + fs::read_dir(stored_bindings_path().parent().unwrap()) + .unwrap() + .count(), + 1 + ); + }); + } + + #[test] + fn invalid_reorders_leave_persisted_bindings_unchanged() { + with_temp_home(|| { + save_binding(binding("a", "A", true, 3)).unwrap(); + save_binding(binding("b", "B", true, 8)).unwrap(); + save_binding(binding("other", "Other", false, 0)).unwrap(); + let original = fs::read(stored_bindings_path()).unwrap(); + for ids in [ + vec!["a", "missing"], + vec!["a", "a"], + vec!["a"], + vec![], + vec!["a", "other"], + vec!["a", "b", "other"], + ] { + let ids = ids.into_iter().map(str::to_string).collect::>(); + assert!(reorder_favorites(&ids).is_err(), "accepted {ids:?}"); + assert_eq!(fs::read(stored_bindings_path()).unwrap(), original); + } + }); + } + + #[cfg(windows)] + #[test] + fn failed_reorder_replacement_keeps_the_complete_old_order() { + use std::os::windows::fs::OpenOptionsExt; + with_temp_home(|| { + save_binding(binding("a", "A", true, 3)).unwrap(); + save_binding(binding("b", "B", true, 8)).unwrap(); + let path = stored_bindings_path(); + let original = fs::read(&path).unwrap(); + let held_file = OpenOptions::new() + .read(true) + .share_mode(1) + .open(&path) + .unwrap(); + assert!(reorder_favorites(&["b".into(), "a".into()]).is_err()); + assert_eq!(fs::read(&path).unwrap(), original); + assert_eq!(fs::read_dir(path.parent().unwrap()).unwrap().count(), 1); + drop(held_file); + }); + } } From 337648ef5aad10f3399107daf781804d25985273 Mon Sep 17 00:00:00 2001 From: postigodev Date: Tue, 6 Oct 2026 12:26:39 -0500 Subject: [PATCH 4/7] fix(config): atomically persist settings and compensate autostart (#36) --- apps/tauri/src-tauri/src/commands.rs | 157 +++++++++++++++++++++++++-- crates/core/src/bindings.rs | 91 +--------------- crates/core/src/config/mod.rs | 119 +++++++++++++++++--- crates/core/src/lib.rs | 1 + crates/core/src/persistence.rs | 69 ++++++++++++ 5 files changed, 323 insertions(+), 114 deletions(-) create mode 100644 crates/core/src/persistence.rs diff --git a/apps/tauri/src-tauri/src/commands.rs b/apps/tauri/src-tauri/src/commands.rs index 50fee43..6c7b88f 100644 --- a/apps/tauri/src-tauri/src/commands.rs +++ b/apps/tauri/src-tauri/src/commands.rs @@ -9,6 +9,8 @@ use serde::Serialize; use tauri::{async_runtime, command, AppHandle}; use tauri_plugin_autostart::ManagerExt; +static SETTINGS_SAVE_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + #[derive(Serialize)] pub struct AppInfo { version: String, @@ -39,8 +41,26 @@ pub fn get_app_info(app: AppHandle) -> AppInfo { #[command] pub async fn save_settings(app: AppHandle, config: AppConfig) -> Result { run_blocking(move || { - sync_autostart_setting(&app, &config).map_err(|e| e.to_string())?; - config.save().map_err(|e| e.to_string())?; + let _guard = SETTINGS_SAVE_LOCK.lock().map_err(|_| { + "Settings save lock is unavailable. Restart Sendo and retry.".to_string() + })?; + let manager = app.autolaunch(); + let previous = manager.is_enabled().map_err(|e| { + format!("Could not read system autostart state. Settings were not saved: {e}") + })?; + save_with_autostart( + config.launch_on_startup, + previous, + |enabled| { + if enabled { + manager.enable()?; + } else { + manager.disable()?; + } + Ok(()) + }, + || config.save(), + )?; Ok(config) }) .await @@ -219,16 +239,33 @@ pub async fn spotify_toggle_tv() -> Result { Ok(ActionResult { message }) } -fn sync_autostart_setting(app: &AppHandle, config: &AppConfig) -> anyhow::Result<()> { - let autostart_manager = app.autolaunch(); - let is_enabled = autostart_manager.is_enabled()?; - - match (config.launch_on_startup, is_enabled) { - (true, false) => autostart_manager.enable()?, - (false, true) => autostart_manager.disable()?, - _ => {} +// OS registration and file replacement cannot share a crash-atomic transaction. +// Compensate reported failures here; apply_startup_preferences already attempts +// to reconcile the OS registration with the persisted preference on next startup. +fn save_with_autostart( + desired: bool, + previous: bool, + mut set_enabled: impl FnMut(bool) -> anyhow::Result<()>, + save: impl FnOnce() -> anyhow::Result<()>, +) -> Result<(), String> { + let changed = desired != previous; + let result = (|| -> anyhow::Result<()> { + if changed { + set_enabled(desired) + .map_err(|e| anyhow::anyhow!("Could not synchronize system autostart: {e:#}"))?; + } + save().map_err(|e| anyhow::anyhow!("Could not persist settings: {e:#}")) + })(); + if let Err(error) = result { + if changed { + if let Err(rollback) = set_enabled(previous) { + return Err(format!("Settings were not saved: {error:#}. Could not restore previous autostart state: {rollback:#}. Check system startup settings and retry.")); + } + } + return Err(format!( + "Settings were not saved: {error:#}. Retry after resolving this error." + )); } - Ok(()) } @@ -281,3 +318,101 @@ pub async fn start_spotify_on_tv() -> Result { Ok(ActionResult { message }) } + +#[cfg(test)] +mod tests { + use super::*; + use std::cell::{Cell, RefCell}; + + #[test] + fn settings_write_failure_restores_autostart() { + let enabled = Cell::new(false); + let changes = RefCell::new(Vec::new()); + let result = save_with_autostart( + true, + false, + |value| { + enabled.set(value); + changes.borrow_mut().push(value); + Ok(()) + }, + || { + assert!(enabled.get()); + anyhow::bail!("disk full") + }, + ); + assert!(!enabled.get()); + assert_eq!(*changes.borrow(), vec![true, false]); + assert!(result.unwrap_err().contains("disk full")); + } + + #[test] + fn autostart_failure_prevents_saving_and_reports_failed_compensation() { + let saved = Cell::new(false); + let changes = RefCell::new(Vec::new()); + let result = save_with_autostart( + true, + false, + |value| { + changes.borrow_mut().push(value); + anyhow::bail!("startup registry denied") + }, + || { + saved.set(true); + Ok(()) + }, + ); + assert!(!saved.get()); + assert_eq!(*changes.borrow(), vec![true, false]); + let error = result.unwrap_err(); + assert!(error.contains("startup registry denied")); + assert!(error.contains("restore")); + } + + #[test] + fn settings_failure_reports_both_save_and_rollback_errors() { + let result = save_with_autostart( + true, + false, + |value| { + if value { + Ok(()) + } else { + anyhow::bail!("rollback denied") + } + }, + || anyhow::bail!("disk full"), + ); + let error = result.unwrap_err(); + assert!(error.contains("disk full")); + assert!(error.contains("rollback denied")); + } + + #[test] + fn settings_save_changes_autostart_only_when_needed() { + for (desired, current, expected) in [ + (true, false, vec![true]), + (false, true, vec![false]), + (true, true, vec![]), + (false, false, vec![]), + ] { + let changes = RefCell::new(Vec::new()); + let saved = Cell::new(false); + save_with_autostart( + desired, + current, + |value| { + changes.borrow_mut().push(value); + Ok(()) + }, + || { + saved.set(true); + Ok(()) + }, + ) + .unwrap(); + assert!(saved.get()); + assert_eq!(*changes.borrow(), expected); + } + } +} diff --git a/crates/core/src/bindings.rs b/crates/core/src/bindings.rs index 7d8ed6f..2bffd10 100644 --- a/crates/core/src/bindings.rs +++ b/crates/core/src/bindings.rs @@ -6,12 +6,7 @@ use crate::{ use anyhow::{bail, Context, Result}; use rand::{distr::Alphanumeric, Rng}; use serde::{Deserialize, Serialize}; -use std::{ - collections::HashSet, - fs::{self, OpenOptions}, - io::Write, - path::PathBuf, -}; +use std::{collections::HashSet, fs, path::PathBuf}; #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] @@ -188,90 +183,9 @@ fn bindings_path() -> Result { fn write_store(store: &BindingStore) -> Result<()> { let path = bindings_path()?; - if let Some(parent) = path.parent() { - fs::create_dir_all(parent) - .with_context(|| format!("failed to create bindings dir at {}", parent.display()))?; - } - let raw = serde_json::to_string_pretty(store).context("failed to serialize bindings")?; - let temp_path = path.with_extension(format!("json.tmp-{}", generate_binding_id())); - let write_result = (|| -> Result<()> { - let mut file = OpenOptions::new() - .write(true) - .create_new(true) - .open(&temp_path) - .with_context(|| { - format!( - "failed to create temporary bindings file at {}", - temp_path.display() - ) - })?; - file.write_all(raw.as_bytes()).with_context(|| { - format!( - "failed to write temporary bindings file at {}", - temp_path.display() - ) - })?; - file.sync_all().with_context(|| { - format!( - "failed to sync temporary bindings file at {}", - temp_path.display() - ) - })?; - Ok(()) - })(); - - if let Err(error) = write_result { - let _ = fs::remove_file(&temp_path); - return Err(error); - } - - if let Err(error) = replace_file(&temp_path, &path) { - let _ = fs::remove_file(&temp_path); - return Err(error) - .with_context(|| format!("failed to replace bindings file at {}", path.display())); - } - - Ok(()) + crate::persistence::atomic_write(&path, raw.as_bytes()) } - -#[cfg(not(windows))] -fn replace_file(temp_path: &PathBuf, path: &PathBuf) -> std::io::Result<()> { - fs::rename(temp_path, path) -} - -#[cfg(windows)] -fn replace_file(temp_path: &PathBuf, path: &PathBuf) -> std::io::Result<()> { - use std::os::windows::ffi::OsStrExt; - use windows_sys::Win32::Storage::FileSystem::{ - MoveFileExW, MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, - }; - - let source: Vec = temp_path - .as_os_str() - .encode_wide() - .chain(std::iter::once(0)) - .collect(); - let destination: Vec = path - .as_os_str() - .encode_wide() - .chain(std::iter::once(0)) - .collect(); - - if unsafe { - MoveFileExW( - source.as_ptr(), - destination.as_ptr(), - MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH, - ) - } == 0 - { - Err(std::io::Error::last_os_error()) - } else { - Ok(()) - } -} - fn generate_binding_id() -> String { rand::rng() .sample_iter(&Alphanumeric) @@ -556,6 +470,7 @@ mod tests { #[cfg(windows)] #[test] fn failed_reorder_replacement_keeps_the_complete_old_order() { + use std::fs::OpenOptions; use std::os::windows::fs::OpenOptionsExt; with_temp_home(|| { save_binding(binding("a", "A", true, 3)).unwrap(); diff --git a/crates/core/src/config/mod.rs b/crates/core/src/config/mod.rs index bb63bac..8b12361 100644 --- a/crates/core/src/config/mod.rs +++ b/crates/core/src/config/mod.rs @@ -36,13 +36,12 @@ impl AppConfig { pub fn save(&self) -> Result<()> { let path = config_file_path()?; - ensure_parent_dir(&path)?; + self.save_to_path(&path) + } + fn save_to_path(&self, path: &Path) -> Result<()> { let raw = serde_json::to_string_pretty(self).context("failed to serialize config")?; - fs::write(&path, raw) - .with_context(|| format!("failed to write config file at {}", path.display()))?; - - Ok(()) + crate::persistence::atomic_write(path, raw.as_bytes()) } pub fn configured_services(&self) -> ConfiguredServices { @@ -122,16 +121,6 @@ fn migrate_app_data_dir(current_path: PathBuf, legacy_path: PathBuf) -> Result

Result<()> { - if let Some(parent) = path.parent() { - fs::create_dir_all(parent).with_context(|| { - format!("failed to create config directory at {}", parent.display()) - })?; - } - - Ok(()) -} - fn default_spotify_target_hints() -> Vec { [ "fire", "tv", "amazon", "spotify", "insignia", "toshiba", "osint", @@ -140,3 +129,103 @@ fn default_spotify_target_hints() -> Vec { .map(|value| value.to_string()) .collect() } + +#[cfg(test)] +mod tests { + use super::*; + + struct TempDir(PathBuf); + impl Drop for TempDir { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } + } + fn temp_dir() -> TempDir { + let path = + env::temp_dir().join(format!("sendo-config-test-{:016x}", rand::random::())); + fs::create_dir(&path).unwrap(); + TempDir(path) + } + + #[test] + fn config_replacement_preserves_previous_file_contents_and_roundtrips() { + let directory = temp_dir(); + let path = directory.0.join("config.json"); + let mut config = AppConfig { + firetv_ip: "192.0.2.1".into(), + ..Default::default() + }; + config.save_to_path(&path).unwrap(); + let original = fs::read_to_string(&path).unwrap(); + let previous_path = directory.0.join("previous.json"); + fs::hard_link(&path, &previous_path).unwrap(); + config.firetv_ip = "192.0.2.2".into(); + config.save_to_path(&path).unwrap(); + assert_eq!(fs::read_to_string(&previous_path).unwrap(), original); + fs::remove_file(previous_path).unwrap(); + let saved: AppConfig = serde_json::from_str(&fs::read_to_string(&path).unwrap()).unwrap(); + assert_eq!(saved.firetv_ip, "192.0.2.2"); + assert_eq!(fs::read_dir(&directory.0).unwrap().count(), 1); + } + + #[test] + fn config_replacement_failure_cleans_up_its_temporary_file() { + let directory = temp_dir(); + let path = directory.0.join("config.json"); + fs::create_dir(&path).unwrap(); + fs::write(path.join("keep"), "original").unwrap(); + let error = AppConfig::default().save_to_path(&path).unwrap_err(); + assert!(error.to_string().contains("config.json")); + assert_eq!(fs::read_to_string(path.join("keep")).unwrap(), "original"); + assert_eq!(fs::read_dir(&directory.0).unwrap().count(), 1); + } + + #[cfg(windows)] + #[test] + fn failed_config_replacement_preserves_the_previous_config() { + use std::os::windows::fs::OpenOptionsExt; + let directory = temp_dir(); + let path = directory.0.join("config.json"); + AppConfig::default().save_to_path(&path).unwrap(); + let original = fs::read(&path).unwrap(); + let held_file = fs::OpenOptions::new() + .read(true) + .share_mode(1) + .open(&path) + .unwrap(); + let updated = AppConfig { + launch_on_startup: true, + ..Default::default() + }; + assert!(updated.save_to_path(&path).is_err()); + assert_eq!(fs::read(&path).unwrap(), original); + assert_eq!(fs::read_dir(&directory.0).unwrap().count(), 1); + drop(held_file); + } + + #[test] + fn app_data_migration_preserves_config_and_token_cache() { + let directory = temp_dir(); + let legacy = directory.0.join("Desk Remote"); + let current = directory.0.join("Sendo"); + fs::create_dir(&legacy).unwrap(); + fs::write(legacy.join("config.json"), "{}").unwrap(); + fs::write(legacy.join("spotify-token.json"), "cached-token").unwrap(); + assert_eq!( + migrate_app_data_dir(current.clone(), legacy.clone()).unwrap(), + current + ); + assert!(!legacy.exists()); + assert_eq!( + fs::read_to_string(current.join("spotify-token.json")).unwrap(), + "cached-token" + ); + AppConfig::default() + .save_to_path(¤t.join("config.json")) + .unwrap(); + assert_eq!( + fs::read_to_string(current.join("spotify-token.json")).unwrap(), + "cached-token" + ); + } +} diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 667a919..e444066 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -1,6 +1,7 @@ pub mod bindings; pub mod config; pub mod firetv; +mod persistence; pub mod spotify; use serde::Serialize; diff --git a/crates/core/src/persistence.rs b/crates/core/src/persistence.rs new file mode 100644 index 0000000..e142991 --- /dev/null +++ b/crates/core/src/persistence.rs @@ -0,0 +1,69 @@ +use anyhow::{Context, Result}; +use std::{ + fs::{self, OpenOptions}, + io::Write, + path::Path, +}; + +// Keep the temporary file beside its destination so replacement stays on one filesystem. +pub(crate) fn atomic_write(path: &Path, contents: &[u8]) -> Result<()> { + if let Some(parent) = path.parent() { + fs::create_dir_all(parent) + .with_context(|| format!("failed to create directory at {}", parent.display()))?; + } + let temp_path = path.with_extension(format!("json.tmp-{:016x}", rand::random::())); + let mut file = OpenOptions::new() + .write(true) + .create_new(true) + .open(&temp_path) + .with_context(|| format!("failed to create temporary file at {}", temp_path.display()))?; + let result = (|| -> Result<()> { + file.write_all(contents).with_context(|| { + format!("failed to write temporary file at {}", temp_path.display()) + })?; + file.sync_all() + .with_context(|| format!("failed to sync temporary file at {}", temp_path.display()))?; + drop(file); + replace_file(&temp_path, path) + .with_context(|| format!("failed to replace file at {}", path.display())) + })(); + if result.is_err() { + let _ = fs::remove_file(&temp_path); + } + result +} + +#[cfg(not(windows))] +fn replace_file(temp_path: &Path, path: &Path) -> std::io::Result<()> { + fs::rename(temp_path, path) +} + +#[cfg(windows)] +fn replace_file(temp_path: &Path, path: &Path) -> std::io::Result<()> { + use std::os::windows::ffi::OsStrExt; + use windows_sys::Win32::Storage::FileSystem::{ + MoveFileExW, MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, + }; + let source: Vec = temp_path + .as_os_str() + .encode_wide() + .chain(std::iter::once(0)) + .collect(); + let destination: Vec = path + .as_os_str() + .encode_wide() + .chain(std::iter::once(0)) + .collect(); + if unsafe { + MoveFileExW( + source.as_ptr(), + destination.as_ptr(), + MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH, + ) + } == 0 + { + Err(std::io::Error::last_os_error()) + } else { + Ok(()) + } +} From 76aa5d185162e4ebfb647c472e08dd7e185de6ae Mon Sep 17 00:00:00 2001 From: postigodev Date: Tue, 6 Oct 2026 12:40:07 -0500 Subject: [PATCH 5/7] fix(spotify): authenticate desktop clients with PKCE (#33) --- CONTRIBUTING.md | 7 +- apps/site/app/install/page.tsx | 12 +- apps/site/content/site.ts | 4 +- apps/tauri/src-tauri/src/commands.rs | 1 - apps/tauri/src/main.ts | 4 +- apps/tauri/src/pages/spotify.ts | 3 +- apps/tauri/src/state.ts | 3 +- apps/tauri/src/types.ts | 3 +- crates/core/src/config/mod.rs | 45 +++- crates/core/src/spotify/mod.rs | 375 ++++++++++++++++++++++----- readme.md | 15 +- 11 files changed, 385 insertions(+), 87 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4d3cdc7..e0f12a4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -34,7 +34,7 @@ If you want to contribute code, start by opening an issue or picking an existing - Rust toolchain - Android Platform Tools with `adb` in `PATH` - Fire TV with ADB debugging enabled for manual device testing -- Spotify Developer app credentials for Spotify auth flows +- Spotify Developer app client ID for Spotify PKCE auth flows After installing Android Platform Tools, open a new PowerShell window and verify: @@ -48,8 +48,9 @@ If `adb` is not found, add the Platform Tools directory to the Windows user machine must be on the same network, ADB debugging must be enabled on the TV, and the TV may show an authorization prompt on the first connection. -Spotify flows require a Spotify Developer app. Use the app's client ID, client -secret, and the redirect URL expected by Sendo. When testing playback routing, +Spotify flows require a Spotify Developer app. Use its public client ID and +register the HTTP loopback redirect URL expected by Sendo. Authorization Code +with PKCE does not require a client secret. When testing playback routing, select the explicit Spotify Connect target in the app instead of relying on the currently active Spotify device. diff --git a/apps/site/app/install/page.tsx b/apps/site/app/install/page.tsx index 3050cdb..d920a9a 100644 --- a/apps/site/app/install/page.tsx +++ b/apps/site/app/install/page.tsx @@ -19,7 +19,7 @@ const requirements = [ "Windows", "adb available from a new PowerShell window", "Fire TV with ADB debugging enabled and authorized", - "Spotify Premium plus developer app credentials", + "Spotify Premium plus a developer app client ID", "Explicit Spotify Connect target selected in Sendo", ]; @@ -40,7 +40,7 @@ export default async function InstallPage() {

Download the Windows build, enable Fire TV control over ADB, - connect Spotify with the correct OAuth credentials, select + connect Spotify with the public client ID and redirect URL, select the intended target device, and keep Sendo available from the tray for repeatable media actions.

@@ -151,7 +151,7 @@ export default async function InstallPage() {

The install itself is straightforward. Most first-run failures come from missing ADB, Fire TV debugging not enabled or not - authorized, Spotify redirect credentials not matching, or + authorized, the Spotify client ID or redirect URL not matching, or selecting the wrong Spotify device.

@@ -196,7 +196,7 @@ export default async function InstallPage() { 03 - Authenticate Spotify with the developer app credentials and + Authenticate Spotify using PKCE with the developer app client ID and explicitly select a target. @@ -341,8 +341,8 @@ function TroubleshootingBody({ title }: { title: string }) {

  • - - Confirm the client ID, client secret, and redirect URL match the - values entered in Sendo. + Confirm the client ID and registered loopback redirect URL match the + values entered in Sendo. PKCE does not require a client secret.
  • diff --git a/apps/site/content/site.ts b/apps/site/content/site.ts index 7666bcc..e6ca89a 100644 --- a/apps/site/content/site.ts +++ b/apps/site/content/site.ts @@ -37,7 +37,7 @@ export const site = { "Install Sendo on Windows and confirm adb works from a new PowerShell window.", "Enable ADB debugging on your Fire TV and approve the first connection prompt.", "Add the Fire TV IP address in Sendo, then test the ADB connection.", - "Connect Spotify with developer app credentials and the expected redirect URL.", + "Connect Spotify with the developer app client ID and registered loopback redirect URL using PKCE.", "Select the exact Spotify Connect target device before running an action.", "Run Start Spotify on TV and save it as a shortcut if you want.", ], @@ -52,7 +52,7 @@ export const site = { }, { title: "Spotify auth fails", - body: "Confirm the client ID, client secret, and redirect URL match the Spotify Developer app exactly.", + body: "Confirm the client ID and registered loopback redirect URL match the Spotify Developer app exactly. Sendo uses PKCE without a client secret.", }, { title: "Auth expired", diff --git a/apps/tauri/src-tauri/src/commands.rs b/apps/tauri/src-tauri/src/commands.rs index 6c7b88f..4ba3b87 100644 --- a/apps/tauri/src-tauri/src/commands.rs +++ b/apps/tauri/src-tauri/src/commands.rs @@ -109,7 +109,6 @@ pub async fn health_check() -> Result { firetv_summary: firetv::status_summary(&config.firetv_ip), spotify_summary: spotify::status_summary( &config.spotify_client_id, - &config.spotify_client_secret, &config.spotify_redirect_url, ), }) diff --git a/apps/tauri/src/main.ts b/apps/tauri/src/main.ts index f4b493f..152ef3d 100644 --- a/apps/tauri/src/main.ts +++ b/apps/tauri/src/main.ts @@ -655,6 +655,7 @@ async function startSpotifyAuth() { try { await persistCurrentConfig(); const result = await api.spotifyStartAuth(); + currentConfig = await api.getSettings(); currentSpotifyDebug = await api.spotifyDebugAuthFlow(); spotifyAuthUrl = result.url; render(); @@ -1054,13 +1055,14 @@ function syncConfigFromInputs() { currentConfig = { firetv_ip: document.querySelector("#firetv-ip")?.value.trim() ?? currentConfig.firetv_ip, spotify_client_id: document.querySelector("#spotify-client-id")?.value.trim() ?? currentConfig.spotify_client_id, - spotify_client_secret: document.querySelector("#spotify-client-secret")?.value.trim() ?? currentConfig.spotify_client_secret, spotify_redirect_url: document.querySelector("#spotify-redirect-url")?.value.trim() ?? currentConfig.spotify_redirect_url, spotify_selected_device_id: document.querySelector("#spotify-selected-device-id")?.value.trim() ?? currentConfig.spotify_selected_device_id, spotify_target_hints: document.querySelector("#spotify-target-hints")?.value.trim() ?? currentConfig.spotify_target_hints, spotify_auth_state: currentConfig.spotify_auth_state, + spotify_auth_verifier: currentConfig.spotify_auth_verifier, + spotify_auth_url: currentConfig.spotify_auth_url, launch_on_startup: document.querySelector("#launch-on-startup")?.checked ?? currentConfig.launch_on_startup, diff --git a/apps/tauri/src/pages/spotify.ts b/apps/tauri/src/pages/spotify.ts index 7bb53f6..b57868d 100644 --- a/apps/tauri/src/pages/spotify.ts +++ b/apps/tauri/src/pages/spotify.ts @@ -66,7 +66,6 @@ export function renderSpotify({
    ${settingsField("spotify-client-id", "Client ID", currentConfig.spotify_client_id, "your-client-id")} - ${settingsField("spotify-client-secret", "Client secret", currentConfig.spotify_client_secret, "your-client-secret")} ${settingsField("spotify-redirect-url", "Redirect URL", currentConfig.spotify_redirect_url, "http://127.0.0.1:8888/callback")} ${ currentConfig.spotify_selected_device_id.trim() @@ -75,7 +74,7 @@ export function renderSpotify({ } ${settingsField("spotify-target-hints", "Target hints", currentConfig.spotify_target_hints, "fire, tv, amazon")}
    -

    Stored locally for Spotify Connect authentication and target matching.

    +

    Uses PKCE with your public client ID. No client secret is required.

    diff --git a/apps/tauri/src/state.ts b/apps/tauri/src/state.ts index 3297675..f680369 100644 --- a/apps/tauri/src/state.ts +++ b/apps/tauri/src/state.ts @@ -13,11 +13,12 @@ import type { export const defaultConfig: AppConfig = { firetv_ip: "", spotify_client_id: "", - spotify_client_secret: "", spotify_redirect_url: "", spotify_selected_device_id: "", spotify_target_hints: "fire, tv, amazon, spotify, insignia, toshiba, osint", spotify_auth_state: "", + spotify_auth_verifier: "", + spotify_auth_url: "", launch_on_startup: false, start_minimized_to_tray: true, }; diff --git a/apps/tauri/src/types.ts b/apps/tauri/src/types.ts index 1eb3a5e..4e3a545 100644 --- a/apps/tauri/src/types.ts +++ b/apps/tauri/src/types.ts @@ -1,11 +1,12 @@ export type AppConfig = { firetv_ip: string; spotify_client_id: string; - spotify_client_secret: string; spotify_redirect_url: string; spotify_selected_device_id: string; spotify_target_hints: string; spotify_auth_state: string; + spotify_auth_verifier: string; + spotify_auth_url: string; launch_on_startup: boolean; start_minimized_to_tray: boolean; }; diff --git a/crates/core/src/config/mod.rs b/crates/core/src/config/mod.rs index 8b12361..dcac4d7 100644 --- a/crates/core/src/config/mod.rs +++ b/crates/core/src/config/mod.rs @@ -10,11 +10,12 @@ use std::{ pub struct AppConfig { pub firetv_ip: String, pub spotify_client_id: String, - pub spotify_client_secret: String, pub spotify_redirect_url: String, pub spotify_selected_device_id: String, pub spotify_target_hints: String, pub spotify_auth_state: String, + pub spotify_auth_verifier: String, + pub spotify_auth_url: String, pub launch_on_startup: bool, pub start_minimized_to_tray: bool, } @@ -47,9 +48,11 @@ impl AppConfig { pub fn configured_services(&self) -> ConfiguredServices { ConfiguredServices { firetv_ready: !self.firetv_ip.trim().is_empty(), - spotify_ready: !self.spotify_client_id.trim().is_empty() - && !self.spotify_client_secret.trim().is_empty() - && !self.spotify_redirect_url.trim().is_empty(), + spotify_ready: crate::spotify::validate_spotify_config( + &self.spotify_client_id, + &self.spotify_redirect_url, + ) + .is_ok(), } } @@ -228,4 +231,38 @@ mod tests { "cached-token" ); } + + #[test] + fn spotify_public_client_config_is_ready_without_a_secret() { + let mut config = AppConfig { + spotify_client_id: "public-client".into(), + spotify_redirect_url: "http://127.0.0.1:8888/callback".into(), + ..Default::default() + }; + assert!(config.configured_services().spotify_ready); + config.spotify_client_id = " ".into(); + assert!(!config.configured_services().spotify_ready); + config.spotify_client_id = "public-client".into(); + config.spotify_redirect_url = "http://192.0.2.1:8888/callback".into(); + assert!(!config.configured_services().spotify_ready); + } + + #[test] + fn legacy_spotify_secret_is_ignored_and_removed_on_next_save() { + let directory = temp_dir(); + let path = directory.0.join("config.json"); + let config: AppConfig = serde_json::from_value(serde_json::json!({ + "spotify_client_id": "public-client", "spotify_client_secret": "legacy-secret", + "spotify_redirect_url": "http://127.0.0.1:8888/callback", + "spotify_selected_device_id": "tv", "launch_on_startup": true + })) + .unwrap(); + assert!(config.configured_services().spotify_ready); + config.save_to_path(&path).unwrap(); + let saved: serde_json::Value = + serde_json::from_str(&fs::read_to_string(&path).unwrap()).unwrap(); + assert!(saved.get("spotify_client_secret").is_none()); + assert_eq!(saved["spotify_selected_device_id"], "tv"); + assert_eq!(saved["launch_on_startup"], true); + } } diff --git a/crates/core/src/spotify/mod.rs b/crates/core/src/spotify/mod.rs index 544a4a4..ebd7064 100644 --- a/crates/core/src/spotify/mod.rs +++ b/crates/core/src/spotify/mod.rs @@ -5,7 +5,7 @@ use crate::{ use anyhow::{anyhow, bail, Context, Result}; use rand::{distr::Alphanumeric, Rng}; use rspotify::{ - clients::OAuthClient, model::AdditionalType, prelude::BaseClient, scopes, AuthCodeSpotify, + clients::OAuthClient, model::AdditionalType, prelude::BaseClient, scopes, AuthCodePkceSpotify, Config, Credentials, OAuth, }; use serde::Serialize; @@ -83,8 +83,8 @@ pub struct SpotifyAuthStart { pub token_cache_path: String, } -pub fn status_summary(client_id: &str, client_secret: &str, redirect_url: &str) -> String { - match validate_spotify_config(client_id, client_secret, redirect_url) { +pub fn status_summary(client_id: &str, redirect_url: &str) -> String { + match validate_spotify_config(client_id, redirect_url) { Ok(()) => "Spotify OAuth settings are present".into(), Err(reason) => reason.into(), } @@ -113,9 +113,7 @@ pub async fn get_status(config: &AppConfig) -> Result { } let spotify = build_spotify(config)?; - let auth_url = spotify - .get_authorize_url(false) - .context("failed to generate Spotify authorize URL")?; + let auth_url = (!config.spotify_auth_url.is_empty()).then(|| config.spotify_auth_url.clone()); let auth_result = ensure_token(&spotify).await; let authenticated = auth_result.is_ok(); @@ -146,7 +144,7 @@ pub async fn get_status(config: &AppConfig) -> Result { playback_device_name: None, now_playing: None, summary: auth_summary, - auth_url: Some(auth_url), + auth_url, token_cache_path, }); } @@ -207,7 +205,7 @@ pub async fn get_status(config: &AppConfig) -> Result { playback_device_name: playback.device_name, now_playing, summary, - auth_url: Some(auth_url), + auth_url, token_cache_path, }) } @@ -217,18 +215,19 @@ pub async fn start_auth(config: &AppConfig) -> Result { bail!("{}", spotify_config_error(config)); } - let spotify = build_spotify(config)?; - let authorize_url = spotify - .get_authorize_url(false) - .context("failed to generate Spotify authorize URL")?; + validate_pending_auth(config)?; Ok(SpotifyAuthStart { - authorize_url, + authorize_url: config.spotify_auth_url.clone(), token_cache_path: token_cache_path()?.display().to_string(), }) } pub fn prepare_auth(config: &mut AppConfig) -> Result<()> { + prepare_auth_at(config, token_cache_path()?) +} + +fn prepare_auth_at(config: &mut AppConfig, cache_path: PathBuf) -> Result<()> { if !spotify_configured(config) { bail!("{}", spotify_config_error(config)); } @@ -239,6 +238,16 @@ pub fn prepare_auth(config: &mut AppConfig) -> Result<()> { .map(char::from) .collect(); + let mut spotify = spotify_client(config, cache_path); + // Each authorize-URL call generates a new verifier. Persist the pair once + // so independently rebuilt callback clients exchange the matching verifier. + config.spotify_auth_url = spotify + .get_authorize_url(None) + .context("failed to generate Spotify PKCE authorize URL")?; + config.spotify_auth_verifier = spotify + .verifier + .context("Spotify did not generate a PKCE verifier")?; + Ok(()) } @@ -247,6 +256,7 @@ pub async fn finish_auth(config: &AppConfig, code_or_callback: &str) -> Result Result Result { Ok(format!("{}. {}", firetv_result.summary, spotify_result)) } -async fn ensure_target_playing(spotify: &AuthCodeSpotify, target: &TargetDevice) -> Result { +async fn ensure_target_playing( + spotify: &AuthCodePkceSpotify, + target: &TargetDevice, +) -> Result { let playback = spotify .current_playback(None, Some(&[AdditionalType::Episode])) .await @@ -483,30 +497,20 @@ async fn ensure_target_playing(spotify: &AuthCodeSpotify, target: &TargetDevice) } fn spotify_configured(config: &AppConfig) -> bool { - validate_spotify_config( - &config.spotify_client_id, - &config.spotify_client_secret, - &config.spotify_redirect_url, - ) - .is_ok() + validate_spotify_config(&config.spotify_client_id, &config.spotify_redirect_url).is_ok() } fn spotify_config_error(config: &AppConfig) -> &'static str { - validate_spotify_config( - &config.spotify_client_id, - &config.spotify_client_secret, - &config.spotify_redirect_url, - ) - .expect_err("spotify_config_error called for an invalid config") + validate_spotify_config(&config.spotify_client_id, &config.spotify_redirect_url) + .expect_err("spotify_config_error called for an invalid config") } -fn validate_spotify_config( +pub(crate) fn validate_spotify_config( client_id: &str, - client_secret: &str, redirect_url: &str, ) -> Result<(), &'static str> { - if client_id.trim().is_empty() || client_secret.trim().is_empty() { - return Err("Spotify client ID and client secret are required"); + if client_id.trim().is_empty() { + return Err("Spotify client ID is required"); } validate_redirect_url(redirect_url) @@ -537,10 +541,13 @@ fn validate_redirect_url(redirect_url: &str) -> Result<(), &'static str> { Ok(()) } -fn build_spotify(config: &AppConfig) -> Result { +fn build_spotify(config: &AppConfig) -> Result { ensure_token_cache_dir()?; + Ok(spotify_client(config, token_cache_path()?)) +} - let creds = Credentials::new(&config.spotify_client_id, &config.spotify_client_secret); +fn spotify_client(config: &AppConfig, cache_path: PathBuf) -> AuthCodePkceSpotify { + let creds = Credentials::new_pkce(&config.spotify_client_id); let oauth = OAuth { redirect_uri: config.spotify_redirect_url.clone(), state: config.spotify_auth_state.clone(), @@ -555,14 +562,27 @@ fn build_spotify(config: &AppConfig) -> Result { let client_config = Config { token_cached: true, token_refreshing: true, - cache_path: token_cache_path()?, + cache_path, ..Default::default() }; - Ok(AuthCodeSpotify::with_config(creds, oauth, client_config)) + let mut spotify = AuthCodePkceSpotify::with_config(creds, oauth, client_config); + spotify.verifier = + (!config.spotify_auth_verifier.is_empty()).then(|| config.spotify_auth_verifier.clone()); + spotify } -async fn ensure_token(spotify: &AuthCodeSpotify) -> Result<()> { +fn validate_pending_auth(config: &AppConfig) -> Result<()> { + if config.spotify_auth_state.is_empty() + || config.spotify_auth_url.is_empty() + || !(43..=128).contains(&config.spotify_auth_verifier.len()) + { + bail!("No pending Spotify PKCE login. Start Spotify auth again before completing the callback."); + } + Ok(()) +} + +async fn ensure_token(spotify: &AuthCodePkceSpotify) -> Result<()> { if let Ok(Some(cached_token)) = spotify.read_token_cache(true).await { let is_expired = cached_token.is_expired(); @@ -612,7 +632,7 @@ async fn ensure_token(spotify: &AuthCodeSpotify) -> Result<()> { async fn resolve_control_target( config: &AppConfig, - spotify: &AuthCodeSpotify, + spotify: &AuthCodePkceSpotify, ) -> Result { for attempt in 0..5 { let available_devices = @@ -639,7 +659,7 @@ async fn resolve_control_target( } async fn fetch_available_devices( - spotify: &AuthCodeSpotify, + spotify: &AuthCodePkceSpotify, hints: &[String], ) -> Result> { let devices = spotify @@ -732,7 +752,7 @@ fn is_playback_on_target(target: Option<&TargetDevice>, playback: &PlaybackSnaps .is_some_and(|(target_id, playback_id)| target_id == playback_id) } -async fn fetch_playback_snapshot(spotify: &AuthCodeSpotify) -> Result { +async fn fetch_playback_snapshot(spotify: &AuthCodePkceSpotify) -> Result { let playback = spotify .current_playback(None, Some(&[AdditionalType::Episode])) .await?; @@ -847,7 +867,7 @@ fn extract_auth_code(code_or_callback: &str) -> Result { } async fn exchange_callback_or_code( - spotify: &AuthCodeSpotify, + spotify: &AuthCodePkceSpotify, code_or_callback: &str, ) -> Result<()> { let code = if code_or_callback.contains("://") { @@ -871,7 +891,7 @@ async fn exchange_callback_or_code( } async fn receive_auth_code_from_local_callback( - spotify: &AuthCodeSpotify, + spotify: &AuthCodePkceSpotify, socket_addr: std::net::SocketAddr, wait_timeout: Duration, ) -> Result { @@ -942,26 +962,239 @@ fn ensure_token_cache_dir() -> Result<()> { mod tests { use super::*; - async fn playback_client( - device_id: &str, - is_playing: bool, - write_statuses: &[u16], - ) -> (AuthCodeSpotify, tokio::task::JoinHandle>) { + fn public_config() -> AppConfig { + AppConfig { + spotify_client_id: "public-client".into(), + spotify_redirect_url: "http://127.0.0.1:8888/callback".into(), + ..Default::default() + } + } + + #[test] + fn pkce_session_survives_config_roundtrip_and_validates_callback_state() { + let mut config = public_config(); + prepare_auth_at(&mut config, PathBuf::from("unused-cache.json")).unwrap(); + let original = config.clone(); + let config: AppConfig = + serde_json::from_str(&serde_json::to_string(&config).unwrap()).unwrap(); + let spotify = spotify_client(&config, PathBuf::from("unused-cache.json")); + let url = Url::parse(&config.spotify_auth_url).unwrap(); + let params = url + .query_pairs() + .collect::>(); + assert_eq!(params["client_id"], "public-client"); + assert_eq!(params["response_type"], "code"); + assert_eq!(params["code_challenge_method"], "S256"); + assert_eq!(params["code_challenge"].len(), 43); + assert_eq!(params["state"], config.spotify_auth_state); + assert_eq!( + spotify.verifier.as_deref(), + Some(config.spotify_auth_verifier.as_str()) + ); + assert!(spotify.creds.secret.is_none()); + assert!(spotify.config.token_cached && spotify.config.token_refreshing); + assert_eq!(config.spotify_auth_url, original.spotify_auth_url); + assert_eq!( + spotify.parse_response_code(&format!( + "{}?code=valid&state={}", + config.spotify_redirect_url, config.spotify_auth_state + )), + Some("valid".into()) + ); + for query in ["code=wrong&state=older", "code=wrong"] { + assert!(spotify + .parse_response_code(&format!("{}?{query}", config.spotify_redirect_url)) + .is_none()); + } + let mut new_session = config.clone(); + prepare_auth_at(&mut new_session, PathBuf::from("unused-cache.json")).unwrap(); + assert_ne!(new_session.spotify_auth_state, config.spotify_auth_state); + assert_ne!( + new_session.spotify_auth_verifier, + config.spotify_auth_verifier + ); + assert_ne!(new_session.spotify_auth_url, config.spotify_auth_url); + } + + #[tokio::test] + async fn legacy_pending_login_requests_reauthentication_instead_of_panicking() { + let config = public_config(); + let error = finish_auth(&config, "code").await.unwrap_err(); + assert!(error.to_string().contains("Start Spotify auth again")); + } + + struct TokenCacheDir(PathBuf); + impl Drop for TokenCacheDir { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } + } + fn token_cache_dir() -> TokenCacheDir { + let directory = + std::env::temp_dir().join(format!("sendo-pkce-test-{:016x}", rand::random::())); + fs::create_dir(&directory).unwrap(); + TokenCacheDir(directory) + } + fn token_response() -> String { + serde_json::json!({"access_token": "fresh-token", "token_type": "Bearer", + "expires_in": 3600, "refresh_token": "rotated-refresh", + "scope": "user-read-playback-state user-modify-playback-state user-read-currently-playing"}).to_string() + } + + #[tokio::test] + async fn pkce_exchange_uses_persisted_verifier_without_a_client_secret_and_caches_token() { + let directory = token_cache_dir(); + let path = directory.0.join("spotify-token.json"); + let mut config = public_config(); + prepare_auth_at(&mut config, path.clone()).unwrap(); + let persisted: AppConfig = + serde_json::from_str(&serde_json::to_string(&config).unwrap()).unwrap(); + let (base_url, server) = + spotify_server(vec![(200, token_response()), (200, token_response())]).await; + let mut spotify = spotify_client(&persisted, path.clone()); + spotify.config.auth_base_url = base_url; + assert!(exchange_callback_or_code( + &spotify, + &format!("{}?code=wrong&state=older", persisted.spotify_redirect_url,) + ) + .await + .is_err()); + exchange_callback_or_code( + &spotify, + &format!( + "{}?code=auth-code&state={}", + persisted.spotify_redirect_url, persisted.spotify_auth_state + ), + ) + .await + .unwrap(); + exchange_callback_or_code(&spotify, "manual-code") + .await + .unwrap(); + let requests = server.await.unwrap(); + assert_eq!(requests.len(), 2); + for (request, code) in requests.iter().zip(["auth-code", "manual-code"]) { + assert!(request.starts_with("POST /api/token ")); + assert!(!request + .to_ascii_lowercase() + .contains("authorization: basic")); + let form = + url::form_urlencoded::parse(request.rsplit("\r\n").next().unwrap().as_bytes()) + .collect::>(); + assert_eq!(form["grant_type"], "authorization_code"); + assert_eq!(form["client_id"], "public-client"); + assert_eq!(form["code"], code); + assert_eq!(form["code_verifier"], persisted.spotify_auth_verifier); + assert!(!form.contains_key("client_secret")); + } + let cached: rspotify::Token = + serde_json::from_str(&fs::read_to_string(&path).unwrap()).unwrap(); + assert_eq!(cached.access_token, "fresh-token"); + let restarted = spotify_client(&persisted, path); + ensure_token(&restarted).await.unwrap(); + assert_eq!( + restarted + .get_token() + .lock() + .await + .unwrap() + .as_ref() + .unwrap() + .access_token, + "fresh-token" + ); + } + + #[tokio::test] + async fn pkce_refresh_retains_cache_and_persists_rotated_refresh_token() { + let directory = token_cache_dir(); + let path = directory.0.join("spotify-token.json"); + let mut expired: serde_json::Value = serde_json::from_str(&token_response()).unwrap(); + expired["expires_at"] = "2000-01-01T00:00:00Z".into(); + expired["refresh_token"] = "old-refresh".into(); + fs::write(&path, expired.to_string()).unwrap(); + let (base_url, server) = spotify_server(vec![(200, token_response())]).await; + let mut spotify = spotify_client(&public_config(), path.clone()); + spotify.config.auth_base_url = base_url; + ensure_token(&spotify).await.unwrap(); + let requests = server.await.unwrap(); + assert_eq!(requests.len(), 1); + assert!(requests[0].contains("grant_type=refresh_token")); + assert!(requests[0].contains("refresh_token=old-refresh")); + assert!(requests[0].contains("client_id=public-client")); + assert!(!requests[0].contains("client_secret")); + let cached: rspotify::Token = + serde_json::from_str(&fs::read_to_string(&path).unwrap()).unwrap(); + assert_eq!(cached.refresh_token.as_deref(), Some("rotated-refresh")); + let restarted = spotify_client(&public_config(), path); + ensure_token(&restarted).await.unwrap(); + assert_eq!( + restarted + .get_token() + .lock() + .await + .unwrap() + .as_ref() + .unwrap() + .refresh_token + .as_deref(), + Some("rotated-refresh") + ); + } + + #[tokio::test] + async fn loopback_callback_preserves_state_validation_for_pkce() { + use tokio::{io::AsyncReadExt, net::TcpStream}; + for (state, valid) in [("expected", true), ("older", false)] { + let reservation = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = reservation.local_addr().unwrap(); + drop(reservation); + let spotify = AuthCodePkceSpotify::new( + Credentials::new_pkce("public-client"), + OAuth { + state: "expected".into(), + redirect_uri: format!("http://{address}/callback"), + ..Default::default() + }, + ); + let receiver = tokio::spawn(async move { + receive_auth_code_from_local_callback(&spotify, address, Duration::from_secs(2)) + .await + }); + let mut stream = None; + for _ in 0..20 { + if let Ok(connected) = TcpStream::connect(address).await { + stream = Some(connected); + break; + } + sleep(Duration::from_millis(10)).await; + } + let mut stream = stream.expect("callback listener bound"); + stream + .write_all( + format!("GET /callback?code=auth-code&state={state} HTTP/1.1\r\n\r\n") + .as_bytes(), + ) + .await + .unwrap(); + let mut response = String::new(); + stream.read_to_string(&mut response).await.unwrap(); + let result = receiver.await.unwrap(); + assert_eq!(result.is_ok(), valid); + if valid { + assert_eq!(result.unwrap(), "auth-code"); + } + assert!(response.starts_with("HTTP/1.1 200 OK")); + } + } + + async fn spotify_server( + responses: Vec<(u16, String)>, + ) -> (String, tokio::task::JoinHandle>) { use tokio::io::AsyncReadExt; let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let address = listener.local_addr().unwrap(); - let body = serde_json::json!({ - "device": {"id": device_id, "name": "TV", "is_active": true, - "is_private_session": false, "is_restricted": false, - "type": "TV", "volume_percent": 50}, - "repeat_state": "off", "shuffle_state": false, - "timestamp": 0, "is_playing": is_playing, "item": null, - "currently_playing_type": "unknown", "actions": {"disallows": {}} - }) - .to_string(); - let mut responses = vec![(200, body)]; - responses.extend(write_statuses.iter().map(|status| (*status, String::new()))); let server = tokio::spawn(async move { let mut requests = Vec::new(); for (status, body) in responses { @@ -993,7 +1226,27 @@ mod tests { } requests }); - let spotify = AuthCodeSpotify::from_token_with_config( + (format!("http://{address}/"), server) + } + + async fn playback_client( + device_id: &str, + is_playing: bool, + write_statuses: &[u16], + ) -> (AuthCodePkceSpotify, tokio::task::JoinHandle>) { + let body = serde_json::json!({ + "device": {"id": device_id, "name": "TV", "is_active": true, + "is_private_session": false, "is_restricted": false, + "type": "TV", "volume_percent": 50}, + "repeat_state": "off", "shuffle_state": false, + "timestamp": 0, "is_playing": is_playing, "item": null, + "currently_playing_type": "unknown", "actions": {"disallows": {}} + }) + .to_string(); + let mut responses = vec![(200, body)]; + responses.extend(write_statuses.iter().map(|status| (*status, String::new()))); + let (base_url, server) = spotify_server(responses).await; + let spotify = AuthCodePkceSpotify::from_token_with_config( rspotify::Token { access_token: "test-token".into(), ..Default::default() @@ -1001,7 +1254,7 @@ mod tests { Credentials::default(), OAuth::default(), Config { - api_base_url: format!("http://{address}/"), + api_base_url: base_url, token_refreshing: false, ..Default::default() }, @@ -1107,12 +1360,12 @@ mod tests { #[test] fn status_summary_explains_invalid_redirect_configuration() { assert_eq!( - status_summary("client", "secret", "http://127.0.0.1/callback"), + status_summary("client", "http://127.0.0.1/callback"), "Spotify redirect URL must be an HTTP loopback URL with a port" ); assert_eq!( - status_summary("", "secret", "http://127.0.0.1:8888/callback"), - "Spotify client ID and client secret are required" + status_summary("", "http://127.0.0.1:8888/callback"), + "Spotify client ID is required" ); } diff --git a/readme.md b/readme.md index 77716d2..fbb392f 100644 --- a/readme.md +++ b/readme.md @@ -90,7 +90,7 @@ TypeScript UI - Android Platform Tools with `adb` in `PATH` - Fire TV with ADB debugging enabled - Spotify Premium -- Spotify Developer app credentials +- Spotify Developer app client ID On Windows, install Android Platform Tools and make sure the folder that contains `adb.exe` is on `PATH`. A quick check from a new PowerShell window is: @@ -107,12 +107,17 @@ Before first run, enable ADB debugging on the Fire TV, keep the TV and Windows machine on the same network, and accept the Fire TV debugging prompt the first time Sendo or `adb connect ` reaches the device. -For Spotify, create a Spotify Developer app, copy the client ID and client -secret into Sendo, and use the redirect URL shown by the app. After +For Spotify, create a Spotify Developer app, copy its public client ID into +Sendo, and register the HTTP loopback redirect URL shown by the app. Sendo uses +Authorization Code with PKCE; no client secret is required. After authentication, select the exact Spotify Connect device you want Sendo to control; do not rely on Spotify's current-device guess when more than one TV, speaker, or browser session is available. +Existing configs with a client secret still load; that obsolete field is ignored +and removed on the next settings save. The token cache is retained. If an older +session cannot refresh, authenticate once again to obtain a PKCE session. + ### Clone and install ```powershell @@ -161,7 +166,7 @@ cd sendo\apps\tauri 1. Configure the Fire TV IP address in `ADB & Device`. 2. Test the ADB connection and approve the debugging prompt on the TV if it appears. -3. Configure Spotify OAuth credentials and redirect URL in `Spotify`. +3. Configure the Spotify client ID and redirect URL in `Spotify`. 4. Authenticate Spotify and select the exact Spotify Connect target device. 5. Use `Start Spotify on TV` from Home, tray, or a binding. 6. Sendo connects to the TV over ADB, wakes it if needed, launches Spotify, and transfers playback to the selected Spotify device. @@ -173,7 +178,7 @@ cd sendo\apps\tauri platform-tools directory is on `PATH` in a new terminal. - Fire TV does not connect: confirm ADB debugging is enabled, both devices are on the same network, and the TV accepted the debugging authorization prompt. -- Spotify auth fails: confirm the client ID, client secret, and redirect URL in +- Spotify auth fails: confirm the client ID and redirect URL in Sendo match the Spotify Developer app exactly. - Playback transfers to the wrong device: open the Spotify page and select the intended Spotify Connect target explicitly before running the action. From 92a44c250b34ea9de69494d3ae17cf73153b13d5 Mon Sep 17 00:00:00 2001 From: postigodev Date: Tue, 6 Oct 2026 14:05:26 -0500 Subject: [PATCH 6/7] fix(spotify): preserve PKCE refresh tokens across concurrent actions (#33) --- Cargo.lock | 1 + crates/core/Cargo.toml | 3 +- crates/core/src/spotify/mod.rs | 246 ++++++++++++++++++++++++++++----- 3 files changed, 213 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d8ef9be..48f185e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -700,6 +700,7 @@ name = "desk_remote_core" version = "0.1.0" dependencies = [ "anyhow", + "async-trait", "rand 0.9.2", "rspotify", "serde", diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index c59b712..e4093f0 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -5,7 +5,8 @@ edition = "2021" [dependencies] anyhow = "1" -tokio = { version = "1", features = ["rt-multi-thread", "macros", "net", "time", "io-util"] } +async-trait = "0.1" +tokio = { version = "1", features = ["rt-multi-thread", "macros", "net", "time", "io-util", "sync"] } serde = { version = "1", features = ["derive"] } serde_json = "1" rspotify = { version = "0.15.3", default-features = false, features = ["client-reqwest", "reqwest-rustls-tls"] } diff --git a/crates/core/src/spotify/mod.rs b/crates/core/src/spotify/mod.rs index ebd7064..2e65418 100644 --- a/crates/core/src/spotify/mod.rs +++ b/crates/core/src/spotify/mod.rs @@ -18,6 +18,67 @@ use tokio::{ use url::Url; const AUTH_CALLBACK_TIMEOUT: Duration = Duration::from_secs(120); +static TOKEN_CACHE_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + +// rspotify 0.15.3's PKCE client drops the old refresh token when Spotify omits +// a replacement. Delegate OAuth to the SDK and preserve that optional field. +#[derive(Clone, Debug, Default)] +struct SpotifyClient(AuthCodePkceSpotify); + +impl std::ops::Deref for SpotifyClient { + type Target = AuthCodePkceSpotify; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl std::ops::DerefMut for SpotifyClient { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +#[async_trait::async_trait] +impl BaseClient for SpotifyClient { + fn get_http(&self) -> &rspotify::http::HttpClient { + self.0.get_http() + } + fn get_token(&self) -> std::sync::Arc>> { + self.0.get_token() + } + fn get_creds(&self) -> &Credentials { + self.0.get_creds() + } + fn get_config(&self) -> &Config { + self.0.get_config() + } + async fn refetch_token(&self) -> rspotify::ClientResult> { + let previous = self + .get_token() + .lock() + .await + .unwrap() + .as_ref() + .and_then(|token| token.refresh_token.clone()); + let mut refreshed = self.0.refetch_token().await?; + if let Some(token) = refreshed.as_mut() { + if token.refresh_token.is_none() { + token.refresh_token = previous; + } + } + Ok(refreshed) + } +} + +#[async_trait::async_trait] +impl OAuthClient for SpotifyClient { + fn get_oauth(&self) -> &OAuth { + self.0.get_oauth() + } + async fn request_token(&self, code: &str) -> rspotify::ClientResult<()> { + self.0.request_token(code).await + } +} #[derive(Debug, Clone, Serialize)] pub struct SpotifyNowPlaying { @@ -246,6 +307,7 @@ fn prepare_auth_at(config: &mut AppConfig, cache_path: PathBuf) -> Result<()> { .context("failed to generate Spotify PKCE authorize URL")?; config.spotify_auth_verifier = spotify .verifier + .take() .context("Spotify did not generate a PKCE verifier")?; Ok(()) @@ -279,10 +341,7 @@ pub async fn finish_auth_via_local_callback(config: &AppConfig) -> Result Result { } let spotify = build_spotify(config)?; - ensure_token(&spotify).await?; + let _token_guard = ensure_token(&spotify).await?; let target = resolve_control_target(config, &spotify).await?; let target_id = target.id.clone(); @@ -365,7 +424,7 @@ pub async fn transfer_to_tv(config: &AppConfig) -> Result { } let spotify = build_spotify(config)?; - ensure_token(&spotify).await?; + let _token_guard = ensure_token(&spotify).await?; let target = resolve_control_target(config, &spotify).await?; let target_id = target.id.clone(); @@ -389,7 +448,7 @@ pub async fn toggle_playback(config: &AppConfig) -> Result { } let spotify = build_spotify(config)?; - ensure_token(&spotify).await?; + let _token_guard = ensure_token(&spotify).await?; let target = resolve_control_target(config, &spotify).await?; let target_id = target.id.clone(); @@ -430,7 +489,7 @@ pub async fn skip_next(config: &AppConfig) -> Result { } let spotify = build_spotify(config)?; - ensure_token(&spotify).await?; + let _token_guard = ensure_token(&spotify).await?; let target = resolve_control_target(config, &spotify).await?; let target_id = target.id.clone(); spotify @@ -446,7 +505,7 @@ pub async fn skip_previous(config: &AppConfig) -> Result { } let spotify = build_spotify(config)?; - ensure_token(&spotify).await?; + let _token_guard = ensure_token(&spotify).await?; let target = resolve_control_target(config, &spotify).await?; let target_id = target.id.clone(); spotify @@ -462,17 +521,14 @@ pub async fn start_on_tv(config: &AppConfig) -> Result { bail!("{}", spotify_config_error(config)); } let spotify = build_spotify(config)?; - ensure_token(&spotify).await?; + let _token_guard = ensure_token(&spotify).await?; let target = resolve_control_target(config, &spotify).await?; let spotify_result = ensure_target_playing(&spotify, &target).await?; Ok(format!("{}. {}", firetv_result.summary, spotify_result)) } -async fn ensure_target_playing( - spotify: &AuthCodePkceSpotify, - target: &TargetDevice, -) -> Result { +async fn ensure_target_playing(spotify: &SpotifyClient, target: &TargetDevice) -> Result { let playback = spotify .current_playback(None, Some(&[AdditionalType::Episode])) .await @@ -541,12 +597,12 @@ fn validate_redirect_url(redirect_url: &str) -> Result<(), &'static str> { Ok(()) } -fn build_spotify(config: &AppConfig) -> Result { +fn build_spotify(config: &AppConfig) -> Result { ensure_token_cache_dir()?; Ok(spotify_client(config, token_cache_path()?)) } -fn spotify_client(config: &AppConfig, cache_path: PathBuf) -> AuthCodePkceSpotify { +fn spotify_client(config: &AppConfig, cache_path: PathBuf) -> SpotifyClient { let creds = Credentials::new_pkce(&config.spotify_client_id); let oauth = OAuth { redirect_uri: config.spotify_redirect_url.clone(), @@ -569,7 +625,7 @@ fn spotify_client(config: &AppConfig, cache_path: PathBuf) -> AuthCodePkceSpotif let mut spotify = AuthCodePkceSpotify::with_config(creds, oauth, client_config); spotify.verifier = (!config.spotify_auth_verifier.is_empty()).then(|| config.spotify_auth_verifier.clone()); - spotify + SpotifyClient(spotify) } fn validate_pending_auth(config: &AppConfig) -> Result<()> { @@ -582,7 +638,15 @@ fn validate_pending_auth(config: &AppConfig) -> Result<()> { Ok(()) } -async fn ensure_token(spotify: &AuthCodePkceSpotify) -> Result<()> { +// PKCE refresh tokens rotate. Keep this guard through the API operation too: +// rspotify can automatically refresh between requests in a multi-step action. +async fn ensure_token(spotify: &SpotifyClient) -> Result> { + let guard = TOKEN_CACHE_LOCK.lock().await; + load_or_refresh_token(spotify).await?; + Ok(guard) +} + +async fn load_or_refresh_token(spotify: &SpotifyClient) -> Result<()> { if let Ok(Some(cached_token)) = spotify.read_token_cache(true).await { let is_expired = cached_token.is_expired(); @@ -632,7 +696,7 @@ async fn ensure_token(spotify: &AuthCodePkceSpotify) -> Result<()> { async fn resolve_control_target( config: &AppConfig, - spotify: &AuthCodePkceSpotify, + spotify: &SpotifyClient, ) -> Result { for attempt in 0..5 { let available_devices = @@ -659,7 +723,7 @@ async fn resolve_control_target( } async fn fetch_available_devices( - spotify: &AuthCodePkceSpotify, + spotify: &SpotifyClient, hints: &[String], ) -> Result> { let devices = spotify @@ -752,7 +816,7 @@ fn is_playback_on_target(target: Option<&TargetDevice>, playback: &PlaybackSnaps .is_some_and(|(target_id, playback_id)| target_id == playback_id) } -async fn fetch_playback_snapshot(spotify: &AuthCodePkceSpotify) -> Result { +async fn fetch_playback_snapshot(spotify: &SpotifyClient) -> Result { let playback = spotify .current_playback(None, Some(&[AdditionalType::Episode])) .await?; @@ -866,10 +930,7 @@ fn extract_auth_code(code_or_callback: &str) -> Result { Ok(trimmed.to_string()) } -async fn exchange_callback_or_code( - spotify: &AuthCodePkceSpotify, - code_or_callback: &str, -) -> Result<()> { +async fn exchange_callback_or_code(spotify: &SpotifyClient, code_or_callback: &str) -> Result<()> { let code = if code_or_callback.contains("://") { spotify .parse_response_code(code_or_callback) @@ -882,8 +943,13 @@ async fn exchange_callback_or_code( extract_auth_code(code_or_callback)? }; + exchange_auth_code(spotify, &code).await +} + +async fn exchange_auth_code(spotify: &SpotifyClient, code: &str) -> Result<()> { + let _guard = TOKEN_CACHE_LOCK.lock().await; spotify - .request_token(&code) + .request_token(code) .await .context("failed to exchange Spotify authorization code for a token")?; @@ -891,7 +957,7 @@ async fn exchange_callback_or_code( } async fn receive_auth_code_from_local_callback( - spotify: &AuthCodePkceSpotify, + spotify: &SpotifyClient, socket_addr: std::net::SocketAddr, wait_timeout: Duration, ) -> Result { @@ -1091,7 +1157,7 @@ mod tests { serde_json::from_str(&fs::read_to_string(&path).unwrap()).unwrap(); assert_eq!(cached.access_token, "fresh-token"); let restarted = spotify_client(&persisted, path); - ensure_token(&restarted).await.unwrap(); + drop(ensure_token(&restarted).await.unwrap()); assert_eq!( restarted .get_token() @@ -1116,7 +1182,7 @@ mod tests { let (base_url, server) = spotify_server(vec![(200, token_response())]).await; let mut spotify = spotify_client(&public_config(), path.clone()); spotify.config.auth_base_url = base_url; - ensure_token(&spotify).await.unwrap(); + drop(ensure_token(&spotify).await.unwrap()); let requests = server.await.unwrap(); assert_eq!(requests.len(), 1); assert!(requests[0].contains("grant_type=refresh_token")); @@ -1127,7 +1193,7 @@ mod tests { serde_json::from_str(&fs::read_to_string(&path).unwrap()).unwrap(); assert_eq!(cached.refresh_token.as_deref(), Some("rotated-refresh")); let restarted = spotify_client(&public_config(), path); - ensure_token(&restarted).await.unwrap(); + drop(ensure_token(&restarted).await.unwrap()); assert_eq!( restarted .get_token() @@ -1142,6 +1208,72 @@ mod tests { ); } + #[tokio::test] + async fn pkce_refresh_without_a_replacement_retains_the_existing_refresh_token() { + let directory = token_cache_dir(); + let path = directory.0.join("spotify-token.json"); + let mut expired: serde_json::Value = serde_json::from_str(&token_response()).unwrap(); + expired["expires_at"] = "2000-01-01T00:00:00Z".into(); + expired["refresh_token"] = "old-refresh".into(); + fs::write(&path, expired.to_string()).unwrap(); + let mut response: serde_json::Value = serde_json::from_str(&token_response()).unwrap(); + response.as_object_mut().unwrap().remove("refresh_token"); + let (base_url, server) = + spotify_server(vec![(200, response.to_string()), (200, token_response())]).await; + let mut spotify = spotify_client(&public_config(), path.clone()); + spotify.config.auth_base_url = base_url; + drop(ensure_token(&spotify).await.unwrap()); + let cached: rspotify::Token = + serde_json::from_str(&fs::read_to_string(&path).unwrap()).unwrap(); + assert_eq!(cached.refresh_token.as_deref(), Some("old-refresh")); + // Exercise the same refresh method used by the SDK's automatic renewal. + spotify.refresh_token().await.unwrap(); + let requests = server.await.unwrap(); + assert_eq!(requests.len(), 2); + assert!(requests + .iter() + .all(|request| request.contains("refresh_token=old-refresh"))); + let cached: rspotify::Token = + serde_json::from_str(&fs::read_to_string(&path).unwrap()).unwrap(); + assert_eq!(cached.refresh_token.as_deref(), Some("rotated-refresh")); + } + + #[tokio::test] + async fn concurrent_clients_refresh_a_rotating_token_only_once() { + let directory = token_cache_dir(); + let path = directory.0.join("spotify-token.json"); + let mut expired: serde_json::Value = serde_json::from_str(&token_response()).unwrap(); + expired["expires_at"] = "2000-01-01T00:00:00Z".into(); + expired["refresh_token"] = "old-refresh".into(); + fs::write(&path, expired.to_string()).unwrap(); + // A rotating refresh token accepts one request; a second use must fail. + let (base_url, server) = spotify_server(vec![(200, token_response())]).await; + let mut first = spotify_client(&public_config(), path.clone()); + first.config.auth_base_url = base_url.clone(); + let mut second = spotify_client(&public_config(), path); + second.config.auth_base_url = base_url; + let (a, b) = tokio::join!(async { ensure_token(&first).await.map(|_| ()) }, async { + ensure_token(&second).await.map(|_| ()) + }); + a.unwrap(); + b.unwrap(); + assert_eq!(server.await.unwrap().len(), 1); + for client in [&first, &second] { + assert_eq!( + client + .get_token() + .lock() + .await + .unwrap() + .as_ref() + .unwrap() + .refresh_token + .as_deref(), + Some("rotated-refresh") + ); + } + } + #[tokio::test] async fn loopback_callback_preserves_state_validation_for_pkce() { use tokio::{io::AsyncReadExt, net::TcpStream}; @@ -1149,14 +1281,14 @@ mod tests { let reservation = TcpListener::bind("127.0.0.1:0").await.unwrap(); let address = reservation.local_addr().unwrap(); drop(reservation); - let spotify = AuthCodePkceSpotify::new( + let spotify = SpotifyClient(AuthCodePkceSpotify::new( Credentials::new_pkce("public-client"), OAuth { state: "expected".into(), redirect_uri: format!("http://{address}/callback"), ..Default::default() }, - ); + )); let receiver = tokio::spawn(async move { receive_auth_code_from_local_callback(&spotify, address, Duration::from_secs(2)) .await @@ -1233,7 +1365,7 @@ mod tests { device_id: &str, is_playing: bool, write_statuses: &[u16], - ) -> (AuthCodePkceSpotify, tokio::task::JoinHandle>) { + ) -> (SpotifyClient, tokio::task::JoinHandle>) { let body = serde_json::json!({ "device": {"id": device_id, "name": "TV", "is_active": true, "is_private_session": false, "is_restricted": false, @@ -1246,7 +1378,11 @@ mod tests { let mut responses = vec![(200, body)]; responses.extend(write_statuses.iter().map(|status| (*status, String::new()))); let (base_url, server) = spotify_server(responses).await; - let spotify = AuthCodePkceSpotify::from_token_with_config( + (test_playback_client(base_url), server) + } + + fn test_playback_client(base_url: String) -> SpotifyClient { + SpotifyClient(AuthCodePkceSpotify::from_token_with_config( rspotify::Token { access_token: "test-token".into(), ..Default::default() @@ -1258,8 +1394,7 @@ mod tests { token_refreshing: false, ..Default::default() }, - ); - (spotify, server) + )) } #[tokio::test] @@ -1301,6 +1436,45 @@ mod tests { assert!(requests[2].starts_with("PUT /me/player/play?device_id=tv ")); } + #[tokio::test] + async fn start_routes_an_absent_session_and_leaves_the_result_playing() { + let playing = serde_json::json!({ + "device": {"id": "tv", "name": "TV", "is_active": true, + "is_private_session": false, "is_restricted": false, + "type": "TV", "volume_percent": 50}, + "repeat_state": "off", "shuffle_state": false, + "timestamp": 0, "is_playing": true, "item": null, + "currently_playing_type": "unknown", "actions": {"disallows": {}} + }) + .to_string(); + let (base_url, server) = spotify_server(vec![ + (204, String::new()), + (204, String::new()), + (204, String::new()), + (200, playing), + ]) + .await; + let spotify = test_playback_client(base_url); + let target = TargetDevice { + id: "tv".into(), + name: "TV".into(), + }; + assert_eq!( + ensure_target_playing(&spotify, &target).await.unwrap(), + "Started Spotify on TV" + ); + assert_eq!( + ensure_target_playing(&spotify, &target).await.unwrap(), + "Spotify is already playing on TV" + ); + let requests = server.await.unwrap(); + assert_eq!(requests.len(), 4); + assert!(requests[1].starts_with("PUT /me/player ")); + assert!(requests[1].contains("\"device_ids\":[\"tv\"]")); + assert!(requests[2].starts_with("PUT /me/player/play?device_id=tv ")); + assert!(requests[3].starts_with("GET /me/player?")); + } + #[tokio::test] async fn start_reports_transfer_and_resume_failures() { let target = TargetDevice { From 9923515c08deccdf43e4882a6f381ad4267cb68b Mon Sep 17 00:00:00 2001 From: postigodev Date: Tue, 6 Oct 2026 14:05:26 -0500 Subject: [PATCH 7/7] docs: use postigodev prefix for working branches --- AGENTS.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..998172e --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,3 @@ +# Repository instructions + +Use `postigodev/` as the prefix for working branches, including future branches.