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

New Issue

clawbot · 2026-08-09T03:46:19+02:00

clawbot commented

2026-08-09 03:46:19 +02:00

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.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/netwatch#29

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