From 7c326a2d89d8812573bc705f5fd0f70edfdeab7d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 04:53:28 +0000 Subject: [PATCH] Relicense to MIT; record why the original results were fabricated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit License: AGPL-3.0 to MIT, at the owner's request. Every runtime dependency is permissive (MIT, BSD-2-Clause, ISC; JSZip is dual-licensed and we take the MIT side), so nothing imposes copyleft obligations downstream. Adds the previously absent `license` field to package.json. Docs: adds CLAUDE.md and docs/POSTMORTEM.md. The first version of this app was generated, looked professional, and produced confident numbers that were substantially invented. That went unnoticed for months. These two files exist so the failure modes are not repeated or forgotten: - CLAUDE.md is the operating contract for anyone, human or agent, touching measurement code. It states the single rule (never substitute a value for a failed measurement), the enforcement mechanism (null plus strictNullChecks, and MetricValue having no fallback prop), a table of what a browser genuinely cannot do and what to report instead, and the specific traps in this codebase — the loosely typed history payload, the Leaflet innerHTML sinks, CSV formula injection, the third-party disclosure contract, and `{0 && ...}` rendering a bare zero. - docs/POSTMORTEM.md is the human-facing record: what was fabricated, and more usefully why review did not catch it. The failure paths were invisible on a working connection; `any` disabled the type checker at the export boundary and hid six wrong field names; @types/react was missing entirely so the whole React surface was implicitly any; and confident presentation outran substance. It ends with the transferable lessons, since none of this is specific to this project. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Dz4sWqBnBqN12tDaDn6b8D --- CLAUDE.md | 115 +++++++++++++++++++++++++++++++++++++++++ LICENSE | 30 ++++++----- README.md | 11 +++- docs/POSTMORTEM.md | 124 +++++++++++++++++++++++++++++++++++++++++++++ package.json | 1 + 5 files changed, 267 insertions(+), 14 deletions(-) create mode 100644 CLAUDE.md create mode 100644 docs/POSTMORTEM.md 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"