thelinmichael / spotify-web-api-node

A Node.js wrapper for Spotify's Web API.
http://thelinmichael.github.io/spotify-web-api-node/
MIT License
3.11k stars 498 forks source link

PSA: Avoid passing object reference to constructor to avoid state leakage #482

Open tobobo opened 1 year ago

tobobo commented 1 year ago

I just ran in to this while testing an app with an auth code grant flow before release and thought it might be helpful for others to avoid the hard-to-debug issue I ran in to.

If your app has your client ID and secret stored in an object, make sure to pass a copy of that object to the constructor to avoid state leakage.

Say you have a route that performs an authorization code grant

// spotifyCredentials.js
export const credentials = {
  clientId: process.env.SPOTIFY_CLIENT_ID,
  clientSecret: process.env.SPOTIFY_CLIENT_SECRET,
};

// server.js

import { credentials } from './spotifyCredentials';
import express from 'express';

const app = express();

app.use((req, res, next) => {
 // middlware to authenticate user and add req.user with spotify credentials
});

app.post('/spotify_auth_code_grant', (req, res) => {
  const spotifyWebApi = new SpotifyWebApi(credentials); // DANGER!! spotifyWebApi can now modify const credentials
  const authResponse = await spotifyWebApi.authorizationCodeGrant(req.body.code);
  const { access_token: accessToken, refresh_token: refreshToken } = authResponse.body;

  // these two lines now modify the original const credentials
  spotifyWebApi.setAccessToken(accessToken);
  spotifyWebApi.setRefreshToken(refreshToken);

  const userResponse = await spotifyWebApi.getMe();

  // ...save spotify account data to User model or something

  res.status(201).end();
});

app.get('/spotify_currently_playing', (req, res) => {
  const { spotifyAccessToken: accessToken, spotifyRefreshToken } = req.user;
  const spotifyWebApi = new SpotifyWebApi({
    accessToken,
    refreshToken,
    ...credentials // DANGER!! Credentials can have accessToken and refreshToken from last request to /spotify_auth_code_grant
  });

  const nowPlayingResponse = await spotifyWebApi.getMyCurrentlyPlayingTrack();
  res.json(nowPlayingResponse.body);
});

Any request to /spotify_currently_playing will have the tokens for its SpotifyWebApi instance overwritten by the tokens from the last user to call /spotify_auth_code_grant.

This could be fixed by putting the ...credentials spread from /spotify_currently_playing before accessToken and refreshToken, but the best fix is to make a shallow copy of credentials when making the SpotifyWebApi request in /spotify_auth_code_grant:

const spotifyWebApi = new SpotifyWebApi({ ...credentials });

Hope this saves someone some time!

d2201 commented 4 months ago

Good catch, spent 20 minutes debugging that issue. 🙄