Merge pull request from GHSA-9wwp-q7wq-jx35

* Add anti session theft provisions Signed-off-by: Matteo Collina <[email protected]> * Update README.md Co-authored-by: James Sumners <[email protected]> Signed-off-by: Matteo Collina <[email protected]> * Apply suggestions from code review Signed-off-by: Manuel Spigolon <[email protected]> --------- Signed-off-by: Matteo Collina <[email protected]> Signed-off-by: Matteo Collina <[email protected]> Signed-off-by: Manuel Spigolon <[email protected]> Co-authored-by: James Sumners <[email protected]> Co-authored-by: Manuel Spigolon <[email protected]>
fastify · Apr 10, 2024 · 56d6664 · 56d6664
1 parent 7b6eebe
commit 56d6664
Show file tree

Hide file tree

Showing 6 changed files with 255 additions and 6 deletions.
diff --git a/README.md b/README.md
@@ -53,6 +53,9 @@ fastify.register(require('@fastify/secure-session'), {
   cookieName: 'my-session-cookie',
   // adapt this to point to the directory where secret-key is located
   key: fs.readFileSync(path.join(__dirname, 'secret-key')),
+  // the amount of time the session is considered valid; this is different from the cookie options
+  // and based on value wihin the session.
+  expiry: 24 * 60 * 60, // Default 1 day
   cookie: {
     path: '/'
     // options for setCookie, see https://github.com/fastify/fastify-cookie
@@ -365,9 +368,12 @@ fastify.get('/', (request, reply) => {
 })
 ```
 
-## TODO
+## Security Notice
 
-- [ ] add an option to just sign, and do not encrypt
+`@fastify/secure-session` stores the session within a cookie, and as a result an attacker could impersonate a user
+if the cookie is leaked. The maximum expiration time of the session is set by the `expiry` option, which has default
+1 day. Adjust this parameter accordingly.
+Moreover, to protect users from further attacks, all cookies are created as "http only" if not specified otherwise.
 
 ## License
 

diff --git a/index.js b/index.js
@@ -50,8 +50,13 @@ function fastifySecureSession (fastify, options, next) {
   for (const sessionOptions of options) {
     const sessionName = sessionOptions.sessionName || 'session'
     const cookieName = sessionOptions.cookieName || sessionName
+    const expiry = sessionOptions.expiry || 86401 // 24 hours
     const cookieOptions = sessionOptions.cookieOptions || sessionOptions.cookie || {}
 
+    if (cookieOptions.httpOnly === undefined) {
+      cookieOptions.httpOnly = true
+    }
+
     let key
     if (sessionOptions.secret) {
       if (Buffer.byteLength(sessionOptions.secret) < 32) {
@@ -120,7 +125,8 @@ function fastifySecureSession (fastify, options, next) {
     sessionNames.set(sessionName, {
       cookieName,
       cookieOptions,
-      key
+      key,
+      expiry
     })
 
     if (!defaultSessionName) {
@@ -139,7 +145,7 @@ function fastifySecureSession (fastify, options, next) {
       throw new Error('Unknown session key.')
     }
 
-    const { key } = sessionNames.get(sessionName)
+    const { key, expiry } = sessionNames.get(sessionName)
 
     // do not use destructuring or it will deopt
     const split = cookie.split(';')
@@ -184,8 +190,15 @@ function fastifySecureSession (fastify, options, next) {
       return null
     }
 
+    const parsed = JSON.parse(msg)
+    if ((parsed.__ts + expiry) * 1000 - Date.now() <= 0) {
+      // maximum validity is reached, resetting
+      log.debug('@fastify/secure-session: expiry reached')
+      return null
+    }
     const session = new Proxy(new Session(JSON.parse(msg)), sessionProxyHandler)
     session.changed = signingKeyRotated
+
     return session
   })
 
@@ -228,7 +241,7 @@ function fastifySecureSession (fastify, options, next) {
         const cookie = request.cookies[cookieName]
         const result = fastify.decodeSecureSession(cookie, request.log, sessionName)
 
-        request[sessionName] = new Proxy((result || new Session({})), sessionProxyHandler)
+        request[sessionName] = result || new Proxy(new Session({}), sessionProxyHandler)
       }
 
       next()
@@ -275,6 +288,10 @@ class Session {
     this[kCookieOptions] = null
     this.changed = false
     this.deleted = false
+
+    if (this[kObj].__ts === undefined) {
+      this[kObj].__ts = Math.round(Date.now() / 1000)
+    }
   }
 
   get (key) {
@@ -296,7 +313,12 @@ class Session {
   }
 
   data () {
-    return this[kObj]
+    const copy = {
+      ...this[kObj]
+    }
+
+    delete copy.__ts
+    return copy
   }
 
   touch () {

diff --git a/package.json b/package.json
@@ -32,6 +32,7 @@
   "homepage": "https://github.com/fastify/fastify-secure-session#readme",
   "devDependencies": {
     "@fastify/pre-commit": "^2.0.2",
+    "@sinonjs/fake-timers": "^11.2.2",
     "@types/node": "^20.1.0",
     "cookie": "^0.6.0",
     "fastify": "^4.0.0",

diff --git a/test/anti-reuse-15-min.js b/test/anti-reuse-15-min.js
@@ -0,0 +1,62 @@
+'use strict'
+
+const t = require('tap')
+const fastify = require('fastify')({ logger: false })
+const sodium = require('sodium-native')
+const FakeTimers = require('@sinonjs/fake-timers')
+const clock = FakeTimers.install({
+  shouldAdvanceTime: true,
+  now: Date.now()
+})
+
+const key = Buffer.alloc(sodium.crypto_secretbox_KEYBYTES)
+sodium.randombytes_buf(key)
+
+fastify.register(require('../'), {
+  key,
+  expiry: 15 * 60 // 15 minutes
+})
+
+fastify.post('/', (request, reply) => {
+  request.session.set('some', request.body.some)
+  request.session.set('some2', request.body.some2)
+  reply.send('hello world')
+})
+
+t.teardown(fastify.close.bind(fastify))
+t.plan(5)
+
+fastify.get('/', (request, reply) => {
+  const some = request.session.get('some')
+  const some2 = request.session.get('some2')
+  reply.send({ some, some2 })
+})
+
+fastify.inject({
+  method: 'POST',
+  url: '/',
+  payload: {
+    some: 'someData',
+    some2: { a: 1, b: undefined, c: 3 }
+  }
+}, (error, response) => {
+  t.error(error)
+  t.equal(response.statusCode, 200)
+  t.ok(response.headers['set-cookie'])
+
+  clock.jump('00:15:01') // default validity is 24 hours
+
+  fastify.inject({
+    method: 'GET',
+    url: '/',
+    headers: {
+      cookie: response.headers['set-cookie']
+    }
+  }, (error, response) => {
+    t.error(error)
+    t.same(JSON.parse(response.payload), {})
+    clock.reset()
+    clock.uninstall()
+    fastify.close()
+  })
+})
diff --git a/test/anti-reuse.js b/test/anti-reuse.js
@@ -0,0 +1,62 @@
+'use strict'
+
+const t = require('tap')
+const fastify = require('fastify')({ logger: false })
+const sodium = require('sodium-native')
+const FakeTimers = require('@sinonjs/fake-timers')
+const clock = FakeTimers.install({
+  shouldAdvanceTime: true,
+  now: Date.now()
+})
+
+const key = Buffer.alloc(sodium.crypto_secretbox_KEYBYTES)
+
+sodium.randombytes_buf(key)
+
+fastify.register(require('../'), {
+  key
+})
+
+fastify.post('/', (request, reply) => {
+  request.session.set('some', request.body.some)
+  request.session.set('some2', request.body.some2)
+  reply.send('hello world')
+})
+
+t.teardown(fastify.close.bind(fastify))
+t.plan(6)
+
+fastify.get('/', (request, reply) => {
+  const some = request.session.get('some')
+  const some2 = request.session.get('some2')
+  reply.send({ some, some2 })
+})
+
+fastify.inject({
+  method: 'POST',
+  url: '/',
+  payload: {
+    some: 'someData',
+    some2: { a: 1, b: undefined, c: 3 }
+  }
+}, (error, response) => {
+  t.error(error)
+  t.equal(response.statusCode, 200)
+  t.ok(response.headers['set-cookie'])
+  t.equal(response.headers['set-cookie'].split(';')[1].trim(), 'HttpOnly')
+
+  clock.jump('24:01:00') // default validity is 24 hours
+
+  fastify.inject({
+    method: 'GET',
+    url: '/',
+    headers: {
+      cookie: response.headers['set-cookie']
+    }
+  }, (error, response) => {
+    t.error(error)
+    t.same(JSON.parse(response.payload), {})
+    clock.reset()
+    clock.uninstall()
+  })
+})
diff --git a/test/http-only.js b/test/http-only.js
@@ -0,0 +1,96 @@
+'use strict'
+
+const tap = require('tap')
+const Fastify = require('fastify')
+const SecureSessionPlugin = require('../')
+const sodium = require('sodium-native')
+const key = Buffer.alloc(sodium.crypto_secretbox_KEYBYTES)
+sodium.randombytes_buf(key)
+
+tap.test('http-only override', async t => {
+  const fastify = Fastify({ logger: false })
+  t.teardown(fastify.close.bind(fastify))
+  t.plan(3)
+
+  await fastify.register(SecureSessionPlugin, {
+    key,
+    cookie: {
+      path: '/',
+      httpOnly: false
+    }
+  })
+
+  fastify.post('/login', (request, reply) => {
+    request.session.set('user', request.body.email)
+    reply.send('Welcome back!')
+  })
+
+  const loginResponse = await fastify.inject({
+    method: 'POST',
+    url: '/login',
+    payload: {
+      email: '[email protected]'
+    }
+  })
+
+  t.equal(loginResponse.statusCode, 200)
+  t.ok(loginResponse.headers['set-cookie'])
+  t.not(loginResponse.headers['set-cookie'].split(';')[1].trim(), 'HttpOnly')
+})
+
+tap.test('Override global options does not change httpOnly default', t => {
+  t.plan(8)
+  const fastify = Fastify()
+  fastify.register(SecureSessionPlugin, {
+    key,
+    cookieOptions: {
+      maxAge: 42,
+      path: '/'
+    }
+  })
+
+  fastify.post('/', (request, reply) => {
+    request.session.set('data', request.body)
+    request.session.options({ maxAge: 1000 * 60 * 60 })
+    reply.send('hello world')
+  })
+
+  t.teardown(fastify.close.bind(fastify))
+
+  fastify.get('/', (request, reply) => {
+    const data = request.session.get('data')
+
+    if (!data) {
+      reply.code(404).send()
+      return
+    }
+    reply.send(data)
+  })
+
+  fastify.inject({
+    method: 'POST',
+    url: '/',
+    payload: {
+      some: 'data'
+    }
+  }, (error, response) => {
+    t.error(error)
+    t.equal(response.statusCode, 200)
+    t.ok(response.headers['set-cookie'])
+    const { maxAge, path } = response.cookies[0]
+    t.equal(maxAge, 1000 * 60 * 60)
+    t.equal(response.headers['set-cookie'].split(';')[3].trim(), 'HttpOnly')
+    t.equal(path, '/')
+
+    fastify.inject({
+      method: 'GET',
+      url: '/',
+      headers: {
+        cookie: response.headers['set-cookie']
+      }
+    }, (error, response) => {
+      t.error(error)
+      t.same(JSON.parse(response.payload), { some: 'data' })
+    })
+  })
+})