mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-08-28 09:04:13 +00:00
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>
91 lines
3.0 KiB
Go
91 lines
3.0 KiB
Go
package main
|
|
|
|
import (
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"os"
|
|
"path/filepath"
|
|
"strings"
|
|
"testing"
|
|
)
|
|
|
|
// TestSpaHandlerPathTraversal asserts that the SPA static handler does not
|
|
// serve files outside its served root, even when the URL contains traversal
|
|
// sequences. The default gorilla/mux + http.FileServer chain already cleans
|
|
// most of these, but we want defense-in-depth so a future SkipClean(true)
|
|
// (or a different router) cannot accidentally expose the filesystem.
|
|
//
|
|
// Audit ref: audit-input-vulns-20260603 (LOW — SPA static handler depends on
|
|
// default mux path-cleaning).
|
|
func TestSpaHandlerPathTraversal(t *testing.T) {
|
|
root := t.TempDir()
|
|
parent := filepath.Dir(root)
|
|
|
|
// Place a sentinel file OUTSIDE the served root. If traversal works the
|
|
// response body will contain the sentinel.
|
|
secretPath := filepath.Join(parent, "secret.txt")
|
|
if err := os.WriteFile(secretPath, []byte("CORESCOPE_SECRET_SENTINEL"), 0644); err != nil {
|
|
t.Fatalf("setup: %v", err)
|
|
}
|
|
t.Cleanup(func() { os.Remove(secretPath) })
|
|
|
|
// Minimal SPA root: index.html + an asset with a dot in the filename to
|
|
// prove legit names aren't false-positived.
|
|
if err := os.WriteFile(filepath.Join(root, "index.html"), []byte("<html>SPA</html>"), 0644); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := os.WriteFile(filepath.Join(root, "app.js"), []byte("console.log('ok')"), 0644); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := os.WriteFile(filepath.Join(root, "customize-v2.js"), []byte("// v2"), 0644); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
themes := filepath.Join(root, "themes")
|
|
os.Mkdir(themes, 0755)
|
|
if err := os.WriteFile(filepath.Join(themes, "dark.css"), []byte("body{}"), 0644); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
|
|
fs := http.FileServer(http.Dir(root))
|
|
handler := spaHandler(root, fs)
|
|
|
|
traversal := []string{
|
|
"/../secret.txt",
|
|
"/..%2fsecret.txt",
|
|
"/%2e%2e/secret.txt",
|
|
"/foo/../../secret.txt",
|
|
"/..\\secret.txt",
|
|
"/static/..%5csecret.txt",
|
|
}
|
|
for _, p := range traversal {
|
|
t.Run("blocks "+p, func(t *testing.T) {
|
|
req := httptest.NewRequest("GET", p, nil)
|
|
w := httptest.NewRecorder()
|
|
handler.ServeHTTP(w, req)
|
|
body := w.Body.String()
|
|
if strings.Contains(body, "CORESCOPE_SECRET_SENTINEL") {
|
|
t.Fatalf("traversal succeeded — secret leaked for path %q: %s", p, body)
|
|
}
|
|
// Defense-in-depth: explicit traversal sequences must be
|
|
// rejected with a 4xx, not silently routed through the FS
|
|
// layer's own cleaning. This is what the green commit adds.
|
|
if w.Code < 400 || w.Code >= 500 {
|
|
t.Fatalf("path %q: expected explicit 4xx rejection, got %d (body=%q)", p, w.Code, body)
|
|
}
|
|
})
|
|
}
|
|
|
|
// Legit asset names with dots must still work.
|
|
legit := []string{"/app.js", "/customize-v2.js", "/themes/dark.css"}
|
|
for _, p := range legit {
|
|
t.Run("serves "+p, func(t *testing.T) {
|
|
req := httptest.NewRequest("GET", p, nil)
|
|
w := httptest.NewRecorder()
|
|
handler.ServeHTTP(w, req)
|
|
if w.Code != 200 {
|
|
t.Fatalf("legit asset %q returned %d, expected 200", p, w.Code)
|
|
}
|
|
})
|
|
}
|
|
}
|