joselfonseca / lighthouse-graphql-passport-auth

Add GraphQL mutations to get tokens from passport for https://lighthouse-php.com/
https://lighthouse-php-auth.com/
MIT License
228 stars 56 forks source link

Login fails when expires_in is greater than 32bit int #134

Closed SlyDave closed 1 year ago

SlyDave commented 3 years ago

If the expires_in number of seconds is larger than a 32bit int the Login fails with the following error

"Expected a value of type \"Int\" but received: 3155673599"

GraphQL enforces Int to be 32bit, but the definition of expires_in for Passport is limited to PHP's Int which is either 32bit or 64bit depending on the underlying architecture support.

If the number of seconds between now() and the expires_at column of the oauth_refresh_tokens table results in a number of seconds greater than a 32bit Int, An error will occur.

There is a Long for GraphQL, but this only supports 52bits (max that JavaScript actually supports).

I don't think there is an actual viable non-data solution here.

Some options to work around this are:

There is another option:

Adding expires_at to the AuthPayload and RefreshTokenPayload would allow getting the expiry date of the token without converting it into an Int, which would then avoid the error, but allow the consumer to know when the expiry is when the value is greater than 32bit seconds away. -- This would return a custom Resolver as I believe this field is provided by Passport?

SlyDave commented 3 years ago

Another option is to not use Type Int for expires_in but rather use Type ID. This is after all what Lighthouse does for the primary key's for models which are more than likely Auto-incrementing UNSIGNED BIGINT (primary key) equivalent columns... but that feels real ugly.

joselfonseca commented 3 years ago

Thanks @SlyDave Very interesting issue, Could you please give me a little more information on how you ran into this? We've never seen this issue, and yes the expires_in is returned by Passport and I believe is the seconds from the creation of the token, it could be turned out into a date. Our expirations are usually 30 days so that number is not usually big.

Do you think someone needs to set the expiration in 68 years?

SlyDave commented 3 years ago

Discovered through the fact that a previous developer on our project set up the default expiry for tokens in AuthServiceProvider for 100 years... /sigh

        Passport::tokensExpireIn(Carbon::now()->addYears(100));

        Passport::refreshTokensExpireIn(Carbon::now()->addYear(100));

Which is massively excessive, I'll be running a script to fix all our tokens to ensure they're set to something sensible - in our use case 1 year is plenty, 30 would be nicer, but there are reasons we're not going lower.

I do not think anyone should be setting tokens with such an expiration, it's an obvious security flaw, and undermines a feature of OAuth best practises.

As such, maybe, this ticket just serves as a warning to others, and an explanation for those that encounter the issue.

joselfonseca commented 3 years ago

Understood. However I feel that having the date like you suggested would be nice. I may add it. Thank you for the information.

joselfonseca commented 1 year ago

Closing due to inactivity