Skip to content

security: harden Spotify OAuth state and token lifecycle - #146

Open
Blackspirits wants to merge 11 commits into
kittinan:masterfrom
Blackspirits:security/harden-oauth-token-lifecycle
Open

security: harden Spotify OAuth state and token lifecycle#146
Blackspirits wants to merge 11 commits into
kittinan:masterfrom
Blackspirits:security/harden-oauth-token-lifecycle

Conversation

@Blackspirits

@Blackspirits Blackspirits commented Aug 9, 2026

Copy link
Copy Markdown

Summary

This PR hardens the Spotify authorization and token lifecycle without changing the public card API.

  • Adds a cryptographically random OAuth state value to the authorization request.
  • Binds state to an HttpOnly, SameSite=Lax cookie and validates it with hmac.compare_digest in the callback.
  • Handles denied/incomplete OAuth callbacks without attempting a token exchange and consumes the state cookie once a valid state-bound flow ends.
  • Persists an absolute expired_ts when the initial access token is issued, avoiding an unnecessary refresh on first card access.
  • Preserves the existing refresh token when Spotify omits refresh_token from a refresh response.
  • Persists a rotated refresh token when Spotify returns one.
  • Stops printing bearer access tokens to application logs.
  • Distinguishes revoked credentials (invalid_grant) from transient/malformed refresh failures, so temporary Spotify errors do not delete stored authorization or incorrectly tell users to re-authenticate.
  • Adds regression tests for OAuth state validation, denied/incomplete callbacks, token expiry metadata, secret logging, refresh-token rotation, and transient refresh failures.

Security rationale

Spotify's Authorization Code flow recommends using state to protect the redirect flow against CSRF. Bearer access tokens are credentials and should not be emitted to application logs. Spotify refresh responses may omit a new refresh token, so the existing refresh token must be retained unless a replacement is returned. A refresh failure other than invalid_grant is not proof that the user's stored authorization is invalid and must not delete those credentials.

References:

Compatibility

  • Existing /api/view parameters and normal rendering behavior are unchanged.
  • No new secrets are required; expired_ts is additive token metadata.
  • Authorization flows started before deployment may need to be restarted once because they will not have the new state cookie.
  • api/view.svg.py is not modified by this PR.
  • PR fix local dev setup + local file playback crashes #144 also changes api/view.py / the .svg endpoint. If fix local dev setup + local file playback crashes #144 lands first, this branch should be rebased and the token-lifecycle changes re-audited against the resulting endpoint implementation before merge.

Tests

Added/updated tests cover:

  • OAuth state generation and hardened cookie attributes.
  • Missing/mismatched state rejection.
  • State cookie removal after successful callback.
  • User-denied (access_denied) authorization without token exchange.
  • Valid-state callbacks missing both code and error.
  • Absolute expiry persistence on initial authorization.
  • No bearer token output to stdout/stderr.
  • Refresh response without token rotation.
  • Refresh response with token rotation.
  • Transient refresh errors preserving stored credentials.
  • Incomplete refresh responses preserving stored credentials.
  • Transient refresh failures returning a retryable server error instead of a re-login message.
  • Invalid expires_in metadata.

Review / CI status

The complete diff has been re-audited after the follow-up fixes. The PR is currently mergeable and contains changes to six files only.

The upstream Python Flask CI/CD workflow is still reported as action_required: GitHub created the workflow run but did not start jobs because this fork contribution requires maintainer approval. The current head therefore still needs the upstream pytest/CI run before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant