improvments - #81
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.
… stability SQL drivers (PostgreSQL, MySQL, SQLite, MSSQL): - Add grade_data, chart_data, latency_underload, ping_during_test, client_id to INSERT statements — fields were silently dropped on all SQL backends - Replace SELECT * with explicit column list using COALESCE for backward compatibility with existing databases - Remove all SQL string concatenation — every query is now a complete literal with bind parameters only - SQLite: add new columns to CREATE TABLE and auto-migrate existing databases via ALTER TABLE ADD COLUMN (error on duplicate column is intentionally ignored) Telemetry data flow: - speedtest_worker.js: add computeGrade() and send grade_data, chart_data, latency_underload, ping_during_test in sendTelemetry() FormData and fallback postData — these were computed but never reaching the server - telemetry.go: read and persist the four new fields from the POST body - results/view.go: render real ChartData via template.JS (no more hardcoded sample data); parse GradeData JSON and display actual grade letter Session key stability (results/stats.go): - Replace securecookie.GenerateRandomKey(32) with SHA-256 of the configured stats password, initialized via sync.Once — previously every process restart invalidated all active sessions Other fixes: - telemetry.go: remove duplicate config.LoadedConfig() call, reuse conf - web/web.go: HTTP redirect now uses https:// only when enable_tls is true, falls back to http:// otherwise - results/stats.go: remove stray closing brace that silently terminated the <style> block early, causing all CSS rules after .badge-filtered to be ignored
|
@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.
2 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