fix(net): validate hop range before checking availability — #2453
test-go / test (push) Successful in 54s
test-go / integration (push) Successful in 5m1s

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.
This commit is contained in:
2026-08-05 10:27:10 -04:00
parent d5ab3b0764
commit 11538095be
+7 -3
View File
@@ -78,15 +78,19 @@ func (s *Service) Hops() int {
// SetHops persists a new depth and refreshes the cache, so an admin change // SetHops persists a new depth and refreshes the cache, so an admin change
// takes effect on the next request with no restart (rule #25). // takes effect on the next request with no restart (rule #25).
func (s *Service) SetHops(ctx context.Context, hops int) error { 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 { if s == nil || s.pool == nil {
// Mirrors Hops()'s nil-tolerance: handlers can be constructed without // Mirrors Hops()'s nil-tolerance: handlers can be constructed without
// this service in tests, and a write attempt there should be an error // this service in tests, and a write attempt there should be an error
// rather than a panic in an HTTP handler. // rather than a panic in an HTTP handler.
return errors.New("network settings unavailable") return errors.New("network settings unavailable")
} }
if hops < 0 || hops > MaxTrustedProxyHops {
return ErrHopsOutOfRange
}
row, err := dbq.New(s.pool).UpdateTrustedProxyHops(ctx, int32(hops)) row, err := dbq.New(s.pool).UpdateTrustedProxyHops(ctx, int32(hops))
if err != nil { if err != nil {
return err return err