Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for state in oauth scenarios #103

Closed
wants to merge 2 commits into from
Closed

Support for state in oauth scenarios #103

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07a0603
chore: add support for state
sameer-coder Dec 14, 2021
3302675
chore: session changed update
sameer-coder Dec 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the state property special?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can only think of adding an extra layer new Proxy() to support this use-case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

const stateKey = session[kObj].state.key
session[stateKey] = { state: session[kObj].state.value }
session.changed = true
} else {
session.changed = signingKeyRotated
}

return session
})

fastify.decorate('createSecureSession', (data) => new Session(data))

fastify.decorate('encodeSecureSession', (session) => {
const nonce = genNonce()

// store state
for (const [key, value] of Object.entries(session)) {
if (key.startsWith('oauth')) {
session[kObj].state = { key, value: value.state }
break
}
}

const msg = Buffer.from(JSON.stringify(session[kObj]))

const cipher = Buffer.allocUnsafe(msg.length + sodium.crypto_secretbox_MACBYTES)
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have checked already if we have state.

if (!session || (!session.changed && !hasState)) {
// nothing to do
request.log.trace('fastify-secure-session: there is no session or the session didn\'t change, leaving it as is')
next()
Expand Down