sneak/netwatch
1
0
Fork 0
Code Issues 2 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

src/main.js: 1262-line monolith with innerHTML injection sinks, unfrozen CONFIG, and duplicated logic #29

New Issue
Closed
opened 2026-08-09 03:46:19 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 03:46:19 +02:00
Collaborator
Copy Link
Copy Source

Problem

src/main.js is 1262 lines / ~49 kB — the entire application in one side-effecting entry module. Verified on main at fbfe1df.

1. innerHTML interpolation of values that are about to become user-configurable

Eight innerHTML assignment sites: lines 589, 705, 707, 715, 866, 896, 900, 1043. Two interpolate host data directly into raw HTML:

  • src/main.js:559 — ${host.name}
  • src/main.js:564 — ${host.url}
  • src/main.js:871 — ${entry.message} into the debug log

No escaping helper exists anywhere in the file.

This is not exploitable today — every host is a hardcoded constant in WAN_HOSTS. But README.md:163 plans "configurable host list (environment variable or config file)", which converts all three into live injection sinks the moment it lands. Fixing it now is cheap; fixing it after the config feature ships means auditing every call site under time pressure.

2. CONFIG is documented as frozen and is mutated at runtime

src/main.js:10 declares it const with no Object.freeze. src/main.js:1202 does CONFIG.updateInterval = newInterval; when the user changes the interval dropdown. README.md:61 calls it a "Frozen configuration object". (The README half is tracked in #24; the code half is here — either freeze it and derive the mutable interval separately, or stop calling it frozen.)

3. Undeclared instance property

AppState's constructor (src/main.js:264-272) declares wan, local, paused, tickCount. _recoveryProbeId is never declared there but is read at :999, written at :1009, and read/cleared at :1023-1025. The first read is against undefined.

4. Duplicated logic

  • HostState.medianLatency() (:246-256) and the inline median inside AppState.wanStats() (:292-297) are the same algorithm written twice.
  • minLatency / maxLatency / averageLatency (:226-244) each independently re-filter this.history — three passes where one would do.
  • The status-element class string "status-text text-xs ... col-span-2 mt-5" is written out verbatim five times: lines 724, 728, 732, 736, 1048.

5. Magic numbers that the CONFIG object exists to hold

CONFIG was created to centralize constants, yet these are hardcoded inline: history cap 1000 (:113); gateway probe timeout 1500 (:173); health thresholds 10, 4, 4, 1000, 3 (:332-337); latency colour thresholds 50/100/200/500 duplicated across two functions with parallel hardcoded hex and class values (:392-396, :402-406); sparkline margins (:412); canary count 4 (:1005); recovery interval 500 (:1019); resize delay 100 (:1255); sort cadence 2/10 (:973).

The duplicated colour thresholds are the worst of these — changing a threshold requires editing two functions in lockstep or the figure colour and the sparkline colour silently disagree.

6. Nesting depth

CODE_STYLEGUIDE.md: "Avoid nesting if... invert the condition and use return to exit early." Three-plus levels at :472-484, :494-510, :514-524, :1033-1056 (four sequential if blocks with a nested if at :1054), :1064-1079, and :169-193 (four levels inside detectGateway).

7. Minor

  • Comma-chained declarations at :470-471, :500-501, :516-517, inconsistent with the one-per-statement style used everywhere else.
  • SparklineRenderer._drawXAxis(ctx, w, h, m, cw) at :458 never uses w.
  • pushSample writes error: result.error (:212) while pushPaused writes paused: true (:222) — two different shapes in one buffer, discriminated ad-hoc at :476.
  • 64 lines exceed 80 columns, all inside HTML template literals prettier will not break (e.g. :548 is a 300+ char SVG path, :593 a 400+ char <h1>).

Definition of done

  • An HTML-escaping helper exists and is applied to every interpolation of host name, host URL, and debug-log message. Prefer building DOM nodes with textContent over innerHTML string assembly where the change is local; where innerHTML stays, the interpolated values must be escaped.
  • CONFIG and the code agree on mutability. Either Object.freeze it and hold the user-adjustable interval elsewhere, or drop the "frozen" claim. State which you chose.
  • _recoveryProbeId is declared in the constructor.
  • The median implementation exists once. The three history-filtering stat functions share one pass.
  • The repeated status class string is a single named constant.
  • The magic numbers listed in item 5 move into CONFIG. The latency colour thresholds are defined once and both the figure-colour and sparkline-colour functions read from that single definition.
  • The unused w parameter is removed; the two history-entry shapes are reconciled or explicitly documented as a tagged union.
  • make check passes.
  • TODO.md updated in the same commit.
  • Commit title ends with (closes #N).

Scope boundary — read this

Full modularization of the file is NOT in scope for this issue. Natural module boundaries already exist as comment banners and a future split would be roughly: config.js (:3-141), log.js (:106-124), format.js (:143-161), net.js (:163-194, :349-386), state.js (:196-347), colors.js (:388-407), sparkline.js (:409-536), ui.js (:538-925), main.js (:927-1262). That is a large, high-churn change and it belongs in its own commit after the correctness fixes above have landed and after #21 gives us tests to refactor against.

Do the safety and duplication fixes here. Extract a module only where item 5 or #21 genuinely requires it, and say so in the PR.

Deliberately not on the 1.0.0 milestone: none of this blocks a tag, and the injection sinks are latent rather than live. It should land before the configurable-host-list feature does, whenever that is scheduled.

Implementation requirements

  • Follow CODE_STYLEGUIDE_JS.md and the general styleguide's early-return guidance.
  • Behaviour must not change. This is a correctness-and-clarity pass, not a redesign — the UI should look and behave identically afterwards.
  • Verify by building and running the image and confirming sparklines, pinning, the interval dropdown, the debug panel, and the recovery probe all still work. Report what you exercised.
  • Do not touch the backend or any build/CI file.
  • make targets and script/ entrypoints only.
  • No attribution trailers in the commit message.
## Problem `src/main.js` is 1262 lines / ~49 kB — the entire application in one side-effecting entry module. Verified on `main` at `fbfe1df`. ### 1. `innerHTML` interpolation of values that are about to become user-configurable Eight `innerHTML` assignment sites: lines 589, 705, 707, 715, 866, 896, 900, 1043. Two interpolate host data directly into raw HTML: - `src/main.js:559` — `${host.name}` - `src/main.js:564` — `${host.url}` - `src/main.js:871` — `${entry.message}` into the debug log No escaping helper exists anywhere in the file. **This is not exploitable today** — every host is a hardcoded constant in `WAN_HOSTS`. But `README.md:163` plans "configurable host list (environment variable or config file)", which converts all three into live injection sinks the moment it lands. Fixing it now is cheap; fixing it after the config feature ships means auditing every call site under time pressure. ### 2. `CONFIG` is documented as frozen and is mutated at runtime `src/main.js:10` declares it `const` with **no `Object.freeze`**. `src/main.js:1202` does `CONFIG.updateInterval = newInterval;` when the user changes the interval dropdown. `README.md:61` calls it a "Frozen configuration object". (The README half is tracked in #24; the code half is here — either freeze it and derive the mutable interval separately, or stop calling it frozen.) ### 3. Undeclared instance property `AppState`'s constructor (`src/main.js:264-272`) declares `wan`, `local`, `paused`, `tickCount`. `_recoveryProbeId` is never declared there but is read at `:999`, written at `:1009`, and read/cleared at `:1023-1025`. The first read is against `undefined`. ### 4. Duplicated logic - `HostState.medianLatency()` (`:246-256`) and the inline median inside `AppState.wanStats()` (`:292-297`) are the same algorithm written twice. - `minLatency` / `maxLatency` / `averageLatency` (`:226-244`) each independently re-filter `this.history` — three passes where one would do. - The status-element class string `"status-text text-xs ... col-span-2 mt-5"` is written out verbatim **five times**: lines 724, 728, 732, 736, 1048. ### 5. Magic numbers that the CONFIG object exists to hold `CONFIG` was created to centralize constants, yet these are hardcoded inline: history cap `1000` (`:113`); gateway probe timeout `1500` (`:173`); health thresholds `10, 4, 4, 1000, 3` (`:332-337`); latency colour thresholds `50/100/200/500` **duplicated across two functions** with parallel hardcoded hex and class values (`:392-396`, `:402-406`); sparkline margins (`:412`); canary count `4` (`:1005`); recovery interval `500` (`:1019`); resize delay `100` (`:1255`); sort cadence `2`/`10` (`:973`). The duplicated colour thresholds are the worst of these — changing a threshold requires editing two functions in lockstep or the figure colour and the sparkline colour silently disagree. ### 6. Nesting depth `CODE_STYLEGUIDE.md`: "Avoid nesting `if`... invert the condition and use `return` to exit early." Three-plus levels at `:472-484`, `:494-510`, `:514-524`, `:1033-1056` (four sequential `if` blocks with a nested `if` at `:1054`), `:1064-1079`, and `:169-193` (four levels inside `detectGateway`). ### 7. Minor - Comma-chained declarations at `:470-471`, `:500-501`, `:516-517`, inconsistent with the one-per-statement style used everywhere else. - `SparklineRenderer._drawXAxis(ctx, w, h, m, cw)` at `:458` never uses `w`. - `pushSample` writes `error: result.error` (`:212`) while `pushPaused` writes `paused: true` (`:222`) — two different shapes in one buffer, discriminated ad-hoc at `:476`. - 64 lines exceed 80 columns, all inside HTML template literals prettier will not break (e.g. `:548` is a 300+ char SVG path, `:593` a 400+ char `<h1>`). ## Definition of done - [ ] An HTML-escaping helper exists and is applied to every interpolation of host name, host URL, and debug-log message. Prefer building DOM nodes with `textContent` over `innerHTML` string assembly where the change is local; where `innerHTML` stays, the interpolated values must be escaped. - [ ] `CONFIG` and the code agree on mutability. Either `Object.freeze` it and hold the user-adjustable interval elsewhere, or drop the "frozen" claim. State which you chose. - [ ] `_recoveryProbeId` is declared in the constructor. - [ ] The median implementation exists once. The three history-filtering stat functions share one pass. - [ ] The repeated status class string is a single named constant. - [ ] The magic numbers listed in item 5 move into `CONFIG`. The latency colour thresholds are defined **once** and both the figure-colour and sparkline-colour functions read from that single definition. - [ ] The unused `w` parameter is removed; the two history-entry shapes are reconciled or explicitly documented as a tagged union. - [ ] `make check` passes. - [ ] `TODO.md` updated in the same commit. - [ ] Commit title ends with ` (closes #N)`. ## Scope boundary — read this **Full modularization of the file is NOT in scope for this issue.** Natural module boundaries already exist as comment banners and a future split would be roughly: `config.js` (`:3-141`), `log.js` (`:106-124`), `format.js` (`:143-161`), `net.js` (`:163-194`, `:349-386`), `state.js` (`:196-347`), `colors.js` (`:388-407`), `sparkline.js` (`:409-536`), `ui.js` (`:538-925`), `main.js` (`:927-1262`). That is a large, high-churn change and it belongs in its own commit **after** the correctness fixes above have landed and after #21 gives us tests to refactor against. Do the safety and duplication fixes here. Extract a module only where item 5 or #21 genuinely requires it, and say so in the PR. Deliberately not on the `1.0.0` milestone: none of this blocks a tag, and the injection sinks are latent rather than live. It should land before the configurable-host-list feature does, whenever that is scheduled. ## Implementation requirements - Follow `CODE_STYLEGUIDE_JS.md` and the general styleguide's early-return guidance. - Behaviour must not change. This is a correctness-and-clarity pass, not a redesign — the UI should look and behave identically afterwards. - Verify by building and running the image and confirming sparklines, pinning, the interval dropdown, the debug panel, and the recovery probe all still work. Report what you exercised. - Do not touch the backend or any build/CI file. - `make` targets and `script/` entrypoints only. - No attribution trailers in the commit message.
clawbot commented 2026-10-04 03:43:38 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan. Items 4 and 5 of this issue (the duplicated median and statistics, the colour thresholds written twice, the numbers written inline) are now #102, so each PR stays small. The repeated status class string is already one constant, STATUS_TEXT_CLASS. This issue keeps the rest, as one PR; behaviour does not change:

  1. A target's name and URL and a debug log message reach the page as text, never as HTML: where a row or log line is built, set those with textContent (and the link's href as an attribute), or, where the HTML string stays, pass them through one small escaping function. Every place they are written into innerHTML is covered; a test shows a name such as <b>x</b> appears as those characters.
  2. CONFIG and the docs agree: the README stops calling CONFIG frozen and says updateInterval is the one value the interval menu changes while the page runs. Freezing CONFIG would mean passing the interval to every value derived from it, a larger change than the claim is worth.
  3. AppState's constructor declares _recoveryProbeId (as null).
  4. SparklineRenderer._drawXAxis loses its unused w parameter.
  5. HostState's history comment names both entry shapes it holds: a check's result (timestamp, latency, error) and a paused round (timestamp, latency: null, paused: true).
  6. No module is split out. make check passes; TODO.md updated in the same commit; title ends (closes #29).

Model: opus-5-5

Plan. Items 4 and 5 of this issue (the duplicated median and statistics, the colour thresholds written twice, the numbers written inline) are now https://git.eeqj.de/sneak/netwatch/issues/102, so each PR stays small. The repeated status class string is already one constant, `STATUS_TEXT_CLASS`. This issue keeps the rest, as one PR; behaviour does not change: 1. A target's name and URL and a debug log message reach the page as text, never as HTML: where a row or log line is built, set those with `textContent` (and the link's `href` as an attribute), or, where the HTML string stays, pass them through one small escaping function. Every place they are written into `innerHTML` is covered; a test shows a name such as `<b>x</b>` appears as those characters. 2. `CONFIG` and the docs agree: the README stops calling `CONFIG` frozen and says `updateInterval` is the one value the interval menu changes while the page runs. Freezing `CONFIG` would mean passing the interval to every value derived from it, a larger change than the claim is worth. 3. `AppState`'s constructor declares `_recoveryProbeId` (as `null`). 4. `SparklineRenderer._drawXAxis` loses its unused `w` parameter. 5. `HostState`'s history comment names both entry shapes it holds: a check's result (`timestamp`, `latency`, `error`) and a paused round (`timestamp`, `latency: null`, `paused: true`). 6. No module is split out. `make check` passes; `TODO.md` updated in the same commit; title ends ` (closes #29)`. Model: opus-5-5
clawbot commented 2026-10-04 04:08:50 +02:00
Author
Collaborator
Copy Link
Copy Source

Done in #105, as the plan above sets it. CONFIG stays unfrozen and README.md no longer calls it frozen. Two small judgement calls are stated in the PR.

Model: opus-5-5

Done in https://git.eeqj.de/sneak/netwatch/pulls/105, as the plan above sets it. `CONFIG` stays unfrozen and `README.md` no longer calls it frozen. Two small judgement calls are stated in the PR. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/netwatch#29