From 05357bcc9dbf5cd8bd6a00099c79fcc86b6f143c Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Fri, 7 Aug 2026 20:04:00 +0200 Subject: [PATCH 1/3] fix: serialize AutoSession creation with Session.Close Associate an AutoSession with its Request before publishing it, and commit session cookies only while the exact Session remains registered and live. Share the cookie commit with manual and middleware-created Sessions so reentrant response writers cannot emit stale live cookies. Add deterministic WebSocket and middleware regressions for close-during-publication behavior. --- README.md | 4 +- jaws.go | 2 +- request.go | 35 ++++++++++---- request_test.go | 119 ++++++++++++++++++++++++++++++++++++++++++++++++ session.go | 79 +++++++++++++++++++++++--------- session_test.go | 107 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 312 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 06a02140..5c645b39 100644 --- a/README.md +++ b/README.md @@ -667,12 +667,12 @@ complete contract. ### Session handling -JaWS has non-persistent session handling integrated. Sessions won't +JaWS has non-persistent session handling integrated. Sessions won't be persisted across restarts and must have an expiry time. Use one of these patterns: -* Wrap page handlers with `Jaws.SessionMiddleware(handler)` to ensure a session exists. +* Wrap page handlers with `Jaws.SessionMiddleware(handler)` to create a session when none exists. * Call `Jaws.NewSession(w, r)` explicitly to create and attach a fresh session cookie. * Set `Jaws.AutoSession` to lazily create an anonymous session during a successful WebSocket upgrade when a Request has none. diff --git a/jaws.go b/jaws.go index db7ec0f6..7386d118 100644 --- a/jaws.go +++ b/jaws.go @@ -100,7 +100,7 @@ type Jid = jid.Jid // convenience alias // concurrency behavior and may be called concurrently when stated. type Jaws struct { CookieName string // Name for session cookies; defaults to a name derived from the executable ([assets.DefaultCookieName]), falling back to "jaws" - AutoSession bool // Create a session during a successful WebSocket upgrade when a Request has none. Defaults to false. + AutoSession bool // Create and associate a session during a successful WebSocket upgrade when a Request has none. Defaults to false. TrustForwardedHeaders bool // Trust X-Forwarded-* headers: governs the session cookie Secure flag (X-Forwarded-Proto) and the client IP used for session/request binding (X-Forwarded-For/X-Real-IP). Defaults to false; only enable behind a single reverse proxy you control that sets these headers. Logger Logger // Optional logger to use Debug bool // Set to true to enable debug info in generated HTML code. Call GenerateHeadHTML after changing it. diff --git a/request.go b/request.go index 760dff19..43b6ad20 100644 --- a/request.go +++ b/request.go @@ -364,17 +364,32 @@ func (rq *Request) sessionDestKey(sess *Session) (k key.Key) { } func (rq *Request) ensureAutoSession(w http.ResponseWriter, r *http.Request) { - if rq.Jaws.AutoSession && rq.Session() == nil { - sess := rq.Jaws.newSession(w, r) - rq.mu.Lock() - if rq.session == nil { - rq.session = sess - sess.addRequest(rq) + if rq.Jaws.AutoSession { + if sess := rq.newAutoSession(r); sess != nil { + sess.addCookie(w, r) } - rq.mu.Unlock() } } +// newAutoSession creates and associates an AutoSession before making it visible +// through Jaws session lookups. +func (rq *Request) newAutoSession(r *http.Request) (sess *Session) { + jw := rq.Jaws + secure := secureheaders.RequestIsSecure(r, jw.TrustForwardedHeaders) + remoteIP := jw.clientIP(r) + jw.mu.Lock() + defer jw.mu.Unlock() + rq.mu.Lock() + defer rq.mu.Unlock() + if rq.session == nil { + sess = jw.newSessionLocked(remoteIP, secure) + sess.addRequest(rq) + rq.session = sess + jw.sessions[sess.sessionID] = sess + } + return +} + // releaseBuffersLocked detaches the reusable storage from a finished Request and // returns it for the caller to return to [Jaws.requestBufferPool]. // @@ -1045,9 +1060,9 @@ func normalizedWebSocketAcceptRequest(r *http.Request) (normalized *http.Request // sets a session cookie. // // Accept writes the 101 through the [http.ResponseWriter] it was given before -// hijacking the connection, so the Set-Cookie header is in the header map in -// time. Should the hijack itself fail after the 101, the session already -// exists and expires through the normal session grace period. +// hijacking the connection, so a live session cookie can enter the header map +// in time. A session that survives this commit but whose hijack fails expires +// through the normal session grace period. type autoSessionWriter struct { http.ResponseWriter rq *Request diff --git a/request_test.go b/request_test.go index 99fd5205..e1e880e2 100644 --- a/request_test.go +++ b/request_test.go @@ -3189,6 +3189,125 @@ func TestWS_AutoSessionCreatesSession(t *testing.T) { } } +type autoSessionCloseResponseWriter struct { + http.ResponseWriter + jw *Jaws + closed chan<- *Session + once sync.Once + t *testing.T +} + +func (w *autoSessionCloseResponseWriter) Header() http.Header { + if sessions := w.jw.Sessions(); len(sessions) > 0 { + w.once.Do(func() { + done := make(chan struct{}) + go func(sess *Session) { + sess.Close() + w.closed <- sess + close(done) + }(sessions[0]) + select { + case <-done: + case <-time.After(testTimeout): + w.t.Error("Session.Close blocked while ResponseWriter.Header re-entered Jaws") + } + }) + } + return w.ResponseWriter.Header() +} + +func (w *autoSessionCloseResponseWriter) Unwrap() http.ResponseWriter { + return w.ResponseWriter +} + +func TestWS_AutoSessionCloseAtPublication(t *testing.T) { + ts := newTestServerNoSession(t) + defer ts.Close() + ts.jw.AutoSession = true + + closedCh := make(chan *Session, 1) + ts.srv.Close() + ts.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ts.ServeHTTP(&autoSessionCloseResponseWriter{ + ResponseWriter: w, + jw: ts.jw, + closed: closedCh, + t: t, + }, r) + })) + ts.setInitialRequestOrigin() + + connectSessionCh := make(chan *Session, 1) + ts.rq.SetConnectFn(func(rq *Request) error { + connectSessionCh <- rq.Session() + return nil + }) + + dialCtx, cancelDial := context.WithTimeout(t.Context(), testTimeout*2) + defer cancelDial() + hdr := http.Header{} + hdr.Set("Origin", ts.origin()) + conn, resp, err := websocket.Dial(dialCtx, ts.Url(), &websocket.DialOptions{HTTPHeader: hdr}) + if err != nil { + t.Fatal(err) + } + defer func() { _ = conn.CloseNow() }() + if resp.StatusCode != http.StatusSwitchingProtocols { + t.Fatalf("WebSocket status = %d, want %d", resp.StatusCode, http.StatusSwitchingProtocols) + } + + var closedSession *Session + select { + case closedSession = <-closedCh: + case <-time.After(testTimeout): + t.Fatal("ResponseWriter.Header did not close the published AutoSession") + } + + var connectSession *Session + select { + case connectSession = <-connectSessionCh: + case <-time.After(testTimeout): + t.Fatal("timeout waiting for WebSocket connect") + } + + const marker = "post-connect marker" + ts.jw.Broadcast(wire.Message{Dest: ts.rq.JawsKey, What: what.Alert, Data: marker}) + readCtx, cancelRead := context.WithTimeout(t.Context(), testTimeout) + defer cancelRead() + var messages strings.Builder + for !strings.Contains(messages.String(), marker) { + messageType, data, err := conn.Read(readCtx) + if err != nil { + t.Fatalf("reading WebSocket messages: %v (got %q)", err, messages.String()) + } + if messageType != websocket.MessageText { + t.Fatalf("WebSocket message type = %v, want text", messageType) + } + messages.Write(data) + } + + if connectSession != nil { + t.Errorf("ConnectFn Session() = %v, want nil after Session.Close", connectSession) + } + if got := ts.rq.Session(); got != nil { + t.Errorf("Request Session() = %v, want nil after Session.Close", got) + } + if requests := closedSession.Requests(); len(requests) != 0 { + t.Errorf("closed Session Requests() = %v, want none", requests) + } + if got := ts.jw.SessionCount(); got != 0 { + t.Errorf("SessionCount() = %d, want 0", got) + } + for _, cookie := range resp.Cookies() { + if cookie.Name == ts.jw.CookieName && cookie.MaxAge >= 0 { + t.Errorf("response contains live session cookie: %v", cookie) + } + } + if got := strings.Count(messages.String(), what.Reload.String()+"\t"); got != 1 { + t.Errorf("got %d Reload commands, want exactly 1: %q", got, messages.String()) + } +} + func TestWS_AutoSessionKeepsExistingSession(t *testing.T) { ts := newTestServer(t) defer ts.Close() diff --git a/session.go b/session.go index a1c945a3..1a266945 100644 --- a/session.go +++ b/session.go @@ -175,6 +175,33 @@ func (sess *Session) Cookie() (cookie *http.Cookie) { return } +// addCookie adds sess's cookie to w and r while sess is current and live. +func (sess *Session) addCookie(w http.ResponseWriter, r *http.Request) { + var h http.Header + if w != nil { + // ResponseWriter.Header is caller code and may re-enter Jaws, including + // by closing sess, so call it before taking either core lock below. + h = w.Header() + } + jw := sess.jw + jw.mu.RLock() + defer jw.mu.RUnlock() + sess.mu.RLock() + defer sess.mu.RUnlock() + if jw.sessions[sess.sessionID] == sess && !sess.isDeadLocked() { + cookie := sess.cookie + if h != nil { + if v := cookie.String(); v != "" { + // Header.Add and Request.AddCookie mutate concrete header maps + // without invoking caller code. Keep both under these read locks so + // cookie publication precedes a losing Session.Close. + h.Add("Set-Cookie", v) + } + } + r.AddCookie(&cookie) + } +} + // Close invalidates and expires the [Session]. // Future [Request] values won't be able to associate with it, and [Session.Cookie] will return a deletion cookie. // @@ -277,7 +304,10 @@ func (jw *Jaws) SessionCount() (n int) { return } -// Sessions returns a list of all active sessions, which may be nil. +// Sessions returns a snapshot of all registered sessions, which may be nil. +// +// Auto-created [Session] values are registered only after their initiating +// [Request] is associated. func (jw *Jaws) Sessions() (sessions []*Session) { jw.mu.RLock() if n := len(jw.sessions); n > 0 { @@ -362,9 +392,13 @@ func (jw *Jaws) GetSession(r *http.Request) (sess *Session) { // match used everywhere else; see [Jaws.GetSession] and [Jaws.TrustForwardedHeaders] // for the reverse-proxy caveat. // -// As a side effect, the session cookie is also added to r itself, so the new -// [Session] is visible to [Jaws.GetSession] and [Jaws.NewRequest] for the -// remainder of the same HTTP request. +// If the new [Session] remains current and live during cookie publication, its +// cookie is written to w when w is non-nil and added to r itself. This makes the +// [Session] visible to [Jaws.GetSession] and [Jaws.NewRequest] for the remainder +// of the same HTTP request. If a concurrent [Session.Close] wins first, neither +// w nor r receives its live cookie. +// +// It returns nil and has no effect if r is nil; w may be nil. // // It panics if the system CSPRNG ([crypto/rand]) fails while generating the session // ID, which does not happen on supported platforms. @@ -389,26 +423,26 @@ func (jw *Jaws) NewSession(w http.ResponseWriter, r *http.Request) (sess *Sessio func (jw *Jaws) newSession(w http.ResponseWriter, r *http.Request) (sess *Session) { secure := secureheaders.RequestIsSecure(r, jw.TrustForwardedHeaders) - var cookie http.Cookie func() { jw.mu.Lock() defer jw.mu.Unlock() - for sess == nil { - sessionID := jw.nonZeroRandomLocked() - if _, ok := jw.sessions[sessionID]; !ok { - sess = newSession(jw, sessionID, jw.clientIP(r), secure) - jw.sessions[sessionID] = sess - cookie = sess.cookie - } - } + sess = jw.newSessionLocked(jw.clientIP(r), secure) + jw.sessions[sess.sessionID] = sess }() + sess.addCookie(w, r) + return +} - // http.SetCookie calls the caller-provided ResponseWriter.Header, which may - // re-enter Jaws, so emit the cookie only after releasing jw.mu. - if w != nil { - http.SetCookie(w, &cookie) +// newSessionLocked allocates a Session whose ID is absent from jw.sessions. +// +// The caller must hold jw.mu and publish the Session before releasing it. +func (jw *Jaws) newSessionLocked(remoteIP netip.Addr, secure bool) (sess *Session) { + for sess == nil { + sessionID := jw.nonZeroRandomLocked() + if _, ok := jw.sessions[sessionID]; !ok { + sess = newSession(jw, sessionID, remoteIP, secure) + } } - r.AddCookie(&cookie) return } @@ -430,10 +464,13 @@ func (sess sessioner) ServeHTTP(w http.ResponseWriter, r *http.Request) { sess.h.ServeHTTP(w, r) } -// SessionMiddleware returns an [http.Handler] that ensures a JaWS [Session] -// exists before invoking h, creating one if the request has none. +// SessionMiddleware returns a session-creating [http.Handler]. +// +// Before invoking h, it creates a JaWS [Session] when the request has none. If +// a concurrent [Session.Close] wins the new Session's cookie publication, h +// runs without that Session or its live cookie. // -// It is the session-ensuring middleware, distinct from the session accessors: +// It is distinct from the session accessors: // [Jaws.GetSession] and [Request.Session] look up an existing [Session], while // this wraps a handler. It composes with [Jaws.SecureHeadersMiddleware]. func (jw *Jaws) SessionMiddleware(h http.Handler) http.Handler { diff --git a/session_test.go b/session_test.go index ab4f5005..e8e87b6e 100644 --- a/session_test.go +++ b/session_test.go @@ -132,6 +132,113 @@ func TestSession_NewSessionCallsResponseWriterOutsideLock(t *testing.T) { } } +func TestSession_NewSessionWithoutResponseWriter(t *testing.T) { + jw, err := New() + if err != nil { + t.Fatal(err) + } + t.Cleanup(jw.Close) + + hr := httptest.NewRequest(http.MethodGet, "http://example.test/", nil) + sess := jw.NewSession(nil, hr) + if sess == nil { + t.Fatal("NewSession returned nil") + } + if got := jw.GetSession(hr); got != sess { + t.Fatalf("GetSession() = %v, want %v", got, sess) + } + cookies := hr.Cookies() + if len(cookies) != 1 || cookies[0].Name != jw.CookieName || cookies[0].Value != sess.CookieValue() { + t.Fatalf("request cookies = %v, want the new Session cookie", cookies) + } +} + +type closingSessionResponseWriter struct { + *httptest.ResponseRecorder + jw *Jaws + closedSession *Session +} + +func (w *closingSessionResponseWriter) Header() http.Header { + if sessions := w.jw.Sessions(); len(sessions) > 0 { + w.closedSession = sessions[0] + w.closedSession.Close() + } + return w.ResponseRecorder.Header() +} + +func TestSessionMiddleware_CloseDuringResponseHeader(t *testing.T) { + jw, err := New() + if err != nil { + t.Fatal(err) + } + go jw.Serve() + waitForServeLoop(t, jw) + + rw := &closingSessionResponseWriter{ + ResponseRecorder: httptest.NewRecorder(), + jw: jw, + } + hr := httptest.NewRequest(http.MethodGet, "http://example.test/", nil) + type result struct { + rq *Request + requestCookies []*http.Cookie + handlerCalled bool + panicValue any + } + done := make(chan result, 1) + go func() { + var got result + defer func() { + got.panicValue = recover() + done <- got + }() + h := jw.SessionMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got.handlerCalled = true + got.requestCookies = r.Cookies() + got.rq = jw.NewRequest(r) + w.WriteHeader(http.StatusNoContent) + })) + h.ServeHTTP(rw, hr) + }() + + var got result + select { + case got = <-done: + case <-time.After(2 * time.Second): + t.Fatal("SessionMiddleware deadlocked while ResponseWriter.Header closed the Session") + } + t.Cleanup(jw.Close) + if got.panicValue != nil { + t.Fatalf("SessionMiddleware panicked while ResponseWriter.Header closed the Session: %v", got.panicValue) + } + if !got.handlerCalled { + t.Fatal("SessionMiddleware did not invoke the wrapped handler") + } + if rw.closedSession == nil { + t.Fatal("ResponseWriter.Header did not observe a published Session") + } + if sess := got.rq.Session(); sess != nil { + t.Errorf("new Request Session() = %v, want nil", sess) + } + if requests := rw.closedSession.Requests(); len(requests) != 0 { + t.Errorf("closed Session Requests() = %v, want none", requests) + } + if count := jw.SessionCount(); count != 0 { + t.Errorf("SessionCount() = %d, want 0", count) + } + for _, cookie := range got.requestCookies { + if cookie.Name == jw.CookieName { + t.Errorf("wrapped handler request contains closed session cookie: %v", cookie) + } + } + for _, cookie := range rw.Result().Cookies() { + if cookie.Name == jw.CookieName && cookie.MaxAge >= 0 { + t.Errorf("response contains live session cookie: %v", cookie) + } + } +} + func TestSession_NewSessionReplacesDuplicateCookieSessions(t *testing.T) { jw, err := New() if err != nil { From f3a02d252a8fa4be27239f2d45afd824a45d7c93 Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Fri, 7 Aug 2026 20:42:16 +0200 Subject: [PATCH 2/3] test: cover expired session cookie publication Exercise the deadline half of the live-session cookie guard and document why it is distinct from Close serialization. Apply the related review cleanups and remove obsolete recoverable CSPRNG failure claims. --- request_test.go | 7 +------ requestpool.go | 3 --- session.go | 17 +++++++---------- session_test.go | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 42 insertions(+), 19 deletions(-) diff --git a/request_test.go b/request_test.go index e1e880e2..d727bcc6 100644 --- a/request_test.go +++ b/request_test.go @@ -3263,12 +3263,7 @@ func TestWS_AutoSessionCloseAtPublication(t *testing.T) { t.Fatal("ResponseWriter.Header did not close the published AutoSession") } - var connectSession *Session - select { - case connectSession = <-connectSessionCh: - case <-time.After(testTimeout): - t.Fatal("timeout waiting for WebSocket connect") - } + connectSession := waitForConnectSession(t, connectSessionCh) const marker = "post-connect marker" ts.jw.Broadcast(wire.Message{Dest: ts.rq.JawsKey, What: what.Alert, Data: marker}) diff --git a/requestpool.go b/requestpool.go index 1992d883..35ec1130 100644 --- a/requestpool.go +++ b/requestpool.go @@ -49,9 +49,6 @@ import ( // unclaimed Request, its key remains unavailable for assignment to another Request // while the retired Request is reachable; no deadline is guaranteed for later key // reuse. -// -// NewRequest panics if the system CSPRNG ([crypto/rand]) fails while generating -// the request key, which does not happen on supported platforms. func (jw *Jaws) NewRequest(r *http.Request) (rq *Request) { remoteIP := jw.clientIP(r) diff --git a/session.go b/session.go index 1a266945..172d1ea6 100644 --- a/session.go +++ b/session.go @@ -188,6 +188,7 @@ func (sess *Session) addCookie(w http.ResponseWriter, r *http.Request) { defer jw.mu.RUnlock() sess.mu.RLock() defer sess.mu.RUnlock() + // Map identity settles Close races; liveness also covers deadline expiry while Header blocks above. if jw.sessions[sess.sessionID] == sess && !sess.isDeadLocked() { cookie := sess.cookie if h != nil { @@ -296,7 +297,7 @@ func (sess *Session) Broadcast(msg wire.Message) { } } -// SessionCount returns the number of active sessions. +// SessionCount returns the number of registered sessions. func (jw *Jaws) SessionCount() (n int) { jw.mu.RLock() n = len(jw.sessions) @@ -399,9 +400,6 @@ func (jw *Jaws) GetSession(r *http.Request) (sess *Session) { // w nor r receives its live cookie. // // It returns nil and has no effect if r is nil; w may be nil. -// -// It panics if the system CSPRNG ([crypto/rand]) fails while generating the session -// ID, which does not happen on supported platforms. func (jw *Jaws) NewSession(w http.ResponseWriter, r *http.Request) (sess *Session) { if r != nil { if sessionIDs := getCookieSessionsIDs(r.Header, jw.CookieName); len(sessionIDs) > 0 { @@ -423,12 +421,11 @@ func (jw *Jaws) NewSession(w http.ResponseWriter, r *http.Request) (sess *Sessio func (jw *Jaws) newSession(w http.ResponseWriter, r *http.Request) (sess *Session) { secure := secureheaders.RequestIsSecure(r, jw.TrustForwardedHeaders) - func() { - jw.mu.Lock() - defer jw.mu.Unlock() - sess = jw.newSessionLocked(jw.clientIP(r), secure) - jw.sessions[sess.sessionID] = sess - }() + remoteIP := jw.clientIP(r) + jw.mu.Lock() + sess = jw.newSessionLocked(remoteIP, secure) + jw.sessions[sess.sessionID] = sess + jw.mu.Unlock() sess.addCookie(w, r) return } diff --git a/session_test.go b/session_test.go index e8e87b6e..d2da8984 100644 --- a/session_test.go +++ b/session_test.go @@ -153,6 +153,40 @@ func TestSession_NewSessionWithoutResponseWriter(t *testing.T) { } } +func TestSession_AddCookieRejectsExpiredRegisteredSession(t *testing.T) { + jw, err := New() + if err != nil { + t.Fatal(err) + } + t.Cleanup(jw.Close) + + creationRequest := httptest.NewRequest(http.MethodGet, "http://example.test/", nil) + sess := jw.NewSession(nil, creationRequest) + if sess == nil { + t.Fatal("NewSession returned nil") + } + sess.mu.Lock() + sess.deadline = time.Now().Add(-time.Second) + sess.mu.Unlock() + if sessions := jw.Sessions(); len(sessions) != 1 || sessions[0] != sess { + t.Fatalf("Sessions() = %v, want the expired registered Session", sessions) + } + + rw := httptest.NewRecorder() + hr := httptest.NewRequest(http.MethodGet, "http://example.test/", nil) + sess.addCookie(rw, hr) + for _, cookie := range hr.Cookies() { + if cookie.Name == jw.CookieName { + t.Errorf("request contains expired session cookie: %v", cookie) + } + } + for _, cookie := range rw.Result().Cookies() { + if cookie.Name == jw.CookieName { + t.Errorf("response contains expired session cookie: %v", cookie) + } + } +} + type closingSessionResponseWriter struct { *httptest.ResponseRecorder jw *Jaws From 28a808f306ba98910f7ed29c1d21e064d7a82c7d Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Fri, 7 Aug 2026 21:56:17 +0200 Subject: [PATCH 3/3] fix: preserve session lock after key reader panic Restore the scoped deferred unlock around session allocation so an error from a replacement random reader cannot wedge Jaws. Pin the nil-request contract and both halves of the live-session publication guard. --- requestpool.go | 3 ++ session.go | 16 ++++-- session_test.go | 130 ++++++++++++++++++++++++++++++++++++++++-------- 3 files changed, 123 insertions(+), 26 deletions(-) diff --git a/requestpool.go b/requestpool.go index 35ec1130..25c65809 100644 --- a/requestpool.go +++ b/requestpool.go @@ -49,6 +49,9 @@ import ( // unclaimed Request, its key remains unavailable for assignment to another Request // while the retired Request is reachable; no deadline is guaranteed for later key // reuse. +// +// It panics if the [crypto/rand.Reader] captured by [New] returns an error while +// generating the request key. Go's default reader does not return errors. func (jw *Jaws) NewRequest(r *http.Request) (rq *Request) { remoteIP := jw.clientIP(r) diff --git a/session.go b/session.go index 172d1ea6..5dc62d23 100644 --- a/session.go +++ b/session.go @@ -188,7 +188,8 @@ func (sess *Session) addCookie(w http.ResponseWriter, r *http.Request) { defer jw.mu.RUnlock() sess.mu.RLock() defer sess.mu.RUnlock() - // Map identity settles Close races; liveness also covers deadline expiry while Header blocks above. + // Close unregisters before marking dead, while an unattached Session can + // expire during Header; require both registry identity and liveness. if jw.sessions[sess.sessionID] == sess && !sess.isDeadLocked() { cookie := sess.cookie if h != nil { @@ -400,6 +401,9 @@ func (jw *Jaws) GetSession(r *http.Request) (sess *Session) { // w nor r receives its live cookie. // // It returns nil and has no effect if r is nil; w may be nil. +// +// It panics if the [crypto/rand.Reader] captured by [New] returns an error while +// generating the session ID. Go's default reader does not return errors. func (jw *Jaws) NewSession(w http.ResponseWriter, r *http.Request) (sess *Session) { if r != nil { if sessionIDs := getCookieSessionsIDs(r.Header, jw.CookieName); len(sessionIDs) > 0 { @@ -422,10 +426,12 @@ func (jw *Jaws) NewSession(w http.ResponseWriter, r *http.Request) (sess *Sessio func (jw *Jaws) newSession(w http.ResponseWriter, r *http.Request) (sess *Session) { secure := secureheaders.RequestIsSecure(r, jw.TrustForwardedHeaders) remoteIP := jw.clientIP(r) - jw.mu.Lock() - sess = jw.newSessionLocked(remoteIP, secure) - jw.sessions[sess.sessionID] = sess - jw.mu.Unlock() + func() { + jw.mu.Lock() + defer jw.mu.Unlock() + sess = jw.newSessionLocked(remoteIP, secure) + jw.sessions[sess.sessionID] = sess + }() sess.addCookie(w, r) return } diff --git a/session_test.go b/session_test.go index d2da8984..c1bae3f7 100644 --- a/session_test.go +++ b/session_test.go @@ -1,6 +1,7 @@ package jaws import ( + "bufio" "context" "errors" "net" @@ -153,37 +154,124 @@ func TestSession_NewSessionWithoutResponseWriter(t *testing.T) { } } -func TestSession_AddCookieRejectsExpiredRegisteredSession(t *testing.T) { +func TestSession_NewSessionWithoutRequest(t *testing.T) { jw, err := New() if err != nil { t.Fatal(err) } t.Cleanup(jw.Close) - creationRequest := httptest.NewRequest(http.MethodGet, "http://example.test/", nil) - sess := jw.NewSession(nil, creationRequest) - if sess == nil { - t.Fatal("NewSession returned nil") + rw := &reentrantSessionResponseWriter{ + ResponseRecorder: httptest.NewRecorder(), + jw: jw, + sessionCount: -1, } - sess.mu.Lock() - sess.deadline = time.Now().Add(-time.Second) - sess.mu.Unlock() - if sessions := jw.Sessions(); len(sessions) != 1 || sessions[0] != sess { - t.Fatalf("Sessions() = %v, want the expired registered Session", sessions) + if sess := jw.NewSession(rw, nil); sess != nil { + t.Fatalf("NewSession() = %v, want nil", sess) + } + if got := jw.SessionCount(); got != 0 { + t.Errorf("SessionCount() = %d, want 0", got) + } + if rw.sessionCount != -1 { + t.Errorf("ResponseWriter.Header called for a nil request; SessionCount() = %d", rw.sessionCount) } + if header := rw.Result().Header; len(header) != 0 { + t.Errorf("response headers = %v, want none", header) + } +} - rw := httptest.NewRecorder() - hr := httptest.NewRequest(http.MethodGet, "http://example.test/", nil) - sess.addCookie(rw, hr) - for _, cookie := range hr.Cookies() { - if cookie.Name == jw.CookieName { - t.Errorf("request contains expired session cookie: %v", cookie) - } +func TestSession_NewSessionUnlocksAfterInjectedRandomReaderPanic(t *testing.T) { + jw, err := New() + if err != nil { + t.Fatal(err) } - for _, cookie := range rw.Result().Cookies() { - if cookie.Name == jw.CookieName { - t.Errorf("response contains expired session cookie: %v", cookie) - } + jw.kg = bufio.NewReader(errReader{}) + + var panicValue any + func() { + defer func() { + panicValue = recover() + }() + jw.NewSession(nil, httptest.NewRequest(http.MethodGet, "http://example.test/", nil)) + }() + if panicValue == nil { + jw.Close() + t.Fatal("NewSession did not panic when the injected random reader failed") + } + if !jw.mu.TryLock() { + // Release the leaked lock so cleanup itself does not deadlock on a regression. + jw.mu.Unlock() + jw.Close() + t.Fatal("NewSession left Jaws locked after the injected random reader panic") + } + jw.mu.Unlock() + jw.Close() +} + +func TestSession_AddCookieRejectsUnavailableSession(t *testing.T) { + tests := []struct { + name string + makeUnavailable func(*Jaws, *Session) + wantRegistered bool + wantDead bool + }{ + { + name: "expired registered", + makeUnavailable: func(_ *Jaws, sess *Session) { + sess.mu.Lock() + sess.deadline = time.Now().Add(-time.Second) + sess.mu.Unlock() + }, + wantRegistered: true, + wantDead: true, + }, + { + name: "live unregistered", + makeUnavailable: func(jw *Jaws, sess *Session) { + sess.mu.Lock() + sess.deadline = time.Now().Add(24 * time.Hour) + sess.mu.Unlock() + jw.deleteSession(sess.sessionID) + }, + wantRegistered: false, + wantDead: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + jw, err := New() + if err != nil { + t.Fatal(err) + } + t.Cleanup(jw.Close) + + creationRequest := httptest.NewRequest(http.MethodGet, "http://example.test/", nil) + sess := jw.NewSession(nil, creationRequest) + if sess == nil { + t.Fatal("NewSession returned nil") + } + tt.makeUnavailable(jw, sess) + if registered := slices.Contains(jw.Sessions(), sess); registered != tt.wantRegistered { + t.Fatalf("registered = %t, want %t", registered, tt.wantRegistered) + } + if dead := sess.isDead(); dead != tt.wantDead { + t.Fatalf("dead = %t, want %t", dead, tt.wantDead) + } + + rw := httptest.NewRecorder() + hr := httptest.NewRequest(http.MethodGet, "http://example.test/", nil) + sess.addCookie(rw, hr) + for _, cookie := range hr.Cookies() { + if cookie.Name == jw.CookieName { + t.Errorf("request contains unavailable session cookie: %v", cookie) + } + } + for _, cookie := range rw.Result().Cookies() { + if cookie.Name == jw.CookieName { + t.Errorf("response contains unavailable session cookie: %v", cookie) + } + } + }) } }