diff --git a/review_r72_combined_test.go b/review_r72_combined_test.go new file mode 100644 index 0000000..7691e31 --- /dev/null +++ b/review_r72_combined_test.go @@ -0,0 +1,99 @@ +package traefikoidc + +import ( + "crypto/rand" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gorilla/sessions" +) + +// TestCombinedChunk_MixedGenerationIndexRejected is a regression for a +// combined-session read-path gap: loadFromCombinedCookies accepted any +// present chunk set without verifying each chunk's own stored index (i) and +// count (n). An interrupted save could leave a mixed-generation cookie set +// that reassembled into valid-looking but wrong session data. The read +// path must now reject a chunk whose metadata disagrees with its cookie +// position (clean re-auth) instead of silently decoding. +func TestCombinedChunk_MixedGenerationIndexRejected(t *testing.T) { + sm := createTestSessionManager(t) + defer sm.Shutdown() + + // Build a session large enough to split into multiple combined chunks. + req := httptest.NewRequest(http.MethodGet, "/", nil) + rw := httptest.NewRecorder() + session, err := sm.GetSession(req) + if err != nil { + t.Fatalf("get session: %v", err) + } + session.SetUserIdentifier("user@example.com") + if err := session.SetAuthenticated(true); err != nil { + t.Fatalf("mark authenticated: %v", err) + } + bulk := make([]byte, 8000) + _, _ = rand.Read(bulk) + session.mainSession.Values["bulk"] = string(bulk) + // Force combined storage so the large payload is split into multiple + // combined chunk cookies (matching the real combined-session path) + // rather than overflowing the single legacy main cookie. + session.useCombinedStorage = true + session.combinedChunks = make(map[int]*sessions.Session) + if err := session.Save(req, rw); err != nil { + t.Fatalf("save session: %v", err) + } + goodCookies := rw.Result().Cookies() + + // Confirm the save actually produced multiple chunks (so the test is + // meaningful) by attempting to load chunk 1. + if _, err := sm.store.Get(req, sm.combinedChunkCookieName(1)); err != nil { + t.Fatalf("expected a multi-chunk combined session, but chunk 1 is not loadable: %v", err) + } + + // Control: the original, unmodified cookie set must load. + okReq := httptest.NewRequest(http.MethodGet, "/", nil) + for _, c := range goodCookies { + okReq.AddCookie(c) + } + okSess, err := sm.GetSession(okReq) + if err != nil { + t.Fatalf("get session from original cookies: %v", err) + } + if okSess.GetUserIdentifier() != "user@example.com" { + t.Fatalf("original combined cookie set did not load user identifier") + } + + // Tamper: rewrite chunk 1 with an index that disagrees with its + // position, giving a "mixed-generation" set. + tamperReq := httptest.NewRequest(http.MethodGet, "/", nil) + for _, c := range goodCookies { + tamperReq.AddCookie(c) + } + ch1, err := sm.store.Get(tamperReq, sm.combinedChunkCookieName(1)) + if err != nil { + t.Fatalf("get chunk 1 for tampering: %v", err) + } + ch1.Values["i"] = 7 + tamperW := httptest.NewRecorder() + if err := ch1.Save(tamperReq, tamperW); err != nil { + t.Fatalf("save tampered chunk 1: %v", err) + } + loadReq := httptest.NewRequest(http.MethodGet, "/", nil) + for _, c := range goodCookies { + if c.Name == sm.combinedChunkCookieName(1) { + continue // replaced below with the tampered chunk + } + loadReq.AddCookie(c) + } + for _, c := range tamperW.Result().Cookies() { + loadReq.AddCookie(c) + } + + tamperedSess, err := sm.GetSession(loadReq) + if err != nil { + t.Fatalf("get session from tampered cookies: %v", err) + } + if got := tamperedSess.GetUserIdentifier(); got != "" { + t.Fatalf("mixed-generation combined cookie set loaded user %q, want clean re-auth (empty)", got) + } +} diff --git a/session.go b/session.go index b1f3672..5386777 100644 --- a/session.go +++ b/session.go @@ -1156,6 +1156,12 @@ func (sm *SessionManager) loadFromCombinedCookies(r *http.Request, sessionData * sm.logger.Debugf("Invalid combined cookie chunk count: %v", firstChunk.Values["n"]) return false } + // The first chunk must carry index 0; a set whose metadata disagrees with + // its cookie position is a mixed-generation set from an interrupted save. + if idx, idxOk := firstChunk.Values["i"].(int); !idxOk || idx != 0 { + sm.logger.Debugf("Combined cookie chunk 0 has invalid index: %v", firstChunk.Values["i"]) + return false + } // Load all chunks chunkSessions := make([]*sessions.Session, totalChunks) @@ -1168,6 +1174,16 @@ func (sm *SessionManager) loadFromCombinedCookies(r *http.Request, sessionData * sm.logger.Debugf("Missing combined cookie chunk %d", i) return false } + // Verify each chunk's own stored index/count match its cookie + // position. Without this a partial write can leave a cookie set of + // mixed generations that reassembles and decompresses into + // valid-looking but wrong session data. + idx, idxOk := chunk.Values["i"].(int) + n, nOk := chunk.Values["n"].(int) + if !idxOk || idx != i || !nOk || n != totalChunks { + sm.logger.Debugf("Combined cookie chunk %d has mismatched metadata (i=%v n=%v, want i=%d n=%d)", i, chunk.Values["i"], chunk.Values["n"], i, totalChunks) + return false + } chunkSessions[i] = chunk sessionData.combinedChunks[i] = chunk }