fix(ui): dashboard error modal dead — inline onclick broke on apostrophes #17

Merged

james merged 1 commit from fix/inline-onclick-injection into main

2026-07-30 15:25:24 +00:00

james commented

2026-07-30 15:24:05 +00:00

Owner

The reported bug

Clicking a failed backup pill on the dashboard did nothing. Console:

Uncaught SyntaxError: Invalid or unexpected token (at (index):1:33)

Root cause

The pill built its click handler as inline JavaScript source inside a single-quoted HTML attribute:

attrs = `onclick='showBackupError(${JSON.stringify(cname)}, ${JSON.stringify(last.error)})'`;

JSON.stringify escapes " and \ — but not '. An apostrophe in the error closes the attribute early, so the browser compiles a truncated fragment:

showBackupError("app-db", "pg_dump: error: couldn      ← unterminated string

The handler never binds, so the click is a no-op. Latent until 02d25c5 started folding raw command stderr into failure messages — pg_dump output is full of couldn't / doesn't.

The same hole was an XSS vector: <script> in an error message rendered as live markup.

The fix

Data never becomes code.

escapeAttr() — the existing escapeHtml() goes through innerHTML, which leaves quotes and apostrophes untouched, so it was never safe for attribute values. That misuse was the shared root of every site below.
delegateClicks(handlers) — binds by data-action. All 26 call sites across dashboard / nodes / stack_backups / users / backups now carry arguments in escaped data-* attributes.

Delegation dispatches only the innermost [data-action], which removes the three event.stopPropagation() calls the node cards needed for nested buttons.

Two bugs found while sweeping (worse than the reported one)

backups.js download/restore/delete interpolated filenames with no escaping at all — not escapeHtml, nothing.
Volume-restore checkbox value="${escapeHtml(item.path)}" — these values decide which paths get restored. A path containing " silently truncated the value, so you'd restore a different set than the one checked. That's data corruption in a restore path, not a dead button.

Also fixed unescaped error text in both volume-contents failure paths, and three attribute-context escapeHtml → escapeAttr in dashboard.js.

Verification

Reproduced first — a standalone script produced the exact production SyntaxError before any fix.
Static cross-check — parsed every data-action site and its emitted attributes, parsed each delegateClicks block for the dataset properties its handlers read, and compared. This catches the failure mode a sweep this size actually hits (data-node-name vs d.nodeName). All 26 sites consistent; no orphan handlers, no unhandled actions.
Two browser harnesses driving the real render functions with hostile values (node-o'brien "prod" <b>, jim's_db "2026".sql.gz, stderr with quotes): 25 checks, all passing. Values round-trip byte-exact, no markup injection, button-inside-card fires only the button, card body still opens detail, disabled controls inert.

No Ruby changed, so RSpec is unaffected — I did not run it, and it proves nothing about this change either way.

Note for deploy

/js/ is served via Sinatra enable :static with no cache-busting. A browser holding a cached common.js would pair old JS with new markup and the buttons would be dead again. Worth a hard refresh when verifying in prod; cache-busted asset URLs would be a reasonable follow-up.

Left alone deliberately

Six onclick handlers remain — constant source (this.parentElement.remove(), closeModal()) or integer arithmetic (pagination). No data flows into any of them, so converting them would be churn without risk reduction.

## The reported bug Clicking a failed backup pill on the dashboard did nothing. Console: ``` Uncaught SyntaxError: Invalid or unexpected token (at (index):1:33) ``` ## Root cause The pill built its click handler as inline JavaScript **source** inside a *single*-quoted HTML attribute: ```js attrs = `onclick='showBackupError(${JSON.stringify(cname)}, ${JSON.stringify(last.error)})'`; ``` `JSON.stringify` escapes `"` and `\` — but **not `'`**. An apostrophe in the error closes the attribute early, so the browser compiles a truncated fragment: ``` showBackupError("app-db", "pg_dump: error: couldn ← unterminated string ``` The handler never binds, so the click is a no-op. Latent until 02d25c5 started folding raw command stderr into failure messages — `pg_dump` output is full of `couldn't` / `doesn't`. The same hole was an **XSS vector**: `<script>` in an error message rendered as live markup. ## The fix Data never becomes code. - **`escapeAttr()`** — the existing `escapeHtml()` goes through `innerHTML`, which leaves quotes and apostrophes untouched, so it was **never** safe for attribute values. That misuse was the shared root of every site below. - **`delegateClicks(handlers)`** — binds by `data-action`. All **26 call sites** across `dashboard` / `nodes` / `stack_backups` / `users` / `backups` now carry arguments in escaped `data-*` attributes. Delegation dispatches only the innermost `[data-action]`, which removes the three `event.stopPropagation()` calls the node cards needed for nested buttons. ## Two bugs found while sweeping (worse than the reported one) 1. **`backups.js` download/restore/delete interpolated filenames with no escaping at all** — not `escapeHtml`, nothing. 2. **Volume-restore checkbox `value="${escapeHtml(item.path)}"`** — these values decide *which paths get restored*. A path containing `"` silently truncated the value, so you'd restore a different set than the one checked. That's data corruption in a restore path, not a dead button. Also fixed unescaped error text in both volume-contents failure paths, and three attribute-context `escapeHtml` → `escapeAttr` in `dashboard.js`. ## Verification - **Reproduced first** — a standalone script produced the exact production `SyntaxError` before any fix. - **Static cross-check** — parsed every `data-action` site and its emitted attributes, parsed each `delegateClicks` block for the dataset properties its handlers read, and compared. This catches the failure mode a sweep this size actually hits (`data-node-name` vs `d.nodeName`). All 26 sites consistent; no orphan handlers, no unhandled actions. - **Two browser harnesses** driving the *real* render functions with hostile values (`node-o'brien "prod" <b>`, `jim's_db "2026".sql.gz`, stderr with quotes): **25 checks, all passing**. Values round-trip byte-exact, no markup injection, button-inside-card fires only the button, card body still opens detail, disabled controls inert. No Ruby changed, so RSpec is unaffected — I did not run it, and it proves nothing about this change either way. ## Note for deploy `/js/` is served via Sinatra `enable :static` with no cache-busting. A browser holding a cached `common.js` would pair old JS with new markup and the buttons would be dead again. Worth a hard refresh when verifying in prod; cache-busted asset URLs would be a reasonable follow-up. ## Left alone deliberately Six `onclick` handlers remain — constant source (`this.parentElement.remove()`, `closeModal()`) or integer arithmetic (pagination). No data flows into any of them, so converting them would be churn without risk reduction.

james added 1 commit

2026-07-30 15:24:05 +00:00

fix(ui): dashboard error modal dead — inline onclick broke on apostrophes

Build, Push & Deploy / build-and-push (pull_request) Has been skipped

Details

Build, Push & Deploy / deploy (Media) (pull_request) Has been skipped

Details

Build, Push & Deploy / deploy (citadel) (pull_request) Has been skipped

Details

Build, Push & Deploy / test (pull_request) Successful in 1m4s

Details

c005ad3e92

Clicking a failed backup pill did nothing and logged
`Uncaught SyntaxError: Invalid or unexpected token`. The pill built its
handler as inline JS *source* inside a single-quoted attribute via
JSON.stringify, which escapes `"` and `\` but not `'`. An apostrophe in the
recorded error closed the attribute early, so the browser compiled a
truncated expression and never bound the handler. Latent until 02d25c5
started folding command stderr into failure messages. The same hole let a
<script> in an error render as live markup.

Sweep the class rather than patch one site:

- escapeAttr() — escapeHtml() goes through innerHTML, which leaves quotes
  untouched, so it was never safe for attribute values.
- delegateClicks() — binds by data-action. All 26 call sites across
  dashboard/nodes/stack_backups/users/backups now carry arguments in
  escaped data-* attributes instead of handler source.

Also fixed: backups.js download/restore/delete interpolated filenames with
no escaping at all; volume-restore checkbox value= used escapeHtml, so a
path containing a double quote truncated the value and restored the wrong
set of paths; unescaped error text in both volume-contents failure paths.

Delegation dispatches only the innermost [data-action], removing the
event.stopPropagation() the node cards needed for nested buttons.

Co-Authored-By: Claude <noreply@anthropic.com>

james merged commit 305471ede3 into main

2026-07-30 15:25:24 +00:00

james referenced this pull request from a commit

2026-07-30 15:25:26 +00:00

Merge pull request 'fix(ui): dashboard error modal dead — inline onclick broke on apostrophes' (#17) from fix/inline-onclick-injection into main