mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-07-04 13:12:46 +00:00
Kpa-clawbot
800d61c382
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>
35 lines
967 B
Go
35 lines
967 B
Go
package main
|
|
|
|
import "testing"
|
|
|
|
// TestClampLimit covers the uniform list-endpoint limit-clamp helper added to
|
|
// fix audit-input-vulns-20260603 (MEDIUM).
|
|
func TestClampLimit(t *testing.T) {
|
|
const def = 50
|
|
const max = 500
|
|
cases := []struct {
|
|
name string
|
|
raw string
|
|
want int
|
|
}{
|
|
{"empty returns default", "", def},
|
|
{"non-numeric returns default", "abc", def},
|
|
{"negative returns default", "-1", def},
|
|
{"zero returns default", "0", def},
|
|
{"mid-range value preserved", "100", 100},
|
|
{"value at cap preserved", "500", 500},
|
|
{"over-cap clamped to max", "999999999", max},
|
|
{"just over cap clamped", "501", max},
|
|
{"whitespace garbage returns default", " 100 ", def},
|
|
{"float-shaped returns default", "10.5", def},
|
|
}
|
|
for _, tc := range cases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
got := clampLimit(tc.raw, def, max)
|
|
if got != tc.want {
|
|
t.Fatalf("clampLimit(%q, %d, %d) = %d, want %d", tc.raw, def, max, got, tc.want)
|
|
}
|
|
})
|
|
}
|
|
}
|