From 11538095be8f4789ed5dbbce5549237f5ae3f2be Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 5 Aug 2026 10:27:10 -0400 Subject: [PATCH] =?UTF-8?q?fix(net):=20validate=20hop=20range=20before=20c?= =?UTF-8?q?hecking=20availability=20=E2=80=94=20#2453?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestSetHops_RejectsOutOfRange caught a real ordering bug in code I wrote in the same commit: the nil-pool guard sat ahead of the range check, so SetHops(-1) on a service with no pool returned "network settings unavailable" instead of ErrHopsOutOfRange. Range first is correct, and the distinction is user-visible rather than cosmetic: the argument is invalid regardless of whether the database is reachable, and admin_network.go maps ErrHopsOutOfRange to 400 while anything else becomes 500. The old order blamed the server for the caller's input. Note this is the first failure in this sequence that wasn't a missed call site — vet and golangci-lint both passed, and a test asserting a specific sentinel error found it. Worth the extra assertion; `err != nil` would have passed happily. --- internal/netsettings/service.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/netsettings/service.go b/internal/netsettings/service.go index 27bcff40..9b526178 100644 --- a/internal/netsettings/service.go +++ b/internal/netsettings/service.go @@ -78,15 +78,19 @@ func (s *Service) Hops() int { // SetHops persists a new depth and refreshes the cache, so an admin change // takes effect on the next request with no restart (rule #25). func (s *Service) SetHops(ctx context.Context, hops int) error { + // Range first, availability second. The argument is wrong regardless of + // whether the database is reachable, and the distinction is user-visible: + // this ordering answers 400 for a bad value, where the reverse would + // report 500 and blame the server for the caller's input. + if hops < 0 || hops > MaxTrustedProxyHops { + return ErrHopsOutOfRange + } if s == nil || s.pool == nil { // Mirrors Hops()'s nil-tolerance: handlers can be constructed without // this service in tests, and a write attempt there should be an error // rather than a panic in an HTTP handler. return errors.New("network settings unavailable") } - if hops < 0 || hops > MaxTrustedProxyHops { - return ErrHopsOutOfRange - } row, err := dbq.New(s.pool).UpdateTrustedProxyHops(ctx, int32(hops)) if err != nil { return err