From f34705539f3db35304971751beaace3c8e9aeaa9 Mon Sep 17 00:00:00 2001 From: Joyjit Nath <96009+joyjit@users.noreply.github.com> Date: Tue, 25 Aug 2026 16:52:00 -0700 Subject: [PATCH] ci: get the lint job running again on golangci-lint v2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lint job has never passed. golangci-lint-action@v6 only knows golangci-lint v1, so `version: latest` resolved to v1.64.8, which is built with Go 1.24 and refuses to start against a go.mod targeting 1.25.0. It failed before reaching any code. Move to golangci-lint-action@v9 with golangci-lint v2.13.1, pinned rather than `latest` — floating is what silently picked a version incompatible with the toolchain. .golangci.yml is migrated to the v2 format (errcheck, govet, ineffassign, staticcheck and unused are v2 defaults; gofmt and goimports move to `formatters`), keeping the same effective linter set. With the linter actually running, it found 16 issues. Two were real: - api/firmware.go: upgradeInProgress was dead code. Both callers hold s.mu and use runningUpgradeLocked directly. Removed, and its note on the one-AP-at-a-time rule moved to the helper that survives. - driver/wax/pin_test.go: the path-stability check compared two identical calls inline, which staticcheck reads as a tautology. Same check, via variables, so the intent is legible. The rest are false positives against deliberate choices, each marked with the reason at the site: the sanitizer's 0644 fixtures, the detached upgrade goroutine, and the auth cookies, whose Secure flag is conditional so that login still works over plain HTTP on a LAN. gosec also flags the pinned-TLS setup for a resumption bypass that cannot happen here — Go only resumes when ClientSessionCache is set, and neither net/http nor we set one, so the pin is checked on every handshake. That is now written down next to the code. gosec's two taint-analysis rules are switched off. They are not deterministic in v2.13.1: on identical source with a cold cache, G703 reported three findings in api/history.go on roughly one run in three and none on the others. What they check is already enforced by core.ValidateDeviceName and covered by tests. Ten consecutive cold runs are now clean. --- .github/workflows/ci.yml | 4 +- .golangci.yml | 74 ++++++++++++++++++++++++++------- internal/api/auth.go | 4 ++ internal/api/firmware.go | 19 ++++----- internal/capture/sanitize.go | 2 + internal/driver/wax/client.go | 8 ++++ internal/driver/wax/pin_test.go | 12 ++++-- 7 files changed, 90 insertions(+), 33 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a917df2..8f95cd1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,6 +43,6 @@ jobs: with: go-version: '1.25' check-latest: true - - uses: golangci/golangci-lint-action@v6 + - uses: golangci/golangci-lint-action@v9 with: - version: latest + version: v2.13.1 diff --git a/.golangci.yml b/.golangci.yml index 4ca7c97..bd725b4 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,23 +1,65 @@ # Lint config for selfsight. Boring, mainstream linters only (DESIGN.md, # principle 5). Kept small so contributors aren't fighting the linter. -run: - timeout: 3m +# +# Format version 2 (golangci-lint v2+). Note that errcheck, govet, ineffassign, +# staticcheck and unused are on by default in v2, so they are not listed here; +# gofmt and goimports moved to their own `formatters` section. +version: "2" linters: enable: - - errcheck - - govet - - ineffassign - - staticcheck - - unused - - gofmt - - goimports - - misspell - gosec + - misspell + settings: + gosec: + # gosec's taint-analysis rules are off. Two reasons, in order: + # + # 1. They are not deterministic in v2.13.1. On identical source with a + # cold cache, G703 reported three findings in internal/api/history.go + # on roughly one run in three and none on the others. A check that + # fails at random is worse than no check — it teaches people to rerun + # CI until it goes green. + # 2. What they look for is already enforced, and tested. Every path they + # flagged is built from a device name that has passed + # core.ValidateDeviceName, which admits a single safe path element + # (letters, digits, dot, dash, underscore — no separators, no ".."), + # and config loading refuses anything else. See the traversal cases in + # internal/core/config_test.go. + # + # Worth revisiting when a later gosec makes these stable. + excludes: + - G703 # path traversal via taint analysis + - G706 # log injection via taint analysis + exclusions: + generated: lax + # These four are golangci-lint's own default exclusion sets, which v1 + # applied implicitly. Listed explicitly because v2 does not. + presets: + - comments + - common-false-positives + - legacy + - std-error-handling + rules: + # testdata holds sanitized device transcripts, not Go code. + - path: testdata/ + linters: [all] + # Tests stand up throwaway HTTP servers and hand them deliberately + # simple cookies and forms. gosec's hardening rules are aimed at + # production handlers and only add noise here. + - path: _test\.go + linters: [gosec] + paths: + - third_party$ + - builtin$ + - examples$ -issues: - exclude-rules: - # testdata holds sanitized device transcripts, not Go code. - - path: testdata/ - linters: - - all +formatters: + enable: + - gofmt + - goimports + exclusions: + generated: lax + paths: + - third_party$ + - builtin$ + - examples$ diff --git a/internal/api/auth.go b/internal/api/auth.go index b46d42e..3ff900e 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -230,6 +230,9 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { s.auth.sessions[token] = now.Add(sessionTTL) s.auth.mu.Unlock() + //nolint:gosec // G124: HttpOnly and SameSite are set; Secure is deliberately + // conditional. selfsight is commonly reached over plain HTTP on a LAN, where + // an unconditional Secure would stop the browser returning the cookie at all. http.SetCookie(w, &http.Cookie{ Name: sessionCookie, Value: token, Path: "/", HttpOnly: true, SameSite: http.SameSiteLaxMode, @@ -312,6 +315,7 @@ func (s *Server) handleLogout(w http.ResponseWriter, r *http.Request) { delete(s.auth.sessions, ck.Value) s.auth.mu.Unlock() } + //nolint:gosec // G124: see the note on the login cookie above. http.SetCookie(w, &http.Cookie{ Name: sessionCookie, Value: "", Path: "/", HttpOnly: true, SameSite: http.SameSiteLaxMode, diff --git a/internal/api/firmware.go b/internal/api/firmware.go index 9cb0fd9..bc088b2 100644 --- a/internal/api/firmware.go +++ b/internal/api/firmware.go @@ -52,20 +52,13 @@ func (s *Server) upgradeFor(name string) *upgradeState { return u } -// upgradeInProgress reports whether ANY device is mid-upgrade. Firmware -// upgrades run strictly one AP at a time across the whole fleet — flashing two -// APs at once risks taking down the network that carries the upgrade itself. +// runningUpgradeLocked names the device currently upgrading. Caller holds s.mu. // -// This is for reporting only (the progress endpoint). Deciding whether an -// upgrade may start must go through claimUpgrade: a separate check and act +// Firmware upgrades run strictly one AP at a time across the whole fleet — +// flashing two APs at once risks taking down the network that carries the +// upgrade itself. Deciding whether an upgrade may start must go through +// claimUpgrade rather than calling this directly: a separate check and act // leaves a window in which two requests both see an idle fleet. -func (s *Server) upgradeInProgress() (string, bool) { - s.mu.Lock() - defer s.mu.Unlock() - return s.runningUpgradeLocked() -} - -// runningUpgradeLocked names the device currently upgrading. Caller holds s.mu. func (s *Server) runningUpgradeLocked() (string, bool) { for name, u := range s.upgrades { u.mu.Lock() @@ -254,6 +247,8 @@ func (s *Server) handleFirmwareUpgrade(w http.ResponseWriter, r *http.Request) { dm := s.managerFor(dev) backupDir := s.deviceDataDir(dev.Name) + //nolint:gosec // G118: detaching from the request context is the point — + // see the comment below. go func() { // Detached from the HTTP request: the upgrade outlives it by minutes. ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute) diff --git a/internal/capture/sanitize.go b/internal/capture/sanitize.go index 9455082..0ca444a 100644 --- a/internal/capture/sanitize.go +++ b/internal/capture/sanitize.go @@ -92,6 +92,8 @@ Then eyeball every output file before committing (git history is permanent). return fmt.Errorf("read %s: %w", e.Name(), err) } clean := s.scrub(raw) + //nolint:gosec // G306: sanitized fixtures are committed to the repo and + // meant to be world-readable; they contain no secrets by construction. if err := os.WriteFile(filepath.Join(*outDir, e.Name()), clean, 0o644); err != nil { return fmt.Errorf("write %s: %w", e.Name(), err) } diff --git a/internal/driver/wax/client.go b/internal/driver/wax/client.go index d0ae111..bc4e4c8 100644 --- a/internal/driver/wax/client.go +++ b/internal/driver/wax/client.go @@ -73,6 +73,14 @@ func New(host string, opts ...Option) *Client { // Pinning hooks into our own transport; a caller-supplied client (tests) // brings its own trust and is left alone. if c.pinPath != "" && c.http == own { + // VerifyPeerCertificate is skipped on a resumed TLS session, which + // would let a resumed connection dodge the pin check. It cannot happen + // here: Go only resumes when tls.Config.ClientSessionCache is set, + // net/http never sets one, and neither do we — so every connection is + // a full handshake and the pin is always checked. Keep it that way; if + // a session cache is ever added, move this to VerifyConnection, which + // runs on resumed handshakes too. + //nolint:gosec // G123: resumption is off, see above. tr.TLSClientConfig.VerifyPeerCertificate = c.verifyPin } return c diff --git a/internal/driver/wax/pin_test.go b/internal/driver/wax/pin_test.go index 8f981ea..6b13885 100644 --- a/internal/driver/wax/pin_test.go +++ b/internal/driver/wax/pin_test.go @@ -77,9 +77,15 @@ func TestCachePathsAreSafeAndStable(t *testing.T) { seen[p] = host } } - // Stable, so a device keeps its own cache across restarts. - if PinPathIn(base, "ap1") != PinPathIn(base, "ap1") { - t.Error("a host's cache path must not change between calls") + // Stable, so a device keeps its own cache across restarts. Held in + // variables rather than compared inline: two identical calls side by side + // read as a tautology (and staticcheck flags them as one), but the point + // is that the path is derived from the host alone, with nothing random or + // time-based mixed in. + first := PinPathIn(base, "ap1") + second := PinPathIn(base, "ap1") + if first != second { + t.Errorf("a host's cache path must not change between calls: %s then %s", first, second) } // A pin and a session are separate files. if PinPathIn(base, "ap1") == SessionPathIn(base, "ap1") {