fix: security & reliability hardening, agent/proxy config, and coverage gates#10
fix: security & reliability hardening, agent/proxy config, and coverage gates#10memetics19 wants to merge 4 commits into
Conversation
memetics19
left a comment
memetics19
left a comment
There was a problem hiding this comment.
Decision: request changes before merge. The race suite and UI build pass, but this review reproduced three release-blocking security/correctness regressions: enumerable public detail routes still bypass the new sanitized status DTO, ping validation is vulnerable to DNS rebinding, and a custom page with zero selected groups widens to all groups. I also reproduced pulse-agent --help exiting 2, infra monitor creation rejecting a private label despite never dialing it, and normal HTTP response bodies preventing connection reuse. The current golangci-lint v2.12.2 run reports 38 errcheck findings. Inline comments contain focused fixes and regression-test expectations.
|
|
||
| // Public read-only (status page client-side fetches) | ||
| r.Get("/api/monitors/{monitorID}/checks", handlers.NewCheckResults(q).List) | ||
| r.Get("/api/monitors/{monitorID}/checks/uptime", handlers.NewCheckResults(q).Uptime) |
There was a problem hiding this comment.
[blocker] These public ID routes bypass the new page-scoped DTO. Any unauthenticated caller can enumerate monitor IDs here (and incident IDs on line 44) without resolving a Host/page or checking membership. CheckResults.List returns generated check rows, so errors can also disclose the monitored target. Please authenticate these routes or replace them with Host-resolved, page-scoped, reduced DTOs, then add cross-page enumeration tests for monitor checks, uptime, and incident updates.
| if err := netguard.ValidateTarget(target, c.allowPrivate); err != nil { | ||
| return Result{Status: "down", ErrorMessage: err.Error(), CheckedAt: time.Now()} | ||
| } | ||
| pinger, err := probing.NewPinger(target) |
There was a problem hiding this comment.
[blocker] Validation and use resolve the hostname separately. ValidateTarget checks one DNS answer, then probing.NewPinger(target) resolves the name again; an attacker can rebind between those lookups and reach a private address. Resolve once, reject every disallowed answer, and configure the pinger with the selected validated IP (while retaining the display hostname). An injected-resolver test should change the second answer to loopback and prove it is never used.
| return | ||
| } | ||
|
|
||
| allGroups := rp.GroupIDs == nil |
There was a problem hiding this comment.
[blocker] An empty custom page silently widens to every group. ResolvePage returns nil when a custom page has zero page_groups, and this treats nil as the default page's all-groups sentinel. I reproduced /api/status returning a monitor from an unrelated group for a custom page with group_ids: []. Represent page scope explicitly (all, empty, or selected IDs) and keep empty non-nil through HTML, JSON, and feed paths; add an exact cross-page regression test.
| if err := netguard.ValidateURL(url, m.allowPrivate); err != nil { | ||
| return err.Error() | ||
| } | ||
| } else { |
There was a problem hiding this comment.
[major] This applies network-target validation to infra, which has no checker and never dials its URL. I reproduced creating an infra monitor with 127.0.0.1 returning 400 when private targets are disabled. Select validation by monitor kind instead of using a catch-all else; infra should skip SSRF address validation, while each network kind should retain its own parser/policy.
| token := fs.String("token", "", "Bearer token (INSECURE: visible in ps/proc; prefer PULSE_AGENT_TOKEN or --token-file)") | ||
| tokenFile := fs.String("token-file", "", "File to read the bearer token from") | ||
| interval := fs.Int("interval", 30, "Push interval in seconds (default 30)") | ||
| if err := fs.Parse(args); err != nil { |
There was a problem hiding this comment.
[major] --help is reported as a usage error. flag.FlagSet.Parse returns flag.ErrHelp after printing help, and this maps it to exit code 2. A fresh binary run confirms pulse-agent --help exits 2. Handle errors.Is(err, flag.ErrHelp) as success (0), preserve 2 for malformed flags, and cover both exit codes in table-driven CLI tests.
| func (c *httpChecker) attempt(ctx context.Context, client *http.Client, target string) Result { | ||
| func (c *httpChecker) attempt(ctx context.Context, client *http.Client, target string, timeout time.Duration) Result { | ||
| // The per-attempt deadline replaces the old per-client Timeout, which can't | ||
| // live on the shared client. Cancel fires after the body is read below. |
There was a problem hiding this comment.
[major] Closing without consuming the ordinary response body defeats the new shared transport. On the no-keyword success path the body never reaches EOF, so Go cannot reuse the HTTP/1.x connection. The existing reuse test uses an empty body and misses this; an overlay test with a non-empty body reproduced a new connection on the second check. Drain/consume the body with an explicit size policy before close, and make the reuse regression return representative content.
| if total > 0 { | ||
| uptime = fmt.Sprintf("%.2f%%", float64(upCount)/float64(total)*100) | ||
| if len(rows) > 0 { | ||
| if pct, err := p.q.UptimePercent(ctx, generated.UptimePercentParams{MonitorID: monitorID, CheckedAt: since}); err == nil { |
There was a problem hiding this comment.
[major] This adds a second history query per visible monitor on an unauthenticated page. monitorSeries already calls ListCheckResults, and now calls UptimePercent, producing at least 2N history queries per render. Public pages are an easy load amplifier as monitor count grows. Batch series/uptime reads for all visible monitor IDs behind one public-status projection, and assert a bounded query count in a many-monitor test.
| q := generated.New(testutil.FailAfter(db, 1)) | ||
| rr := do(handlers.NewPages(q).Create, "POST", "/api/pages", | ||
| map[string]any{"domain": "a.com", "title": "A", "group_ids": []int64{g.ID}}) | ||
| assert.Equal(t, http.StatusInternalServerError, rr.Code) |
There was a problem hiding this comment.
[major] This test codifies the 500 but misses the broken invariant: the page insert has already committed. When AddPageGroup fails, a partial status page remains persisted (and updates can similarly leave partial associations). The handler needs one transaction for page fields plus all group mutations; extend this fault test to query the database and assert the entire mutation rolled back.
| parts := strings.Split(r.Header.Get("X-Forwarded-For"), ",") | ||
| for i := len(parts) - 1; i >= 0; i-- { | ||
| ip := strings.TrimSpace(parts[i]) | ||
| if ip != "" && !ipInAny(ip, trusted) { |
There was a problem hiding this comment.
[major] The selected X-Forwarded-For token is returned without parsing or canonicalization. A client behind a configured trusted proxy can vary invalid strings (or alternate textual IP forms) to mint rate-limit buckets and grow the limiter map. Parse each hop with net.ParseIP, ignore invalid entries, and return ip.String() so one address has one stable key; add malformed and IPv6 canonicalization cases.
| - uses: golangci/golangci-lint-action@v6 | ||
| with: | ||
| version: latest | ||
| working-directory: api |
There was a problem hiding this comment.
[major] The new lint job covers only api, unlike the Go test matrix. agent and cli can accumulate unchecked errors and other lint regressions unnoticed; this PR already changes both. Matrix lint across all three modules (with per-module working directories/config if needed), pin the linter version for reproducibility, and make it blocking once the 38 current errcheck findings are resolved.
1 participant
Summary
Hardens the API and worker against a set of security and correctness bugs found in review (SSRF across all monitor types, a public /api/status data leak, stored XSS, a silently-dying worker, WAL that was never actually enabled), adds agent-token and reverse-proxy hardening, reworks the admin UI's destructive-action UX, and raises test coverage across all three Go modules to CI-enforced gates.
Type of change
Changes
api
worker
agent
ui
ci / build
docs
test
Breaking changes
GET /api/status previously returned the raw monitor model (target URLs, thresholds, source, external ID) for all monitors, unscoped. It now returns a page-scoped, reduced shape — group and monitor name + status only. Any consumer reading monitor URLs/thresholds/IDs from this public endpoint must switch to the authenticated /api/monitors API. No database migration is required.
Test plan
Checklist
Deployment notes