Support for state in oauth scenarios #103
Conversation
|
I don't understand this or why it's needed, could you please clarify? |
|
Can you please add a unit test? |
| @@ -117,14 +117,31 @@ module.exports = fp(function (fastify, options, next) { | |||
| } | |||
|
|
|||
| const session = new Session(JSON.parse(msg)) | |||
| session.changed = signingKeyRotated | |||
|
|
|||
| if (session[kObj].state) { | |||
There was a problem hiding this comment.
Why is the state property special?
There was a problem hiding this comment.
it is actually the problem of passport-oauth2 which directly mutating the request.session object.
The usage of fastify-secure-session only allow to use the get and set function to mutate the session.
There was a problem hiding this comment.
I can only think of adding an extra layer new Proxy() to support this use-case.
There was a problem hiding this comment.
@climba03003 yes, I think you are right. If passport-oauth2 uses get and set functions to mutate the session then this problem won't exist
| @@ -153,7 +170,8 @@ module.exports = fp(function (fastify, options, next) { | |||
| fastify.addHook('onSend', (request, reply, payload, next) => { | |||
| const session = request.session | |||
|
|
|||
| if (!session || !session.changed) { | |||
| const hasState = session && Object.values(session).some(v => (typeof v === 'object' && !!v.state)) | |||
There was a problem hiding this comment.
We have checked already if we have state.
|
@mcollina Maybe I have got this completely wrong! I was waiting it to be reviewed to see if to see if the implementation makes sense before adding a unit test. I have got a reproducible repo here: https://github.com/sameer-coder/secure-session-test |
|
Since the issue is with the way |
This PR tries to fix the issue reported in
fastify-passport(fastify/fastify-passport#376)Issue: When
stateis used in an oauth strategy along withfastify-secure-sessionthen thestateis not being encoded because we only encodesession[kObj]. As a result when the callback happens the local state does not exist.Fix: I have tried to add the
statetosession[kObj]before the encoding happens and restore it after decoding so that state verification is done correctly.I am not sure if this fix is the right way to do it. Any suggestions to improve the PR are welcome!