Don't mutate/use passport.authenticate options object

[CrabDude]

It seems that reusing an options object causes very bizarre very difficult to debug bugs. I haven't looked at the code, but I feel pretty confident it's because the options object is being mutated to store middleware specific state that's causing the passReqToCallback callback's return value to be set on req.account when it should be req.user and ends up as a 404 (passes through to next route, but no other routes catch obviously) instead of redirecting to successRedirect.

404s: "Cannot GET /auth/facebook/callback?code=..."

let accountRedirects = {
  successRedirect: '/profile',
  failureRedirect: '/'
}
app.get('/auth/facebook', passport.authenticate('facebook'))
app.get('/connect/facebook', passport.authorize('facebook'))
app.get('/auth/facebook/callback', passport.authenticate('facebook', accountRedirects))
app.get('/connect/facebook/callback', passport.authorize('facebook', accountRedirects))

Succeeds:

app.get('/auth/facebook', passport.authenticate('facebook'))
app.get('/connect/facebook', passport.authorize('facebook'))
app.get('/auth/facebook/callback', passport.authenticate('facebook', {
  successRedirect: '/profile',
  failureRedirect: '/'
}))
app.get('/connect/facebook/callback', passport.authorize('facebook', {
  successRedirect: '/profile',
  failureRedirect: '/'
}))

FWIW, this example only uses Facebook, but the same issue occurs with Twitter. Haven't tested with others.

[jaredhanson]

Can you look at the code and find out if what you suspect is true? If so, I'd appreciate a pull request with a fix.

Sent from my iPhone

On Apr 19, 2015, at 1:16 AM, Adam Crabtree notifications@github.com wrote:

It seems that reusing an options object causes very bizarre very difficult to debug bugs. I haven't looked at the code, but I feel pretty confident it's because the options object is being mutated to store middleware specific state that's causing the passReqToCallback callback's return value to be set on req.account when it should be req.user and ends up as a 404 (passes through to next route, but no other routes catch obviously) instead of redirecting to successRedirect.

404s: "Cannot GET /auth/facebook/callback?code=..."

let accountRedirects = { successRedirect: '/profile', failureRedirect: '/' } app.get('/auth/facebook', passport.authenticate('facebook')) app.get('/connect/facebook', passport.authorize('facebook')) app.get('/auth/facebook/callback', passport.authenticate('facebook', accountRedirects)) app.get('/connect/facebook/callback', passport.authorize('facebook', accountRedirects)) Succeeds:

app.get('/auth/facebook', passport.authenticate('facebook')) app.get('/connect/facebook', passport.authorize('facebook')) app.get('/auth/facebook/callback', passport.authenticate('facebook', { successRedirect: '/profile', failureRedirect: '/' })) app.get('/connect/facebook/callback', passport.authorize('facebook', { successRedirect: '/profile', failureRedirect: '/' })) — Reply to this email directly or view it on GitHub.

[CrabDude]

Sure, but not ATM. Perhaps in the coming month. Feel free to ping me here if I forget.

[ig3]

I have had a similar problem using passport-azure-ad-oauth2. This inherits from passport-oauth2 but it overrides authorizationParams to

Strategy.prototype.authorizationParams = function (options) {
  return options;
};

Where the original in passport-oauth2 returns an empty object literal. The overridden version returns a reference to the options object which is then modified to form parameters for the request to the authenticating server, which clobbers options, causing problems in subsequent attempts to authenticate.

This can be fixed in passport-azure-ad-oauth2 by returning a clone, rather than a reference to the options object, or by not overriding the method at all, as it isn't obvious (to me) if or when one might actually want options to the authenticate strategy to be mingled with parameters to the request to the authorizing server. In my case, the options object is and should remain empty.

An alternative would be to pass a clone of the options object to strategy.authenticate, in passport/lib/middleware/authenticate.js. This would have the advantage that it would solve the problem in any other strategies that do the same as passport-azure-ad-oauth2.