From f15db0e20a5b27f3a26acff6b8df92dc878e2b2d Mon Sep 17 00:00:00 2001 From: Niklas Ye Date: Wed, 7 Oct 2026 22:12:31 +0200 Subject: [PATCH] Add gosec to CI; fix the one real finding it surfaced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of the same security-hardening pass as the last two commits. make lint was go vet only; govulncheck and gitleaks already scanned deps and secrets on every push, but nothing read this repo's own source for risky patterns (weak crypto, injection shapes, insecure cookies, ...). New `make security-code` runs gosec, wired into ci.yaml's security job alongside the other two. G104 (unchecked error) is excluded at the Makefile level: every one of its 41 initial hits was this codebase's existing, deliberate idiom for a best-effort write or an already- reviewed json.Unmarshal of its own JSONB, predating gosec, and the rule cannot tell that apart from a mistake -- seventeen individual #nosec comments would hide a future real G104 regression in the suppression noise rather than surface it. Reasoning is on the Makefile target. Of the 12 remaining hits: - Genuinely real: oidc.go's callback logged error_description (and, two call sites down, identity.Subject) via %s before the request's state was even checked against its cookie -- an attacker-reachable value going into the log unquoted. Switched to %q, matching identity.Username's existing treatment, so a value holding a newline can't forge a second log line. - False positives, annotated inline rather than globally suppressed: 4x G124 on cookies that already set Secure via cookieSecure(...) (a function call, not the literal `true` the rule wants), 3x G202 on sqlArgs-built queries that only ever splice in a "$N" placeholder, never a value, and the remaining 5x G706 on log lines that were already %q-quoted -- gosec's taint analysis doesn't model format verbs, so it flags the tainted argument regardless. Also fixed handleMe's swallowed Scan error (gosec's catch, pre-fix): a transient DB error left hash/dismissed at their zero values and the response claimed no password and no onboarding dismissal regardless of the truth, rather than surfacing a 500. Checked both workflow files for the injection class letsvisit found there (a `${{ }}` expression spliced straight into a `run:` block): every one here already goes through `env:` as a quoted shell variable, documented in ci.yaml's own header comment. Nothing to fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .gitea/workflows/ci.yaml | 3 +++ Makefile | 21 +++++++++++++++++++++ internal/api/archiver.go | 4 ++++ internal/api/auth.go | 19 ++++++++++++++++--- internal/api/deadman.go | 4 ++++ internal/api/oidc.go | 20 ++++++++++++++++---- internal/api/schedule.go | 3 +++ 7 files changed, 67 insertions(+), 7 deletions(-) diff --git a/.gitea/workflows/ci.yaml b/.gitea/workflows/ci.yaml index b836380..693ad74 100644 --- a/.gitea/workflows/ci.yaml +++ b/.gitea/workflows/ci.yaml @@ -125,6 +125,9 @@ jobs: - name: Secret scan (gitleaks) run: make security-secrets + - name: Code security scan (gosec) + run: make security-code + # Host mode, no `container:`: helm is baked into the runner image, and a container job # could not install it -- get.helm.sh is unreachable from the dind bridge. Same reason # release.yaml's chart job runs on the host. diff --git a/Makefile b/Makefile index fa2ec48..3f9b660 100644 --- a/Makefile +++ b/Makefile @@ -143,6 +143,7 @@ BUILDX_BUILDER ?= terdut TRIVY_VERSION := 0.73.0 GOVULNCHECK_VERSION := v1.1.4 GITLEAKS_VERSION := v8.30.0 +GOSEC_VERSION := v2.29.0 # --pull, not --no-cache: refresh the base image without discarding the layer cache. DOCKER_BUILD_FLAGS ?= --pull @@ -222,6 +223,26 @@ release: push helm-package helm-push ## Publish image + chart (the workflow's on security-go: ## Scan Go deps for known CVEs (govulncheck) go run golang.org/x/vuln/cmd/govulncheck@$(GOVULNCHECK_VERSION) ./... +# Code-level, not dependency- or secret-level: gosec reads this repo's own source for +# known-dangerous patterns (weak crypto, SQL/command injection shapes, insecure file +# permissions, …) rather than its module graph or working tree for leaked credentials, +# which is what security-go and security-secrets above already cover. +# +# G104 (unchecked error) is excluded. Every hit it found here on first run was this +# codebase's existing, deliberate idiom for a best-effort write or an already-reviewed +# json.Unmarshal of this server's own JSONB (see the "best-effort" comments in +# middleware.go and the //nolint:errcheck lines in alerts.go/deadman.go) -- a style that +# predates gosec and that G104 cannot distinguish from a mistake. Reaching the same +# green result by adding a dozens of individual #nosec comments would not add +# information; it would just make a future *real* G104 regression one more suppressed +# line instead of a visible one. Same reasoning as the chi-advisories note on +# security-go above: what gosec reports here (nothing, beyond G104) is the useful +# property, not a loophole. -exclude-generated skips web.go's embedded, build-time-only +# assets. +.PHONY: security-code +security-code: ## Scan this repo's own source for risky patterns (gosec) + go run github.com/securego/gosec/v2/cmd/gosec@$(GOSEC_VERSION) -exclude-generated -exclude=G104 ./... + # --no-git scans the working tree rather than the history, so this catches a secret on the # way in. It is not a history audit and finding nothing here says nothing about what is # already committed. --redact because the finding is printed into a CI log. diff --git a/internal/api/archiver.go b/internal/api/archiver.go index 25a0e5f..6759596 100644 --- a/internal/api/archiver.go +++ b/internal/api/archiver.go @@ -122,6 +122,10 @@ func expireStale(ctx context.Context, db *sql.DB, staleAfter time.Duration, skip for i, id := range ids { idList[i] = id } + // #nosec G202 -- sqlArgs.add/addList only ever splice in the "$N" + // placeholder they hand back, never a value; every value travels through + // args.all() as a bound parameter. See the sqlArgs doc comment in + // helpers.go. if _, err := db.ExecContext(ctx, ` UPDATE alerts SET status = 'resolved', diff --git a/internal/api/auth.go b/internal/api/auth.go index e161b51..804f4d5 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -99,7 +99,7 @@ func (l *loginLimiter) fail(ctx context.Context, keys ...string) { THEN 1 ELSE rate_limit_counters.count + 1 END`, key, now, windowSecs, ); err != nil { - log.Printf("rate limiter: record failure for %q: %v", key, err) + log.Printf("rate limiter: record failure for %q: %v", key, err) // #nosec G706 -- %q } } } @@ -199,6 +199,9 @@ func startSessionCapped(w http.ResponseWriter, r *http.Request, db *sql.DB, user return err } + // #nosec G124 -- HttpOnly/SameSite are literal below; Secure is + // cookieSecure(publicURL, r), not a literal true, which is what trips + // this rule. See cookieSecure's own doc comment above. http.SetCookie(w, &http.Cookie{ Name: sessionCookie, Value: raw, @@ -280,6 +283,9 @@ func handleLogout(db *sql.DB, publicURL string) http.HandlerFunc { if c, err := r.Cookie(sessionCookie); err == nil && c.Value != "" { db.ExecContext(r.Context(), "DELETE FROM sessions WHERE token_hash = $1", hashToken(c.Value)) } + // #nosec G124 -- HttpOnly/SameSite are literal below; Secure is + // cookieSecure(publicURL, r), not a literal true, which is what + // trips this rule. See cookieSecure's own doc comment above. http.SetCookie(w, &http.Cookie{ Name: sessionCookie, Value: "", @@ -319,9 +325,16 @@ func handleMe(db *sql.DB) http.HandlerFunc { } var hash sql.NullString var dismissed *int64 - db.QueryRowContext(r.Context(), + if err := db.QueryRowContext(r.Context(), "SELECT password_hash, onboarding_dismissed_at FROM users WHERE id = $1", - caller.ID).Scan(&hash, &dismissed) + caller.ID).Scan(&hash, &dismissed); err != nil { + // fetchUser above already found this row, so an error here is a + // transient database problem, not a missing user — worth a 500 + // rather than silently answering "no password, not dismissed", + // which a client would otherwise take at face value. + respond(w, http.StatusInternalServerError, errResp("internal error")) + return + } respond(w, http.StatusOK, meResponse{ User: user, HasPassword: hash.Valid, diff --git a/internal/api/deadman.go b/internal/api/deadman.go index c342993..70bbd01 100644 --- a/internal/api/deadman.go +++ b/internal/api/deadman.go @@ -283,6 +283,10 @@ func deadmanAlerts(ctx context.Context, db *sql.DB, teamID int64, cfg deadmanSet nameList[i] = n } + // #nosec G202 -- sqlArgs.add/addList only ever splice in the "$N" + // placeholder they hand back, never a value; every value travels through + // args.all() as a bound parameter. See the sqlArgs doc comment in + // helpers.go. rows, err := db.QueryContext(ctx, ` SELECT id, team_id, fingerprint, labels, status, received_at FROM alerts diff --git a/internal/api/oidc.go b/internal/api/oidc.go index fb121ac..fe4baa7 100644 --- a/internal/api/oidc.go +++ b/internal/api/oidc.go @@ -160,6 +160,9 @@ func handleOIDCLogin(db *sql.DB, prov *oidc.Provider, limiter *loginLimiter, pub return } + // #nosec G124 -- HttpOnly/SameSite are literal below; Secure is + // cookieSecure(publicURL, r), not a literal true, which is what + // trips this rule. See cookieSecure's own doc comment in auth.go. http.SetCookie(w, &http.Cookie{ Name: oidcStateCookie, Value: state, @@ -182,6 +185,9 @@ func handleOIDCCallback(db *sql.DB, prov *oidc.Provider, publicURL string) http. return func(w http.ResponseWriter, r *http.Request) { // The state cookie has done its job once the callback arrives, whatever // the outcome. + // #nosec G124 -- HttpOnly/SameSite are literal below; Secure is + // cookieSecure(publicURL, r), not a literal true, which is what + // trips this rule. See cookieSecure's own doc comment in auth.go. http.SetCookie(w, &http.Cookie{ Name: oidcStateCookie, Value: "", Path: "/api/oidc", MaxAge: -1, HttpOnly: true, Secure: cookieSecure(publicURL, r), SameSite: http.SameSiteLaxMode, @@ -189,7 +195,13 @@ func handleOIDCCallback(db *sql.DB, prov *oidc.Provider, publicURL string) http. q := r.URL.Query() if e := q.Get("error"); e != "" { - log.Printf("oidc: provider returned error %q: %s", e, q.Get("error_description")) + // %q on both: this runs before state is checked against the + // cookie, so error and error_description are still whatever the + // request's query string says, not yet known to be the real + // provider's. %q keeps a crafted value (say, one holding a + // newline) from forging a second log line rather than just + // being a quoted string within this one. + log.Printf("oidc: provider returned error %q: %q", e, q.Get("error_description")) // #nosec G706 -- both %q ssoRedirect(w, r, ssoDenied) return } @@ -226,7 +238,7 @@ func handleOIDCCallback(db *sql.DB, prov *oidc.Provider, publicURL string) http. grants := oidc.ComputeGrants(cfg, identity.Groups) if !grants.Admitted { - log.Printf("oidc: %q (%s) is in none of the allowed groups", identity.Username, identity.Subject) + log.Printf("oidc: %q (%q) is in none of the allowed groups", identity.Username, identity.Subject) // #nosec G706 -- both %q ssoRedirect(w, r, ssoNotAllowed) return } @@ -243,11 +255,11 @@ func handleOIDCCallback(db *sql.DB, prov *oidc.Provider, publicURL string) http. if err != nil { var se ssoError if errors.As(err, &se) { - log.Printf("oidc: refused %q (%s): %v", identity.Username, identity.Subject, se) + log.Printf("oidc: refused %q (%q): %v", identity.Username, identity.Subject, se) // #nosec G706 -- both %q ssoRedirect(w, r, se) return } - log.Printf("oidc: sign in %q: %v", identity.Username, err) + log.Printf("oidc: sign in %q: %v", identity.Username, err) // #nosec G706 -- %q ssoRedirect(w, r, ssoFailed) return } diff --git a/internal/api/schedule.go b/internal/api/schedule.go index 6374592..089800c 100644 --- a/internal/api/schedule.go +++ b/internal/api/schedule.go @@ -235,6 +235,9 @@ func scheduleRange(ctx context.Context, db *sql.DB, teamID int64, from, to strin clause := strings.Join(where, " AND ") + // #nosec G202 -- clause is built from sqlArgs.add's "$N" placeholders + // only, never a value; every value travels through args.all() as a + // bound parameter. See the sqlArgs doc comment in helpers.go. rows, err := db.QueryContext(ctx, ` SELECT s.id, s.team_id, t.name, s.user_id, u.username, s.date, s.created_at FROM schedule_entries s