fix(web): bind six dead controls programmatically, restrict client UIDs, clamp config inputs, add page checker to CI

[dorkmo]

Stacked PR. Base branch is website-style-consistency (#314) because the server's pages are single very long source lines, so two PRs that touch the same page conflict textually. Merge order: #314 → #315 → #316 → #317. This PR's diff shows only its own changes.

Summary

Functional fix for the six dead page controls (master review S-W02), a server-side character set for client UIDs, two config-generator input clamps that were silently exceeding client firmware limits, and a CI check so the dead-handler class of bug cannot come back.

Six controls that threw ReferenceError on click

Every page script runs inside (async () => { ... })();. A function name(){} declared there is not visible to an inline onclick="name()", so the click threw ReferenceError: name is not defined and nothing else reported it:

Page	Control	Handler
/site-config	Expect Update, Remove Client (plus Edit Config and the click-to-copy UID, same pattern)	expectUpdate, deleteClient, copyUid
/calibration	per-sensor Reset, sensor-name link (view data points)	resetCalibration, viewTankPoints
/transmission-log	Cancel on a pending config	cancelPendingConfig
/historical	custom date range start/end (onchange)	renderLevelChart
/ (dashboard), /client-console	Update, Snooze Reminders, Clear Relay, Remove Client; Edit Config(uration), Approve Deletion (already working, same UID splice; second commit)	requestUpdate, snoozeAlarm, clearRelays, deleteClient, approveDeletion
/config-generator	cloud client picker rows (already working, same UID splice; third commit)	fetchClientConfig

Those attributes also spliced client UIDs into JavaScript source (onclick="resetCalibration('${cal.clientUid}',…)"), so a UID containing ' could run code the moment the handlers started working. The controls now carry their UID or key in HTML-escaped data-* attributes and are bound by one delegated click/change listener inside the page IIFE; the handler functions stay closure-local and no inline handler or window.* export is needed for them. The second commit gives the dashboard and client-console controls the same treatment (encodeURIComponent() does not encode a single quote either, so the Edit Config links were converted as well) and HTML-escapes the client console's UID readout. No page renders a client UID into JavaScript source any more. The third commit makes the attribute values themselves safe: six pages implemented escapeHtml() by serialising a text node, which does not escape quotes, so they now share the five-character escaper (& < > " ') the dashboard already had.

Client UIDs are restricted at the source

isValidClientUid() (telemetry, alarm and daily ingestion) accepted anything after dev: up to 47 characters. It now also requires letters, digits, :, _, - or ., which covers Blues device UIDs (IMEI or hex), so every UID the server stores is safe to embed in attributes, JSON, log lines and file names. handleConfigPost applies the same check to operator-submitted UIDs before storing a config snapshot, and a request that carries a config without a usable client UID is answered with 400 instead of a silent 200; findOrCreateClientMetadata() refuses to create metadata for a malformed UID, which covers the OTA acknowledgement, location reply and /api/ota/expect paths (every caller already handles a null result), and the calibration POST/DELETE handlers, the client serial-log buffer (serial_log/serial_ack notes) and the unload handler reject a malformed UID before any lookup, write or notification (those snapshots are rendered on the dashboard as "Configured Client" cards), the bare dev: prefix is rejected, and records persisted before this validation existed (the sensor registry, calibrations and their summaries, hot-tier history and FTP cache summaries, client config snapshots, client metadata) are validated when they are loaded at boot, on the complete stored value before it is copied into the fixed-size field (so an overlong id cannot pass as its truncated prefix), and skipped (empty ids included) with a serial warning if their UID is not a valid device UID; calibration log entries with an invalid UID are omitted from /api/calibration, and a registry that dropped a record is marked dirty so it is re-saved clean. Rejected UIDs are logged through a sanitising printer (printable ASCII, \xNN otherwise, 64 chars max) so a stray control byte cannot forge a log line.

Config generator clamps

• Sample Minutes max 1440 → 1092, in both the input's max attribute and the collectConfig clamp (which now parses with Number() and truncates, so exponent input such as 1e3 is read as 1000, not 1). The client stores sampleSeconds in a uint16_t; 1440 min = 86 400 s does not fit, so is<uint16_t>() failed and the client quietly fell back to its default interval (CLAUDE review M-08).
• Momentary relay durations max 86400 → 65535, and collectConfig now clamps each of the four values to 0…65535 (parsed with Number() and truncated). The client parses relayMomentaryDurations[] with as<uint16_t>(), which yields 0 for anything above 65535, so the firmware-side > 86400 guard could never engage (CLAUDE review M-06).

check_web_pages.py + CI job

TankAlarm-112025-Server-BluesOpta/check_web_pages.py extracts every PROGMEM page from the server and viewer sketches (joining the )HTML" R"HTML( segments), runs node --check on each <script> block, and asserts that every function called from an inline on<event> attribute resolves to a depth-0 function fn( or a window.fn = assignment that actually executes on load:

• every call in an attribute value is checked (onclick="a(); b()", onclick="if (x) b()"), not only the first, after string literals, comments and regex literals in the value are blanked (alert('missing()') calls only alert); method calls (a.b()), callable browser globals (alert, open, … but not location) and ${…} template splices (which run at render time; strings, regex literals and nested templates inside a splice are lexed so a { in a string or a regex cannot swallow the handler) are skipped;
• any on<event> attribute is matched case-insensitively (onClick), quoted or unquoted (onclick=missing()), with optional whitespace around =, including attributes inside JS template strings;
• a window.fn = counts only when it starts a statement (not if (x) window.fn = fn;, else window.fn = fn;, x && (window.fn = fn), y ? window.fn = fn : 0), every enclosing function body is an IIFE that is a statement of its own (not false && (() => {…})()) or a load/DOMContentLoaded listener, and no conditional block (if/else/for/while/switch/catch) encloses it; an assignment inside an uncalled function, an ordinary callback, an expression-bodied arrow (.then(() => window.fn = fn)) or a branch that may not run does not;
• an assignment that follows an unconditional return/throw in the same block is unreachable and does not count;
• a depth-0 function name( counts only when it starts a statement; a named function expression (const holder = function dead(){}) does not expose dead;
• the assigned value must be callable: a function expression, an arrow, or the name of a function declared in the assignment's own scope or an enclosing one; window.dead = undefined, a plain variable, or a function declared inside some other function does not count;
• the tokeniser skips strings, template literals (nested ${}), comments and regex literals, including regex literals after keywords such as return;
• it fails closed: a missing node (for the scan and for --selftest), a missing sketch, or a sketch yielding no pages is a failure;
• --selftest runs 60 synthetic pages covering each of these rules.

A new check-web-pages job in arduino-ci-112025.yml (with actions/setup-node) runs the self-test and then the scan on every push and pull request; build-firmware depends on it, so firmware binaries are not built and committed while a page check fails. The workflow's pull_request trigger no longer filters on the base branch, so stacked PRs like this one get the same checks.

Before this PR the scan reported exactly the six handlers above; after it, 17 pages checked, 0 problem(s) (the stylesheet page has nothing to check).

Verification

• Browser (loopback fixture, pages extracted from this branch): every converted control runs its handler through the delegated listener with the right payload (calibration Reset → DELETE with the row's UID and index; sensor link → its toast; Expect Update → POST /api/ota/expect; Remove Client → DELETE /api/client; Cancel → POST /api/config/cancel; custom-range dates → chart re-render; dashboard Update / Snooze / Clear Relay / Remove Client → their four endpoints; client console Approve Deletion → DELETE /api/client) with zero error events. A fixture row whose UID contains ');alert(1);// renders as inert text, the handler receives it verbatim and alert is never called.
• Clamps: Sample Minutes 5000 → sampleSeconds: 65520; relay durations 99999 / -5 / 3600 / abc → [65535, 0, 3600, 0] in the downloaded JSON.
• Compile at arduino:mbed_opta 4.5.0, --warnings all: 1,022,836 B (52%) flash on this stacked branch (+3040 B over the Website: unify page chrome and fix layout/style inconsistencies #314 build), 360,720 B (68%) static RAM (unchanged); warning set identical to the master build (line numbers aside). CI's compile-check and check-web-pages both pass on this PR.
• check_web_pages.py --selftest: 60/60; the scan: 17 pages, 0 problems (the stylesheet page has no script and no handlers).

Out of scope

The dashboard's Clear Relay sends the sensor's position in the /api/clients ts[] array, which the client applies as its monitor-array index; a client whose sensors report out of order can clear the wrong monitor. That is pre-existing (the inline handler sent the same value) and needs a server/client contract change, so it is tracked as S-T06 in the TODO rather than fixed here.
The client-side parse (as<uint16_t>() with a dead > 86400 guard) is a firmware change and stays with the client items in the TODO. S-W01 (config round-trip) and S-W03 (calibration identity) follow in #316.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues remain in the page checker and CI enforcement.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR fixes six dead inline handlers, clamps configuration values to firmware limits, and adds embedded-page validation to CI.

Changes:

• Exports six handlers to window.
• Clamps sample intervals and relay durations.
• Adds page extraction, syntax, and handler checks to CI.

File summaries

File	Summary	Final review comments
TankAlarm-112025-Server-BluesOpta/TankAlarm-112025-Server-BluesOpta.ino	Handler exports and configuration clamps	None
TankAlarm-112025-Server-BluesOpta/check_web_pages.py	Embedded-page validation	Critical (1 vote): regex detection mishandles literals after keywords. Moderate (3 votes): event allow-list omits valid inline events. Moderate (2 votes): missing sketches do not fail CI. Moderate (1 vote): missing Node fails open. Moderate (1 vote): zero extracted pages can pass silently.
.github/workflows/arduino-ci-112025.yml	CI integration	Moderate (1 vote): build-firmware does not depend on the page-check job.

Review details

Suppressed comments (3)

.github/workflows/arduino-ci-112025.yml:231

• This job is not included in build-firmware's needs list, so on a push a failing page check can run in parallel while the firmware build still succeeds and commits artifacts. If this check is meant to prevent the regression from reaching the main branch/release path, make the artifact-producing job depend on check-web-pages (or otherwise enforce it as a required status).

  check-web-pages:
    runs-on: ubuntu-latest
    permissions:
      contents: read
    steps:
      - name: Checkout repository
        uses: actions/checkout@v5

      - name: Check embedded web pages (script syntax, inline handler reachability)
        run: python3 TankAlarm-112025-Server-BluesOpta/check_web_pages.py

TankAlarm-112025-Server-BluesOpta/check_web_pages.py:178

• When Node is unavailable, this checker only prints a warning and still returns success, while the workflow does not install or pin Node. That makes the syntax-check portion fail open and can report a green CI result with zero JavaScript validation; missing required tooling should fail the job or be provisioned explicitly.

    node = shutil.which('node')
    if not node:
        print('WARNING: node not found on PATH; skipping syntax checks', file=sys.stderr)

TankAlarm-112025-Server-BluesOpta/check_web_pages.py:187

• If PAGE_RE stops matching because the sketch declaration format changes, extract_pages(sk) can return no pages and the loop simply does nothing; the checker then reports success with 0 pages checked. Assert that each required sketch yielded at least one page so parser drift cannot disable the check silently.

        for name, page in extract_pages(sk).items():
            if '<script' not in page and 'on' not in page:
                continue

• Files reviewed: 2/3 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new handler checker has false negatives that can allow unreachable inline handlers to pass CI.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

TankAlarm-112025-Server-BluesOpta/check_web_pages.py:34

• This pattern does not cover every inline on* handler as documented: it requires = with no surrounding whitespace and hard-codes a subset of event names. For example, both onclick = "missing()" and onpointerdown="missing()" currently produce no failure, allowing the same unreachable-handler regression to bypass CI. Match arbitrary on... attributes and optional whitespace around = instead.

HANDLER_RE = re.compile(
    r'\bon(?:click|change|input|submit|keyup|keydown|keypress|load|blur|focus|mouseenter|mouseleave|mouseover|mouseout)'
    r'=\\?["\']\s*(?:window\.)?([A-Za-z_$][\w$]*)\s*\(')

• Files reviewed: 2/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Newly activated handlers permit stored script injection, and the checker has reachability gaps.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Attribute escaping remains unsafe for persisted UIDs, and the checker silently accepts some unreachable handlers.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Legacy UID paths still permit stored script injection, and the new checker has a control-flow false negative.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Number parsing misreads valid exponent inputs, and the checker has false-negative paths for unreachable handlers.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

TankAlarm-112025-Server-BluesOpta/check_web_pages.py:451

• _rhs_kind treats a call expression as a bare identifier because this match stops after the callee name. Thus (async()=>{ function make(){}; window.bad=make(); })() is reported as exporting bad, even though make() may return undefined; an inline bad() then fails at runtime while this checker passes. Reject identifiers followed by a call/member-expression token (and add this case to selftest) unless the resulting value is provably callable.

    m = re.match(r'\s*(async\s+)?([A-Za-z_$][\w$]*)\s*(=>)?', js[i:i + 80])
    if m and m.group(2) not in ('undefined', 'null', 'true', 'false', 'new', 'void', 'typeof'):
        return '' if m.group(3) else m.group(2)

• Files reviewed: 2/3 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The configuration API can falsely accept missing UIDs, and the new checker has paths that silently accept unreachable handlers.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

UID validation remains bypassable, missing-client requests can silently succeed, and the checker has a confirmed false-negative case.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Missing UID handling remains bypassable, calibration writes still accept invalid UIDs, and the checker allows a non-callable browser global.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Missing-UID requests still return 200, some UID ingestion paths remain unvalidated, and the checker has false-positive and false-negative cases.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 2/3 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Missing config UIDs can still return 200, and the page checker accepts exports unreachable after an enclosing return.

Review details

Suppressed comments (1)

TankAlarm-112025-Server-BluesOpta/check_web_pages.py:477

• Unreachability is checked only on the innermost frame. For example, (async()=>{ function dead(){} return; { window.dead=dead; } })() is accepted even though dead is never exported, allowing the dead-handler regression this checker is meant to block. Propagate unreachability from every enclosing frame (and add this case to the self-test).

                    unreachable = stack[-1][4] if stack else top_unreachable

• Files reviewed: 2/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced