diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..211dfe0 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,115 @@ +# NetReady — working notes for AI agents + +Read this before changing measurement code. It exists because an earlier generated +version of this app shipped invented numbers that looked entirely convincing, and the +project owner only found out during a line-by-line review months later. The rules below +are the ones that would have prevented it. + +## What this project is + +A browser-only network diagnostics suite. No backend, no account, no build-time secrets. +Everything runs client-side; `npm run build` emits static files for GitHub Pages. + +**Stack:** React 19, TypeScript (`strict`), Vite 6, Tailwind v4, Leaflet, Recharts, +Vitest. `npm run check` = typecheck + lint + tests, and CI gates deployment on it. + +--- + +## The one rule that matters + +> **Never substitute a value for a measurement that failed.** + +If something could not be measured, it is `null`, and the UI renders `—` with a reason. +Not zero, not a typical value, not an extrapolation from a different metric. + +This is not a style preference. A diagnostic tool that invents numbers is worse than no +tool, because the user acts on the invention. An offline machine used to produce a +complete speed test and an A grade from this codebase. + +### In practice + +- Failed measurements are `number | null`. `strictNullChecks` then forces every display + site to handle absence — that is the enforcement mechanism, so do not weaken it. +- Use `??`, never `||`, when defaulting anything numeric. `||` rewrites a genuine `0`. +- Attach a `MeasurementFailure` (`{ metric, reason, detail }`) so the UI can say *why*. + Render it with `` and values with ``. +- `` deliberately has **no** `fallback` prop. Do not add one. +- Derived statistics need enough samples to exist. Jitter from one sample is not a small + number, it is no number — see `meanConsecutiveDelta`, which returns `null` below n=2. + +### Before you commit + +``` +grep -rnE '\|\|\s*[0-9]|\?\?\s*[1-9]' src/utils/ src/components/ +grep -rn 'Math.random' src/utils/ +``` + +The first should return only genuine coefficients in formulas. The second should return +only ID generation and cache-busters — `createId()` and `_cb=`/`_nr=` query params. + +--- + +## Know what a browser genuinely cannot do + +Half the original bugs came from simulating a capability rather than reporting its +absence. A browser **cannot**: + +| Not possible | Why | What to do instead | +|---|---|---| +| Traceroute | No raw sockets, no IP TTL control | `EdgePathExplorer` — measure the endpoints precisely | +| Read a cross-origin HTTP status | `no-cors` responses are opaque by construction | Report reachability and timing only | +| Read cross-origin phase timings | Needs `Timing-Allow-Origin` | Detect and report `timing-allow-origin-missing` | +| See handshake timings on a reused connection | Spec collapses them onto `fetchStart` | Detect and report `connection-reused` | +| Prove a TCP port is open | Connection failures are deliberately hidden | Timing heuristic, labelled as such | +| Read the LAN IP via WebRTC | Modern browsers return mDNS `.local` candidates | Say so; ask the user to enter it | +| Read security headers cross-origin | Not CORS-safelisted | Say the browser cannot see them | + +When you hit one of these, **say so in the UI**. The honesty is a feature — it is the +thing this tool has that mainstream speed tests do not. + +--- + +## Traps specific to this codebase + +- **`HistoryItem.data` is loosely typed.** This is how six CSV field-name mismatches + (`downloadMbps` vs `downloadSpeed`, `sent` vs `packetsSent`, …) shipped invisibly and + produced exports with entirely empty columns. Cast to a concrete `Partial` at the + boundary and let tsc check it. Every CSV generator has a test — keep it that way. +- **Leaflet `divIcon({html})` and `bindPopup(html)` are `innerHTML` sinks.** They were + fed raw user hostnames and third-party GeoIP strings, which was live XSS. Build DOM + nodes and set `textContent`. See `buildHopPopup` / `buildPopup`. +- **CSV needs formula-injection escaping**, not just quoting. `escapeCsv` prefixes + `=`, `+`, `-`, `@`, tab and CR. Always use it; never hand-roll quoting. +- **Third parties must stay disclosed.** `THIRD_PARTY_DISCLOSURES` in + `PrivacySafetyModal.tsx` and the README table are the contract with the user. If you + add a probe endpoint, add it there in the same commit. +- **Silent truncation is a lie by omission.** A `/16` expands to 65,534 hosts and only + 256 are scanned. `describeTargetExpansion` surfaces that. Do the same for any new cap. +- **`{value && }` renders a bare `0`** when `value` is `0`. Use an explicit + comparison. This shipped in the navbar. + +--- + +## Conventions + +- Comments explain *why*, especially where the non-obvious choice is deliberate. Several + comments in `network.ts` and `edgePath.ts` record what a line used to do wrong; leave + them, they are the guardrail. +- Tests go next to the code as `*.test.ts`. Prioritise pure logic where failures are + silent — parsers, formatters, classifiers, anything feeding an export. +- Prefer widening an existing honest abstraction over adding a parallel one. +- UI copy should be plain and specific. Avoid inflated language; the app previously + described a `localStorage` write as "Browser Persistence Online". + +## Verification + +Run `npm run check`. For anything touching measurement or rendering, also drive the real +app — `npm run build && npm run preview`, then Playwright against `127.0.0.1:4173` +(Chromium at `/opt/pw-browsers/chromium`). + +**The regression test that matters most:** go offline in DevTools and run every tool. +Every metric must read `—` with a reason. A number appearing anywhere is a P0. + +Browser checks have repeatedly caught what static review missed — a default `A+` +bufferbloat grade, a stray `0` in the navbar, a measured value only reachable through a +map popup. Do not skip them. diff --git a/LICENSE b/LICENSE index 79d5d83..01e677d 100644 --- a/LICENSE +++ b/LICENSE @@ -1,17 +1,21 @@ -GNU AFFERO GENERAL PUBLIC LICENSE -Version 3, 19 November 2007 +MIT License -Copyright (C) 2026 NetReady Contributors +Copyright (c) 2026 TechLuddite -This program is free software: you can redistribute it and/or modify -it under the terms of the GNU Affero General Public License as published by -the Free Software Foundation, either version 3 of the License, or -(at your option) any later version. +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: -This program is distributed in the hope that it will be useful, -but WITHOUT ANY WARRANTY; without even the implied crappy software warranty of -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -GNU Affero General Public License for more details. +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. -You should have received a copy of the GNU Affero General Public License -along with this program. If not, see . +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/README.md b/README.md index 72dcd21..69cd119 100644 --- a/README.md +++ b/README.md @@ -10,6 +10,11 @@ Its distinguishing rule: **NetReady never invents a number.** When a measurement `—` and tells you why. Most speed tests will happily hand you a plausible figure derived from nothing; this one won't. +That rule exists for a reason. An earlier generated version of this app fabricated results +whenever a measurement failed — convincingly enough that an offline machine still produced a full +report card and an A grade. [`docs/POSTMORTEM.md`](docs/POSTMORTEM.md) records what was wrong and +why it was hard to spot; [`CLAUDE.md`](CLAUDE.md) holds the rules that keep it from coming back. + --- ## 🛠️ Tools @@ -165,4 +170,8 @@ the [IEEE OUI registry](https://standards.ieee.org/products-programs/regauth/oui ## 🛡️ License -GNU Affero General Public License v3.0 — see [LICENSE](LICENSE). +MIT — see [LICENSE](LICENSE). Use it, fork it, ship it commercially; just keep the +copyright notice. + +All runtime dependencies are permissively licensed (MIT, BSD-2-Clause, ISC), so nothing +here imposes copyleft obligations downstream. diff --git a/docs/POSTMORTEM.md b/docs/POSTMORTEM.md new file mode 100644 index 0000000..daf5fdc --- /dev/null +++ b/docs/POSTMORTEM.md @@ -0,0 +1,124 @@ +# Postmortem: a diagnostics tool that invented its own results + +NetReady's first version was generated in Google AI Studio. It ran, it looked +professional, it had an animated map and live charts, and it produced confident +numbers. Those numbers were substantially fabricated, and the project owner did not +notice until a full line-by-line review much later. + +This document records what was wrong and — more usefully — *why it was hard to see*. It +is kept in the repo because the failure modes are general. Any generated codebase can +have them, and the same review habits catch them. + +--- + +## The headline + +**An offline machine produced a complete speed test and an A grade.** + +Every phase had a fallback that substituted a plausible value when measurement failed: + +| Location | On failure, reported | +|---|---| +| `network.ts:360` | `navigator.connection.downlink \|\| 25` Mbps | +| `network.ts:412` | Upload = `max(2.5, download × 0.45)`, plus half of every un-sent chunk counted as transferred | +| `network.ts:445` | Upload = `download × 0.4` | +| `network.ts:449` | Loaded ping = `ping + 14` — which then *graded bufferbloat* | +| `network.ts:241` | Ping = `18` ms, jitter = `3` ms | +| `network.ts:818` | Score defaulted to `dl=30, ul=10, lat=35, jit=5` | + +None of these was flagged in the UI. They flowed into `localStorage` and into exported +CSV reports — the artefact you would hand to an ISP. + +The scoring function had a second, subtler bug: it used `||` rather than `??`, so a +genuine measured **0 Mbps** was silently rewritten as **30 Mbps**. The failure case and +the "worst possible real result" case were indistinguishable. + +## The traceroute did not traceroute + +`tracert.ts` generated the entire middle of every route with `Math.random()`: hop IPs in +`162.219.x.x` (a real, allocated block), hostnames and ISPs cycled from a hardcoded +"transit backbone" table, interpolated coordinates, and a simulated 6% packet-loss rate +so the output would look realistically imperfect. + +A browser cannot send ICMP packets or set an IP TTL, so this was never going to work. +The correct response was to say so. Instead the UI rendered a CLI-styled terminal stream +reading `Tracing route to X over a maximum of 20 hops`, and exported the invented hops +to CSV with columns headed "Hop IP" and "Hop ISP / ASN". + +Related: when the GeoIP providers failed, lookups returned hardcoded Washington DC or +San Francisco coordinates — and **every private address resolved to San Francisco**, so +users saw their own LAN gateway pinned in California. + +## Why review did not catch it + +Four reasons, all worth internalising. + +**1. The failure paths were invisible in normal use.** On a working connection the app +is broadly correct. Fabrication only appears when something fails, which is exactly when +nobody is watching closely — and exactly when a diagnostic tool matters most. + +**2. `any` disabled the type checker at the critical boundary.** `HistoryItem.data` was +typed `any`, so the CSV exporter could read `downloadMbps`, `sent`, `queryTimeMs` and +`r.ttl` — **six field names that did not exist** — and compile cleanly. Every value fell +through to `''`, producing structurally valid CSVs with entirely empty data columns. A +silent, total failure of the export feature. + +**3. `@types/react` was missing from `package.json` entirely.** The whole React surface +was implicitly `any`. Adding the package and enabling `strict` produced **zero errors**, +which means the type safety had been available all along and simply switched off. + +**4. Presentation outran substance.** The animated map, the live charts and the confident +copy ("microsecond latency analysis", "Verified Location", "Zero Server Telemetry") all +signalled rigour. The gap between how trustworthy the app *looked* and how trustworthy it +*was* is the actual lesson here. + +## Also found + +- **DOM XSS.** Leaflet's `divIcon({html})` and `bindPopup()` are `innerHTML` sinks, fed + the user's raw typed hostname and unvalidated third-party GeoIP strings. +- **CSV formula injection.** Quotes were escaped, `=`/`+`/`-`/`@` were not. A hostname of + `=cmd|'/c calc'!A1` reached Excel as a live formula. +- **The privacy statement was false.** It promised no IP addresses, scan targets or + domain names were transmitted, while the browser sent them to twelve third parties. + True of the project's own servers — of which there are none — but that is not what the + sentence said. +- **Dead code presented as features.** A three-way speed-test server selector whose + "Auto-Detect Best" option never compared anything and whose "App Server" option pointed + at a backend absent from the static build. A subnet auto-detector that fell back to a + hardcoded `192.168.1.50` and labelled it "Detected Interface". A settings module with + zero call sites. +- **A fallback probe path that could never run.** The port scanner captured one `start` + timestamp before all three probe strategies, so every fallback saw its predecessor's + elapsed time and could only ever return `filtered`. + +## What changed + +The full fix is in [PR #1](https://github.com/TechLuddite/NetReady/pull/1) and +[PR #2](https://github.com/TechLuddite/NetReady/pull/2). In short: + +- Every fabrication removed. Failed measurements are `null` with a typed reason, rendered + as `—` plus an explanation. +- `strict` mode on, `HistoryItem.data` narrowed at every boundary, ESLint and Vitest + added, CI gated on all three. +- XSS and CSV injection fixed. +- Privacy copy rewritten to enumerate every third party and what it receives. +- The traceroute replaced by the **Edge Path Explorer**, which measures what a browser + genuinely can: real DNS/TCP/TLS/TTFB phase timings, the CDN edge that answered, HTTP/3 + negotiation as evidence of UDP blocking, and a speed-of-light distance bound. The old + route model is retained, clearly labelled simulated, so existing history still renders. + +## Transferable lessons + +1. **Judge generated code by its failure paths, not its happy path.** Ask "what does this + print when the network is down?" for every metric. +2. **`any` is where bugs hide from the compiler.** Especially at serialisation + boundaries. `strict` mode cost nothing here and would have caught the export bug. +3. **Check that declared dependencies are actually used and required ones are present.** + This project shipped `@google/genai`, `motion` and `dotenv` unused, while missing + `@types/react`. +4. **A fallback that produces a plausible value is worse than an error.** Errors get + noticed. Plausible values get trusted, saved and exported. +5. **Run the thing in a browser.** Static review missed a default `A+` bufferbloat grade + shown before any test ran, and a bare `0` rendered in the navbar. +6. **Impressive presentation is not evidence of correctness** — and for generated code, + the two are close to uncorrelated. diff --git a/package.json b/package.json index 1b29a70..5f7c91c 100644 --- a/package.json +++ b/package.json @@ -2,6 +2,7 @@ "name": "netready", "private": true, "version": "0.1.0", + "license": "MIT", "type": "module", "engines": { "node": ">=20"