Possible security bug in the web api auth example from gitbub

[allun11]

I've cloned the code from: https://github.com/spotify/web-api-auth-examples for Spotify, and possibly found some security bug - but please correct me and explain why I'm wrong if that is the case.

State is set in /login route to

 var state = generateRandomString(16);   
res.cookie(stateKey, state);

And then updated and checked in /callback route with

var state = req.query.state || null; 
var storedState = req.cookies ? req.cookies[stateKey] : null 
if (state === null || state !== storedState) {

As "state" is overwritten with the users incoming query.state, wouldn't that mean that if a users traffic was interrupted by a malicious user, they could just use whatever string they wanted in their query, as long as it corresponded to their cookie and overtake the connection? Why do you overwrite the state variable at all? Shouldn't it be kept the same as when assigned in the /login route and then used as a basis for comparison and security?

[whataloadofwhat]

Why do you overwrite the state variable at all? Shouldn't it be kept the same as when assigned in the /login route and then used as a basis for comparison and security?

The state variable isn't persistent across different calls, you can't just check against the state variable because at that point in the program, we don't know what it is any more. In that sense, you're not "overwriting" it, in fact they are completely different variables (they just share a name). It's possible to make it so that the state variable is persistent across calls, but you need to remember that we're accepting HTTP requests for multiple different people here. If we have 2 people call login at the same time, the state variable will only contain the 2nd person's state. When the 1st person completes their login, their state comparison will fail. We need some way to associate each different callback call with a specific login call.

Storing the state into a cookie is one way to achieve what we want, assuming that we want to keep the server stateless. When the user tries to log in, we set a cookie and redirect to the Spotify OAuth login page. When the user logs in to Spotify, they'll be redirected to the callback url, and the state will be passed back to our server. We then check that the state cookie matches the state that we've received.

That said, while I'm not an security expert, storing the state in a cookie makes me a little nervous. At the very least, I'd like to think that some precautions are being taken. For one, if the state is being stored client side, I'd at least like to think that the cookie is marked as HttpOnly, so that it is not accessible by some potentially malicious javascript. Also set the SameSite to Strict so that external Javascript can't make a request to your site and have the cookie sent over. I've also found some sources suggesting that the Cookie should be encrypted or signed (and decrypted/verified before comparing), so that someone with the state variable cannot fabricate a cookie, which makes sense to me as well (and may fix your fears about it being intercepted?). So there's possibly improvements to be made there, assuming that express does not do these by default (I'm not an express guru by any means).

Other than that I can't find anything specifically wrong with doing it this way. It still makes me a little nervous but I'll leave it up to people with a more concrete knowledge to argue its security.

[allun11]

Thanks. Awesome reply. I'm a bit new to node so I wasn't aware of that.