search

jmreyes / passport-google-id-token

Google ID token authentication strategy for Passport and Node.js.
MIT License
44 stars 22 forks source link

exception thrown when token.data.aud doesn't match clientID or token is expired #5

Closed tomhoag closed 8 years ago

tomhoag commented 8 years ago

Observed behavior: When an id_token doesn't match the clientID or the id_token is expired, an exception is thrown. (stack trace at the bottom of this issue)

Expected behavior: When an id_token doesn't match the clientID or the id_token is expired, return an informative error.

Notes:

In strategy.js, when parser.decode (line 145) calls the callback function(err,token)

1) without an err == null and a parsed token 2) either/both of aud_valid or not_expired are false 3) the done callback is called with a null err.

GoogleTokenStrategy.prototype._verifyGoogleToken = function(parser, idToken, clientID, done) {
  parser.decode(idToken, function(err, token) { // line 145
    if(err) { // err is null
      done(err);
    } else {
      var aud_valid = (token.data.aud === clientID);
      var not_expired = (token.data.exp <= (new Date().getTime()));
      if (not_expired && aud_valid) { // if this evaluates to false
        done(err, token); 
      } else {
        done(err); // done is invoked with a null err 
      }
    }
  });
}

This leads to a problem at line 131 (and 129) in GoogleTokenStrategy.prototype.authenticate, when calling the _verify callback as parsedToken is null and an exception is thrown when attempting to access parsedToken.data.sub

GoogleTokenStrategy.prototype.authenticate = function(req, options) {

//<snip>

self._verifyGoogleToken(parser, idToken, self._clientID, function(err, parsedToken) { // line 119
    if (err) { return self.fail(err); }; // err is null

    function verified(err, parsedToken, info) { 
      if (err) { return self.error(err); }
      if (!parsedToken) { return self.fail(info); }
      self.success(parsedToken, info);
    }

    if (self._passReqToCallback) {
      self._verify(req, parsedToken, parsedToken.data.sub, verified); // error thrown here attempting to access parsedToken.data.sub as parsedToken is undefined.
    } else {
      self._verify(parsedToken, parsedToken.data.sub, verified); // error thrown here attempting to access parsedToken.data.sub as parsedToken is undefined.
    }
  });

Stack trace:

/Users/hobbes/Projects/calvin/node_modules/passport-google-id-token/lib/passport-google-id-token/strategy.js:131
      self._verify(parsedToken, parsedToken.data.sub, verified);
                                           ^

TypeError: Cannot read property 'data' of undefined
    at /Users/hobbes/Projects/calvin/node_modules/passport-google-id-token/lib/passport-google-id-token/strategy.js:131:44
    at /Users/hobbes/Projects/calvin/node_modules/passport-google-id-token/lib/passport-google-id-token/strategy.js:154:9
    at /Users/hobbes/Projects/calvin/node_modules/passport-google-id-token/node_modules/google-id-token/index.js:53:5
    at /Users/hobbes/Projects/calvin/node_modules/passport-google-id-token/node_modules/google-id-token/index.js:66:5
    at Request._callback (/Users/hobbes/Projects/calvin/node_modules/passport-google-id-token/lib/passport-google-id-token/strategy.js:26:7)
    at Request.self.callback (/Users/hobbes/Projects/calvin/node_modules/passport-google-id-token/node_modules/request/request.js:198:22)
    at emitTwo (events.js:87:13)
    at Request.emit (events.js:172:7)
    at Request.<anonymous> (/Users/hobbes/Projects/calvin/node_modules/passport-google-id-token/node_modules/request/request.js:1063:14)
    at emitOne (events.js:82:20)
    at Request.emit (events.js:169:7)
    at IncomingMessage.<anonymous> (/Users/hobbes/Projects/calvin/node_modules/passport-google-id-token/node_modules/request/request.js:1009:12)
    at emitNone (events.js:72:20)
    at IncomingMessage.emit (events.js:166:7)
    at endReadableNT (_stream_readable.js:903:12)
    at doNTCallback2 (node.js:439:9)
tomhoag commented 8 years ago

My fix for now is the following:

GoogleTokenStrategy.prototype._verifyGoogleToken = function(parser, idToken, clientID, done) {

  var self = this; // add this
  parser.decode(idToken, function(err, token) {

    if(err) {
      done(err);
    } else {
      var aud_valid = (token.data.aud === clientID);
      var not_expired = (token.data.exp <= (new Date().getTime()));
      if (not_expired && aud_valid) {
        done(err, token);
      } else {
// done(err) // remove this 
// begin add
        if(!not_expired) {
            return self.fail("id_token expired", 401);
        }else if(!aud_valid) {
            return self.fail("id_token clientID mismatch", 401);
        }
      }
// end add
    }
  });
}

I'm calling self.fail so that the message and status will be propagated to the caller. Simply calling done with a new Error() loses the status and message.

With the code change above, passport will return a user set to false so I can do something like:

   if(!user) { // if there is no user, something went wrong . . .
    return res.json(status, info);
  }

Note that the status is delivered in the fourth parameter of the callback supplied to passport.authenticate i.e. 

passport.authenticate('google-id-token', {session:false}, function(err, user, info, status) { . . . }

Let me know if the fix is appropriate and I'll submit a pull request.

jmreyes commented 8 years ago

Hi, thanks a lot for such a detailed issue!

About your fix, I'd rather create relevant Errors at those points instead of failing directly, just to let the fail() function be called at the same point for all error cases. I've been having a look and this seems to be more similar to what other passport strategies do.

Something along these lines:

GoogleTokenStrategy.prototype._verifyGoogleToken = function(parser, idToken, clientID, done) {
...
        if(!not_expired) {
            done(new Error('id_token expired'));
        }else if(!aud_valid) {
            done(new Error('id_token clientID mismatch'));
        }
...
}

I hope this is okay with you, thanks again!

tomhoag commented 8 years ago

Yes -- I've been looking more closely and I agree its better to follow the done() pattern. I'll take a look and let you know what I find. Calling done rather than fail also allows other chained strategies to execute I think (?)

More soon . . . .

tomhoag commented 8 years ago

I looked into this a bit further and think that a better solution is 

GoogleTokenStrategy.prototype._verifyGoogleToken = function(parser, idToken, clientID, done) {
. . . 
        if(!not_expired) {
          done(null, false, {message: "id_token expired"});
        } else if(!aud_valid) {
          done(null, false, {message: "id_token clientID mismatch"});
        }
. . .
}

I'm passing null as the first parameter as an error as the first argument would indicate that the verification failed due to a system error (e.g. database not accessible). Passing false as the second parameter (the userID) done(null, false, message) indicates that the id_token couldn't be validated. Passing in the message as the third parameter is kinda pointless as the message is lost in the subsequent callbacks, but it serves as some in-line documentation . . .

Also, the following should be changed to avoid the error that started this whole thread

self._verifyGoogleToken(parser, idToken, self._clientID, function(err, parsedToken, message) {
. . .
    if (self._passReqToCallback) {
      self._verify(req, parsedToken, parsedToken.data.sub, verified);
    } else {
        if(parsedToken) {
            self._verify(parsedToken, parsedToken.data.sub, verified);
        } else {
            self._verify(false, null, verified);
        }
    }
. . . 
}
jmreyes commented 8 years ago

Yes, you're right, much better that way. This really is more tricky than I thought in the first place. I'll definitely try to write some tests in the near future.

Now there's yet another thing that pops up: I think we shouldn't be calling verify() with false/null parsedToken/googleId. If we take a look at Local or HTTP Bearer strategies, it seems they are failing early, so they never send the application null/false values.

I think that's what we should do as well, passing the info message you mention as the parameter to info() so that it doesn't get lost. So basically I'd follow your suggestion for _verifyGoogleToken, and also fail early before self._verify to enforce a valid googleId inside the application's callback.

self._verifyGoogleToken(parser, idToken, self._clientID, function(err, parsedToken, message) {
...
    if (!parsedToken) { return self.fail(message); }

    if (self._passReqToCallback) {
      self._verify(req, parsedToken, parsedToken.data.sub, verified);
    } else {
      self._verify(parsedToken, parsedToken.data.sub, verified);
    }
...
}
tomhoag commented 8 years ago

Yes, more tricky than I'd originally thought as well. I agree on failing early which also allows the message to propagate through the callbacks. Here's what I've put together based on our conversation:

GoogleTokenStrategy.prototype.authenticate = function(req, options) {
...
self._verifyGoogleToken(parser, idToken, self._clientID, function(err, parsedToken, message) {
    if (err) { return self.fail(err); };

    if (!parsedToken) { return self.fail(message);}

    function verified(err, parsedToken, info) {
      if (err) { return self.error(err); }
      if (!parsedToken) { return self.fail(info); }
      self.success(parsedToken, info);
    }

    if (self._passReqToCallback) {
      self._verify(req, parsedToken, parsedToken.data.sub, verified);
    } else {
      self._verify(parsedToken, parsedToken.data.sub, verified);
    }
  });
}

and

GoogleTokenStrategy.prototype._verifyGoogleToken = function(parser, idToken, clientID, done) {
  parser.decode(idToken, function(err, token) {
    if(err) {
      done(err);
    } else {
      var aud_valid = (token.data.aud === clientID);
      var not_expired = (token.data.exp <= (new Date().getTime()));
      if (not_expired && aud_valid) {
        done(err, token);
      } else {
        if(!not_expired) {
          done(null, false, {message: "id_token expired"});
        } else if(!aud_valid) {
          done(null, false, {message: "id_token clientID mismatch"});
        }
      }
    }
  });
}

When calling the done callback, I'm passing null as the first parameter to re-enforce that there was no error and false as the second parameter to flag that the token was not valid. I think this is in line with what passport expects (?)

but I may be confusing the issue here :)

jmreyes commented 8 years ago

Awesome! Yes, I also think that's the way to go (the done(null, false, ..) thing as well). I can add this later today/tomorrow, or you can create a PR, as you see fit!

tomhoag commented 8 years ago

Thanks for putting this in -- sorry I didn't get the PR done more quickly.

tom

jmreyes commented 8 years ago

No worries, I wanted to fix some other things as well, thank you! Have a look at 1acef69, I found that the expiration logic was wrong...

tomhoag commented 8 years ago

yikes -- nice catch!!

PieterJanVdb commented 8 years ago

By the way, the exp field on the parsed token is in seconds, while new Date().getTime()is in milliseconds. Furthermore, shouldn't it be token.data.exp >= (new Date().getTime() / 1000)instead of token.data.exp <= (new Date().getTime() / 1000) ? Right now <=works because it will always be smaller considering now Date().getTime()is in milliseconds, but it doesn't really check if token.data.exp is expired.

I could be wrong though.

EDIT: I just noticed I was working on 0.3.0, it's fixed on 0.3.1. My bad!

jmreyes commented 8 years ago

Yes, I referenced the wrong commit up there. Edited.