Session cookie: not Secure over plain http on a local-network host

README.md

@@ -95,7 +95,7 @@ Set `TRUSTED_PROXIES` so `X-Forwarded-For` headers are trusted for client IP res
 TRUSTED_PROXIES=172.16.0.0/12 ./goblog
 ```
 
-The session cookie is `HttpOnly`, `SameSite=Lax` and `Secure`, so it is only sent over HTTPS (browsers exempt `localhost`, so local development on `http://localhost:7000` still works). If you serve goblog over plain HTTP on any other host, set `SESSION_SECURE=false` or logins will not stick. Mutating `/api/v1` requests must be sent as `application/json` (`/api/v1/upload` as `multipart/form-data`); anything else gets `415 Unsupported Media Type`.
+The session cookie is `HttpOnly`, `SameSite=Lax` and `Secure`, so it is only sent over HTTPS. The one exception goblog makes by itself is a request over plain HTTP for a host that can only be on a local network (an IP address, a bare host name, or a name ending in `.local`, `.lan`, `.internal`, `.home.arpa` or `.localhost`): there the cookie is not marked `Secure`, so a home-server install at `http://192.168.1.20:7000` can log in. `SESSION_SECURE=true` or `SESSION_SECURE=false` overrides that decision either way; if you serve goblog over plain HTTP on a public host name, set `SESSION_SECURE=false` or logins will not stick. Mutating `/api/v1` requests must be sent as `application/json` (`/api/v1/upload` as `multipart/form-data`); anything else gets `415 Unsupported Media Type`.
 
 ### Admin Password
 The admin account the wizard creates signs in on the login page with its email and password. The email is only a sign-in name; goblog sends nothing to it. Passwords are at least 10 characters and stored as bcrypt hashes, and sign-in attempts are rate limited per client address.

csrf_test.go

@@ -1,25 +1,99 @@
 package main
 
 import (
+	"crypto/tls"
 	"net/http"
 	"net/http/httptest"
 	"os"
 	"strings"
 	"testing"
 
+	"github.com/gin-contrib/sessions"
+	"github.com/gin-contrib/sessions/cookie"
 	"github.com/gin-gonic/gin"
 )
 
 func TestSessionOptions(t *testing.T) {
-	o := sessionOptions("")
+	o := sessionOptions(true)
 	if !o.HttpOnly || !o.Secure || o.SameSite != http.SameSiteLaxMode || o.Path != "/" || o.MaxAge <= 0 {
-		t.Errorf("default options: %+v", o)
+		t.Errorf("options: %+v", o)
 	}
-	if sessionOptions("false").Secure {
-		t.Error("SESSION_SECURE=false should clear Secure")
+	if sessionOptions(false).Secure {
+		t.Error("sessionOptions(false) is Secure")
 	}
-	if !sessionOptions("true").Secure {
-		t.Error("SESSION_SECURE=true should keep Secure")
+}
+
+// TestCookieSecure: the session cookie is Secure unless the operator said
+// otherwise or the request is plain http for a host on a local network,
+// where a Secure cookie would be dropped and nobody could log in (#665).
+func TestCookieSecure(t *testing.T) {
+	type req struct {
+		env, host, proto string
+		tls              bool
+	}
+	for name, tc := range map[string]struct {
+		req
+		want bool
+	}{
+		"LAN address":                    {req{host: "192.168.1.20:7000"}, false},
+		"loopback address":               {req{host: "127.0.0.1:7000"}, false},
+		"IPv6 address":                   {req{host: "[fd00::1]:7000"}, false},
+		"bare host name":                 {req{host: "nas:7000"}, false},
+		"mDNS name":                      {req{host: "blog.local"}, false},
+		"localhost":                      {req{host: "localhost:7000"}, false},
+		"public name, plain http":        {req{host: "blog.example.com"}, true}, // or a proxy that forwards no headers
+		"public name behind https proxy": {req{host: "blog.example.com", proto: "https"}, true},
+		"LAN address behind https proxy": {req{host: "192.168.1.20", proto: "HTTPS, http"}, true},
+		"LAN address over TLS":           {req{host: "192.168.1.20:7000", tls: true}, true},
+		"no host":                        {req{host: ""}, true},
+		"forced on, LAN address":         {req{env: "true", host: "192.168.1.20:7000"}, true},
+		"forced off, public name":        {req{env: "false", host: "blog.example.com", proto: "https"}, false},
+	} {
+		r := httptest.NewRequest(http.MethodGet, "/", nil)
+		r.Host = tc.host
+		if tc.proto != "" {
+			r.Header.Set("X-Forwarded-Proto", tc.proto)
+		}
+		if tc.tls {
+			r.TLS = &tls.ConnectionState{}
+		}
+		if got := cookieSecure(tc.env, r); got != tc.want {
+			t.Errorf("%s: Secure = %v, want %v", name, got, tc.want)
+		}
+	}
+}
+
+// TestSessionCookieSecurity: the middleware's decision is what ends up on
+// the Set-Cookie header.
+func TestSessionCookieSecurity(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	r := gin.New()
+	store := cookie.NewStore([]byte("test"))
+	store.Options(sessionOptions(true))
+	r.Use(sessions.Sessions("session", store))
+	r.Use(sessionCookieSecurity(""))
+	r.GET("/", func(c *gin.Context) {
+		s := sessions.Default(c)
+		s.Set("k", "v")
+		if err := s.Save(); err != nil {
+			t.Fatal(err)
+		}
+	})
+	for host, wantSecure := range map[string]bool{"192.168.1.20:7000": false, "blog.example.com": true} {
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		req.Host = host
+		w := httptest.NewRecorder()
+		r.ServeHTTP(w, req)
+		header := w.Header().Get("Set-Cookie")
+		if header == "" {
+			t.Fatalf("%s: no Set-Cookie", host)
+		}
+		if got := strings.Contains(header, "Secure"); got != wantSecure {
+			t.Errorf("%s: Secure = %v, want %v (%s)", host, got, wantSecure, header)
+		}
+		if !strings.Contains(header, "HttpOnly") || !strings.Contains(header, "SameSite=Lax") {
+			t.Errorf("%s: cookie lost HttpOnly or SameSite: %s", host, header)
+		}
 	}
 }
 

goblog.go

@@ -21,6 +21,7 @@ import (
 	"gorm.io/gorm"
 	"log"
 	"mime"
+	"net"
 	"net/http"
 	"os"
 	"strings"
@@ -479,9 +480,10 @@ func main() {
 	router.Use(requireJSON())
 	router.Use(gplugin.Middleware(registry))
 	store := cookie.NewStore([]byte(sessionKey))
-	store.Options(sessionOptions(os.Getenv("SESSION_SECURE")))
+	store.Options(sessionOptions(true))
 	hostname, err := os.Hostname()
 	router.Use(sessions.Sessions(hostname, store))
+	router.Use(sessionCookieSecurity(os.Getenv("SESSION_SECURE")))
 	log.Println("Hostname: ", hostname)
 	// Load templates from the active theme directory, falling back to "default".
 	// activeTheme is read by the static handler on every /theme/* request and
@@ -720,18 +722,73 @@ func CORS() gin.HandlerFunc {
 
 // sessionOptions returns the session cookie's attributes: HttpOnly and
 // SameSite=Lax, so a cross-site form post or fetch does not carry an admin's
-// session, and Secure unless SESSION_SECURE=false (plain http on a host
-// other than localhost, which browsers exempt).
-func sessionOptions(secureEnv string) sessions.Options {
+// session, and Secure as cookieSecure decides for the request.
+func sessionOptions(secure bool) sessions.Options {
 	return sessions.Options{
 		Path:     "/",
 		MaxAge:   30 * 24 * 60 * 60,
 		HttpOnly: true,
-		Secure:   secureEnv != "false",
+		Secure:   secure,
 		SameSite: http.SameSiteLaxMode,
 	}
 }
 
+// cookieSecure decides whether the session cookie for this request is
+// marked Secure. SESSION_SECURE=true or false is the operator's answer and
+// is final. Otherwise the cookie is Secure, with one exception: a request
+// that arrived over plain http for a host that only exists on a local
+// network (an IP address, a bare name, .local and the like). A browser
+// there would silently drop a Secure cookie, so nobody could get past the
+// setup code or log in on a home server (#665), and there is no https for
+// the flag to protect.
+//
+// A public host name stays Secure even when the request looks like plain
+// http, because that is also what a TLS-terminating proxy that forwards no
+// headers looks like.
+func cookieSecure(secureEnv string, r *http.Request) bool {
+	switch secureEnv {
+	case "true":
+		return true
+	case "false":
+		return false
+	}
+	forwarded, _, _ := strings.Cut(r.Header.Get("X-Forwarded-Proto"), ",")
+	if r.TLS != nil || strings.EqualFold(strings.TrimSpace(forwarded), "https") {
+		return true
+	}
+	return !localNetworkHost(r.Host)
+}
+
+// localNetworkHost reports whether host (a Host header, port allowed) can
+// only be a machine on a local network: an IP address, a name with no dot,
+// or a name under a suffix reserved for private use.
+func localNetworkHost(host string) bool {
+	if h, _, err := net.SplitHostPort(host); err == nil {
+		host = h
+	}
+	host = strings.ToLower(strings.TrimSuffix(strings.Trim(host, "[]"), "."))
+	if host == "" {
+		return false
+	}
+	if net.ParseIP(host) != nil || !strings.Contains(host, ".") {
+		return true
+	}
+	for _, suffix := range []string{".local", ".lan", ".internal", ".home.arpa", ".localhost"} {
+		if strings.HasSuffix(host, suffix) {
+			return true
+		}
+	}
+	return false
+}
+
+// sessionCookieSecurity sets the session cookie's attributes for each
+// request. It has to run after sessions.Sessions.
+func sessionCookieSecurity(secureEnv string) gin.HandlerFunc {
+	return func(c *gin.Context) {
+		sessions.Default(c).Options(sessionOptions(cookieSecure(secureEnv, c.Request)))
+	}
+}
+
 // requireJSON rejects mutating /api/v1 requests whose body is not JSON
 // (multipart is allowed for /api/v1/upload only). Every goblog client sends
 // application/json; an HTML form on another site can only send form

scripts/install-smoke-test.sh

@@ -120,8 +120,9 @@ esac
 step "starting goblog ($DB${LEGACY:+, no data directory})"
 start_app
 
-# The session cookie is Secure, which curl will not send over plain http, so
-# it is carried by hand. login stores whatever cookies the last response set.
+# The session cookie is carried by hand, so the test does not depend on what
+# curl's cookie jar makes of its attributes. keep_cookies stores whatever
+# cookies the last response set.
 COOKIE=""
 keep_cookies() {
   set_cookies=$(grep -i '^set-cookie:' "$TMP/headers" | sed -E 's/^[^:]*: *([^;]*).*/\1/' | paste -sd ';' - | sed 's/;/; /g' || true)
@@ -154,6 +155,9 @@ SETUP_CODE=$(docker logs "$APP" 2>&1 | sed -n 's/.*GoBlog setup code: \([A-Z0-9-
 [ -n "$SETUP_CODE" ] || fail "no setup code in the log"
 as_owner "POST /wizard/unlock" 303 -X POST "$BASE/wizard/unlock" -d "setup_code=$SETUP_CODE"
 [ -n "$COOKIE" ] || fail "POST /wizard/unlock set no cookie"
+# Plain http to an IP address: a Secure cookie would be dropped by a browser,
+# and the install could not get past this page (#665).
+! grep -i '^set-cookie:' "$TMP/headers" | grep -qi '; *secure' || fail "the session cookie is Secure over plain http on an IP address"
 as_owner "GET /" 200 "$BASE/"
 body_has 'name="dbtype"' "GET / with the setup code"
 

themes/default/templates/wizard_unlock.html

@@ -29,7 +29,7 @@ <h5>Setup code</h5>
                 </div>
                 <button type="submit" class="btn btn-primary">Continue</button>
             </form>
-            <p class="text-muted small mt-4">The code changes every time goblog restarts. If this page comes back after you enter the right code, your browser is not keeping the session cookie: goblog marks it Secure, which browsers only accept over https or on localhost. Use one of those, or start goblog with <code>SESSION_SECURE=false</code>.</p>
+            <p class="text-muted small mt-4">The code changes every time goblog restarts. If this page comes back after you enter the right code, your browser is not keeping the session cookie: on a public host name goblog marks it Secure, which browsers only accept over https. Use https, or start goblog with <code>SESSION_SECURE=false</code>.</p>
             <div class="version text-center">{{ template "_powered_by" . }}</div>
         </div>
     </div>