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..d727bcc6 100644 --- a/request_test.go +++ b/request_test.go @@ -3189,6 +3189,120 @@ 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") + } + + connectSession := waitForConnectSession(t, connectSessionCh) + + 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/requestpool.go b/requestpool.go index 1992d883..25c65809 100644 --- a/requestpool.go +++ b/requestpool.go @@ -50,8 +50,8 @@ import ( // 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. +// 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 a1c945a3..5dc62d23 100644 --- a/session.go +++ b/session.go @@ -175,6 +175,35 @@ 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() + // 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 { + 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. // @@ -269,7 +298,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) @@ -277,7 +306,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,12 +394,16 @@ 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 panics if the system CSPRNG ([crypto/rand]) fails while generating the session -// ID, which does not happen on supported platforms. +// 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 { @@ -389,26 +425,27 @@ 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 + remoteIP := jw.clientIP(r) 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(remoteIP, 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 +467,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..c1bae3f7 100644 --- a/session_test.go +++ b/session_test.go @@ -1,6 +1,7 @@ package jaws import ( + "bufio" "context" "errors" "net" @@ -132,6 +133,234 @@ 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) + } +} + +func TestSession_NewSessionWithoutRequest(t *testing.T) { + jw, err := New() + if err != nil { + t.Fatal(err) + } + t.Cleanup(jw.Close) + + rw := &reentrantSessionResponseWriter{ + ResponseRecorder: httptest.NewRecorder(), + jw: jw, + sessionCount: -1, + } + 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) + } +} + +func TestSession_NewSessionUnlocksAfterInjectedRandomReaderPanic(t *testing.T) { + jw, err := New() + if err != nil { + t.Fatal(err) + } + 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) + } + } + }) + } +} + +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 {