Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
tobobo opened this issue Feb 17, 2023 · 0 comments
Open

Comments

@tobobo
Copy link

tobobo commented Feb 17, 2023

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!

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

No branches or pull requests

1 participant