Skip to content

Commit 7926e5b

Browse files
sixcolorssixcolors
authored
Merge pull request from GHSA-98j2-3j3p-fw2v
* fix: token injection vulnerability GHSA-98j2-3j3p-fw2v - Ensure session IDs are securely generated server-side. - Add validation to prevent user-supplied session IDs. - Update tests to verify correct session token use. This update addresses the critical session middleware vulnerability identified in versions 2 and above of GoFiber. * test(middleware/csrf): Save session after generating new session ID This commit saves the session after generating a new session ID to ensure that the updated session ID is persisted. This change is necessary to address a critical session middleware vulnerability identified in versions 2 and above of GoFiber. * chore: Save session ID in context for middleware chain The code changes add functionality to save the newly generated session ID in the context, allowing it to be accessible to subsequent middlewares in the chain. This improvement ensures that the session ID is available for use throughout the middleware stack. * test: Fix session freshness check in session_test The code changes in `session_test.go` fix the session freshness check by updating the assertions for `sess.Fresh()` and `sess.ID()`. The previous assertions were incorrect and have been corrected to ensure the session ID remains the same and the session is not fresh. * refactor(session.go): general clean-up * chore: Revert session freshness behavior The code changes in `session_test.go` fix the session freshness check by updating the assertions for `sess.Fresh()` and `sess.ID()`. The previous assertions were incorrect and have been corrected to ensure the session ID remains the same and the session is not fresh.
1 parent 4262f5b commit 7926e5b

File tree

4 files changed

+144
-74
lines changed

4 files changed

+144
-74
lines changed

‎middleware/csrf/csrf_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,8 @@ func Test_CSRF_WithSession(t *testing.T) {
8888

8989
// the session string is no longer be 123
9090
newSessionIDString := sess.ID()
91+
sess.Save()
92+
9193
app.AcquireCtx(ctx).Request().Header.SetCookie("_session", newSessionIDString)
9294

9395
// middleware config
@@ -221,6 +223,8 @@ func Test_CSRF_ExpiredToken_WithSession(t *testing.T) {
221223

222224
// get session id
223225
newSessionIDString := sess.ID()
226+
sess.Save()
227+
224228
app.AcquireCtx(ctx).Request().Header.SetCookie("_session", newSessionIDString)
225229

226230
// middleware config

‎middleware/session/session_test.go

Lines changed: 84 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,23 @@ func Test_Session(t *testing.T) {
2525
ctx := app.AcquireCtx(&fasthttp.RequestCtx{})
2626
defer app.ReleaseCtx(ctx)
2727

28+
// Get a new session
29+
sess, err := store.Get(ctx)
30+
utils.AssertEqual(t, nil, err)
31+
utils.AssertEqual(t, true, sess.Fresh())
32+
token := sess.ID()
33+
sess.Save()
34+
35+
app.ReleaseCtx(ctx)
36+
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
37+
2838
// set session
29-
ctx.Request().Header.SetCookie(store.sessionName, "123")
39+
ctx.Request().Header.SetCookie(store.sessionName, token)
3040

3141
// get session
32-
sess, err := store.Get(ctx)
42+
sess, err = store.Get(ctx)
3343
utils.AssertEqual(t, nil, err)
34-
utils.AssertEqual(t, true, sess.Fresh())
44+
utils.AssertEqual(t, false, sess.Fresh())
3545

3646
// get keys
3747
keys := sess.Keys()
@@ -64,12 +74,14 @@ func Test_Session(t *testing.T) {
6474

6575
// get id
6676
id := sess.ID()
67-
utils.AssertEqual(t, "123", id)
77+
utils.AssertEqual(t, token, id)
6878

6979
// save the old session first
7080
err = sess.Save()
7181
utils.AssertEqual(t, nil, err)
7282

83+
app.ReleaseCtx(ctx)
84+
7385
// requesting entirely new context to prevent falsy tests
7486
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
7587
defer app.ReleaseCtx(ctx)
@@ -108,7 +120,6 @@ func Test_Session_Types(t *testing.T) {
108120

109121
// fiber context
110122
ctx := app.AcquireCtx(&fasthttp.RequestCtx{})
111-
defer app.ReleaseCtx(ctx)
112123

113124
// set cookie
114125
ctx.Request().Header.SetCookie(store.sessionName, "123")
@@ -120,6 +131,10 @@ func Test_Session_Types(t *testing.T) {
120131

121132
// the session string is no longer be 123
122133
newSessionIDString := sess.ID()
134+
135+
app.ReleaseCtx(ctx)
136+
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
137+
123138
ctx.Request().Header.SetCookie(store.sessionName, newSessionIDString)
124139

125140
type User struct {
@@ -177,6 +192,11 @@ func Test_Session_Types(t *testing.T) {
177192
err = sess.Save()
178193
utils.AssertEqual(t, nil, err)
179194

195+
app.ReleaseCtx(ctx)
196+
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
197+
198+
ctx.Request().Header.SetCookie(store.sessionName, newSessionIDString)
199+
180200
// get session
181201
sess, err = store.Get(ctx)
182202
utils.AssertEqual(t, nil, err)
@@ -203,6 +223,7 @@ func Test_Session_Types(t *testing.T) {
203223
utils.AssertEqual(t, vfloat64, sess.Get("vfloat64").(float64))
204224
utils.AssertEqual(t, vcomplex64, sess.Get("vcomplex64").(complex64))
205225
utils.AssertEqual(t, vcomplex128, sess.Get("vcomplex128").(complex128))
226+
app.ReleaseCtx(ctx)
206227
}
207228

208229
// go test -run Test_Session_Store_Reset
@@ -214,7 +235,6 @@ func Test_Session_Store_Reset(t *testing.T) {
214235
app := fiber.New()
215236
// fiber context
216237
ctx := app.AcquireCtx(&fasthttp.RequestCtx{})
217-
defer app.ReleaseCtx(ctx)
218238

219239
// get session
220240
sess, err := store.Get(ctx)
@@ -228,6 +248,12 @@ func Test_Session_Store_Reset(t *testing.T) {
228248

229249
// reset store
230250
utils.AssertEqual(t, nil, store.Reset())
251+
id := sess.ID()
252+
253+
app.ReleaseCtx(ctx)
254+
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
255+
defer app.ReleaseCtx(ctx)
256+
ctx.Request().Header.SetCookie(store.sessionName, id)
231257

232258
// make sure the session is recreated
233259
sess, err = store.Get(ctx)
@@ -302,25 +328,37 @@ func Test_Session_Save_Expiration(t *testing.T) {
302328
// set value
303329
sess.Set("name", "john")
304330

331+
token := sess.ID()
332+
305333
// expire this session in 5 seconds
306334
sess.SetExpiry(sessionDuration)
307335

308336
// save session
309337
err = sess.Save()
310338
utils.AssertEqual(t, nil, err)
311339

340+
app.ReleaseCtx(ctx)
341+
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
342+
312343
// here you need to get the old session yet
344+
ctx.Request().Header.SetCookie(store.sessionName, token)
313345
sess, err = store.Get(ctx)
314346
utils.AssertEqual(t, nil, err)
315347
utils.AssertEqual(t, "john", sess.Get("name"))
316348

317349
// just to make sure the session has been expired
318350
time.Sleep(sessionDuration + (10 * time.Millisecond))
319351

352+
app.ReleaseCtx(ctx)
353+
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
354+
defer app.ReleaseCtx(ctx)
355+
320356
// here you should get a new session
357+
ctx.Request().Header.SetCookie(store.sessionName, token)
321358
sess, err = store.Get(ctx)
322359
utils.AssertEqual(t, nil, err)
323360
utils.AssertEqual(t, nil, sess.Get("name"))
361+
utils.AssertEqual(t, true, sess.ID() != token)
324362
})
325363
}
326364

@@ -364,7 +402,15 @@ func Test_Session_Destroy(t *testing.T) {
364402

365403
// set value & save
366404
sess.Set("name", "fenny")
405+
id := sess.ID()
367406
utils.AssertEqual(t, nil, sess.Save())
407+
408+
app.ReleaseCtx(ctx)
409+
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
410+
defer app.ReleaseCtx(ctx)
411+
412+
// get session
413+
ctx.Request().Header.Set(store.sessionName, id)
368414
sess, err = store.Get(ctx)
369415
utils.AssertEqual(t, nil, err)
370416

@@ -408,7 +454,8 @@ func Test_Session_Cookie(t *testing.T) {
408454
}
409455

410456
// go test -run Test_Session_Cookie_In_Response
411-
func Test_Session_Cookie_In_Response(t *testing.T) {
457+
// Regression: https://github.com/gofiber/fiber/pull/1191
458+
func Test_Session_Cookie_In_Middleware_Chain(t *testing.T) {
412459
t.Parallel()
413460
store := New()
414461
app := fiber.New()
@@ -421,15 +468,17 @@ func Test_Session_Cookie_In_Response(t *testing.T) {
421468
sess, err := store.Get(ctx)
422469
utils.AssertEqual(t, nil, err)
423470
sess.Set("id", "1")
471+
id := sess.ID()
424472
utils.AssertEqual(t, true, sess.Fresh())
425473
utils.AssertEqual(t, nil, sess.Save())
426474

427475
sess, err = store.Get(ctx)
428476
utils.AssertEqual(t, nil, err)
429477
sess.Set("name", "john")
430478
utils.AssertEqual(t, true, sess.Fresh())
479+
utils.AssertEqual(t, id, sess.ID()) // session id should be the same
431480

432-
utils.AssertEqual(t, "1", sess.Get("id"))
481+
utils.AssertEqual(t, sess.ID() != "1", true)
433482
utils.AssertEqual(t, "john", sess.Get("name"))
434483
}
435484

@@ -441,24 +490,31 @@ func Test_Session_Deletes_Single_Key(t *testing.T) {
441490
app := fiber.New()
442491

443492
ctx := app.AcquireCtx(&fasthttp.RequestCtx{})
444-
defer app.ReleaseCtx(ctx)
445493

446494
sess, err := store.Get(ctx)
447495
utils.AssertEqual(t, nil, err)
448-
ctx.Request().Header.SetCookie(store.sessionName, sess.ID())
449-
496+
id := sess.ID()
450497
sess.Set("id", "1")
451498
utils.AssertEqual(t, nil, sess.Save())
452499

500+
app.ReleaseCtx(ctx)
501+
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
502+
ctx.Request().Header.SetCookie(store.sessionName, id)
503+
453504
sess, err = store.Get(ctx)
454505
utils.AssertEqual(t, nil, err)
455506
sess.Delete("id")
456507
utils.AssertEqual(t, nil, sess.Save())
457508

509+
app.ReleaseCtx(ctx)
510+
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
511+
ctx.Request().Header.SetCookie(store.sessionName, id)
512+
458513
sess, err = store.Get(ctx)
459514
utils.AssertEqual(t, nil, err)
460515
utils.AssertEqual(t, false, sess.Fresh())
461516
utils.AssertEqual(t, nil, sess.Get("id"))
517+
app.ReleaseCtx(ctx)
462518
}
463519

464520
// go test -run Test_Session_Reset
@@ -475,6 +531,9 @@ func Test_Session_Reset(t *testing.T) {
475531
defer app.ReleaseCtx(ctx)
476532

477533
t.Run("reset session data and id, and set fresh to be true", func(t *testing.T) {
534+
t.Parallel()
535+
// fiber context
536+
ctx := app.AcquireCtx(&fasthttp.RequestCtx{})
478537
// a random session uuid
479538
originalSessionUUIDString := ""
480539

@@ -491,6 +550,9 @@ func Test_Session_Reset(t *testing.T) {
491550
err = freshSession.Save()
492551
utils.AssertEqual(t, nil, err)
493552

553+
app.ReleaseCtx(ctx)
554+
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
555+
494556
// set cookie
495557
ctx.Request().Header.SetCookie(store.sessionName, originalSessionUUIDString)
496558

@@ -524,6 +586,8 @@ func Test_Session_Reset(t *testing.T) {
524586
// Check that the session id is not in the header or cookie anymore
525587
utils.AssertEqual(t, "", string(ctx.Response().Header.Peek(store.sessionName)))
526588
utils.AssertEqual(t, "", string(ctx.Request().Header.Peek(store.sessionName)))
589+
590+
app.ReleaseCtx(ctx)
527591
})
528592
}
529593

@@ -551,6 +615,12 @@ func Test_Session_Regenerate(t *testing.T) {
551615
err = freshSession.Save()
552616
utils.AssertEqual(t, nil, err)
553617

618+
// release the context
619+
app.ReleaseCtx(ctx)
620+
621+
// acquire a new context
622+
ctx = app.AcquireCtx(&fasthttp.RequestCtx{})
623+
554624
// set cookie
555625
ctx.Request().Header.SetCookie(store.sessionName, originalSessionUUIDString)
556626

@@ -566,6 +636,9 @@ func Test_Session_Regenerate(t *testing.T) {
566636

567637
// acquiredSession.fresh should be true after regenerating
568638
utils.AssertEqual(t, true, acquiredSession.Fresh())
639+
640+
// release the context
641+
app.ReleaseCtx(ctx)
569642
})
570643
}
571644

0 commit comments

Comments
 (0)