Stop allowing credentialed cross-origin requests (closes #261)

CORS() allowed every origin with credentials, so a page on another origin of the same site could read what the signed-in user sees.

CORS() now uses the CONVENTIONS.md template's options (AllowedOrigins "*", AllowCredentials false) and covers only the two public, read-only routes, GET /api/v1/server and GET /.well-known/healthcheck.json. Every other route sends no Access-Control-Allow-* header. A preflight to the public routes gets 405 like any other path.

Deviation: CORS() is not in the global middleware chain CONVENTIONS.md lists, because REPO_POLICIES.md forbids * on authenticated routes.

Model: opus-5-5

README.md

@@ -168,8 +168,10 @@ either.
 - Request bodies for `POST /api/v1/session`, `POST /api/v1/login` and
   `POST /api/v1/messages` are limited to `MAX_MESSAGE_SIZE` bytes (default
   `4096`). A larger body gets `400` `invalid request body`.
-- The server allows cross-origin requests from any origin, with credentials. The
+- `GET /api/v1/server` and `GET /.well-known/healthcheck.json` answer a request
-  cookie is `SameSite=Strict`, so a browser sends it only from the same site.
+  from any origin with `Access-Control-Allow-Origin: *`. Every other route sends
+  no `Access-Control-Allow-*` header, so a page on another origin cannot read
+  its responses.
 
 ### The auth cookie
 
@@ -1893,14 +1895,14 @@ The IRC listener has no TLS.
   seconds. A request still running after 60 seconds is cancelled, and the
   connection closes without a response. Headers may be up to 1 MiB.
 - A long poll waits at most 30 seconds.
-- Every response to a well-formed request, except to a cross-origin preflight,
+- Every response to a well-formed request carries a `Content-Security-Policy`
-  carries a `Content-Security-Policy` header:
+  header:
   `default-src 'self'; script-src 'self'; style-src 'self'; connect-src 'self'; img-src 'self'; font-src 'self'; object-src 'none'; frame-ancestors 'none'; base-uri 'self'; form-action 'self'`.
   The web client works within it.
-- Cross-origin requests are allowed from any origin, with credentials. The
+- `GET /api/v1/server` and `GET /.well-known/healthcheck.json` answer a request
-  allowed methods are `GET`, `POST`, `PUT`, `DELETE` and `OPTIONS`. The allowed
+  from any origin with `Access-Control-Allow-Origin: *`. Every other route sends
-  request headers are `Accept`, `Content-Type` and `X-CSRF-Token`. The cookie's
+  no `Access-Control-Allow-*` header, so a page on another origin cannot read
-  `SameSite=Strict` still keeps it to the same site.
+  its responses.
 
 ### Logging
 

internal/db/db_test.go

@@ -27,7 +27,11 @@ const (
 		(SELECT 1 UNION ALL SELECT x + 1 FROM n)
 		SELECT COUNT(*) FROM n`
 
-	endlessQueryTimeout = 100 * time.Millisecond
+	// endlessQueryTimeout is how long the endless query runs
+	// before its context ends. A query whose context ends before
+	// it starts is never run, and the pool keeps its connection,
+	// so the timeout leaves a loaded host time to start it.
+	endlessQueryTimeout = 2 * time.Second
 )
 
 // TestDefaultDBURLSetsWALAndBusyTimeout opens a file database

internal/ircserver/server_test.go

@@ -31,7 +31,10 @@ import (
 	_ "modernc.org/sqlite"
 )
 
-const testTimeout = 5 * time.Second
+// testTimeout is how long a test waits for something before it
+// fails. A passing test never waits this long, so it is generous
+// enough for a heavily loaded host.
+const testTimeout = 30 * time.Second
 
 // testPollInterval is how often the relay of a test server
 // checks the client output queue without a broker
@@ -565,27 +568,49 @@ func TestQuietClientAnsweringPingStaysConnected(t *testing.T) {
 func TestQuietClientNotAnsweringPingIsClosed(t *testing.T) {
 	t.Parallel()
 
-	// latest leaves room for a loaded host, but is sooner than
+	// A read deadline of two ping intervals would close the
-	// a read deadline of two ping intervals would close it.
+	// connection a second after latest. The ping interval is
+	// longer than testPingInterval, so that the time between
+	// the expected close and latest leaves room for a loaded
+	// host.
 	const (
-		earliest = testPingInterval + testPongDeadline
+		interval = 4 * time.Second
-		latest   = 2 * testPingInterval
+		earliest = interval + testPongDeadline
+		latest   = 2*interval - time.Second
 	)
 
-	env := newTestEnvWithShortPings(t)
+	env := newTestEnvWithPingTimes(
+		t,
+		&config.Config{ServerName: testServerName},
+		interval, testPongDeadline,
+	)
 	client := env.dial(t)
 
 	lastSent := time.Now()
 
 	client.register("silent")
-	client.readServerPing(lastSent)
+
+	// The server starts both waits once it has handled the
+	// registration, so the time registration took does not
+	// count against latest.
+	registered := time.Now()
+
+	client.readUntil(isServerPing)
 	client.readUntilClosed()
 
 	waited := time.Since(lastSent)
-	if waited < earliest || waited >= latest {
+	if waited < earliest {
 		t.Errorf(
-			"closed %v after the client's last line, want %v to %v",
+			"closed %v after the client's last line, want at least %v",
-			waited, earliest, latest,
+			waited, earliest,
+		)
+	}
+
+	sinceRegistered := time.Since(registered)
+	if sinceRegistered >= latest {
+		t.Errorf(
+			"closed %v after registration, want less than %v",
+			sinceRegistered, latest,
 		)
 	}
 }

internal/middleware/middleware.go

@@ -200,24 +200,19 @@ func (mware *Middleware) Logging() func(http.Handler) http.Handler {
 	}
 }
 
-// CORS returns middleware that handles Cross-Origin Resource Sharing.
+// CORS returns middleware that lets a page on any origin read the
-// AllowCredentials is true so browsers include cookies in
+// response. Use it only on public, read-only routes.
-// cross-origin API requests.
 func (mware *Middleware) CORS() func(http.Handler) http.Handler {
 	return cors.Handler(cors.Options{ //nolint:exhaustruct // optional fields
-		AllowOriginFunc: func(
+		AllowedOrigins: []string{"*"},
-			_ *http.Request, _ string,
-		) bool {
-			return true
-		},
 		AllowedMethods: []string{
 			"GET", "POST", "PUT", "DELETE", "OPTIONS",
 		},
 		AllowedHeaders: []string{
-			"Accept", "Content-Type", "X-CSRF-Token",
+			"Accept", "Authorization", "Content-Type", "X-CSRF-Token",
 		},
 		ExposedHeaders:   []string{"Link"},
-		AllowCredentials: true,
+		AllowCredentials: false,
 		MaxAge:           corsMaxAge,
 	})
 }

internal/server/routes.go

@@ -30,7 +30,6 @@ func (srv *Server) SetupRoutes() {
 		srv.router.Use(srv.mw.Metrics())
 	}
 
-	srv.router.Use(srv.mw.CORS())
 	srv.router.Use(srv.mw.CSP())
 	srv.router.Use(middleware.Timeout(routeTimeout))
 
@@ -45,7 +44,7 @@ func (srv *Server) SetupRoutes() {
 	}
 
 	// Health check.
-	srv.router.Get(
+	srv.router.With(srv.mw.CORS()).Get(
 		"/.well-known/healthcheck.json",
 		srv.handlers.HandleHealthCheck(),
 	)
@@ -69,7 +68,7 @@ func (srv *Server) SetupRoutes() {
 }
 
 func (srv *Server) setupAPIv1(router chi.Router) {
-	router.Get(
+	router.With(srv.mw.CORS()).Get(
 		"/server",
 		srv.handlers.HandleServerInfo(),
 	)

internal/server/server_test.go

@@ -6,8 +6,10 @@ package server_test
 
 import (
 	"fmt"
+	"maps"
 	"net"
 	"net/http"
+	"net/http/httptest"
 	"strconv"
 	"strings"
 	"testing"
@@ -188,6 +190,88 @@ func TestServeAfterStopDoesNotStopApp(t *testing.T) {
 	}
 }
 
+// TestCORSHeaders checks the Access-Control-Allow-* headers on the
+// answers to requests from another origin. The public, read-only
+// routes answer with Access-Control-Allow-Origin: * alone. Routes
+// that need the cookie, and a preflight to one, answer with none.
+func TestCORSHeaders(t *testing.T) {
+	var srv *server.Server
+
+	startApp(t, fx.Populate(&srv))
+
+	tests := []struct {
+		name          string
+		method        string
+		path          string
+		requestMethod string
+		wantHeaders   map[string]string
+	}{
+		{
+			name:        "server information",
+			method:      http.MethodGet,
+			path:        "/api/v1/server",
+			wantHeaders: map[string]string{"Access-Control-Allow-Origin": "*"},
+		},
+		{
+			name:        "health check",
+			method:      http.MethodGet,
+			path:        "/.well-known/healthcheck.json",
+			wantHeaders: map[string]string{"Access-Control-Allow-Origin": "*"},
+		},
+		{
+			name:   "send",
+			method: http.MethodPost,
+			path:   "/api/v1/messages",
+		},
+		{
+			name:   "state",
+			method: http.MethodGet,
+			path:   "/api/v1/state",
+		},
+		{
+			name:          "preflight for send",
+			method:        http.MethodOptions,
+			path:          "/api/v1/messages",
+			requestMethod: http.MethodPost,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			request := httptest.NewRequestWithContext(
+				t.Context(), test.method,
+				"http://chat.example.com"+test.path, nil,
+			)
+			request.Header.Set("Origin", "http://blog.example.com")
+
+			if test.requestMethod != "" {
+				request.Header.Set(
+					"Access-Control-Request-Method", test.requestMethod,
+				)
+			}
+
+			recorder := httptest.NewRecorder()
+
+			srv.ServeHTTP(recorder, request)
+
+			gotHeaders := map[string]string{}
+
+			for name := range recorder.Header() {
+				if strings.HasPrefix(name, "Access-Control-Allow-") {
+					gotHeaders[name] = recorder.Header().Get(name)
+				}
+			}
+
+			if !maps.Equal(gotHeaders, test.wantHeaders) {
+				t.Errorf(
+					"Access-Control-Allow-* headers are %v, want %v",
+					gotHeaders, test.wantHeaders,
+				)
+			}
+		})
+	}
+}
+
 // startApp starts the app newApp builds, with the HTTP port
 // set to 0, so the system gives the server a free port.
 func startApp(