improvments - #81
improvments#81alexwbaule wants to merge 1 commit into
Conversation
maddie
left a comment
maddie
left a comment
There was a problem hiding this comment.
Review
Nice work on this PR, especially isClientGone(), device grouping, and proper pagination. Found a few things that need fixing before merge:
1. SQL drivers don't persist the 5 new schema fields
The TelemetryData struct added GradeData, ChartData, LatencyUnderload, PingDuringTest, and ClientID, but the INSERT statements in all 4 SQL drivers weren't updated:
database/mysql/mysql.go:31-32— still only 11 columnsdatabase/postgresql/postgresql.go:31-32— samedatabase/sqlite/sqlite.go:29-43— CREATE TABLE also only has old columnsdatabase/mssql/mssql.go:43-47— same
This means client_id (sent by the worker and read in Record()) is silently dropped on SQL backends. Device grouping on /stats won't work. Also, SELECT * + Scan on existing databases will fail because column count doesn't match.
BoltDB and Memory are fine since they serialize the full struct.
2. Chart/grade/latency-under-load data never reaches the server
speedtest_worker.jssendTelemetry()— only appendsclient_idto FormData, doesn't includedlChartData,ulChartData, orlatencyUnderloadtelemetry.goRecord()— only readsclient_id, not grade/chart/underload fieldsresults/view.go:437-438— chart renders hardcoded demo data, not{{ .ChartData }}
3. Stray } breaks CSS in stats template
results/stats.go:557 — extra closing brace right after .badge-filtered block. Everything from .logout-btn downward won't parse. Same class of bug the PR description mentions fixing in the logout button.
4. Session key regenerated every restart
results/stats.go:160-162 — securecookie.GenerateRandomKey(32) runs at init, so a new key is generated every time the process starts. All existing sessions invalidated on restart; multi-instance deployments will have constant re-login prompts. Should be a fixed key from config.
5. config.LoadedConfig() called twice
telemetry.go lines 175 and 193 — the second call can reuse the first result.
Would be happy to re-review once the SQL column sync and telemetry data flow are addressed.
|
@maddie , fixed all things that you comment on review. |
maddie
left a comment
maddie
left a comment
There was a problem hiding this comment.
Review — re-check on 5ab1cc6 (base: 0bb00da)
Thanks for the fixes. Re-reviewed the full diff against origin/master — three of the five points are genuinely fixed, two are only partially fixed, and the re-review surfaced one stored-XSS, one frontend regression, and a few smaller items.
Previously raised
1. SQL drivers don't persist the 5 new schema fields — partially fixed. All four drivers now carry the 16 columns in INSERT/SELECT, and SQLite gets a CREATE TABLE + migration. But the three bootstrap schemas were not updated:
database/mysql/telemetry_mysql.sqldatabase/postgresql/telemetry_postgresql.sqldatabase/mssql/telemetry_mssql.sql
All three still create the old 13-column table (no grade_data, chart_data, latency_underload, ping_during_test, client_id), and these drivers have no runtime migration. README step 4 tells users to import exactly these files, so fresh installs on MySQL/PostgreSQL/MSSQL will fail on every INSERT and SELECT — telemetry recording returns 500 and stats breaks. COALESCE(...,'') handles NULL, not missing columns, so the backward-compat claim doesn't hold on those backends. Please update the three .sql files (and either add an idempotent migration or document the ALTERs for existing DBs).
2. Chart/grade/latency-under-load data never reaches the server — fixed. Worker sends all five fields on both telemetry paths; Record() reads them; the view renders real ChartData.
3. Stray } breaks stats CSS — fixed (removed in 5ab1cc6).
4. Session key regenerated every restart — fixed. Now derived from the stats password (sha256("speedtest-stats:"+StatsPassword)) with sync.Once — stable across restarts and multi-instance deployments.
5. config.LoadedConfig() called twice — partially fixed. RedactIP reuses the first result, but telemetry.go:236 (EnableIDObfuscation) still calls it a second time. Harmless in practice — just reuse the local result for consistency.
New findings
6. P1 — Stored XSS via chart_data. Record() stores r.FormValue("chart_data") verbatim (telemetry.go:219), and ViewPage injects it as raw JS:
chartJS := template.JS(record.ChartData) // results/view.go:88POST /results/telemetry and GET /results/view are both unauthenticated, and the stats page embeds /results/view in a same-origin iframe — so anyone can submit a crafted chart_data for an arbitrary UUID, and every visitor (including an admin with a live session) executes their script. Fix: json.Unmarshal into {dl:[{t,v}], ul:[{t,v}]} and reject invalid shapes before storing; only then pass it through template.JS (or render from the parsed JSON instead of injecting raw text).
7. P1 — index-modern.html is broken by the index.js rewrite. The rewritten web/assets/javascript/index.js targets the new DOM ids (dl-gauge, ul-gauge, ip-display, result-dl), but index-modern.html (unchanged, still the modern design behind README's ?design=new) uses the old ids (download-gauge, upload-gauge, …). On the first IP sample document.getElementById('ip-display').innerHTML throws on null; the rAF render loop dies and the gauges stay blank. Either port index-modern.html to the new ids or make index.js fall back to the old ones.
8. P2 — web/assets/design-switch.js is now dead code. New index.html no longer references it (zero references in web/), so the ?design=new toggle silently stops working. Delete the file or restore the reference.
9. P2 — Gauge drawing logic is duplicated. G_START/G_SWEEP, valueToAngle, buildLogTicks, drawGauge, etc. exist both in index.html's inline script (~701–773) and in javascript/index.js; the two copies have already started drifting. Consolidate into one.
10. P3 — SQLite runs blind ALTERs and swallows errors. database/sqlite/sqlite.go:53-57 unconditionally runs ALTER TABLE ... ADD COLUMN for the 5 new columns on every startup and ignores all errors. On a fresh DB each ALTER fails by design; on a broken migration a real error is hidden. Check PRAGMA table_info(speedtest_users) before altering.
11. P3 — seen map in results/stats.go (~333–341) is redundant. len(seen) always equals len(groups) — use len(groups) directly.
12. P3 — PingDuringTest is write-only. The worker reports it, all drivers store it, nothing reads or renders it. Either consume it on the view page or drop the column.
Happy to re-review once the XSS, schema files, and index-modern fixes are in.
|
@alexwbaule Can you please take a look at @maddie's comment? |
maddie
left a comment
maddie
left a comment
There was a problem hiding this comment.
Review — re-check on 4496367 (base: 59cff12)
All twelve items from the previous review are resolved, and the chart_data fix in particular holds on both the write and the render path (including for rows that were stored before the fix). Only outstanding items follow. I checked each one against master so I'm not reporting pre-existing behavior — every item below is introduced by this PR.
A. P1 — the default UI cannot run a test on an insecure origin (plain HTTP, non-localhost). Merge-blocking.
index.html:1130 hashes the device fingerprint with crypto.subtle.digest, and index.html:1218-1221 calls it with no .catch:
getClientIdentity().then(id => {
sp.setParameter('client_id', id);
sp.start();
});crypto.subtle is undefined on any insecure origin (WebCrypto is secure-context-only in every browser), so the promise rejects and sp.start() is never reached — silently and permanently.
Evidence (headless Chromium, same build, same page, only the origin differs):
http://192.0.2.10:8989 — a LAN address, i.e. an insecure origin:
window.isSecureContext === false,typeof crypto.subtle === 'undefined'getClientIdentity()→REJECTED: TypeError: Cannot read properties of undefined (reading 'digest')- after one click on Start Test on a cold-reloaded page: an unhandled
TypeError: Cannot read properties of undefined (reading 'digest')captured over CDP,state === 'running',data === null,sp.workernever constructed, zero requests togarbage.php/empty.php/getIP.php. The button stays on "Abort" forever; only a reload recovers.
http://localhost:8989 — a secure context, same page and binary: a full test completes (dl 27747 Mbps, ul 9601 Mbps, ping 1.00 ms, jitter 0.00 ms), the results panel and history populate, and the telemetry row is stored with a client_id of the form 3ff8c2a7-…:9adc1256559dfc0c.
Attribution: crypto.subtle appears nowhere on master (git grep crypto.subtle origin/master -- web/ → no matches). On master the root index.html was a 16-line stub that redirected to index-modern.html, which has no such call, so plain-HTTP deployments worked. This PR turns index.html into the real default UI, so every http://<ip>:<port> install now gets a page whose Start button does nothing. That is the common self-hosted shape (README step 4 walks users through plain HTTP), and this PR adds an HTTP→HTTPS redirect option, so plain HTTP is clearly an expected deployment.
The missing .catch is a broader hazard than crypto: if localStorage is blocked (private mode, restrictive cookie policy), getClientIdentity() rejects the same way and bricks the default UI identically.
Suggested fix — identity must never be able to swallow start():
getClientIdentity().catch(() => null).then(id => {
if (id) sp.setParameter('client_id', id);
sp.start();
});plus a feature-detect inside sha256hex()/getClientIdentity() (if (!crypto.subtle) return clientId;) — crypto.getRandomValues, which produces the UUID half, does work on insecure origins, so the UUID alone is a valid degraded identity. index.html:1130 is the only crypto.subtle use in web/assets/; javascript/index.js has none.
B. P2 — the grade treats a legitimate 0 as "missing" and penalizes it
speedtest_worker.js:743-744:
const pingMs = parseFloat(ping) || 999;
const jitterMs = parseFloat(jitter) || 999;0 ms jitter (LAN, loopback, or just a very stable line) becomes 999, which trips jitterMs > 50 → −15 points. Observed on a live run: measured jitter 0.00, stored grade_data = {"grade":"B","criteria":{…,"jitter":999,…}}, and /results/view renders <div class="grade-letter grade-b">B</div> for a 27.7 Gbps / 1.00 ms / 0.00 ms-jitter result. The sentinel should apply only when the measurement is genuinely absent:
const jitterMs = jitter === "" || jitter == null ? 999 : (parseFloat(jitter) || 0);(same for pingMs.) Attribution: master has neither the 999 sentinel nor computeGrade — this is new code in this PR.
C. P2 — three grade implementations, one of them dead, and the stored one ignores packet loss
- The description says the grade "factors in download, upload, ping, jitter, packet loss, and latency under load".
- The grade that is actually stored and displayed comes from the worker's
computeGrade()(speedtest_worker.js:737-753), which takes no loss argument at all. The stored record confirms it:criteriais{dl, ul, ping, jitter, latencyUnderload}— there is nolossterm anywhere in the score. index.html:1016-1055 calculateGrade()is loss-aware, but its result is assigned togradeData(declared at1119, assigned at1349) and never read again — nothing is sent or rendered from it.chartData(1118/1339) is likewise write-only. Roughly 45 lines of dead code.- The two algorithms are structurally different (100-point score vs. per-criterion letter grades), so the dead copy will keep drifting — the same class of problem as the duplicated gauge code from the previous round.
Please collapse this into one implementation that includes packet loss, treats 0 as measured rather than missing, and is the value that actually gets stored; then delete the page-side copy (or render the stored grade in the page instead of recomputing it). Attribution: master has no computeGrade, no calculateGrade/gradeData, no client_id/grade_data — the feature is entirely new here.
D. P3 — web/assets/styling/results.css (260 lines) is now dead CSS
This commit replaced index-modern.html's .gauge-layout / .gauge.download|upload / .progress / .speed / .ping / .jitter markup with canvas elements. results.css styles exactly that old markup, is imported only by styling/index.css:13, and index-modern.html is the only page that loads index.css — no element matches those selectors any more, and the new index.html does not load index.css at all. Delete the file and its @import. Attribution: on master those classes were live (index-modern.html used that markup and loaded styling/index.css); this PR is what orphaned them.
E. P3 — README documents a design toggle that no longer exists
README.md:154: "The Go version ships with two built-in UI designs (classic gauges and modern CSS), switchable via ?design=new URL parameter". design-switch.js was the only implementation of that toggle and it is now deleted, while the new index.html never referenced it — so ?design=new does nothing and index-classic.html is not linked from anywhere. Attribution: README.md is untouched by this PR (git diff origin/master...feature/new-interface -- README.md is empty, and line 154 is identical on both sides) — deleting design-switch.js is what made the existing documentation false. Either restore a switch or update the README; while there, note that index-modern.html is no longer a distinct "modern CSS" design but the same canvas-gauge UI wearing the fromScratch stylesheet.
F. P3 (optional) — a malformed chart_data now discards the entire result
telemetry.go:258-262 returns 400 when chart_data does not parse, so dl/ul/ping/etc. are dropped as well, for what is purely a cosmetic client-side payload problem. Since view.go re-validates before injecting into template.JS, safety does not depend on rejecting the insert; consider clearing just the chart field and recording the test. Attribution: new code — neither chart_data nor the sanitizer exists on master.
Verification notes. go build ./..., go vet ./... and go test ./... are clean at 4496367. I exercised the three previously-flagged areas against a live server: storage of all five new fields on SQLite; /results/view rendering (chart, grade, ping-under-load averages); and /stats login, numeric and substring filters, pagination and device grouping — including a deliberately poisoned legacy row inserted straight into the database, whose payload did not reach either the view page or the stats page. Finding A reproduced on three separate attempts on the insecure origin, one of them a cold reload with a single click and CDP exception capture; the secure-context run completed a full test.
Verdict. The correctness fixes from the previous round hold up. A is merge-blocking — the default UI cannot run a test on plain-HTTP deployments — and B/C mean the headline grade on the new results page is wrong for the fastest connections and does not include the packet loss the description claims. D–F are cleanups.
BKPepe
left a comment
BKPepe
left a comment
There was a problem hiding this comment.
Thanks for the work on this PR. Before the next review, could you please address the following points:
- Revisit the packet-loss measurement. The current implementation is based on HTTP request timeouts, which is not a reliable measurement of actual packet loss.
- Please clarify and/or redesign
client_id. It is fully client-controlled, so it should not be treated as a trusted device identity. - Fix the HTTP→HTTPS redirect so it does not blindly trust the incoming
Hostheader. - Add sensible limits/validation for telemetry payloads and
chart_datasize/contents. - Avoid loading up to 100,000 records and filtering them in memory for every search. Ideally move filtering into the database layer.
- Use deterministic ordering for pagination, e.g.
timestamp DESC, id DESC. - Please make the grade calculation and bufferbloat/latency metrics use a single consistent source of truth.
Also, please squash the 10 commits into a small number of coherent commits, preferably one logical commit for this PR.
The final commit message should clearly describe what the change does instead of using a generic message such as improvments. For example:
feat: add results view and improve speed test telemetry
The commit body should briefly summarize the main changes and any important implementation details.
Once these changes are in and the history is cleaned up, I'll do another full review.
Adds a shareable results page, a device-grouped stats/admin page, richer
telemetry (chart data, connection grade, latency-under-load, buffer bloat,
client-reported device id), and hardens the whole path end to end:
- Sanitize chart_data server-side (strict {dl,ul}:[{t,v}] shape, re-validated
again before being injected into the results page), closing a stored XSS
on the unauthenticated telemetry/results endpoints.
- Enforce size limits on every telemetry field and the request body itself,
so an unauthenticated endpoint can't be used to store unbounded data.
- Move stats-page search/filtering into the database layer (SQL WHERE for
the relational backends, in-place scans for bolt/memory) instead of
loading up to 100k rows into memory per search; pagination now orders by
timestamp DESC, id DESC for a stable, deterministic sort.
- Fix the HTTP->HTTPS redirect to stop trusting the client-supplied Host
header blindly: added an optional redirect_host config value, with strict
hostname validation as a fallback.
- Document that client_id is a self-reported, unauthenticated grouping hint
(never a verified device identity) everywhere it's produced and consumed.
- Made the packet-loss probe self-calibrate its timeout to the observed RTT
and abort in-flight requests instead of abandoning them, and labeled it
honestly in the UI as an HTTP-based estimate (browsers can't measure real
packet loss).
- Unified the buffer-bloat display and the stored connection grade on one
latency-under-load measurement (the worker's own ping-during-test samples)
instead of running a second, independent probe loop for the same thing.
- Synced the MySQL/PostgreSQL/MSSQL/SQLite schemas and drivers with the new
telemetry fields, with safe migrations for existing installs.
- Consolidated the duplicated canvas-gauge drawing code shared by index.html
and index-modern.html into javascript/gauges.js, and removed dead code
left over from the redesign (design-switch.js, results.css, an unused
client-side grade calculator).
295ed81 to
f70feba
Compare
|
Done |
3 participants
Summary
This PR modernizes the LibreSpeed Go backend with a new results view page, a redesigned admin
interface, improved test accuracy, stable device identification, and several reliability fixes.
Speed Test Improvements
and 20s max time (
time_auto: true), eliminating the previous asymmetry where upload ransignificantly shorter than download
t=0with a false zero — the timestampanchor (
dlT0/ulT0) is only set when the first non-zero speed sample arrives, so the graphalways starts from the actual measurement point
Results Share Page (
/results/view)/results/view?id=UUIDshowing a full breakdown of a test result(A–F), latency-under-load, and ISP/location info parsed from JSON into a structured grid
jitter, packet loss, and latency under load
shows the
/results/viewURL. Both the main share button and the history table share buttonsbehave identically
Stable Device Identification
clientIdstored inlocalStorage(UUIDv4), combined with aSHA-256 fingerprint of
userAgent + language + hardwareConcurrency + deviceMemory + platform + screenResolution + timezoneclientId:fingerprint16is sent with each test via theclient_idtelemetry fieldAdmin Statistics Interface (
/stats)Complete redesign of the admin page:
/results/viewpage embedded (iframe)Last100/L1000hack with properFetchAll(offset, limit)+Count()across all database drivers≥=≤><): Download, Upload, Ping, JitterClientID; devices without an ID shown as"Unknown Devices"
var conf = config.LoadedConfig()at package level was evaluated beforemain()loadedsettings.toml, causing stats to always show "Statistics Disabled" regardlessof the configured password — moved to inside the handler function
}closed the<style>block early, making all CSSrules after that point invisible to the browser
Database Layer
FetchAll(offset, limit int)andCount()to theDataAccessinterfaceTelemetryDatastruct was declared outside the scan loop inFetchLast100; sincejson.Unmarshaldoes not zero absent fields, values from one recordleaked into the next — moved declaration inside the loop
Schema Extensions
New fields added to
TelemetryData:GradeData{grade, criteria}ChartData{dl: [{t,v}], ul: [{t,v}]}LatencyUnderloadPingDuringTest{dl: [...], ul: [...]}— pings per phaseClientIDServer & Reliability
redirect_fromconfig option starts a plain-HTTP listeneron the specified port (e.g.
80) and issues a301redirect to the main server. Useshttp://or
https://based on whetherenable_tlsis set; omits port from the URL when it matchesthe scheme default (80/443)
http2: stream closed,broken pipe,connection reset by peer, andcontext canceledare normal when a browser abortsdownload/upload streams at test end — added
isClientGone()check so these no longerappear as errors in the log
/results/viewroute ordering fix: route was registered after the/results/wildcard,causing 404s — moved before the wildcard