web: validate the flag configuration in Serve and ServeMultiple

web/serve_flags_test.go

@@ -0,0 +1,73 @@
+// Copyright The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package web
+
+import (
+	"errors"
+	"net"
+	"net/http"
+	"testing"
+)
+
+func testListener(t *testing.T) net.Listener {
+	t.Helper()
+	l, err := net.Listen("tcp", "localhost:0")
+	if err != nil {
+		t.Fatalf("Unable to listen: %v", err)
+	}
+	t.Cleanup(func() { l.Close() })
+	return l
+}
+
+// TestServeRejectsIncompleteFlagConfig checks that Serve and ServeMultiple
+// report an incomplete FlagConfig instead of dereferencing a nil field. They
+// are exported for callers that create their own listeners, and those callers
+// build a FlagConfig by hand rather than getting one from kingpinflag.
+func TestServeRejectsIncompleteFlagConfig(t *testing.T) {
+	for _, tc := range []struct {
+		name  string
+		flags *FlagConfig
+	}{
+		{name: "nil FlagConfig", flags: nil},
+		{name: "no WebConfigFile", flags: &FlagConfig{}},
+		{name: "no WebConfigFile with listen addresses", flags: &FlagConfig{
+			WebListenAddresses: &[]string{"localhost:0"},
+			WebSystemdSocket:   new(false),
+		}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Run("Serve", func(t *testing.T) {
+				err := Serve(testListener(t), &http.Server{}, tc.flags, testlogger)
+				if !errors.Is(err, ErrMissingFlag) {
+					t.Errorf("Serve() = %v, expected %v", err, ErrMissingFlag)
+				}
+			})
+			t.Run("ServeMultiple", func(t *testing.T) {
+				err := ServeMultiple([]net.Listener{testListener(t)}, &http.Server{}, tc.flags, testlogger)
+				if !errors.Is(err, ErrMissingFlag) {
+					t.Errorf("ServeMultiple() = %v, expected %v", err, ErrMissingFlag)
+				}
+			})
+		})
+	}
+}
+
+// TestServeDoesNotRequireListenAddresses checks that Serve accepts a
+// FlagConfig without listener fields. The caller passes the listener in, so
+// requiring them would reject a legitimate configuration.
+func TestServeDoesNotRequireListenAddresses(t *testing.T) {
+	if err := (&FlagConfig{WebConfigFile: new("")}).checkWebConfigFlag(); err != nil {
+		t.Errorf("checkWebConfigFlag() = %v, expected nil", err)
+	}
+}

web/tls_config.go

@@ -81,14 +81,21 @@ type FlagConfig struct {
 	WebConfigFile *string
 }
 
+// checkWebConfigFlag validates that the flag configuration contains the web
+// config field. Serve and ServeMultiple only need this much, because their
+// caller supplies the listeners.
+func (c *FlagConfig) checkWebConfigFlag() error {
+	if c == nil || c.WebConfigFile == nil {
+		return ErrMissingFlag
+	}
+	return nil
+}
+
 // checkFlags validates that the flag configuration contains the required
 // listener and web config fields needed by the web package.
 func (c *FlagConfig) checkFlags() error {
-	if c == nil {
-		return ErrMissingFlag
-	}
-	if c.WebConfigFile == nil {
-		return ErrMissingFlag
+	if err := c.checkWebConfigFlag(); err != nil {
+		return err
 	}
 	// Listen addresses are only optional when systemd socket activation is
 	// actually enabled. Checking that WebSystemdSocket is non-nil is not
@@ -413,6 +420,9 @@ func Serve(l net.Listener, server *http.Server, flags *FlagConfig, logger *slog.
 }
 
 func configureServer(server *http.Server, flags *FlagConfig, logger *slog.Logger) (*HTTPConfig, error) {
+	if err := flags.checkWebConfigFlag(); err != nil {
+		return nil, err
+	}
 	setErrorLog(server, logger)
 	tlsConfigPath := *flags.WebConfigFile
 	if tlsConfigPath == "" {