mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-09-26 01:13:33 +00:00
c283f42c1fd1e43fc3dbd78ae6ed4ef4178bd067
6
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Anupam MedirattaandClaude Opus 5
|
de237fc29c |
fix(qa): bind TEST_PUBKEY as a SQLite parameter instead of interpolating it (#1982)
Closes #1977. Supersedes #1952. Follow-up filed as #1983. ## What §10.2 did ```bash q="SELECT COUNT(*) FROM transmissions WHERE from_node = '$TEST_PUBKEY';" qq=$(printf %q "$q") if ! count=$(ssh_t "docker exec … sqlite3 … $qq" 2>/dev/null); then count=$(ssh_t "sqlite3 … $qq" 2>/dev/null || echo "") fi ``` The injection is not reachable today — `TEST_PUBKEY` is hex-gated and the script `exit 2`s before the SQL is built. The problem is that the SQL layer's safety rests entirely on that outer gate rather than on the SQL layer itself. #1952 proposed doubling embedded quotes; that is string escaping, not parameterisation, which is why it was withdrawn in favour of this. ## What this does Per the four points in the sign-off on #1977: **1. Bind the value.** A constant `SELECT` and a bound `:pubkey`, fed to sqlite3 on stdin. The SQL no longer crosses the remote shell as a command word, so there is no `printf %q` on the query at all any more. **Why hex rather than `.parameter set :pk '<value>'`.** Dot-command arguments are split on whitespace, so a payload containing a space produces too many arguments — and sqlite3 responds by printing the `.parameter` help to **stdout**, exiting **0**, and leaving `:pk` **unbound**. `COUNT(*)` then returns 0, which reads exactly like a passing security fix. `-bail` does not catch it. Verified on 3.51.0: ``` $ printf ".parameter set :pk '' OR 1=1 --'\nSELECT COUNT(*) FROM transmissions WHERE from_node = :pk;\n" \ | sqlite3 -bail ptest.db .parameter CMD ... Manage SQL parameter bindings # <- help, on stdout … 0 # <- :pk never bound $ echo $? 0 ``` `.parameter set :pk 1+1` also binds the integer `2` — the value is evaluated as an SQL expression and only falls back to a text literal when evaluation fails. So interpolating into the `.parameter set` line trades one hazard for another. Hex-encoding removes the quoting layer instead of adding one: the value is bound as `cast(x'<hex>' as text)`, so its contribution to the SQL text is drawn from the alphabet `[0-9a-f]` only. Nothing to quote, no tokenizer arity hazard, and it holds for **arbitrary** input rather than only for hex-gated input — which is the point. Verified against a fixture table holding two rows, one of them `deadbeef`: | value | result | exit | |---|---|---| | `deadbeef`, bound as `cast(x'6465616462656566' as text)` | `1` | 0 | | `' OR 1=1 --`, bound the same way | `0` | 0 | | `' OR 1=1 --`, interpolated the current way | `2` (whole table) | 0 | | query against a DB with no `transmissions` table, `-bail` | `Parse error … no such table` on **stderr** | **1** | **2. Probe the capability, not a version.** `resolve_sqlite_runner` binds `corescope-probe-ok` and asserts it comes back — a round trip, not a bare `.parameter init`, so the positive control runs against the operator's actual binary rather than one we pin. If neither the container nor the host qualifies, it fails loudly and names what is needed: ``` ❌ retain-failed: no sqlite3 able to bind a parameter on the target tried: docker exec -i corescope-prod sqlite3, then sqlite3 on runner@example need: the sqlite3 CLI reachable over ssh, supporting '.parameter set' OCI runtime exec failed: exec: "sqlite3": executable file not found in $PATH bash: line 1: sqlite3: command not found ``` There is deliberately **no** interpolating fallback. That would leave the vulnerable path in place under a nicer name. **3. The hex gate is kept**, with its comment updated to say why: for the SQL layer it is now defence in depth rather than the only guard. Redundant is not the same as wrong. **4. The exit status and stderr survive.** `-batch -bail -init /dev/null -noheader -list` (stop at the first SQL error; ignore the operator's `~/.sqliterc`, where a stray `.mode` would make the count unparseable; stdout is exactly the number). Query stderr is captured and printed on failure rather than sent to `/dev/null`, so a broken query is distinguishable from a legitimately empty result. Probe stderr is collected too, and printed only if *both* probes fail — the container miss is the known-normal case, so surfacing it on every run would be noise. ## Also fixed An existing double-count in §10.2: the `TARGET_DB_PATH unset` branch incremented `$fails` and then left `count=""`, so the generic branch incremented it a **second** time for the same failure. `read_retain_count` now gives §10.2 exactly one increment point. Opportunistic cleanup in a file already being touched (AGENTS.md line 318). ## Tests New `qa/scripts/test-blacklist-sql.sh`, wired into the `go-test` job. 24 assertions, modelled on `scripts/staging/test-disk-monitor.sh`. Both directions are asserted, because a zero from a command that failed proves nothing: - **Positive control** — `deadbeef` still returns its row (`1`, exit 0), and so does `cafebabe`; an absent pubkey returns `0`. - **Negative** — `' OR 1=1 --` returns `0` while the table demonstrably holds 2 rows, and the old interpolated form is asserted to leak all `2`. That last assertion is what makes the `0` above worth something. - **Error surfacing** — the same SQL against a DB with no `transmissions` table exits non-zero with a message on stderr and nothing on stdout. - **Alphabet** — `sql_hex_literal` output matches `^x'[0-9a-f]*'$` for the SQL payloads, a backslash, `$(id)` / backticks, an embedded newline, `héllo`, and a 4096-byte repetitive string. That last one is a regression guard for `od -v`: without the flag `od` collapses repeated identical lines to `*`. - `run_sqlite` with no resolved runner refuses rather than guessing. Group 2 skips loudly (rather than silently) if `sqlite3` is not on PATH; group 1 needs no sqlite3 and always runs. **Mutation-tested** — each of these breaks the suite, so the assertions have teeth: | mutation | caught by | |---|---| | restore full interpolation | `injection payload → 0 rows — expected '0' got '2'` | | naive `.parameter set '%s'` | `expected '0' got '.parameter CMD ...'` | | drop `od -v` | alphabet failure on `*`, plus `expected '8192' got '33'` | Commit 1 is a behaviour-neutral refactor that moves the imperative body into `main()` behind a `BASH_SOURCE` guard, so the test can source the script and exercise individual helpers. Same idiom as `scripts/staging/disk-monitor.sh:99`. ## Verification - `bash qa/scripts/test-blacklist-sql.sh` → 24 passed, 0 failed - `bash -n` on both scripts - All three runtime paths exercised end to end with PATH shims for `ssh`/`docker`/`sqlite3` against a real fixture DB: success (`sqlite3 runner: host`, count 2), query failure (classified message + `Parse error … no such table`, `fails=1`), and no-capability (the loud block above, both probe stderrs, `fails=1` — not 2) - The new step lands inside `go-test`, which runs when `changes.outputs.code == 'true'`; `qa/scripts/*.sh` does not match that job's `^docs/|[.]md$|^LICENSE$` documentation filter, so it is not skipped ## Deliberately out of scope - **The `docker exec` branch is dead on current images** → filed as #1983. The app container has no `sqlite3` at all: `Dockerfile:15` is pure-Go SQLite with no CGO, and the `apk add` installs only `mosquitto mosquitto-clients supervisor caddy wget`. So the host "fallback" is the only path that has ever executed, silently, because both branches discarded stderr. This change keeps both branches and merely makes the outcome visible (`sqlite3 runner: …` on every run). - **`-readonly` on the target DB.** Tempting, and verified compatible with `.parameter` (the binding table lives in the TEMP database), but a WAL database needing journal recovery can refuse a read-only open. Adding it here risks exactly the "trades an unreachable injection for a script that does not run" outcome flagged in the #1952 thread. Worth its own issue. - **The other `2>/dev/null` sites** in this file, which also sit awkwardly with `qa/README.md`'s "Don't silence stderr". Only the §10.2 lines named in the sign-off are touched. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Kpa-clawbotandKpa-clawbot
|
f30e6bef28 |
qa(plan): reconcile §8.2/§5.3/§6.2 + add §8.7 (Recent Packets readability) (#838)
Doc-only reconciliation of v3.6.0-rc plan with what actually shipped. ## Changes - **§8.2** — desktop deep link now opens full-screen view (post-#823/#824), not split panel as the plan still asserted. - **§5.3** — pin that severity now derives from `recentMedianSkewSec` (#789), not the all-time `medianSkewSec` — a re-tester needs to know which field drives the badge. - **§6.2** — pin the existing observer-graph element location (`public/analytics.js:2048-2051`). - **New §8.7** — side-panel "Recent Packets" entries must navigate to a valid packet detail (DB-fallback per #827) AND text must be readable in the current theme (explicit color per #829). Both bugs surfaced this session. No CI gates. Co-authored-by: Kpa-clawbot <agent@corescope.local> |
||
|
Kpa-clawbotandKpa-clawbot
|
e31e14cae9 |
qa(plan): apply v3.6.0-rc QA findings (#832/#833/#836) (#837)
Apply v3.6.0-rc QA learnings to the plan. ## Changes - **§1.1** — 1 GB cap is unrealistic on real DBs without `GOMEMLIMIT` + bounded cold-load. Raised target to 3 GB and pointed to follow-up **#836**. (Investigation showed cold-load transient blows past any sub-2GB cap regardless of `maxMemoryMB` setting because `runtime.MemStats.NextGC` ignores cgroup ceilings.) - **§1.4** — `trackedBytes`/`trackedMB` is in-store packet bytes only and under-reports RSS by ~3–5× (no indexes, caches, runtime overhead, cgo). Switched the assertion to use `processRSSMB` exposed by **#832** (PR **#835**). - **§11.1** — noted the Playwright deep-link E2E assertion was updated by **#833** (PR **#834**) to match the post-#823 full-screen behavior. ## Why Three real findings from the QA ops sweep ([§1.4 fail comment](https://github.com/Kpa-clawbot/CoreScope/issues/809#issuecomment-4286113141)). Updating the plan so the next run doesn't replay the same false-fail/false-pass conditions. Co-authored-by: Kpa-clawbot <agent@corescope.local> |
||
|
Kpa-clawbotandMeshCore Agent
|
3f26dc7190 |
obs: surface real RSS alongside tracked store bytes in /api/stats (#832) (#835)
Closes #832. ## Root cause confirmed \`trackedMB\` (\`s.trackedBytes\` in \`store.go\`) only sums per-packet struct + payload sizes recorded at insertion. It excludes the index maps (\`byHash\`, \`byTxID\`, \`byNode\`, \`byObserver\`, \`byPathHop\`, \`byPayloadType\`, hash-prefix maps, name lookups), the analytics LRUs (rfCache/topoCache/hashCache/distCache/subpathCache/chanCache/collisionCache), WS broadcast queues, and Go runtime overhead. It's \"useful packet bytes,\" not RSS — typically 3–5× off on staging. ## Fix (Option C from the issue) Expose four memory fields on \`/api/stats\` from a single cached snapshot: | Field | Source | Semantics | |---|---|---| | \`storeDataMB\` | \`s.trackedBytes\` | in-store packet bytes; eviction watermark input | | \`goHeapInuseMB\` | \`runtime.MemStats.HeapInuse\` | live Go heap | | \`goSysMB\` | \`runtime.MemStats.Sys\` | total Go-managed memory | | \`processRSSMB\` | \`/proc/self/status VmRSS\` (Linux), falls back to \`goSysMB\` | what the kernel sees | \`trackedMB\` is retained as a deprecated alias for \`storeDataMB\` so existing dashboards/QA scripts keep working. Field invariants are documented on \`MemorySnapshot\`: \`processRSSMB ≥ goSysMB ≥ goHeapInuseMB ≥ storeDataMB\` (typical). ## Performance Single \`getMemorySnapshot\` call cached for 1s — \`runtime.ReadMemStats\` (stop-the-world) and the \`/proc/self/status\` read are amortized across burst polling. \`/proc\` read is bounded to 8 KiB, parsed with \`strconv\` only — no shell-out, no untrusted input. \`cgoBytesMB\` is omitted: the build uses pure-Go \`modernc.org/sqlite\`, so there is no cgo allocator to measure. Documented in code comment. ## Tests \`cmd/server/stats_memory_test.go\` asserts presence, types, sign, and ordering invariants. Avoids the flaky \"matches RSS to ±X%\" pattern. \`\`\` $ go test ./... -count=1 -timeout 180s ok github.com/corescope/server 19.410s \`\`\` ## QA plan §1.4 now compares \`processRSSMB\` against procfs RSS (the right invariant); threshold stays at 0.20. --------- Co-authored-by: MeshCore Agent <meshcore-agent@openclaw.local> |
||
|
|
5ff4b75a07 |
qa: automate §10.1/§10.2 nodeBlacklist test (#822)
Automates QA plan §10.1 (nodeBlacklist hide) and §10.2 (DB retain), flipping both rows from `human` to `auto`. Stacks on top of #808. **What** - New `qa/scripts/blacklist-test.sh` — env-driven harness: - Args: `BASELINE_URL TARGET_URL TEST_PUBKEY` - Env: `TARGET_SSH_HOST`, `TARGET_SSH_KEY` (default `/root/.ssh/id_ed25519`), `TARGET_CONFIG_PATH`, `TARGET_CONTAINER`, optional `TARGET_DB_PATH` / `ADMIN_API_TOKEN`. - Edits `nodeBlacklist` on target via remote `jq` (python3 fallback), atomic move with preserved perms. - Restarts container, waits up to 120 s for `/api/stats == 200`. - §10.1 asserts `/api/nodes/{pk}` is 404 **or** absent from `/api/nodes` listing, and `/api/topology` does not reference the pubkey. - §10.2 prefers `/api/admin/transmissions` if `ADMIN_API_TOKEN` set, else falls back to `sqlite3` inside the container (and host as last resort). - **Teardown is mandatory** (`trap … EXIT INT TERM`): removes pubkey, restarts, verifies the node is visible again. Teardown failures count toward exit code. - Exit code = number of failures; per-step ✅/❌ with classified failure modes (`ssh-failed`, `restart-stuck`, `hide-failed`, `retain-failed`, `teardown-failed`). - `qa/plans/v3.6.0-rc.md` §10.1 / §10.2 mode → `auto (qa/scripts/blacklist-test.sh)`. **Why** Manual blacklist verification was the slowest item in the §10 block and the easiest to get wrong (forgetting teardown leaks state into the next QA pass). Now it's a single command, public-repo-safe (zero PII / hardcoded hosts), and the trap guarantees the target is restored. `bash -n` passes locally. Live run requires staging credentials. --------- Co-authored-by: meshcore-agent <agent@meshcore> Co-authored-by: meshcore-agent <meshcore@openclaw.local> |
||
|
Kpa-clawbotandyou
|
f701121672 |
Add qa/ — project-specific QA artifacts for the qa-suite skill (#808)
Adds the CoreScope-side artifacts that pair with the generic [`qa-suite` skill](https://github.com/Kpa-clawbot/ai-sdlc/pull/1). ## Layout ``` qa/ ├── README.md ├── plans/ │ └── v3.6.0-rc.md # 34-commit test plan since v3.5.1 └── scripts/ └── api-contract-diff.sh # CoreScope-tuned API contract diff ``` The skill ships the reusable engine + qa-engineer persona + an example plan. This PR adds the CoreScope-tuned plan and the CoreScope-tuned script (correct seed lookups for our `{packets, total}` response shape, our endpoint list, our `resolved_path` requirement). Read by the parent agent at runtime. ## How to use From chat: - `qa staging` — runs the latest `qa/plans/v*-rc.md` against staging, files a fresh GH issue with the report - `qa pr <N>` — uses `qa/plans/pr-<N>.md` if present, else latest RC plan; comments on the PR - `qa v3.6.0-rc` — runs that specific plan The qa-engineer subagent walks every step, classifying each as `auto` (script) / `browser` (UI assertion) / `human` (manual) / `browser+auto`. Quantified pass criteria are mandatory — banned phrases: 'visually aligned' / 'fast' / 'no regression'. ## Plan v3.6.0-rc contents Covers the 34 commits since v3.5.1: - §1 Memory & Load (#806, #790, #807) — heap thresholds, sawtooth pattern - §2 API contract (#806) — every endpoint that should carry `resolved_path`, auto-checked by `api-contract-diff.sh` - §3 Decoder & hashing (#787, #732, #747, #766, #794, #761) - §4 Channels (#725 series M1–M5) - §5 Clock skew (#690 series M1–M3) - §6 Observers (#764, #774) - §7 Multi-byte hash adopters (#758, #767) - §8 Frontend nav & deep linking (#739, #740, #779, #785, #776, #745) - §9 Geofilter (#735, #734) - §10 Node blacklist (#742) - §11 Deploy/ops Release blockers: §1.2, §2, §3. §4 is the headline-feature gate. ## Adding new plans Per release: copy `plans/v<last>-rc.md` to `plans/v<new>-rc.md` and update commit-range header, new sections, GO criteria. Per PR: create `plans/pr-<N>.md` with the bare minimum for that PR's surface area. Co-authored-by: you <you@example.com> |