jaredhanson / passport-facebook

Facebook authentication strategy for Passport and Node.js.
https://www.passportjs.org/packages/passport-facebook/?utm_source=github&utm_medium=referral&utm_campaign=passport-facebook&utm_content=about
MIT License
1.29k stars 446 forks source link

Returning 500 error instead of redirecting to failureRedirect #100

Open Sicria opened 9 years ago

Sicria commented 9 years ago

I'm using the following code for my callback route.

app.get('/auth/facebook/callback', passport.authenticate('facebook', {
        successRedirect : '/profile',
        failureRedirect : '/',
        failureFlash : true
}));

It works, authentication works as expected, but one thing I just can't seem to understand is why if I visit /auth/facebook/callback?code=gfdgsdgfg it throws a 500 error and displays a stacktrace to the user?

Is there no way to use the failureRedirect?

Is there a work around or anything that can be done so that if the authentication fails (for any reason, mainly because a user has tampered with the url) it redirects the user to the failureRedirect location and gives them a generic error response.

cortezcristian commented 9 years ago

Same issue here. Turns out the applications return:

This authorization code has been used.

If you go to node_modules/passport-facebook/lib/strategy.js around line 195

Strategy.prototype.parseErrorResponse = function(body, status) {
  var json = JSON.parse(body);
  if (json.error && typeof json.error == 'object') {
    return new FacebookTokenError(json.error.message, json.error.type, json.error.code, json.error.error_subcode);
  }
  return OAuth2Strategy.prototype.parseErrorResponse.call(this, body, status);
};

Code enters into the first if statement cause json.error.message = "This authorization code has been used."

To be honest, no clue about this parsing an errored response but the login still works... not sure if it is about token expiration time, double request to auth, I feel your pain.

Sicria commented 9 years ago

Yes the login works, but if someone visits /auth/facebook/callback?code=anythinghere it will throw a 500 error and display the entire stacktrace to them, the only solution I can think of to avoid the user viewing the error information is to put the application behind an nginx server and handle the error there, but it would be easier if passport could redirect the user to the url supplied.

tellnes commented 9 years ago

You should not see the stack trace when you put the application in production mode (assuming you are using express), but it will still throw you a 500 error.

But 500 is wrong status code. It is not an internal server error but the user providing an invalid code. So probably a 400 or 404 status code would be correct.

I think this should be fixed in Passport, but you can easily catch this error your self and respond with the correct response.

srlowe commented 9 years ago

Having the same issue here. I feel a 400/404 would not be appropriate though. IMO passport should be forwarding to the failure redirect.

kri8or commented 9 years ago

Hi Sicria, stumbled up exactly the same issue, did you came up with a solution ?

ikb42 commented 9 years ago

Also seeing this, IMO failure is a failure and hence failure redirect should happen. (seeing this with google login in my case, not fb)

epoberezkin commented 8 years ago

It also happens if you add some wrong field in profile fields: {"error":"(#100) Tried accessing nonexisting field (abc) on node type (User)"}

koutsenko commented 8 years ago

middle of 2016. Any updates?

trestletech commented 8 years ago

The issue I had was that I was throwing an err if the user didn't exist -- done(new Error("Invalid username"), null); and that doesn't seem to be what Passport expects.

It supports the notion of a falsy user in done as another sign of failure. So the appropriate signature would be done(null, null) if you don't find a user.

That got things working for me.

Inateno commented 8 years ago

Same here, this sounds like something really basic and we are handling a lot of error from facebook like "This authorization code has been used."

And the "error page" is really bad for user experience.

jwerre commented 7 years ago

I'm having a similar issue that may be related though my setup is slightly different. The issue is that errors are not passed back to the authenticate callback so I'm not able to handle them.

router.get("/authorize/facebook/callback", function(req, res, next) {

    passport.authenticate("facebook", function(err, profile, options) {

        if (err) {
            return res.redirect("/500");
        }
        if (!profile) {
            return res.redirect("/404");
        }

    })(req, res, next);

});

Then, if I go to /authorize/facebook/callback?code=flubber my redirects are never reached and the following error is thrown:

error: {"name":"FacebookTokenError","message":"Invalid verification code format.","type":"OAuthException","code":100,"traceID":"FXjfQTrvjGS","status":500}
stack: FacebookTokenError: Invalid verification code format.
    at Strategy.parseErrorResponse (/node_modules/passport-facebook/lib/strategy.js:196:12)
    at Strategy.OAuth2Strategy._createOAuthError (/node_modules/passport-oauth2/lib/strategy.js:376:16)
    at /node_modules/passport-oauth2/lib/strategy.js:166:45
    at /node_modules/oauth/lib/oauth2.js:177:18
    at passBackControl (/node_modules/oauth/lib/oauth2.js:123:9)
    at IncomingMessage.<anonymous> (/node_modules/oauth/lib/oauth2.js:143:7)
    at emitNone (events.js:91:20)
    at IncomingMessage.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickDomainCallback (internal/process/next_tick.js:122:9)
oliver-moran commented 7 years ago

Strangely having pretty much the same experience as @jwerre except using the Google module.

If I go to http://canvass.prototype.website/auth/google/callback?code=flubber

Error
    at Strategy.OAuth2Strategy.parseErrorResponse (/var/lib/openshift/587529732d52711f6d000124/app-root/runtime/repo/node_modules/passport-oauth2/lib/strategy.js:329:12)
    at Strategy.OAuth2Strategy._createOAuthError (/var/lib/openshift/587529732d52711f6d000124/app-root/runtime/repo/node_modules/passport-oauth2/lib/strategy.js:376:16)
    at /var/lib/openshift/587529732d52711f6d000124/app-root/runtime/repo/node_modules/passport-oauth2/lib/strategy.js:166:45
    at /var/lib/openshift/587529732d52711f6d000124/app-root/runtime/repo/node_modules/oauth/lib/oauth2.js:191:18
    at passBackControl (/var/lib/openshift/587529732d52711f6d000124/app-root/runtime/repo/node_modules/oauth/lib/oauth2.js:132:9)
    at IncomingMessage.<anonymous> (/var/lib/openshift/587529732d52711f6d000124/app-root/runtime/repo/node_modules/oauth/lib/oauth2.js:157:7)
    at emitNone (events.js:91:20)
    at IncomingMessage.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
ianomad commented 7 years ago

Is there any resolution for this?

matthewerwin commented 7 years ago

The resolution is to pass a custom callback to passport.authenticate( ) and handle the error using the callback flow. The design of everything in Passport is based on the middleware approach so understand that means understanding a custom callback must be provided for anything other than the default handling.

Here is an example:

passport.use(new FacebookStrategy({
       clientID: settings.server.facebook.clientId,
       clientSecret: settings.server.facebook.clientSecret,
       callbackURL: settings.server.facebook.oAuthRedirect1,
       profileFields: ['displayName', 'email'],
       passReqToCallback: true
       }, 
    //done method is the passport.authenticate callback
    async (req, accessToken, refreshToken, profile, done) => { 
    try {
      var r = await myapi.authenticate(accessToken, profile); 
      if(!r.authorized) {
        done('unauthorized'); //error (calls the passport.authenticate callback)
      } else {
        done(null, { //no error (calls the passport.authenticate callback)
          token: r.token,
          fbid: profile.id,
          fb_access_token: accessToken,
          profile: profile
        });
      }
    }
    catch (e) {
      logger.error(e);
    }
    })
 );

This is the route that calls the above strategy and upon completion has the "done" handler defined which can provide custom logging, ensure the redirect occurs, whatever.

   router.get('/login/facebook/return',
   (req, res, next) => {
   return passport.authenticate('facebook', {
          failureRedirect: '/login',
          session: false
          },
       (err, user, info) => {
          if(err) {
            logger.error(err);
            res.redirect('/login');
          }
          else
          {
              next();
          }
      })(req, res, next);
  })

No matter what error you get from the facebook strategy (including invalid profile fields, invalid code, etc) will all result in logging it and redirecting to /login in the above coded scenario

pszabop commented 6 years ago

I tried the workaround suggested by @matthewerwin and I note a couple of things:

  1. if you are using a session, then in the success state in the /login/facebook/return route you have to call req.logIn(user, err => {...dosomething... }). From what I can tell from sparse documentation when you take over the authenticate function you have to do all the steps yourself. the next() in the above code doesn't appear to do anything. Also the options for successRedirect and failureRedirect appear to be inoperative as well.

  2. I note that there should be a check for the user as well, if the user is not there an appropriate step should be taken (e.g. res.redirect(/some_error_url).

  3. I am unable to get the error message unauthorized passed into the callback handler using passport-mocked. Unfortunate, as I'd like to pass the message back to the client. (I'm attempting implement account suspension for facebook users... I'd like to tell them suspended in the response text). passport-mocked has a bug

jakwuh commented 6 years ago

You can handle it in an express error handler:

import * as TokenError from 'passport-oauth2/lib/errors/tokenerror';
// ...
app.use((error, req, res, next) => {
        if (error instanceof HttpError) {
            res.status(error.httpCode).json({
                name: error.constructor.name,
                ...omit(error, 'name', 'stack', 'httpCode')
            });
        } else if (error instanceof TokenError) { // this is 
            res.redirect('/login/');
        } else {
            console.error(error);
            res.status(500).json({});
        }
    });
jayarjo commented 5 years ago

@matthewerwin thumbs up, but one will need to set req.user manually in that callback or it won't be available in the next middleware.

524c commented 2 years ago

in my case the problem is with Google, which in some cases terminates the connection abruptly.

GET /login/federated/google 302 8.502 ms - 0 error: InternalOAuthError: Failed to obtain access token at /projects/todos-express-google/node_modules/.pnpm/passport-openidconnect@0.1.1/node_modules/passport-openidconnect/lib/strategy.js:143:29 at /projects/todos-express-google/node_modules/.pnpm/oauth@0.9.15/node_modules/oauth/lib/oauth2.js:191:18 at ClientRequest. (/projects/todos-express-google/node_modules/.pnpm/oauth@0.9.15/node_modules/oauth/lib/oauth2.js:162:5) at ClientRequest.emit (node:events:527:28) at TLSSocket.socketErrorListener (node:_http_client:454:9) at TLSSocket.emit (node:events:527:28) at emitErrorNT (node:internal/streams/destroy:157:8) at emitErrorCloseNT (node:internal/streams/destroy:122:3) at processTicksAndRejections (node:internal/process/task_queues:83:21) { oauthError: Error: read ECONNRESET at TLSWrap.onStreamRead (node:internal/stream_base_commons:217:20) { errno: -54, code: 'ECONNRESET', syscall: 'read' } } GET /oauth2/redirect/google?state=t7JpZbiB%2FBnQeOphxeGFP1Kk&code=4%2F0AX4XfWiiFXSart1lbG05fb3OGScHbbHaY3ImXrM0mbvLqJ7umrOuGfAJ1SeSKSOZlfak3w&scope=profile+openid+https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fuserinfo.profile&authuser=0&prompt=consent 302 513.387 ms - 46 process.on handler Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client at new NodeError (node:internal/errors:372:5) at ServerResponse.setHeader (node:_http_outgoing:576:11) at ServerResponse.header (//todos-express-google/node_modules/.pnpm/express@4.16.4/node_modules/express/lib/response.js:767:10) at ServerResponse.location (/projects/todos-express-google/node_modules/.pnpm/express@4.16.4/node_modules/express/lib/response.js:884:15) at ServerResponse.redirect (/projects/todos-express-google/node_modules/.pnpm/express@4.16.4/node_modules/express/lib/response.js:922:18) at complete (/projects/todos-express-google/node_modules/.pnpm/passport@0.5.3/node_modules/passport/lib/middleware/authenticate.js:266:26) at /projects/todos-express-google/node_modules/.pnpm/passport@0.5.3/node_modules/passport/lib/middleware/authenticate.js:278:15 at pass (/projects/todos-express-google/node_modules/.pnpm/passport@0.5.3/node_modules/passport/lib/authenticator.js:428:14) at Authenticator.transformAuthInfo (/projects/todos-express-google/node_modules/.pnpm/passport@0.5.3/node_modules/passport/lib/authenticator.js:450:5) at /projects/todos-express-google/node_modules/.pnpm/passport@0.5.3/node_modules/passport/lib/middleware/authenticate.js:275:22 { code: 'ERR_HTTP_HEADERS_SENT' }

So the only solution was to catch the errors in the process that I can't intercept in the Express flow: // catch global uncaught exceptions

process.on('uncaughtException', function(err) {
  console.log('process.on handler');
  console.log(err);
});