feat(auth): add Spotify OAuth PKCE - #57
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review in progressClawSweeper is reviewing this revision. This supersedes any previous blocked status. |
|
Codex review: blocked before merge. Reviewed September 4, 2026, 3:00 PM ET / 19:00 UTC. ClawSweeper reviewWhat this changesThis PR adds optional local Spotify OAuth with PKCE, secure per-profile token caching, and OAuth-backed Web API access while retaining cookie authentication and Connect support. Merge readiness⛔ Blocked before merge - 5 items remain Keep open: the exact PR head still lets cookie-required Connect and auto operations fall through to an OAuth-backed Web API client when cookies are absent. The persistent credential flow also needs recorded maintainer acceptance required by the repository vision. Priority: P1 Review scores
Verification
How this fits togetherspogo is a Spotify command-line client. Commands choose an engine, which uses browser cookies for Connect endpoints and either cookies or OAuth tokens for public Web API requests. flowchart LR
Command[CLI command] --> Settings[Profile and flags]
Settings --> Engine[Selected engine]
Settings --> Cookies[Browser cookies]
Settings --> OAuth[OAuth token cache]
Engine --> Connect[Connect endpoints]
Engine --> Web[Spotify Web API]
Cookies --> Connect
Cookies --> Web
OAuth --> Web
Decision needed
Why: VISION.md requires an explicit product decision for persistent credential flows, and the supplied discussion does not record a maintainer-authored approval. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Retain the bounded OAuth Web API feature, but reject missing cookies before Connect or Connect-first auto fallbacks and record maintainer acceptance of the persistent local credential flow. Do we have a high-confidence way to reproduce the issue? Yes—source inspection gives a high-confidence path: configure OAuth without cookies and invoke a Connect-first operation whose internal request fails and falls through to the injected Web API client. Is this the best way to solve the issue? No—the OAuth feature is viable, but this implementation must preserve the existing missing-cookie failure before using OAuth Web API fallback from Connect or auto. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against e89c46fefed8. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (9 earlier review cycles; latest 8 shown)
|
efea2fb to
2d85ec8
Compare
|
@steipete, this should now be ready for your approval and merge when you’re happy with it. I cannot merge it myself because The current head includes the repairs from #61, the additional cross-profile config serialization, and the profile-derived path containment fix from the latest ClawSweeper review. CI is green, the final security review found no remaining path or collision issues, and the PR body contains redacted real-account proof from the installed final binary: automatic token refresh plus successful search and saved-library reads with zero stderr. Could you provide the explicit bounded OAuth approval required by I’d also love to help with |
Co-authored-by: VACInc <3279061+VACInc@users.noreply.github.com> Worked on by: - @VACInc
377856a to
62f34e8
Compare
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: feat(auth): add Spotify OAuth PKCE This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
Summary
High Level TLDR
Add Spotify Authorization Code with PKCE as an optional, local OAuth path for official Web API commands, while keeping cookies as the default and preserving Connect behavior. This version includes the lifecycle-safety repairs from #61, rebases onto current
main, and adds config-wide serialization so concurrent updates to different profiles cannot overwrite each other.spogo auth oauth login|status|clearand per-profile OAuth settingsProduct decision
Maintainer direction on September 2, 2026 accepted this bounded OAuth mode for spogo's purpose: it is optional, local-only, limited to the existing Web API surface, and does not change the cookie-first default or make Connect depend on a hosted credential service. This satisfies the explicit product-decision requirement for new persistent credential flows in
VISION.md.Security and consistency
0700directory with0600files on POSIX systemsMoveFileExreplacement with write-through on WindowsCompatibility
Cookie authentication remains the default. OAuth selects the token provider for public Web API operations; Spotify Connect and internal endpoints still require browser cookies. Existing command names, output formats, and cookie-auth profiles remain unchanged.
Verification
Validated on exact head
62f34e8:rebased onto upstream
e89c46f(September 4, 2026)go test ./...go test -race ./...go run golang.org/x/tools/cmd/deadcode@v0.49.0 -test ./...go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.13.2 run(0 issues)./scripts/check-coverage.sh 90(90.1%)focused OAuth lifecycle and config-lock regressions repeated 20 times under the race detector
profile-derived OAuth path containment and collision regressions cover traversal, Unix/Windows separators, reserved names, mixed-case aliases, and encoded-name collisions
node scripts/build-docs-site.mjsVCS-stamped
go build ./cmd/spogoWindows amd64 test-binary cross-compilation for
cmd/spogoandinternal/spotifybuilt CLI against an isolated config:
false cookies false0was rejected before browser or token exchangemock end-to-end OAuth login covered callback state handling, authorization-code exchange, token persistence, profile persistence, and subsequent clear behavior
final review found and fixed a cross-profile config lost-update race; final rereview found no actionable defects
Redacted end-to-end proof
The exact final head was exercised with synthetic credentials and isolated temporary state. Callback URLs, authorization state, token values, client identifiers, filesystem paths, and local environment details are intentionally omitted. No real credentials were printed or committed.
This proves the complete local OAuth lifecycle: loopback callback handling, state validation, authorization-code exchange, secure token persistence, refresh, profile persistence, failure-safe clearing, same-profile lifecycle serialization, and cross-profile config serialization. The repository CI build also passed on the exact final commit.
Inherited live endpoint proof from #61
The incorporated #61 head exercised the built CLI against its normal loopback callback and Spotify's real accounts token endpoint using dummy public values only. Invalid state was rejected without exchange; valid state reached Spotify's endpoint and received the expected rejection for dummy values. No credential or real token was used, printed, or persisted.
Redacted real-account end-to-end proof
On September 4, 2026, the exact final head
62f34e8was installed as the activespogoCLI and run using an existing real Spotify OAuth authorization. The persisted access token was expired before the run, so the first authenticated request exercised automatic refresh. Account identity, client identifier, token values, callback details, item names, library totals, device details, filesystem paths, and token hashes are omitted.Observed result: the installed exact-head binary refreshed the expired real-account token, atomically persisted the refreshed cache with owner-only permissions, and completed authenticated search, saved-library, device, playback-status, affinity-ranking, and listening-history reads without diagnostics.
autoalso completed through the OAuth Web API path when browser cookies were unavailable. Cookie-backed Connect correctly reported missing cookies with the documented exit code3; live Connect behavior therefore remains credential-blocked rather than falsely claimed as tested. No playback mutation was performed, and no credential or private account content was printed or added to the repository.Worked on by