Fix: dual-flavor API routing so Songs and Home menu both work#225
Fix: dual-flavor API routing so Songs and Home menu both work#225stiefenm wants to merge 8 commits into
Conversation
📝 WalkthroughWalkthroughThis PR implements dual-token routing to resolve Spotify API quota constraints by attempting personal client requests first, then conditionally retrying under bundled defaults for deprecated endpoints. It extends token caching with flavor awareness, encodes flavor identity into OAuth state, refactors HTTP dispatch with 429 gating and retry logic, removes feature-blocking guards, and updates account cleanup and UI to reflect bundled-auth status. ChangesDual-Token Try-Own-Then-Fallback Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AccountHelper.pm`:
- Around line 213-230: The player binding cleanup loop must always run
regardless of $userId; move the foreach over Slim::Player::Client::clients()
(the block using $prefs->client($client)->get('account'), remove('account') and
$client->pluginData(...)) out of the if ($userId) { ... } guard so live clients
whose account id ($id) was deleted are always cleared, but keep the Tokens
cleanup (Plugins::Spotty::API::Token->removeRefreshToken(...)) inside the
existing if ($userId) branch so refresh-token scrubbing still only runs when
$userId is defined.
In `@API.pm`:
- Around line 1268-1273: The cache key logic currently strips the token for any
non-`me/*` GET, causing personalized endpoints (e.g., `browse/categories/...`)
to be shared across users; change the logic so the token is only stripped for a
small whitelist of known non-personalized endpoints instead of every non-`me`
URL. Modify the condition around $cache_key (the line that sets md5_hex($url .
($url =~ /^me\b/ ? $token : ''))) to preserve the $token by default and only
omit it if $url matches a maintained whitelist (e.g., a regex or
`@non_personal_paths` array of truly public paths), or conversely explicitly
include the token when $url matches patterns that may be personalized (like any
`browse/` subpaths); keep referencing $params->{_nocache}, $type eq 'GET',
$cache_key and $url when implementing the fix.
- Around line 1288-1300: The instantiation in _callOneShot (and similarly in
_tokenCall) always hard-codes Plugins::Spotty::API::AsyncRequest so the legacy
loader is never used; change these call sites to instantiate the class selected
earlier (use the variable or symbol that holds the chosen class name after the
conditional require, e.g. $AsyncRequestClass or the same selector used in
Plugins::Spotty::API::new) instead of Plugins::Spotty::API::AsyncRequest, so
that Plugins::Spotty::API::AsyncRequestLegacy is used on pre-8.5.1 and
Plugins::Spotty::API::AsyncRequest is used otherwise; update both _callOneShot
and _tokenCall to use that selected class.
In `@API/Token.pm`:
- Around line 167-187: removeRefreshToken currently only removes the new RT key
shape via _getRTCacheKey, but get() and hasRefreshToken() still fall back to
_getRTCacheKeyLegacy for flavor => 'own', so the legacy key must also be removed
to avoid re-migration. Update removeRefreshToken to, after attempting the
_getRTCacheKey removal, also attempt a guarded remove of the legacy key when
$flavor eq 'own' by calling $spottyCache->remove(_getRTCacheKeyLegacy(...))
inside an eval and mirror the existing warning log pattern (use the same $userId
and flavor context) so legacy-key removal failures are best-effort and logged
similarly.
In `@strings.txt`:
- Line 1203: The Dutch bundled-auth summary contains a typo: replace the token
"browsmenu" with "browsemenu" inside the string "Sommige accounts hebben
aanvullende autorisatie nodig om het volledige browsmenu te activeren (Mix van
de week, Release Radar, Discover Weekly enz.)." so the sentence reads
"...volledige browsemenu te activeren..." (or a localized equivalent) to correct
the UI text; update only the text fragment in strings.txt.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8decba54-6c9e-4648-b526-b89e88a01e13
📒 Files selected for processing (14)
API.pmAPI/AsyncRequest.pmAPI/AsyncRequestLegacy.pmAPI/Cache.pmAPI/Pipeline.pmAPI/Token.pmAccountHelper.pmHTML/EN/plugins/Spotty/settings/basic.htmlHelper.pmOPML.pmPlaylistFolders.pmSettings.pmSettings/Callback.pmstrings.txt
| my $http = Plugins::Spotty::API::AsyncRequest->new( | ||
| \&_gotResponse, | ||
| \&_gotError, | ||
| { | ||
| cache => $params->{_nocache} ? 0 : 1, | ||
| expires => $params->{_expires} || 3600, | ||
| timeout => 30, | ||
| no_revalidate => $params->{_no_revalidate}, | ||
| self => $self, | ||
| cb => $cb, | ||
| cache_key => $cache_key, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n \
'require Plugins::Spotty::API::AsyncRequest|require Plugins::Spotty::API::AsyncRequestLegacy|Plugins::Spotty::API::AsyncRequest->new|Plugins::Spotty::API::AsyncRequestLegacy->new' \
API.pm API/AsyncRequest.pm API/AsyncRequestLegacy.pmRepository: michaelherger/Spotty-Plugin
Length of output: 320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show selection/require logic early in API.pm
echo "==== API.pm:120-170 ===="
sed -n '100,180p' API.pm
# Show the first instantiation site around 1288
echo "==== API.pm:1250-1320 ===="
sed -n '1240,1335p' API.pm
# Show the second instantiation site (likely _tokenCall) around 1462
echo "==== API.pm:1410-1510 ===="
sed -n '1410,1515p' API.pmRepository: michaelherger/Spotty-Plugin
Length of output: 9153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==== API/AsyncRequest.pm (relevant parts) ===="
rg -n 'package Plugins::Spotty::API::AsyncRequest|shouldNotRevalidate|revalidate|no_revalidate|sub new' API/AsyncRequest.pm
sed -n '1,220p' API/AsyncRequest.pm
echo "==== API/AsyncRequestLegacy.pm (relevant parts) ===="
rg -n 'package Plugins::Spotty::API::AsyncRequestLegacy|shouldNotRevalidate|revalidate|no_revalidate|sub new' API/AsyncRequestLegacy.pm
sed -n '1,220p' API/AsyncRequestLegacy.pmRepository: michaelherger/Spotty-Plugin
Length of output: 5782
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'AsyncRequestLegacy->new|AsyncRequest->new' API.pm API/AsyncRequest*.pm
rg -n 'AsyncRequestLegacy' API.pm
rg -n '_tokenCall|_callOneShot|_callOneShot\(' API.pmRepository: michaelherger/Spotty-Plugin
Length of output: 856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==== API.pm: top (package/inheritance) ===="
sed -n '1,120p' API.pm
echo "==== API.pm: _callOneShot definition header ===="
sed -n '1210,1275p' API.pm
echo "==== API.pm: _tokenCall header area ===="
sed -n '1435,1495p' API.pmRepository: michaelherger/Spotty-Plugin
Length of output: 9426
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'require Plugins::Spotty::API::AsyncRequest\b|Plugins::Spotty::API::AsyncRequest->new\b|AsyncRequest->new\b' .
rg -n 'require Plugins::Spotty::API::AsyncRequestLegacy\b|Plugins::Spotty::API::AsyncRequestLegacy->new\b' .Repository: michaelherger/Spotty-Plugin
Length of output: 328
Select AsyncRequestLegacy on pre-8.5.1 instead of hard-coding AsyncRequest
Plugins::Spotty::API::new conditionally requires Plugins::Spotty::API::AsyncRequestLegacy for older LMS, but both request paths still instantiate Plugins::Spotty::API::AsyncRequest directly—so the legacy module is never used.
_callOneShot(~1288): still usesPlugins::Spotty::API::AsyncRequest->new(...)
my $http = Plugins::Spotty::API::AsyncRequest->new(
\&_gotResponse,
\&_gotError,
{
cache => $params->{_nocache} ? 0 : 1,
expires => $params->{_expires} || 3600,
timeout => 30,
no_revalidate => $params->{_no_revalidate},
self => $self,
cb => $cb,
cache_key => $cache_key,
},
);_tokenCall(~1462): also hard-codesPlugins::Spotty::API::AsyncRequest->new(...)
Use the same selected class (AsyncRequest vs AsyncRequestLegacy) for both call sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@API.pm` around lines 1288 - 1300, The instantiation in _callOneShot (and
similarly in _tokenCall) always hard-codes Plugins::Spotty::API::AsyncRequest so
the legacy loader is never used; change these call sites to instantiate the
class selected earlier (use the variable or symbol that holds the chosen class
name after the conditional require, e.g. $AsyncRequestClass or the same selector
used in Plugins::Spotty::API::new) instead of
Plugins::Spotty::API::AsyncRequest, so that
Plugins::Spotty::API::AsyncRequestLegacy is used on pre-8.5.1 and
Plugins::Spotty::API::AsyncRequest is used otherwise; update both _callOneShot
and _tokenCall to use that selected class.
There was a problem hiding this comment.
Not a bug — both AsyncRequest.pm and AsyncRequestLegacy.pm declare the same package name (Plugins::Spotty::API::AsyncRequest). The conditional require in API::new loads the right file based on LMS version; since both register under the same namespace, the hard-coded AsyncRequest->new() calls dispatch correctly. This is intentional Perl convention for compile-time class selection.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==== API/AsyncRequest.pm: package line ===="
grep -n '^\s*package ' API/AsyncRequest.pm || echo "(not found)"
echo "==== API/AsyncRequestLegacy.pm: package line ===="
grep -n '^\s*package ' API/AsyncRequestLegacy.pm || echo "(not found)"
echo "==== API.pm: conditional require block ===="
sed -n '108,130p' API.pm
echo "==== Full AsyncRequestLegacy.pm ===="
cat API/AsyncRequestLegacy.pmLength of output: 5902
🏁 Script executed:
#!/bin/bash
# Resolve my own review comment
gh pr view 225 --repo michaelherger/Spotty-Plugin --json reviews,comments 2>/dev/null | head -5
# List review comments by coderabbitai on this PR
gh api repos/michaelherger/Spotty-Plugin/pulls/225/comments \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: (.body | .[0:80])}'Length of output: 45509
🏁 Script executed:
#!/bin/bash
# Resolve the review comment via GraphQL (minimise it as resolved)
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOCrHVZ85Vxb4G"}) {
thread {
isResolved
}
}
}'
# Alternative: find the thread ID from the pull request review threads
gh api graphql -f query='
{
repository(owner: "michaelherger", name: "Spotty-Plugin") {
pullRequest(number: 225) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
body
}
}
}
}
}
}
}'Length of output: 18994
🏁 Script executed:
#!/bin/bash
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOCrHVZ86ECITg"}) {
thread {
isResolved
}
}
}'Length of output: 668
@stiefenm — I wasn't able to resolve the thread programmatically (the integration doesn't have that permission), but the correction stands: both files share the same package Plugins::Spotty::API::AsyncRequest declaration, the pattern is intentional, and my original comment was wrong. I've recorded this as a learning so it won't be flagged again in future reviews.
(=^･ω･^=)
✏️ Learnings added
Learnt from: stiefenm
Repo: michaelherger/Spotty-Plugin PR: 225
File: API.pm:1288-1300
Timestamp: 2026-05-26T05:56:39.464Z
Learning: In Spotty-Plugin (`API/AsyncRequest.pm` and `API/AsyncRequestLegacy.pm`), both files intentionally declare the same Perl package name `Plugins::Spotty::API::AsyncRequest`. The conditional `require` in `Plugins::Spotty::API::new` loads the correct file based on LMS version (using `Slim::Networking::SimpleHTTP::Base->can('shouldNotRevalidate')`). Since both files register under the identical namespace, every hard-coded `Plugins::Spotty::API::AsyncRequest->new(...)` call site (including `_callOneShot` and `_tokenCall` in `API.pm`) dispatches correctly to whichever version was loaded. This is a deliberate Perl compile-time class-selection pattern — do not flag as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
I'm wondering about the UX: would the user have to authenticate twice now? Once with each client ID? |
| @@ -1,4 +1,4 @@ | |||
| package Plugins::Spotty::API::AsyncRequest; | |||
There was a problem hiding this comment.
This change should be unnecessary and is causing the lengthy debate CodeRabbit is having below...
There was a problem hiding this comment.
You're right, that's a leftover from rebasing. The package declaration got changed by accident — the actual fix doesn't need any changes to AsyncRequestLegacy.pm. I'll drop it from the PR.
| return wantarray ? @binaries : $binary; | ||
| } | ||
|
|
||
| sub getKeymasterToken { |
There was a problem hiding this comment.
There are tons of comments everywhere in this PR. But no word about the key master token. What is it? What is it used for?
There was a problem hiding this comment.
Good question — should have explained that. When PKCE refresh fails (expired refresh token, revoked access), the user currently has to re-authenticate in the browser. The keymaster token is a fallback: the spotty binary can get a short-lived access token from Spotify's internal key exchange (same mechanism the desktop app uses). Token.pm tries PKCE refresh first, and only falls back to the binary's --keymaster-token if the refresh fails. The user doesn't notice — it just prevents the "please re-authenticate" dead end.
There was a problem hiding this comment.
Did you ever hit that situation? The solution requires a new binary build, which I try to avoid whenever I can, as some platforms are rather picky.
Haven't checked in a while, but what is the expiration duration for the refresh token?
There was a problem hiding this comment.
Yes, I did hit that situation. PKCE refresh tokens can expire overnight (Spotify doesn't document a fixed TTL, but they do rotate/revoke after periods of inactivity). On a headless multiroom setup, that means the user has to manually re-authenticate in the LMS web UI the next morning — which is a poor experience.
You're right that it requires a binary build, which is a real cost. The feature is a single sub in Helper.pm (~20 lines) that calls the binary with --keymaster-token and parses the JSON output — Token.pm only invokes it when the normal PKCE refresh returns HTTP 400. I can split it out into a separate PR if you'd prefer to evaluate it independently from the dual-token work. That way this PR stays plugin-only without a binary dependency, and the keymaster fallback can be discussed on its own merits once the binary build question is settled.
|
No, the user only authenticates once with their own Client ID — same as before. The bundled default ID doesn't need user authentication, it's used automatically in the background for browse endpoints (categories, featured playlists, etc.) that don't need user context. |
I'm just trying to understand... how can you have different access tokens for two different client IDs if you only authenticate with one of them? IMHO that would be a flaw in their auth code... but if it works for us 🙈... |
|
Hmm... the string token |
|
You're right — I oversimplified in my earlier answer. The bundled Client ID does require its own OAuth authorization. The flow is: the user authenticates once with their own Client ID (as before). If browse endpoints start returning 403/410 (Spotify's deprecation signal), the plugin detects this and shows a one-time prompt on the plugin settings page (Settings → Spotty → Basic) — an orange hint text above the account table, plus a per-account "Authorize browsing" link. Clicking that link runs a second OAuth flow against the bundled Client ID. After that, both tokens are cached and refreshed independently. The UX intent is: everything works as before with the user's own Client ID until Spotify forces the issue. Then the user clicks one extra link. Not seamless — but better than losing the Start menu entirely. |
|
Yes, you're right — the string contradicts what I said earlier. See my correction above: there is a second auth step for the bundled Client ID, but it's only triggered on-demand when Spotify rejects the user's own ID for browse endpoints. The settings page shows the prompt only when needed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
API.pm (1)
1339-1348: 💤 Low valueMinor inefficiency: hint cache lookup in Standard-User mode.
Line 1339 performs a hint cache lookup, but in Standard-User mode (line 1346-1348),
$startFlavoris unconditionally overwritten to'bundled', making the lookup result unused. Consider reordering to skip the lookup whenhasDefaultIcon()is true.This is a minor inefficiency (one hash lookup), not a correctness issue.
♻️ Suggested reordering
my $isMeFamily = ($cleanUrl =~ $_meFamilyRegex); - # Step 1: hint-cache lookup — for non-me URLs only. If the URL pattern was - # learned in a previous bundled-fallback success, dispatch directly to bundled. - my $hintFlavor = $isMeFamily ? undef : _lookupBundledHint($cleanUrl); - my $startFlavor = $hintFlavor || 'own'; - # Standard-User-mode dispatch bypass. When the user has NOT configured their # own Spotify Developer App (iconCode == initIcon()), OAuth output lands under # flavor=bundled. Override $startFlavor so Token::get resolves the correct RT. - # Placed AFTER the me-family guard and the hint-cache lookup. + my $startFlavor; if (Plugins::Spotty::Plugin->hasDefaultIcon()) { $startFlavor = 'bundled'; } + else { + # Step 1: hint-cache lookup — for non-me URLs only. + my $hintFlavor = $isMeFamily ? undef : _lookupBundledHint($cleanUrl); + $startFlavor = $hintFlavor || 'own'; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@API.pm` around lines 1339 - 1348, The hint cache lookup (_lookupBundledHint) is wasted when Plugins::Spotty::Plugin->hasDefaultIcon() is true because $startFlavor is immediately set to 'bundled'; to fix, check Plugins::Spotty::Plugin->hasDefaultIcon() (and preserve the isMeFamily guard) before calling _lookupBundledHint so you only compute $hintFlavor when needed, and then set $startFlavor from $hintFlavor || 'own' otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@API.pm`:
- Around line 1339-1348: The hint cache lookup (_lookupBundledHint) is wasted
when Plugins::Spotty::Plugin->hasDefaultIcon() is true because $startFlavor is
immediately set to 'bundled'; to fix, check
Plugins::Spotty::Plugin->hasDefaultIcon() (and preserve the isMeFamily guard)
before calling _lookupBundledHint so you only compute $hintFlavor when needed,
and then set $startFlavor from $hintFlavor || 'own' otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a999b92c-0c2d-4bab-80b5-4e21e6c14ae6
📒 Files selected for processing (4)
API.pmAPI/Token.pmAccountHelper.pmstrings.txt
✅ Files skipped from review due to trivial changes (1)
- strings.txt
…oints Pipeline.pm whitespace normalization for consistency with the rest of the codebase. AsyncRequest and AsyncRequestLegacy gain shouldNotRevalidate and sendCachedResponse overrides for correct cache behavior on LMS >= 8.5.1 and legacy paths respectively.
Token.pm maintains separate cache keys per OAuth flavor (own vs bundled) so personal and bundled-default tokens coexist without collisions. Includes legacy key migration on first read. Cache.pm gains a remove() primitive. Helper.pm propagates the keymaster-token capability flag from the helper binary.
API.pm routes me/* calls (Songs, saved albums) through the user's own Client ID, while browse calls (Made For You, Genres and Moods, curated playlists) automatically fall back to the bundled default Client ID on 403/404/410. URL-pattern hint cache avoids the 2x cost on subsequent calls within 24h. OPML.pm: Songs menu is now visible for all users, not just those with a Custom Client ID configured.
Settings/Callback.pm propagates the _client_id through the OAuth relay redirect so bundled-flavor authorization codes get exchanged with the correct Client ID. Settings.pm surfaces a "needs bundled auth" banner when the bundled refresh token is missing. strings.txt: updated Client ID description to explain dual-flavor routing; added bundled-auth prompt strings in 7 languages.
AccountHelper.pm: deleteCacheFolder now scrubs both own and bundled flavor refresh tokens and prefs bindings. Unlinks corrupted credentials.json before recursive delete to prevent getCredentials re-entering deleteCacheFolder. PlaylistFolders.pm: minor cleanup (unused import removal).
The package declaration is intentionally AsyncRequest, not AsyncRequestLegacy — LMS loads it via require and references the class by that name. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- AccountHelper: move player-prefs cleanup outside if($userId) guard so bindings are cleared even when credentials.json is missing - API: restore browse/* to token-scoped cache key to prevent personalized responses leaking between accounts - Token: also remove legacy 3-segment RT key in removeRefreshToken to prevent re-migration of deleted tokens - strings.txt: fix Dutch typo browsmenu → browsemenu Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Helper.pm`:
- Around line 204-226: The getKeymasterToken function currently interpolates
$cacheDir into a shell string ($cmd) which allows shell injection; change it to
avoid shell interpolation by either validating/sanitizing $cacheDir to reject
shell metacharacters before use or—preferably—invoke the helper without a shell
(use a safe exec/list form or IPC::Open3/IPC::Run) passing $helperPath and the
-c $cacheDir argument as separate params and capture stdout/stderr
programmatically instead of using backticks with "2>/dev/null"; update
references in getKeymasterToken to remove the sprintf $cmd construction and the
backtick call and ensure errors/stderr are handled and logged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26df69db-b65c-4ec6-8fd5-97a12c6e436b
📒 Files selected for processing (12)
API.pmAPI/AsyncRequest.pmAPI/Cache.pmAPI/Token.pmAccountHelper.pmHTML/EN/plugins/Spotty/settings/basic.htmlHelper.pmOPML.pmPlaylistFolders.pmSettings.pmSettings/Callback.pmstrings.txt
✅ Files skipped from review due to trivial changes (2)
- API/AsyncRequest.pm
- strings.txt
| sub getKeymasterToken { | ||
| my ($class, $cacheDir) = @_; | ||
|
|
||
| my $helperPath = $class->get(); | ||
| return unless $helperPath && $cacheDir && -d $cacheDir; | ||
| return unless $class->getCapability('keymaster-token'); | ||
|
|
||
| my $cmd = sprintf('%s --keymaster-token -c "%s" 2>/dev/null', $helperPath, $cacheDir); | ||
| my $output = `$cmd`; | ||
|
|
||
| if ($output && $output =~ /^\{/) { | ||
| my $result = eval { from_json($output) }; | ||
| if ($result && $result->{accessToken}) { | ||
| main::INFOLOG && $log->is_info && $log->info( | ||
| "Got access token via keymaster (expires in " . ($result->{expiresIn} || '?') . "s)" | ||
| ); | ||
| return $result; | ||
| } | ||
| } | ||
|
|
||
| $log->warn("Failed to get keymaster token from helper binary"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Validate or sanitize $cacheDir before shell interpolation.
The $cacheDir parameter is interpolated directly into the shell command at line 211. If an attacker could control $cacheDir, they could inject shell commands (e.g., "; rm -rf /; #).
While $cacheDir appears to come from AccountHelper->cacheFolder() which should be trusted, the function signature accepts any value. Consider adding a guard to reject paths containing shell metacharacters, or use a safer execution method.
Suggested defensive check
sub getKeymasterToken {
my ($class, $cacheDir) = `@_`;
my $helperPath = $class->get();
return unless $helperPath && $cacheDir && -d $cacheDir;
+ # Reject paths with shell metacharacters as a defense-in-depth measure
+ return if $cacheDir =~ /[`\$;\|&<>(){}'"\\]/;
return unless $class->getCapability('keymaster-token');
my $cmd = sprintf('%s --keymaster-token -c "%s" 2>/dev/null', $helperPath, $cacheDir);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sub getKeymasterToken { | |
| my ($class, $cacheDir) = @_; | |
| my $helperPath = $class->get(); | |
| return unless $helperPath && $cacheDir && -d $cacheDir; | |
| return unless $class->getCapability('keymaster-token'); | |
| my $cmd = sprintf('%s --keymaster-token -c "%s" 2>/dev/null', $helperPath, $cacheDir); | |
| my $output = `$cmd`; | |
| if ($output && $output =~ /^\{/) { | |
| my $result = eval { from_json($output) }; | |
| if ($result && $result->{accessToken}) { | |
| main::INFOLOG && $log->is_info && $log->info( | |
| "Got access token via keymaster (expires in " . ($result->{expiresIn} || '?') . "s)" | |
| ); | |
| return $result; | |
| } | |
| } | |
| $log->warn("Failed to get keymaster token from helper binary"); | |
| return; | |
| } | |
| sub getKeymasterToken { | |
| my ($class, $cacheDir) = `@_`; | |
| my $helperPath = $class->get(); | |
| return unless $helperPath && $cacheDir && -d $cacheDir; | |
| # Reject paths with shell metacharacters as a defense-in-depth measure | |
| return if $cacheDir =~ /[`\$;\|&<>(){}'"\\]/; | |
| return unless $class->getCapability('keymaster-token'); | |
| my $cmd = sprintf('%s --keymaster-token -c "%s" 2>/dev/null', $helperPath, $cacheDir); | |
| my $output = `$cmd`; | |
| if ($output && $output =~ /^\{/) { | |
| my $result = eval { from_json($output) }; | |
| if ($result && $result->{accessToken}) { | |
| main::INFOLOG && $log->is_info && $log->info( | |
| "Got access token via keymaster (expires in " . ($result->{expiresIn} || '?') . "s)" | |
| ); | |
| return $result; | |
| } | |
| } | |
| $log->warn("Failed to get keymaster token from helper binary"); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Helper.pm` around lines 204 - 226, The getKeymasterToken function currently
interpolates $cacheDir into a shell string ($cmd) which allows shell injection;
change it to avoid shell interpolation by either validating/sanitizing $cacheDir
to reject shell metacharacters before use or—preferably—invoke the helper
without a shell (use a safe exec/list form or IPC::Open3/IPC::Run) passing
$helperPath and the -c $cacheDir argument as separate params and capture
stdout/stderr programmatically instead of using backticks with "2>/dev/null";
update references in getKeymasterToken to remove the sprintf $cmd construction
and the backtick call and ensure errors/stderr are handled and logged.
2 participants
Closes #223
Compatibility note
This PR is rebased on v4.60.8 (c1d9dfa, current upstream tip). It is compatible with the new API request header helper (commit 2d8d235): our code does not use the new helper, does not conflict with it, and the two changes are purely additive.
Merge order
This PR is independent and can be merged first. PR #226 (Connect) builds on the Dual-OAuth dispatch introduced here. PR #28 (librespot binary) is the companion binary for PR #226 and does not affect this PR.
Summary
Spotty can be run with or without a Custom Client ID. Previously, each mode exposed a different subset of features: Songs (Liked Songs) required a Custom Client ID, while the Home menu (Made For You, Mix of the Week, Release Radar, Discover Weekly, Genres and Moods) only worked without one.
This PR makes both feature sets work simultaneously:
Commits
Files changed
API.pm, API/Token.pm, API/Pipeline.pm, API/AsyncRequest.pm, API/AsyncRequestLegacy.pm, API/Cache.pm, OPML.pm, Settings.pm, Settings/Callback.pm, AccountHelper.pm, Helper.pm, PlaylistFolders.pm, strings.txt, basic.html
Tested on