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

fix(ui): stop caching shared entry pages in public caches

Shared entry pages were sent with "Cache-Control: public, immutable"
for 72 hours, while the rendered HTML contained data from the
visitor's web session, including the CSRF token. A reverse proxy or
CDN could store one visitor's token and serve it to everyone else,
and unshared entries stayed reachable from caches.

Shared entries are now rendered with a new public view that never
includes the CSRF token or flash messages, and the response is no
longer cacheable since it still depends on the visitor's theme and
language.

As a safeguard, the response builder now downgrades any public
Cache-Control header to "private, no-store" when the response also
sets a cookie.
Frédéric Guillot 16 часов назад
Родитель
Сommit
b8da688674

+ 8 - 1
internal/http/response/builder.go

@@ -126,7 +126,7 @@ func (b *Builder) WithoutCompression() *Builder {
 	return b
 }
 
-// WithCaching adds caching headers to the response.
+// WithCaching marks the response as publicly cacheable; use it only when the body never depends on the session.
 func (b *Builder) WithCaching(etag string, duration time.Duration, callback func(*Builder)) {
 	etag = normalizeETag(etag)
 	b.headers.Set("ETag", etag)
@@ -226,6 +226,13 @@ func (b *Builder) writeHeaders() {
 
 	maps.Copy(b.w.Header(), b.headers)
 
+	// Shared caches must never replay a cookie to other visitors.
+	headers := b.w.Header()
+	if len(headers.Values("Set-Cookie")) > 0 && strings.Contains(headers.Get("Cache-Control"), "public") {
+		headers.Set("Cache-Control", "private, no-store")
+		headers.Del("Expires")
+	}
+
 	b.w.WriteHeader(b.statusCode)
 }
 

+ 33 - 0
internal/http/response/builder_test.go

@@ -255,6 +255,39 @@ func TestBuildResponseWithCachingEnabled(t *testing.T) {
 	}
 }
 
+func TestBuildResponseWithCachingAndCookie(t *testing.T) {
+	r, err := http.NewRequest("GET", "/", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	w := httptest.NewRecorder()
+
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.SetCookie(w, &http.Cookie{Name: "session", Value: "secret"})
+		NewBuilder(w, r).WithCaching("etag", 1*time.Minute, func(b *Builder) {
+			b.WithBodyAsString("cached body")
+			b.Write()
+		})
+	})
+
+	handler.ServeHTTP(w, r)
+	resp := w.Result()
+
+	expectedHeader := "private, no-store"
+	if actualHeader := resp.Header.Get("Cache-Control"); actualHeader != expectedHeader {
+		t.Fatalf(`Unexpected cache control header, got %q instead of %q`, actualHeader, expectedHeader)
+	}
+
+	if expires := resp.Header.Get("Expires"); expires != "" {
+		t.Fatalf(`Expires header should be removed, got %q`, expires)
+	}
+
+	if cookie := resp.Header.Get("Set-Cookie"); cookie == "" {
+		t.Fatal(`Set-Cookie header should be kept`)
+	}
+}
+
 func TestBuildResponseWithCachingAndIfNoneMatch(t *testing.T) {
 	tests := []struct {
 		name           string

+ 16 - 22
internal/ui/share.go

@@ -5,7 +5,6 @@ package ui // import "miniflux.app/v2/internal/ui"
 
 import (
 	"net/http"
-	"time"
 
 	"miniflux.app/v2/internal/http/request"
 	"miniflux.app/v2/internal/http/response"
@@ -41,26 +40,21 @@ func (h *handler) sharedEntry(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	etag := shareCode
-	response.NewBuilder(w, r).WithCaching(etag, 72*time.Hour, func(b *response.Builder) {
-		entry, err := h.store.NewAnonymousQueryBuilder().
-			WithShareCode(shareCode).
-			GetEntry()
-
-		if err != nil {
-			response.HTMLServerError(w, r, err)
-			return
-		}
-		if entry == nil {
-			response.HTMLNotFound(w, r)
-			return
-		}
-
-		view := view.New(h.tpl, r)
-		view.Set("entry", entry)
+	entry, err := h.store.NewAnonymousQueryBuilder().
+		WithShareCode(shareCode).
+		GetEntry()
+	if err != nil {
+		response.HTMLServerError(w, r, err)
+		return
+	}
+	if entry == nil {
+		response.HTMLNotFound(w, r)
+		return
+	}
 
-		b.WithHeader("Content-Type", "text/html; charset=utf-8")
-		b.WithBodyAsBytes(view.Render("entry"))
-		b.Write()
-	})
+	// The page still depends on the visitor's theme and language, so it must
+	// never be stored by shared caches.
+	view := view.NewPublic(h.tpl, r)
+	view.Set("entry", entry)
+	response.HTML(w, r, view.Render("entry"))
 }

+ 22 - 11
internal/ui/view/view.go

@@ -33,18 +33,29 @@ func (v *view) Render(template string) []byte {
 // New returns a new view with default parameters.
 func New(tpl *template.Engine, r *http.Request) *view {
 	webSession := request.WebSession(r)
-	theme := webSession.Theme()
 	flashSuccessMessage, flashErrorMessage := webSession.ConsumeMessages()
+	v := newView(tpl, r)
+	v.params["csrf"] = webSession.CSRF()
+	v.params["flashSuccessMessage"] = flashSuccessMessage
+	v.params["flashErrorMessage"] = flashErrorMessage
+	return v
+}
+
+// NewPublic returns a view for public pages, without the CSRF token or flash messages.
+func NewPublic(tpl *template.Engine, r *http.Request) *view {
+	return newView(tpl, r)
+}
+
+func newView(tpl *template.Engine, r *http.Request) *view {
+	webSession := request.WebSession(r)
+	theme := webSession.Theme()
 	return &view{tpl, r, map[string]any{
-		"menu":                "",
-		"csrf":                webSession.CSRF(),
-		"flashSuccessMessage": flashSuccessMessage,
-		"flashErrorMessage":   flashErrorMessage,
-		"theme":               theme,
-		"language":            webSession.Language(),
-		"theme_checksum":      static.StylesheetBundles[theme+".css"].Checksum,
-		"app_js_checksum":     static.JavascriptBundles["app.js"].Checksum,
-		"sw_js_checksum":      static.JavascriptBundles["service-worker.js"].Checksum,
-		"webAuthnEnabled":     config.Opts.WebAuthn(),
+		"menu":            "",
+		"theme":           theme,
+		"language":        webSession.Language(),
+		"theme_checksum":  static.StylesheetBundles[theme+".css"].Checksum,
+		"app_js_checksum": static.JavascriptBundles["app.js"].Checksum,
+		"sw_js_checksum":  static.JavascriptBundles["service-worker.js"].Checksum,
+		"webAuthnEnabled": config.Opts.WebAuthn(),
 	}}
 }

+ 66 - 0
internal/ui/view/view_test.go

@@ -0,0 +1,66 @@
+// SPDX-FileCopyrightText: Copyright The Miniflux Authors. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package view // import "miniflux.app/v2/internal/ui/view"
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"miniflux.app/v2/internal/config"
+	"miniflux.app/v2/internal/http/request"
+	"miniflux.app/v2/internal/model"
+)
+
+func configureTestOptions(t *testing.T) {
+	t.Helper()
+
+	parsedOptions, err := config.NewConfigParser().ParseEnvironmentVariables()
+	if err != nil {
+		t.Fatalf(`Unable to configure test options: %v`, err)
+	}
+
+	previousOptions := config.Opts
+	config.Opts = parsedOptions
+	t.Cleanup(func() {
+		config.Opts = previousOptions
+	})
+}
+
+func TestNewPublicOmitsSessionSecrets(t *testing.T) {
+	configureTestOptions(t)
+	session, _ := model.NewWebSession("test", "127.0.0.1")
+	session.SetSuccessMessage("saved")
+
+	r := httptest.NewRequest(http.MethodGet, "/share/code", nil)
+	r = r.WithContext(context.WithValue(r.Context(), request.WebSessionContextKey, session))
+
+	publicView := NewPublic(nil, r)
+
+	for _, key := range []string{"csrf", "flashSuccessMessage", "flashErrorMessage"} {
+		if value, ok := publicView.params[key]; ok {
+			t.Fatalf(`Public view should not set %q, got %v`, key, value)
+		}
+	}
+
+	successMessage, _ := session.ConsumeMessages()
+	if successMessage != "saved" {
+		t.Fatalf(`Flash message should be kept for the next page, got %q`, successMessage)
+	}
+}
+
+func TestNewIncludesCSRFToken(t *testing.T) {
+	configureTestOptions(t)
+	session, _ := model.NewWebSession("test", "127.0.0.1")
+
+	r := httptest.NewRequest(http.MethodGet, "/unread", nil)
+	r = r.WithContext(context.WithValue(r.Context(), request.WebSessionContextKey, session))
+
+	defaultView := New(nil, r)
+
+	if csrf := defaultView.params["csrf"]; csrf != session.CSRF() || csrf == "" {
+		t.Fatalf(`Unexpected CSRF token, got %v`, csrf)
+	}
+}