Skip to content

fix(syncedlyrics): redact Musixmatch usertoken from error messages before propagation #234

@InstaZDLL

Description

@InstaZDLL

Context

The push-time security review flagged a token leak vector in `src-tauri/crates/syncedlyrics/src/lib.rs`.

`reqwest::Error`'s `Display` implementation includes the request URL — query string and all. The Musixmatch provider passes `usertoken` as a query parameter:

```rust
http.get(format!("{ROOT}/track.search"))
.query(&[
("q", options.query.as_str()),
("usertoken", token.as_str()), // <— rides in the URL
// …
])
```

Any error from that request path (5xx, network reset, body cap exceeded) propagates the raw `reqwest::Error` up through `Result<…, Error>` → `last_error` in `SyncedLyricsClient::search` → `AppError::Other(format!("{err}"))` in `commands/lyrics.rs`. The token reaches the logs (`tracing::warn!`) and the IPC error returned to the frontend.

Scope

Bounded but real:

  • Musixmatch is opt-in (`MUSIXMATCH_ENABLED = AtomicBool::new(false)` at boot, hydrated from `app_setting['lyrics.musixmatch_enabled']`). Default install never hits this path.
  • The token is short-lived — Musixmatch's `token.get` mints fresh on every `search` call.
  • The other providers (LRCLIB, NetEase, Megalobiz, Genius) carry no credentials in URLs, so they're not affected.

Still a defence-in-depth gap worth closing before more users opt into the Musixmatch toggle.

Proposed fix

Two layers:

  1. Provider-local rewrite. In `providers/musixmatch.rs`, catch errors at the request boundary and rebuild the message via the existing `http::redact_url` helper:

```rust
let response = http.get(url.clone()).send().await
.map_err(|err| Error::Provider(format!(
"musixmatch search failed at {}: {err}",
http::redact_url(url.as_str()),
)))?
.error_for_status()
.map_err(|err| Error::Provider(format!(
"musixmatch search returned non-2xx at {}: {err}",
http::redact_url(url.as_str()),
)))?;
```

`redact_url` already strips userinfo + query, so the rewritten message exposes the host + path only.

  1. Header instead of query. Longer term: switch Musixmatch to send the token via an `Authorization` header so it's outside the URL surface entirely. Their endpoint accepts this — verified against the Musicolet client's reverse-engineered config.

(2) is the right fix; (1) is the band-aid we ship now.

Acceptance

  • All Musixmatch error paths route through `Error::Provider` with a redacted URL message.
  • Unit test asserts `format!("{err}")` on the rewritten error never contains the literal token.
  • `commands/lyrics.rs`'s `tracing::warn!` on `last_error` is reviewed for the same property.

Discovered via

Automated push-time security review on commit `02f6a14` of `fix(plugin-sdk): upsert full app_setting columns to satisfy schema NOT NULLs` — the review surfaced the propagation chain, not the active commit itself.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions