mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-10-15 18:50:17 +00:00
Follow-up to v3.8.3 security train. Found by non-XSS input-validation audit. Three findings closed in one PR — all defense-in-depth: medium is genuinely DoS-only (no data exposure), lows tighten log hygiene and SPA path handling so future router changes can't silently expose the filesystem. ## Findings addressed ### MEDIUM — unbounded `limit` on list endpoints - **What:** four list endpoints accepted `limit=999999999` and passed the value straight to SQL `LIMIT ?` and Go `make(..., 0, limit)`. - **Where:** `cmd/server/routes.go` — handlePackets (incl. multi-node branch), handleNodes, handleChannelMessages, handleAnalyticsSubpaths, handleAnalyticsSubpathsBulk per-group lim, handleDroppedPackets. - **Fix:** new `clampLimit(raw, def, max)` helper in `cmd/server/clamp_limit.go` plus `queryLimit(r, def, max)` HTTP wrapper. Caps: packets/nodes/channels/dropped = 500, analytics buckets / bulk-health = 200. Already-clamped endpoints (handleBulkHealth) migrated to the helper for uniformity. Silent clamp — no response-shape change. Negative / zero / non-numeric → default. ### LOW — log injection via newline in advert name - **What:** advert `name` field allows `\n` / `\t` (sanitizeName intentionally preserves them for display). Logged at two MQTT-ingest sites, an attacker with publish ACL could forge log lines. - **Where:** `cmd/ingestor/main.go:659,690`. - **Fix:** new `sanitizeLogString` in `cmd/ingestor/sanitize_log.go` strips control bytes < 0x20 and DEL with `?`. Wrapped at the two log call sites that interpolate `name=` and `observer=`. Stored display values untouched. ### LOW — SPA static handler depends on default mux path-cleaning - **What:** `cmd/server/main.go:469` joins `r.URL.Path` to root; safe today only because gorilla/mux runs `path.Clean` and `http.FileServer` rejects `..`. A future `SkipClean(true)` or router swap would silently expose the filesystem. - **Where:** `cmd/server/main.go` (spaHandler). - **Fix:** new `isSafeStaticPath` rejects requests whose decoded or raw path contains `..`, `%2e%2e`, `\\`, or `%5c` with a 400. Legit asset names with dots (`/app.js`, `/customize-v2.js`, `/themes/dark.css`) are unaffected. ## TDD - Commit 1 (red): adds `TestClampLimit`, `TestSpaHandlerPathTraversal`, `TestSanitizeLogString` with stub helpers — tests fail on assertions (not build errors), proving they gate the change. - Commit 2 (green): production fix. Revert the green commit and the red commit's assertions fail. ## Audit reference Source: non-XSS input-validation audit dated 2026-06-03 (workspace). Sibling PR `fix/xss-r2-trace-obs-anl` owns the XSS findings — not included here. --------- Co-authored-by: clawbot <clawbot@users.noreply.github.com>
37 lines
1.0 KiB
Go
37 lines
1.0 KiB
Go
package main
|
|
|
|
import (
|
|
"net/http"
|
|
"strconv"
|
|
)
|
|
|
|
// clampLimit parses a `limit`-shaped string and clamps it into [1, max].
|
|
// Empty / non-numeric / zero / negative inputs return def.
|
|
// Values exceeding max are clamped to max.
|
|
//
|
|
// This is the uniform helper for list-endpoint `limit` parameters; prefer it
|
|
// over inline `if limit > N { limit = N }` patterns so the absolute caps stay
|
|
// consistent across handlers. See audit-input-vulns-20260603 (MEDIUM —
|
|
// unbounded `limit` on list endpoints).
|
|
func clampLimit(raw string, def, max int) int {
|
|
if raw == "" {
|
|
return def
|
|
}
|
|
n, err := strconv.Atoi(raw)
|
|
if err != nil || n <= 0 {
|
|
return def
|
|
}
|
|
if n > max {
|
|
return max
|
|
}
|
|
return n
|
|
}
|
|
|
|
// queryLimit reads the `limit` query parameter from r and clamps it through
|
|
// clampLimit. Convenience wrapper used by HTTP handlers so existing
|
|
// queryInt(r, "limit", def) call sites can become queryLimit(r, def, max).
|
|
func queryLimit(r *http.Request, def, max int) int {
|
|
return clampLimit(r.URL.Query().Get("limit"), def, max)
|
|
}
|
|
|