Просмотр исходного кода

fix(ui): don't create web sessions for unauthenticated protected routes

The web session middleware inserts a row for every UI request without a
valid cookie, before it checks whether the route needs authentication.
Crawlers and scanners never send the cookie back, so each request adds
a row, including requests for paths that only get redirected to the
login page. Those rows are kept for CLEANUP_REMOVE_SESSIONS_DAYS like
signed-in sessions.

On one single-user instance, 24,070 of 24,073 rows in web_sessions were
anonymous; two scanner runs alone added 5,951 rows each.

Redirect requests for protected routes to the login page without
creating a session, since the redirect URL travels in the query string
and nothing needs to be stored yet. The login page and other public
routes still create one for the CSRF token.
Dave Marquard 2 недель назад
Родитель
Сommit
d5c529b94c
2 измененных файлов с 76 добавлено и 0 удалено
  1. 7 0
      internal/ui/web_session_middleware.go
  2. 69 0
      internal/ui/web_session_middleware_test.go

+ 7 - 0
internal/ui/web_session_middleware.go

@@ -37,6 +37,13 @@ func (m *webSessionMiddleware) handle(next http.Handler) http.Handler {
 			return
 		}
 		if session == nil {
+			// Visitors without a session are sent to the login page and have
+			// nothing to remember yet, so don't persist a session for them.
+			if !isPublicRoute(r) {
+				response.HTMLRedirect(w, r, loginRedirectURL(m.basePath, r.RequestURI))
+				return
+			}
+
 			var secret string
 			session, secret = model.NewWebSession(r.UserAgent(), request.ClientIP(r))
 			if language, theme, ok := preferencesFromCookie(r); ok {

+ 69 - 0
internal/ui/web_session_middleware_test.go

@@ -0,0 +1,69 @@
+// SPDX-FileCopyrightText: Copyright The Miniflux Authors. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package ui // import "miniflux.app/v2/internal/ui"
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+func TestWebSessionMiddlewareRedirectsWithoutCreatingSession(t *testing.T) {
+	scenarios := []struct {
+		name   string
+		cookie *http.Cookie
+	}{
+		{"no cookie", nil},
+		{"malformed cookie", &http.Cookie{Name: sessionCookieName, Value: "invalid"}},
+	}
+
+	for _, scenario := range scenarios {
+		t.Run(scenario.name, func(t *testing.T) {
+			// A nil store makes the test fail if the middleware tries to persist a session.
+			middleware := newWebSessionMiddleware("/base", nil)
+			handler := middleware.handle(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				t.Fatal(`The next handler should not be called`)
+			}))
+
+			r := httptest.NewRequest(http.MethodGet, "/unread?page=2", nil)
+			if scenario.cookie != nil {
+				r.AddCookie(scenario.cookie)
+			}
+			w := httptest.NewRecorder()
+			handler.ServeHTTP(w, r)
+
+			if w.Code != http.StatusFound {
+				t.Fatalf(`Unexpected status code, got: %d`, w.Code)
+			}
+
+			if location := w.Header().Get("Location"); location != "/base/?redirect_url=%2Funread%3Fpage%3D2" {
+				t.Fatalf(`Unexpected redirect location, got: %q`, location)
+			}
+
+			if cookie := w.Header().Get("Set-Cookie"); cookie != "" {
+				t.Fatalf(`No session cookie should be set, got: %q`, cookie)
+			}
+		})
+	}
+}
+
+func TestWebSessionMiddlewareSkipsStaticAssets(t *testing.T) {
+	middleware := newWebSessionMiddleware("", nil)
+
+	called := false
+	handler := middleware.handle(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		called = true
+	}))
+
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/favicon.ico", nil))
+
+	if !called {
+		t.Fatal(`The next handler should be called for static assets`)
+	}
+
+	if cookie := w.Header().Get("Set-Cookie"); cookie != "" {
+		t.Fatalf(`No session cookie should be set, got: %q`, cookie)
+	}
+}