niklas/terdut-server
1
0
Fork 0
Code Issues 8 Pull Requests Actions Packages Projects Releases 55 Wiki Activity

Add gosec to CI; fix the one real finding it surfaced

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Niklas Ye
2026-10-07 22:12:31 +02:00
parent b82c10acf4
commit f15db0e20a
7 changed files with 67 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/api/archiver.go
+4
View File
@@ -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',
internal/api/auth.go
+16 -3
View File
@@ -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,
internal/api/deadman.go
+4
View File
@@ -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
internal/api/oidc.go
+16 -4
View File
@@ -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
}
internal/api/schedule.go
+3
View File
@@ -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