Skip to content

Commit 6817698

Browse files
shockalotticlaude
andcommitted
security: fix scanner findings from rapid-audit-kit recon pass
Prep work before building the self-serve audit kit — ran govulncheck, gosec, gitleaks (full history), syft SBOM, pnpm audit, and semgrep against the current codebase for the first time, then fixed everything real and documented everything false-positive rather than letting the kit launch against an unverified scan. Real fixes: - golang.org/x/image bumped from a 2021 pseudo-version to v0.43.0 — the old pin carried 11 known CVEs (TIFF/WebP/BMP decode DoS/panic) reachable via the branding-logo upload path. govulncheck now reports zero vulnerabilities. - Avatar/branding upload directories created 0750 instead of 0755 (gosec G301). Noise cleanup (37 gosec G104 "unhandled error" findings, all verified low-risk): - worker.go job-status/webhook-delivery bookkeeping updates and avatar/logo temp-file cleanup now log on failure (genuine value — a stuck job or an orphaned temp file is worth knowing about). - The rest (rows.Close()/resp.Body.Close() on already-consumed/already-erroring paths, and a few best-effort exists-checks backed by a DB UNIQUE constraint) are documented `#nosec G104` with the reasoning inline, not silently ignored. semgrep (9 findings, all false positives, now inline-documented + suppressed): - var-in-href on .PrivacyURL/.TermsURL: server-validated to require an http(s) scheme before storage (validatedLegalURL), so a javascript: URI never reaches the template. - unquoted-attribute-var on the intake-question id/name attrs: admin-configured question IDs, and html/template context-escapes them regardless. - var-in-script-tag on .BookingLogicJS (book.html + manage.html): a static go:embed'd JS asset, never request/database data. Verified false positives left as-is (gosec G202/G701 "SQL injection" — all parameterized with ? placeholders; G203 "XSS" — goldmark markdown has no WithUnsafe(), template.JS wraps embedded/JSON-marshaled data; G304 path traversal — validated + filepath.Base()'d; G124/G101 — cookies do set Secure/HttpOnly/SameSite, "secret" was a public OAuth endpoint URL). govulncheck: 0. gosec: 82->43 (all remaining verified false positives). semgrep: 9->0. gitleaks: clean across 321 commits. go test ./...: all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent f26a7f3 commit 6817698

22 files changed

Lines changed: 101 additions & 57 deletions

frontend/embed.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ func (h *spaHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
4848
f, err := h.fs.Open(r.URL.Path)
4949
if err == nil {
5050
st, statErr := f.Stat()
51-
f.Close()
51+
f.Close() // #nosec G104 -- handle already Stat()-ed above; nothing actionable on close error
5252
// Treat directories as not-found so the SPA shell handles the route
5353
// instead of http.FileServer rendering a directory listing.
5454
if statErr == nil && st.IsDir() {

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ require (
99
github.com/pressly/goose/v3 v3.27.1
1010
github.com/yuin/goldmark v1.8.2
1111
golang.org/x/crypto v0.53.0
12-
golang.org/x/image v0.0.0-20211028202545-6944b10bf410
12+
golang.org/x/image v0.43.0
1313
golang.org/x/oauth2 v0.36.0
1414
modernc.org/sqlite v1.49.1
1515
)

go.sum

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,8 @@ go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN8
5151
golang.org/x/crypto v0.53.0 h1:QZ4Muo8THX6CizN2vPPd5fBGHyogrdK9fG4wLPFUsto=
5252
golang.org/x/crypto v0.53.0/go.mod h1:DNLU434OwVakk9PzuwV8w62mAJpRJL3vsgcfp4Qnsio=
5353
golang.org/x/image v0.0.0-20191009234506-e7c1f5e7dbb8/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
54-
golang.org/x/image v0.0.0-20211028202545-6944b10bf410 h1:hTftEOvwiOq2+O8k2D5/Q7COC7k5Qcrgc2TFURJYnvQ=
55-
golang.org/x/image v0.0.0-20211028202545-6944b10bf410/go.mod h1:023OzeP/+EPmXeapQh35lcL3II3LrY8Ic+EFFKVhULM=
54+
golang.org/x/image v0.43.0 h1:FLxcP4ec2350nTfOC8ysKtqYSIFbk/QGjw1ZHNP4tsY=
55+
golang.org/x/image v0.43.0/go.mod h1:rrpelvGFt+kLPAjPM4HeWPgrl0FtafueU//e5N0qk/Q=
5656
golang.org/x/mod v0.33.0 h1:tHFzIWbBifEmbwtGz65eaWyGiGZatSrT9prnU8DbVL8=
5757
golang.org/x/mod v0.33.0/go.mod h1:swjeQEj+6r7fODbD2cqrnje9PnziFuw4bmLbBZFrQ5w=
5858
golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs=
@@ -62,8 +62,6 @@ golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0=
6262
golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw=
6363
golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw=
6464
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
65-
golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
66-
golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
6765
golang.org/x/tools v0.42.0 h1:uNgphsn75Tdz5Ji2q36v/nsFSfR/9BRFvqhGBaJGd5k=
6866
golang.org/x/tools v0.42.0/go.mod h1:Ma6lCIwGZvHK6XtgbswSoWroEkhugApmsXyrUmBhfr0=
6967
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=

internal/booking/service.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -475,12 +475,12 @@ func (s *Service) Reschedule(ctx context.Context, bookingID string, newStart, ne
475475
for hostRows.Next() {
476476
var u string
477477
if err := hostRows.Scan(&u); err != nil {
478-
hostRows.Close()
478+
hostRows.Close() // #nosec G104 -- already returning the scan error; nothing more actionable
479479
return nil, fmt.Errorf("booking: reschedule hosts scan: %w", err)
480480
}
481481
hostIDs = append(hostIDs, u)
482482
}
483-
hostRows.Close()
483+
hostRows.Close() // #nosec G104 -- rows already fully consumed above; nothing actionable on close error
484484
if len(hostIDs) == 0 { // legacy booking with no booking_hosts rows
485485
hostIDs = []string{b.HostID}
486486
}

internal/calendar/microsoft/freebusy.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,11 @@ func (c *Client) freeBusyForClient(ctx context.Context, hc *http.Client, from, t
7777
var cv calendarViewResp
7878
if resp.StatusCode != http.StatusOK {
7979
msg := graphErrBody(resp)
80-
resp.Body.Close()
80+
resp.Body.Close() // #nosec G104 -- already returning a more specific error; nothing actionable on close error
8181
return nil, fmt.Errorf("microsoft: calendarView status %d: %s", resp.StatusCode, msg)
8282
}
8383
derr := json.NewDecoder(resp.Body).Decode(&cv)
84-
resp.Body.Close()
84+
resp.Body.Close() // #nosec G104 -- body already decoded above; nothing actionable on close error
8585
if derr != nil {
8686
return nil, fmt.Errorf("microsoft: calendarView decode: %w", derr)
8787
}

internal/handler/auth_google.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,11 @@ func (h *Handler) Logout(w http.ResponseWriter, r *http.Request) {
9797
// Only delete the session if the cookie value corresponds to an actual row,
9898
// so a forged or empty cookie cannot be used to trigger arbitrary deletes.
9999
if cookie, err := r.Cookie(sessionCookieName); err == nil && cookie.Value != "" {
100-
h.db.ExecContext(r.Context(), //nolint:errcheck
100+
// Best-effort: logout must proceed (cookie is cleared below) even if this fails;
101+
// worst case is a harmless stale row that the session's own expiry cleans up.
102+
//nolint:errcheck
103+
// #nosec G104
104+
h.db.ExecContext(r.Context(),
101105
`DELETE FROM sessions WHERE id = ?`, cookie.Value)
102106
}
103107
http.SetCookie(w, &http.Cookie{

internal/handler/avatar.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ func (h *Handler) UploadAvatar(w http.ResponseWriter, r *http.Request) {
8080
}
8181

8282
avatarDir := filepath.Join(h.dataDir, "avatars")
83-
if err := os.MkdirAll(avatarDir, 0o755); err != nil {
83+
if err := os.MkdirAll(avatarDir, 0o750); err != nil {
8484
h.logger.ErrorContext(r.Context(), "avatar: mkdir", "error", err)
8585
h.writeError(w, http.StatusInternalServerError, "internal error")
8686
return
@@ -96,9 +96,11 @@ func (h *Handler) UploadAvatar(w http.ResponseWriter, r *http.Request) {
9696
tmpPath := tmp.Name()
9797
committed := false
9898
defer func() {
99-
tmp.Close()
99+
tmp.Close() // #nosec G104 -- file already written/renamed by this point; nothing actionable
100100
if !committed {
101-
os.Remove(tmpPath)
101+
if rerr := os.Remove(tmpPath); rerr != nil && !os.IsNotExist(rerr) {
102+
h.logger.Warn("avatar: cleanup temp file", "error", rerr, "path", tmpPath)
103+
}
102104
}
103105
}()
104106

internal/handler/branding_settings.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ func (h *Handler) UploadBrandingLogo(w http.ResponseWriter, r *http.Request) {
248248
}
249249

250250
dir := h.brandingDir()
251-
if err := os.MkdirAll(dir, 0o755); err != nil {
251+
if err := os.MkdirAll(dir, 0o750); err != nil {
252252
h.logger.ErrorContext(r.Context(), "logo: mkdir", "error", err)
253253
h.writeError(w, http.StatusInternalServerError, "internal error")
254254
return
@@ -263,9 +263,11 @@ func (h *Handler) UploadBrandingLogo(w http.ResponseWriter, r *http.Request) {
263263
tmpPath := tmp.Name()
264264
committed := false
265265
defer func() {
266-
tmp.Close()
266+
tmp.Close() // #nosec G104 -- file already written/renamed by this point; nothing actionable
267267
if !committed {
268-
os.Remove(tmpPath)
268+
if rerr := os.Remove(tmpPath); rerr != nil && !os.IsNotExist(rerr) {
269+
h.logger.Warn("logo: cleanup temp file", "error", rerr, "path", tmpPath)
270+
}
269271
}
270272
}()
271273
if _, err := tmp.Write(out.Bytes()); err != nil {

internal/handler/calendar_reconcile.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ func (h *Handler) reconcileReschedules(ctx context.Context, gc *calendar.Service
8686
items = append(items, d)
8787
}
8888
}
89-
rows.Close()
89+
rows.Close() // #nosec G104 -- rows already fully consumed above; nothing actionable on close error
9090

9191
for _, d := range items {
9292
start, err1 := time.Parse(time.RFC3339Nano, d.startStr)
@@ -128,7 +128,7 @@ func (h *Handler) reconcileCancellations(ctx context.Context, gc *calendar.Servi
128128
orphans = append(orphans, o)
129129
}
130130
}
131-
rows.Close()
131+
rows.Close() // #nosec G104 -- rows already fully consumed above; nothing actionable on close error
132132

133133
for _, o := range orphans {
134134
if err := gc.CancelEvent(ctx, o.userID, o.eventID); err != nil {
@@ -181,7 +181,7 @@ func (h *Handler) reconcileCreations(ctx context.Context, gc *calendar.Service)
181181
items = append(items, m)
182182
}
183183
}
184-
rows.Close()
184+
rows.Close() // #nosec G104 -- rows already fully consumed above; nothing actionable on close error
185185

186186
for _, m := range items {
187187
has, err := gc.HasDestination(ctx, m.userID)

internal/handler/claim.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,12 @@ func (h *Handler) AuthStatus(w http.ResponseWriter, r *http.Request) {
3131
providers = append(providers, "microsoft")
3232
}
3333

34+
// Best-effort status flag: on query error emailLoginCount simply stays 0 (treated
35+
// as "no email-login users yet"), which is a safe default for this UI hint.
3436
var emailLoginCount int
35-
h.db.QueryRowContext(r.Context(), //nolint:errcheck
37+
//nolint:errcheck
38+
// #nosec G104
39+
h.db.QueryRowContext(r.Context(),
3640
`SELECT COUNT(*) FROM users WHERE email_login = 1`).Scan(&emailLoginCount)
3741

3842
h.writeJSON(w, http.StatusOK, map[string]any{

0 commit comments

Comments
 (0)