From a236bc94b335011a20ed510724f87244245295bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20R=C3=BCger?= Date: Wed, 2 Sep 2026 14:28:17 +0200 Subject: [PATCH] web: validate the flag configuration in Serve and ServeMultiple MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ListenAndServe validates its FlagConfig before using it, but Serve and ServeMultiple went straight to dereferencing flags.WebConfigFile. Both are exported for callers that create their own listeners, and such a caller builds a FlagConfig by hand rather than getting one from kingpinflag, so passing one without WebConfigFile set is an easy mistake. It crashed the process with a nil pointer dereference instead of returning ErrMissingFlag. Check the flags in both, using a narrower check than checkFlags. The listener fields are deliberately not required here: the caller supplies the listener, so demanding a listen address or the systemd socket flag would reject a legitimate configuration. checkFlags keeps requiring them for ListenAndServe, which does create the listeners itself, and now shares the web config half of the validation. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Manuel Rüger --- web/serve_flags_test.go | 82 +++++++++++++++++++++++++++++++++++++++++ web/tls_config.go | 22 +++++++++-- 2 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 web/serve_flags_test.go diff --git a/web/serve_flags_test.go b/web/serve_flags_test.go new file mode 100644 index 00000000..8050e914 --- /dev/null +++ b/web/serve_flags_test.go @@ -0,0 +1,82 @@ +// 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: OfBool(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: OfString("")}).checkWebConfigFlag(); err != nil { + t.Errorf("checkWebConfigFlag() = %v, expected nil", err) + } +} + +// TestCheckFlagsStillRequiresListeners checks that the listener requirement is +// unchanged for ListenAndServe, which does create the listeners. +func TestCheckFlagsStillRequiresListeners(t *testing.T) { + flags := &FlagConfig{WebConfigFile: OfString("")} + if err := flags.checkFlags(); !errors.Is(err, ErrNoListeners) { + t.Errorf("checkFlags() = %v, expected %v", err, ErrNoListeners) + } +} diff --git a/web/tls_config.go b/web/tls_config.go index b40be6bb..381e1ce1 100644 --- a/web/tls_config.go +++ b/web/tls_config.go @@ -75,15 +75,25 @@ type FlagConfig struct { WebConfigFile *string } -// checkFlags validates that the flag configuration contains the required -// listener and web config fields needed by the web package. -func (c *FlagConfig) checkFlags() error { +// checkWebConfigFlag validates the fields needed to apply the web +// configuration. It deliberately does not require the listener fields, because +// Serve and ServeMultiple are handed their listeners by the caller. +func (c *FlagConfig) checkWebConfigFlag() error { if c == nil { return ErrMissingFlag } if 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 err := c.checkWebConfigFlag(); err != nil { + return err + } if c.WebSystemdSocket == nil && (c.WebListenAddresses == nil || len(*c.WebListenAddresses) == 0) { return ErrNoListeners } @@ -302,6 +312,9 @@ func ConfigToTLSConfig(c *TLSConfig) (*tls.Config, error) { // ServeMultiple starts the server on the given listeners. The FlagConfig is // also passed on to Serve. func ServeMultiple(listeners []net.Listener, server *http.Server, flags *FlagConfig, logger *slog.Logger) error { + if err := flags.checkWebConfigFlag(); err != nil { + return err + } errs := new(errgroup.Group) for _, l := range listeners { errs.Go(func() error { @@ -378,6 +391,9 @@ func parseVsockPort(address string) (uint32, error) { // Server starts the server on the given listener. Based on the file path // WebConfigFile in the FlagConfig, TLS or basic auth could be enabled. func Serve(l net.Listener, server *http.Server, flags *FlagConfig, logger *slog.Logger) error { + if err := flags.checkWebConfigFlag(); err != nil { + return err + } logger.Info("Listening on", "address", l.Addr().String()) tlsConfigPath := *flags.WebConfigFile if tlsConfigPath == "" {