From 57057e64d1189f972f858423bcac6ec38ca7f0c3 Mon Sep 17 00:00:00 2001 From: Snider Date: Thu, 5 Feb 2026 01:34:13 +0000 Subject: [PATCH] fix(unifi): address CodeRabbit review feedback - Reject conflicting --wired and --wireless flags in clients command - Complete --type flag help text with bgp and ospf route types - URL-escape site name in routes API path - Wrap all command errors with log.E for contextual diagnostics - Set TLS MinVersion to 1.2 on UniFi client - Simplify redundant fmt.Sprintf in Print calls Co-Authored-By: Claude Opus 4.5 --- internal/cmd/unifi/cmd_clients.go | 12 +++++++++--- internal/cmd/unifi/cmd_devices.go | 8 ++++---- internal/cmd/unifi/cmd_networks.go | 5 +++-- internal/cmd/unifi/cmd_routes.go | 9 +++++---- internal/cmd/unifi/cmd_sites.go | 9 ++++----- pkg/unifi/client.go | 5 ++++- pkg/unifi/routes.go | 3 ++- 7 files changed, 31 insertions(+), 20 deletions(-) diff --git a/internal/cmd/unifi/cmd_clients.go b/internal/cmd/unifi/cmd_clients.go index 398aa3e2..69188ae9 100644 --- a/internal/cmd/unifi/cmd_clients.go +++ b/internal/cmd/unifi/cmd_clients.go @@ -1,9 +1,11 @@ package unifi import ( + "errors" "fmt" "github.com/host-uk/core/pkg/cli" + "github.com/host-uk/core/pkg/log" uf "github.com/host-uk/core/pkg/unifi" ) @@ -33,9 +35,13 @@ func addClientsCommand(parent *cli.Command) { } func runClients() error { + if clientsWired && clientsWireless { + return log.E("unifi.clients", "conflicting flags", errors.New("--wired and --wireless cannot both be set")) + } + client, err := uf.NewFromConfig("", "", "", "") if err != nil { - return err + return log.E("unifi.clients", "failed to initialise client", err) } clients, err := client.GetClients(uf.ClientFilter{ @@ -44,7 +50,7 @@ func runClients() error { Wireless: clientsWireless, }) if err != nil { - return err + return log.E("unifi.clients", "failed to fetch clients", err) } if len(clients) == 0 { @@ -79,7 +85,7 @@ func runClients() error { } cli.Blank() - cli.Print(" %s\n\n", fmt.Sprintf("%d clients", len(clients))) + cli.Print(" %d clients\n\n", len(clients)) table.Render() return nil diff --git a/internal/cmd/unifi/cmd_devices.go b/internal/cmd/unifi/cmd_devices.go index a9dc19d8..9cbbbe4d 100644 --- a/internal/cmd/unifi/cmd_devices.go +++ b/internal/cmd/unifi/cmd_devices.go @@ -1,10 +1,10 @@ package unifi import ( - "fmt" "strings" "github.com/host-uk/core/pkg/cli" + "github.com/host-uk/core/pkg/log" uf "github.com/host-uk/core/pkg/unifi" ) @@ -34,12 +34,12 @@ func addDevicesCommand(parent *cli.Command) { func runDevices() error { client, err := uf.NewFromConfig("", "", "", "") if err != nil { - return err + return log.E("unifi.devices", "failed to initialise client", err) } devices, err := client.GetDeviceList(devicesSite, strings.ToLower(devicesType)) if err != nil { - return err + return log.E("unifi.devices", "failed to fetch devices", err) } if len(devices) == 0 { @@ -67,7 +67,7 @@ func runDevices() error { } cli.Blank() - cli.Print(" %s\n\n", fmt.Sprintf("%d devices", len(devices))) + cli.Print(" %d devices\n\n", len(devices)) table.Render() return nil diff --git a/internal/cmd/unifi/cmd_networks.go b/internal/cmd/unifi/cmd_networks.go index d487f20a..67fc2c4f 100644 --- a/internal/cmd/unifi/cmd_networks.go +++ b/internal/cmd/unifi/cmd_networks.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/host-uk/core/pkg/cli" + "github.com/host-uk/core/pkg/log" uf "github.com/host-uk/core/pkg/unifi" ) @@ -31,12 +32,12 @@ func addNetworksCommand(parent *cli.Command) { func runNetworks() error { client, err := uf.NewFromConfig("", "", "", "") if err != nil { - return err + return log.E("unifi.networks", "failed to initialise client", err) } networks, err := client.GetNetworks(networksSite) if err != nil { - return err + return log.E("unifi.networks", "failed to fetch networks", err) } if len(networks) == 0 { diff --git a/internal/cmd/unifi/cmd_routes.go b/internal/cmd/unifi/cmd_routes.go index ed7faa87..e217c800 100644 --- a/internal/cmd/unifi/cmd_routes.go +++ b/internal/cmd/unifi/cmd_routes.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/host-uk/core/pkg/cli" + "github.com/host-uk/core/pkg/log" uf "github.com/host-uk/core/pkg/unifi" ) @@ -25,7 +26,7 @@ func addRoutesCommand(parent *cli.Command) { } cmd.Flags().StringVar(&routesSite, "site", "", "Site name (default: \"default\")") - cmd.Flags().StringVar(&routesType, "type", "", "Filter by route type (static, connected, kernel)") + cmd.Flags().StringVar(&routesType, "type", "", "Filter by route type (static, connected, kernel, bgp, ospf)") parent.AddCommand(cmd) } @@ -33,12 +34,12 @@ func addRoutesCommand(parent *cli.Command) { func runRoutes() error { client, err := uf.NewFromConfig("", "", "", "") if err != nil { - return err + return log.E("unifi.routes", "failed to initialise client", err) } routes, err := client.GetRoutes(routesSite) if err != nil { - return err + return log.E("unifi.routes", "failed to fetch routes", err) } // Filter by type if requested @@ -78,7 +79,7 @@ func runRoutes() error { } cli.Blank() - cli.Print(" %s\n\n", fmt.Sprintf("%d routes", len(routes))) + cli.Print(" %d routes\n\n", len(routes)) table.Render() return nil diff --git a/internal/cmd/unifi/cmd_sites.go b/internal/cmd/unifi/cmd_sites.go index 65c114ff..b55df2d5 100644 --- a/internal/cmd/unifi/cmd_sites.go +++ b/internal/cmd/unifi/cmd_sites.go @@ -1,9 +1,8 @@ package unifi import ( - "fmt" - "github.com/host-uk/core/pkg/cli" + "github.com/host-uk/core/pkg/log" uf "github.com/host-uk/core/pkg/unifi" ) @@ -24,12 +23,12 @@ func addSitesCommand(parent *cli.Command) { func runSites() error { client, err := uf.NewFromConfig("", "", "", "") if err != nil { - return err + return log.E("unifi.sites", "failed to initialise client", err) } sites, err := client.GetSites() if err != nil { - return err + return log.E("unifi.sites", "failed to fetch sites", err) } if len(sites) == 0 { @@ -47,7 +46,7 @@ func runSites() error { } cli.Blank() - cli.Print(" %s\n\n", fmt.Sprintf("%d sites", len(sites))) + cli.Print(" %d sites\n\n", len(sites)) table.Render() return nil diff --git a/pkg/unifi/client.go b/pkg/unifi/client.go index 2bbddb2f..0a6c61fe 100644 --- a/pkg/unifi/client.go +++ b/pkg/unifi/client.go @@ -28,7 +28,10 @@ func New(url, user, pass, apikey string) (*Client, error) { // Skip TLS verification for self-signed certs httpClient := &http.Client{ Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, //nolint:gosec + MinVersion: tls.VersionTLS12, + }, }, } diff --git a/pkg/unifi/routes.go b/pkg/unifi/routes.go index d3a2de89..6454b163 100644 --- a/pkg/unifi/routes.go +++ b/pkg/unifi/routes.go @@ -3,6 +3,7 @@ package unifi import ( "encoding/json" "fmt" + "net/url" "github.com/host-uk/core/pkg/log" ) @@ -31,7 +32,7 @@ func (c *Client) GetRoutes(siteName string) ([]Route, error) { siteName = "default" } - path := fmt.Sprintf("/api/s/%s/stat/routing", siteName) + path := fmt.Sprintf("/api/s/%s/stat/routing", url.PathEscape(siteName)) raw, err := c.api.GetJSON(path) if err != nil {