fix: harden config loading and ssh addresses, stop resolver on API errors

- read netssh.yaml from the user config directory only, not from the
  working directory
- refuse addresses from NetBox or the cache that are neither an IP
  address nor a hostname (ssh read "-oProxyCommand=…" as an option)
- resolver chain stops at an API failure; only "no address" falls
  through to the next strategy
- netssh --help and -h show netssh's help
- start at most one background cache refresh per 5 minutes
- validate the legacy netbox.url
- page through all IP addresses of a host
- CI: run format check, vet, and tests on pull requests and main
- changelog for v0.0.25

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

internal/cache/cache.go

@@ -83,6 +83,26 @@ func (c *Cache) NeedsRefresh(d time.Duration) bool {
 	return time.Since(t) >= d
 }
 
+// ClaimRefresh reports whether a background refresh should start now: the last
+// full refresh is older than d and no attempt was claimed within retry. It
+// records the attempt, so parallel shells and repeated commands start one
+// refresh per retry interval instead of one each.
+func (c *Cache) ClaimRefresh(d, retry time.Duration) bool {
+	if !c.NeedsRefresh(d) {
+		return false
+	}
+	attempt := filepath.Join(filepath.Dir(c.refreshStamp), "refresh_attempt")
+	if info, err := os.Stat(attempt); err == nil && time.Since(info.ModTime()) < retry {
+		return false
+	}
+	if err := os.MkdirAll(filepath.Dir(attempt), 0o755); err != nil {
+		return true // cannot record the attempt; refreshing is still right
+	}
+	// Best effort: two processes racing here may both refresh once.
+	_ = os.WriteFile(attempt, nil, 0o600)
+	return true
+}
+
 // SetRefreshed records the current time as the last successful full refresh.
 func (c *Cache) SetRefreshed() error {
 	if c.refreshStamp == "" {

internal/cache/cache_test.go

@@ -557,3 +557,23 @@ func TestStaticHosts(t *testing.T) {
 		t.Fatalf("removed static host still on disk:\n%s", data)
 	}
 }
+
+func TestClaimRefreshThrottlesAttempts(t *testing.T) {
+	c := New(filepath.Join(t.TempDir(), "netssh", "hosts.json"), 60)
+	if !c.ClaimRefresh(24*time.Hour, time.Minute) {
+		t.Fatal("a never refreshed cache must claim a refresh")
+	}
+	// NetBox is down: the stamp stays stale, but the attempt is remembered.
+	if c.ClaimRefresh(24*time.Hour, time.Minute) {
+		t.Error("a second refresh within the retry interval must not start")
+	}
+	if !c.ClaimRefresh(24*time.Hour, 0) {
+		t.Error("after the retry interval a refresh must start again")
+	}
+	if err := c.SetRefreshed(); err != nil {
+		t.Fatal(err)
+	}
+	if c.ClaimRefresh(24*time.Hour, 0) {
+		t.Error("a freshly refreshed cache must not claim a refresh")
+	}
+}

internal/config/config.go

@@ -113,8 +113,9 @@ func Load() (*Config, error) {
 	if err == nil {
 		v.SetConfigName("netssh")
 		v.SetConfigType("yaml")
-		v.AddConfigPath(filepath.Join(configDir))
-		v.AddConfigPath(".")
+		// Only the user's own directory: a netssh.yaml in the working directory
+		// (e.g. of a cloned repository) could inject ssh options and a NetBox URL.
+		v.AddConfigPath(configDir)
 	}
 
 	v.SetEnvPrefix("NETSSH")

internal/config/config_test.go

@@ -196,3 +196,27 @@ func TestLoad_InvalidShortcutRewrite(t *testing.T) {
 		}
 	}
 }
+
+// A netssh.yaml in the working directory (e.g. of a cloned repository) must
+// not be loaded: it could inject ssh options such as ProxyCommand.
+func TestLoad_IgnoresConfigInWorkingDirectory(t *testing.T) {
+	t.Setenv("XDG_CONFIG_HOME", t.TempDir())
+	t.Setenv("NETSSH_NETBOX_URL", "")
+	t.Chdir(writeConfig(t, "hosts:\n  - name: evil\n    address: 10.0.0.1\nssh:\n  options:\n    ProxyCommand: touch /tmp/pwned\n"))
+	cfg, err := Load()
+	if err != nil {
+		t.Fatalf("Load: %v", err)
+	}
+	if len(cfg.Hosts) != 0 || len(cfg.SSH.Options) != 0 {
+		t.Errorf("config from the working directory was loaded: %+v", cfg)
+	}
+}
+
+func TestLoad_RejectsInvalidLegacyNetBoxURL(t *testing.T) {
+	for _, bad := range []string{"netbox.example.com", "ftp://netbox.example.com", "https://netbox.example.com/?x=1"} {
+		t.Setenv("XDG_CONFIG_HOME", writeConfig(t, "netbox:\n  url: \""+bad+"\"\n  token: t\n"))
+		if _, err := Load(); err == nil || !strings.Contains(err.Error(), "netbox.url") {
+			t.Errorf("%s: got %v, want a netbox.url error", bad, err)
+		}
+	}
+}

internal/config/instances.go

@@ -17,6 +17,9 @@ func (c *Config) ValidateNetBoxes() error {
 	if len(c.NetBoxes) > 0 && c.NetBox.URL != "" {
 		return fmt.Errorf("configure either netbox or netboxes, not both")
 	}
+	if c.NetBox.URL != "" && !validNetBoxURL(c.NetBox.URL) {
+		return fmt.Errorf("netbox.url %q must be an HTTP(S) URL without query or fragment", c.NetBox.URL)
+	}
 	seen := make(map[string]bool)
 	for _, source := range c.NetBoxes {
 		if !instanceName.MatchString(source.Name) {
@@ -27,8 +30,7 @@ func (c *Config) ValidateNetBoxes() error {
 			return fmt.Errorf("duplicate NetBox instance name %q", source.Name)
 		}
 		seen[name] = true
-		u, err := url.Parse(source.URL)
-		if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" || u.RawQuery != "" || u.Fragment != "" {
+		if !validNetBoxURL(source.URL) {
 			return fmt.Errorf("NetBox %q requires an HTTP(S) URL without query or fragment", source.Name)
 		}
 		if source.TokenVersion < 0 || source.TokenVersion > 2 {
@@ -38,6 +40,12 @@ func (c *Config) ValidateNetBoxes() error {
 	return nil
 }
 
+// validNetBoxURL accepts base URLs that API paths can be appended to.
+func validNetBoxURL(raw string) bool {
+	u, err := url.Parse(raw)
+	return err == nil && (u.Scheme == "http" || u.Scheme == "https") && u.Host != "" && u.RawQuery == "" && u.Fragment == ""
+}
+
 func (c *Config) HasNetBox() bool { return c.NetBox.URL != "" || len(c.NetBoxes) > 0 }
 
 // HasHosts reports whether netssh has anything to connect to: NetBox or static hosts.

internal/netbox/client.go

@@ -176,24 +176,32 @@ func (c *Client) GetIPs(ctx context.Context, entry HostEntry) ([]string, error)
 		}
 		return source.GetIPs(ctx, host)
 	}
-	var apiURL string
 	switch entry.Kind {
 	case "device":
-		apiURL = fmt.Sprintf("%s/api/ipam/ip-addresses/?device_id=%d&limit=100", c.baseURL, entry.ID)
+		return c.fetchAllIPs(ctx, fmt.Sprintf("device_id=%d", entry.ID))
 	case "vm":
-		apiURL = fmt.Sprintf("%s/api/ipam/ip-addresses/?virtual_machine_id=%d&limit=100", c.baseURL, entry.ID)
-	default:
-		return nil, fmt.Errorf("unknown host kind: %q", entry.Kind)
+		return c.fetchAllIPs(ctx, fmt.Sprintf("virtual_machine_id=%d", entry.ID))
 	}
+	return nil, fmt.Errorf("unknown host kind: %q", entry.Kind)
+}
 
-	var resp netboxIPListResponse
-	if err := c.get(ctx, apiURL, &resp); err != nil {
-		return nil, err
-	}
-
-	ips := make([]string, 0, len(resp.Results))
-	for _, r := range resp.Results {
-		ips = append(ips, stripPrefix(r.Address))
+// fetchAllIPs pages through the IP addresses matching filterParams, so hosts
+// with more addresses than one page do not lose their management address.
+func (c *Client) fetchAllIPs(ctx context.Context, filterParams string) ([]string, error) {
+	const pageSize = 100
+	var ips []string
+	for offset := 0; ; offset += pageSize {
+		apiURL := fmt.Sprintf("%s/api/ipam/ip-addresses/?%s&limit=%d&offset=%d", c.baseURL, filterParams, pageSize, offset)
+		var resp netboxIPListResponse
+		if err := c.get(ctx, apiURL, &resp); err != nil {
+			return nil, err
+		}
+		for _, r := range resp.Results {
+			ips = append(ips, stripPrefix(r.Address))
+		}
+		if len(resp.Results) < pageSize || len(ips) >= resp.Count {
+			break
+		}
 	}
 	return ips, nil
 }
@@ -203,16 +211,7 @@ func (c *Client) GetIPsWithFilter(ctx context.Context, filterParams string) ([]s
 	if c.sources != nil {
 		return nil, fmt.Errorf("IP filters require a host instance; use GetIPsWithFilterForHost")
 	}
-	apiURL := fmt.Sprintf("%s/api/ipam/ip-addresses/?%s&limit=100", c.baseURL, filterParams)
-	var resp netboxIPListResponse
-	if err := c.get(ctx, apiURL, &resp); err != nil {
-		return nil, err
-	}
-	ips := make([]string, 0, len(resp.Results))
-	for _, r := range resp.Results {
-		ips = append(ips, stripPrefix(r.Address))
-	}
-	return ips, nil
+	return c.fetchAllIPs(ctx, filterParams)
 }
 
 func (c *Client) searchDevices(ctx context.Context, query, tag string) ([]HostEntry, error) {
@@ -348,7 +347,7 @@ func (c *Client) request(ctx context.Context, method, apiURL string, body io.Rea
 	}
 
 	if resp.StatusCode != http.StatusOK {
-		return "", fmt.Errorf("netbox returned %d for %s", resp.StatusCode, apiURL)
+		return "", &StatusError{Code: resp.StatusCode, URL: apiURL}
 	}
 
 	if err := json.NewDecoder(resp.Body).Decode(out); err != nil {
@@ -399,6 +398,16 @@ func stripPrefix(cidr string) string {
 	return cidr
 }
 
+// StatusError is an unexpected HTTP status from NetBox.
+type StatusError struct {
+	Code int
+	URL  string
+}
+
+func (e *StatusError) Error() string {
+	return fmt.Sprintf("netbox returned %d for %s", e.Code, e.URL)
+}
+
 // ErrHostNotFound distinguishes a missing exact match from API failures or ambiguity.
 var ErrHostNotFound = errors.New("host not found")
 

internal/netbox/client_test.go

@@ -3,8 +3,11 @@ package netbox
 import (
 	"context"
 	"encoding/json"
+	"errors"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"strconv"
 	"strings"
 	"testing"
 )
@@ -499,3 +502,45 @@ func TestCommentConflictIsReported(t *testing.T) {
 		t.Fatal("conflict ignored")
 	}
 }
+
+// Hosts with more addresses than one page must not lose the later ones.
+func TestGetIPs_Paginates(t *testing.T) {
+	const total = 230
+	var offsets []string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		offsets = append(offsets, r.URL.Query().Get("offset"))
+		offset, _ := strconv.Atoi(r.URL.Query().Get("offset"))
+		limit, _ := strconv.Atoi(r.URL.Query().Get("limit"))
+		var addrs []string
+		for i := offset; i < offset+limit && i < total; i++ {
+			addrs = append(addrs, fmt.Sprintf("10.0.%d.%d/24", i/250, i%250+1))
+		}
+		resp := ipListResponse(addrs...)
+		resp.Count = total
+		json.NewEncoder(w).Encode(resp)
+	}))
+	defer srv.Close()
+
+	ips, err := NewClient(srv.URL, "token", 0).GetIPs(context.Background(), HostEntry{ID: 1, Kind: "device"})
+	if err != nil {
+		t.Fatalf("GetIPs: %v", err)
+	}
+	if len(ips) != total || ips[total-1] != "10.0.0.230" {
+		t.Errorf("got %d IPs (last %q), want %d", len(ips), ips[len(ips)-1], total)
+	}
+	if strings.Join(offsets, ",") != "0,100,200" {
+		t.Errorf("offsets %v, want 0,100,200", offsets)
+	}
+}
+
+func TestRequest_StatusErrorCarriesCode(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusBadRequest)
+	}))
+	defer srv.Close()
+	_, err := NewClient(srv.URL, "token", 0).GetIPsWithFilter(context.Background(), "device_id=1&interface=nope")
+	var status *StatusError
+	if !errors.As(err, &status) || status.Code != http.StatusBadRequest {
+		t.Fatalf("got %v, want a StatusError with code 400", err)
+	}
+}

internal/resolver/chain.go

@@ -2,6 +2,7 @@ package resolver
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"git.zb-server.de/Sebi/ssh-netbox-wrapper/internal/config"
@@ -32,6 +33,12 @@ func (c *Chain) Resolve(ctx context.Context, entry *netbox.HostEntry, client *ne
 		if err == nil {
 			return ip, nil
 		}
+		// Only "this strategy has no address" moves on. After an API failure
+		// the next strategy would connect to (and cache) a different address
+		// than the configured order asks for.
+		if !errors.Is(err, ErrNoIP) {
+			return "", fmt.Errorf("resolver strategy %q for %q: %w", s.Name(), entry.Name, err)
+		}
 	}
 	return "", fmt.Errorf("no strategy resolved an IP for %q", entry.Name)
 }

internal/resolver/chain_test.go

@@ -153,3 +153,17 @@ func TestNew_MultipleStrategies(t *testing.T) {
 		t.Errorf("got %d strategies, want 2", len(c.strategies))
 	}
 }
+
+// An API failure must not fall through: the next strategy would connect to
+// and cache a different address than the configured order asks for.
+func TestChain_APIErrorStopsTheChain(t *testing.T) {
+	apiErr := errors.New("connection refused")
+	c := &Chain{strategies: []Strategy{
+		&stubStrategy{name: "management_subnet", err: apiErr},
+		&stubStrategy{name: "primary_ip", ip: "10.0.0.2"},
+	}}
+	ip, err := c.Resolve(context.Background(), &netbox.HostEntry{Name: "host"}, nil)
+	if !errors.Is(err, apiErr) || ip != "" {
+		t.Fatalf("got %q, %v; want the API error and no address", ip, err)
+	}
+}

internal/resolver/interface_name.go

@@ -2,7 +2,9 @@ package resolver
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"net/http"
 	"net/url"
 
 	"git.zb-server.de/Sebi/ssh-netbox-wrapper/internal/netbox"
@@ -31,6 +33,10 @@ func (s *InterfaceNameStrategy) Resolve(ctx context.Context, entry *netbox.HostE
 	}
 
 	ips, err := client.GetIPsWithFilterForHost(ctx, *entry, filterParam)
+	var status *netbox.StatusError
+	if errors.As(err, &status) && status.Code == http.StatusBadRequest {
+		return "", ErrNoIP // the host has no interface of this name
+	}
 	if err != nil {
 		return "", fmt.Errorf("fetching IPs for interface %q: %w", s.name, err)
 	}

internal/resolver/interface_name_test.go

@@ -2,6 +2,7 @@ package resolver
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -54,3 +55,16 @@ func urlQuery(s string) (string, error) {
 	q, err := url.ParseQuery(s)
 	return q.Encode(), err
 }
+
+// Only the HTTP 400 of a missing interface means "no address here"; a server
+// error must stop the chain instead of silently using the next strategy.
+func TestInterfaceNameStrategyServerErrorIsNotNoIP(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusBadGateway)
+	}))
+	defer srv.Close()
+	_, err := (&InterfaceNameStrategy{name: "mgmt0"}).Resolve(context.Background(), &netbox.HostEntry{ID: 7, Kind: "device"}, netbox.NewClient(srv.URL, "t", 1))
+	if err == nil || errors.Is(err, ErrNoIP) {
+		t.Fatalf("got %v, want an API error", err)
+	}
+}

internal/ssh/options.go

@@ -2,6 +2,7 @@ package ssh
 
 import (
 	"fmt"
+	"net/netip"
 	"regexp"
 	"sort"
 	"strings"
@@ -142,6 +143,18 @@ type Result struct {
 	Notice string
 }
 
+var destinationName = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]*$`)
+
+// ValidAddress reports whether addr is an IP address or a hostname. Addresses
+// come from NetBox and the cache file; one such as "-oProxyCommand=…" would be
+// read by ssh as an option and run a command.
+func ValidAddress(addr string) bool {
+	if _, err := netip.ParseAddr(addr); err == nil {
+		return !strings.ContainsAny(addr, " \t\r\n")
+	}
+	return destinationName.MatchString(addr)
+}
+
 var plainHostname = regexp.MustCompile(`^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$`)
 
 // Apply replaces the destination at destIdx and inserts the configured -o

internal/ssh/options_test.go

@@ -128,3 +128,17 @@ func TestQuote(t *testing.T) {
 		t.Fatalf("got %s", got)
 	}
 }
+
+func TestValidAddress(t *testing.T) {
+	for _, addr := range []string{"10.0.0.1", "fd00:30::60", "fe80::1%eth0", "web01", "web01.example.com", "db_1.lan"} {
+		if !ValidAddress(addr) {
+			t.Errorf("ValidAddress(%q) = false, want true", addr)
+		}
+	}
+	// ssh would read the first as an option and run the command.
+	for _, addr := range []string{"-oProxyCommand=touch /tmp/pwned", "-J", "", "a b", "host;id", "user@host", "host/path", "fe80::1%eth0 x", "$(id)"} {
+		if ValidAddress(addr) {
+			t.Errorf("ValidAddress(%q) = true, want false", addr)
+		}
+	}
+}

internal/storage/file_test.go

@@ -0,0 +1,29 @@
+package storage
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestWriteFileReplacesAtomically(t *testing.T) {
+	dir := filepath.Join(t.TempDir(), "new", "dir")
+	path := filepath.Join(dir, "file")
+	for _, content := range []string{"first", "second"} {
+		if err := WriteFile(path, []byte(content), 0o600); err != nil {
+			t.Fatal(err)
+		}
+		got, err := os.ReadFile(path)
+		if err != nil || string(got) != content {
+			t.Fatalf("got %q, %v; want %q", got, err, content)
+		}
+	}
+	info, err := os.Stat(path)
+	if err != nil || info.Mode().Perm() != 0o600 {
+		t.Errorf("mode %v, %v; want 0600", info.Mode().Perm(), err)
+	}
+	entries, _ := os.ReadDir(dir)
+	if len(entries) != 1 {
+		t.Errorf("temporary files left behind: %v", entries)
+	}
+}