fastify / fastify-oauth2

Enable to perform login using oauth2 protocol
MIT License
253 stars 73 forks source link

Don't set cookie if using custom state generation and verification #245

Closed fa-sharp closed 9 months ago

fa-sharp commented 10 months ago

Prerequisites

🚀 Feature Proposal

I propose that the library shouldn't set a cookie if a user is implementing their own custom generateStateFunction and checkStateFunction. Presumably, if a user is implementing these functions on their own, we should assume that they've come up with their own way of generating and verifying state.

Motivation

I know the cookie behavior was added due to a security vulnerability when using the default configuration. This makes total sense. But if someone is implementing their own state generation and verification functions, we should assume that they have come up with their own way to do it, and at that point, security is in their hands.

This is the case in one of my own applications, and I'm noticing that those 'oauth2-redirect-state' cookies are still being created every time a user logs in, even though my application doesn't use them (I'm using redis and sessions to do the verification, as suggested in the docs). I don't think it makes sense to still have those cookies being set by the library in this scenario.

Of course, this would be a breaking change, because some folks might be relying on this cookie behavior even with their custom functions. So this should be considered before moving forward.

An alternative would be to make it an option to disable the cookie functionality. But I think this would add complexity and is not ideal IMO. It would be better to keep it very simple:

Default configuration -> state cookie is set and verified automatically for you Custom state functions -> you're on your own, no cookie is set

Example

As indicated in the docs, when using custom state functions, an application would probably be using its own cookies/sessions to implement state verification:

generateStateFunction: async function (request) {
      const state = request.query.customCode
      await this.redis.set(stateKey, state)
      return state
},
checkStateFunction: async function (request, callback) {
     if (request.query.state !== request.session.state) {
        throw new Error('Invalid state')
     }
    return true
}

The oauth2-redirect-state cookie isn't needed in this scenario, as we have our own session/cookie implementation doing the work.

mcollina commented 10 months ago

I'm ok with it as long as we offer some utility to implement this easily for people that wants to use the functions and use the cookie.

fa-sharp commented 10 months ago

@mcollina It's easy to recreate the same behavior by using the @fastify/cookie library in the custom functions. If I added a clear example of how to do that in the docs, would that be enough? So there would be an example of custom functions using @fastify/cookie, and then another example of using sessions.

mcollina commented 10 months ago

I think so.

fa-sharp commented 9 months ago

It seems there isn't a simple way to do this unfortunately. Since the state functions only get the request object as a parameter, users won't be able to set a cookie from there. And I'm not sure it would be worth making more changes to the API.

I don't think it's a big deal to have those cookies being created, so I'll go ahead and close this.