Skip to content

Commit 0510185

Browse files
authored
Harden WebUI done-frame stats against markup injection (#285)
Replace the innerHTML assignment for per-message turn stats with DOM construction (createElement/textContent). Extract turnStatsSpans from turnStatsHTML so the footer data is plain, numeric-coerced spans, and escape the HTML renderer's title/text as defense in depth. Pin the invariant with node tests: hostile done-frame values must render inert.
1 parent bc9c566 commit 0510185

3 files changed

Lines changed: 78 additions & 17 deletions

File tree

‎cmd/odek/ui/js/metrics.js‎

Lines changed: 24 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import { S } from './state.js';
1414
import { getLimits, getModels } from './api.js';
1515
import { formatNum } from './utils.js';
16+
import { escapeHtml, escapeAttr } from './escape.js';
1617

1718
// Metrics state (kept on S so the health popover can read it too).
1819
S.metrics = {
@@ -243,27 +244,39 @@ export function resetMetrics() {
243244
renderMetrics();
244245
}
245246

246-
// turnStatsHTML is the per-message footer on done. Speed uses this-call
247-
// fields only — never cumulative outputTokens / run latency.
248-
export function turnStatsHTML(event) {
249-
if (!event || event.latency == null) return '';
247+
// turnStatsSpans is the per-message footer data on done: one entry per
248+
// stat chip as {title, text}. Speed uses this-call fields only — never
249+
// cumulative outputTokens / run latency. Returns plain data (no markup) so
250+
// callers can render via textContent; every field is numeric-coerced before
251+
// it leaves this function, so a hostile done frame can never smuggle markup.
252+
export function turnStatsSpans(event) {
253+
if (!event || event.latency == null) return [];
250254
const lat = Number(event.latency);
251255
const latSafe = Number.isFinite(lat) ? lat : 0;
252256
const spans = [];
253-
spans.push('<span title="Response time">⚡ ' + (latSafe < 1 ? (latSafe * 1000).toFixed(0) + 'ms' : latSafe.toFixed(1) + 's') + '</span>');
254-
if (event.inputTokens != null) spans.push('<span title="Input tokens (run total, incl. sub-agents)">⌂ ' + formatNum(event.inputTokens) + '</span>');
255-
if (event.outputTokens != null) spans.push('<span title="Output tokens (completion)">↳ ' + formatNum(event.outputTokens) + '</span>');
257+
spans.push({ title: 'Response time', text: '⚡ ' + (latSafe < 1 ? (latSafe * 1000).toFixed(0) + 'ms' : latSafe.toFixed(1) + 's') });
258+
if (event.inputTokens != null) spans.push({ title: 'Input tokens (run total, incl. sub-agents)', text: '⌂ ' + formatNum(event.inputTokens) });
259+
if (event.outputTokens != null) spans.push({ title: 'Output tokens (completion)', text: '↳ ' + formatNum(event.outputTokens) });
256260
const cache = (event.cacheReadTokens || 0) + (event.cacheCreationTokens || 0) + (event.cachedTokens || 0);
257-
if (cache > 0) spans.push('<span title="Cached tokens">⛁ ' + formatNum(cache) + '</span>');
261+
if (cache > 0) spans.push({ title: 'Cached tokens', text: '⛁ ' + formatNum(cache) });
258262
const picked = pickTokPerSec(event);
259263
if (picked.rate > 0) {
260-
spans.push('<span title="' + tokPerSecTitle(picked.kind) + '">↗ ' + formatTokPerSec(picked.rate) + '</span>');
264+
spans.push({ title: tokPerSecTitle(picked.kind), text: '↗ ' + formatTokPerSec(picked.rate) });
261265
}
262266
const turnCost = turnCostUSD(event.inputTokens || 0, event.outputTokens || 0);
263267
if (turnCost != null && turnCost > 0) {
264-
spans.push('<span title="Estimated cost of this turn at current prices">$ ' + turnCost.toFixed(4) + '</span>');
268+
spans.push({ title: 'Estimated cost of this turn at current prices', text: '$ ' + turnCost.toFixed(4) });
265269
}
266-
return spans.join(' · ');
270+
return spans;
271+
}
272+
273+
// turnStatsHTML renders the same spans as an HTML string. Kept for callers
274+
// that still need markup; both the title attribute and the body are escaped
275+
// so even a future non-numeric span value cannot open an injection channel.
276+
export function turnStatsHTML(event) {
277+
return turnStatsSpans(event).map(sp =>
278+
'<span title="' + escapeAttr(sp.title) + '">' + escapeHtml(sp.text) + '</span>'
279+
).join(' · ');
267280
}
268281

269282
// sessionCostUSD estimates the current session's spend from its totals.

‎cmd/odek/ui/js/metrics.test.js‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ globalThis.localStorage = (() => {
5353
})();
5454

5555
const { S } = await import('./state.js');
56-
const { formatUSD, sessionCostUSD, renderMetrics, metricsDone, resetMetrics, pickTokPerSec, formatTokPerSec, metricsApplySpeed, metricsResetSpeed, turnStatsHTML, metricsBeginTurn, metricsLiveUsage, metricsLiveContext, metricsNoteOutput } = await import('./metrics.js');
56+
const { formatUSD, sessionCostUSD, renderMetrics, metricsDone, resetMetrics, pickTokPerSec, formatTokPerSec, metricsApplySpeed, metricsResetSpeed, turnStatsHTML, turnStatsSpans, metricsBeginTurn, metricsLiveUsage, metricsLiveContext, metricsNoteOutput } = await import('./metrics.js');
5757

5858
beforeEach(() => {
5959
S.metrics.pricesConfigured = false;
@@ -181,6 +181,45 @@ test('turnStatsHTML includes tok/s from this-call fields, never invented from to
181181
assert.equal(none.includes('tok/s'), false);
182182
});
183183

184+
// Pins the injection resistance of the done-frame stats footer: every span
185+
// value must be numeric-coerced (turnStatsSpans) or escaped (turnStatsHTML)
186+
// so a hostile done frame can never smuggle markup into the transcript.
187+
const HOSTILE = '<img src=x onerror="alert(1)">';
188+
189+
test('turnStatsSpans coerces hostile done-frame values to inert numbers', () => {
190+
const spans = turnStatsSpans({
191+
latency: HOSTILE, inputTokens: HOSTILE, outputTokens: HOSTILE,
192+
cacheReadTokens: HOSTILE, generationTokensPerSecond: HOSTILE,
193+
});
194+
assert.ok(spans.length > 0);
195+
for (const sp of spans) {
196+
assert.equal(sp.text.includes('<'), false, `markup leaked in text: ${sp.text}`);
197+
assert.equal(sp.title.includes('<'), false, `markup leaked in title: ${sp.title}`);
198+
}
199+
// Coerced latency is finite → shows as 0ms, never the raw string.
200+
assert.match(spans[0].text, /^⚡ 0ms$/);
201+
});
202+
203+
test('turnStatsHTML escapes span titles and text against attribute/body injection', () => {
204+
const html = turnStatsHTML({ latency: 1.2, inputTokens: 5 });
205+
assert.doesNotMatch(html, /onerror|<img/i);
206+
// No raw quote ever reaches the title attribute position.
207+
const hostile = turnStatsSpans({ latency: HOSTILE });
208+
const htmlHostile = turnStatsHTML({ latency: HOSTILE });
209+
assert.equal(htmlHostile.includes(HOSTILE), false);
210+
assert.equal(hostile.length > 0, true);
211+
});
212+
213+
test('turnStatsHTML renders identical content to turnStatsSpans for normal events', () => {
214+
const ev = { latency: 2.4, inputTokens: 1200, outputTokens: 300, generationTokensPerSecond: 30 };
215+
const spans = turnStatsSpans(ev);
216+
const html = turnStatsHTML(ev);
217+
for (const sp of spans) {
218+
assert.ok(html.includes(sp.text), `missing span text: ${sp.text}`);
219+
}
220+
assert.equal(html.split('<span').length - 1, spans.length);
221+
});
222+
184223
test('usage overlays run-cumulative tokens onto session totals mid-turn', () => {
185224
S.metrics.pricesConfigured = true;
186225
S.metrics.inPrice = 1;

‎cmd/odek/ui/js/ws.js‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import { queueApproval, dismissApproval, clearApprovals, expireApproval } from '
1616
import { queueClarify, dismissClarify, clearClarify, expireClarify } from './clarify.js';
1717
import { loadSessions } from './sessions.js';
1818
import { onPong, onServerInfo, startHeartbeat, stopHeartbeat, notifyUser } from './health.js';
19-
import { metricsLiveContext, metricsDone, metricsApplySpeed, metricsResetSpeed, metricsBeginTurn, metricsLiveUsage, turnStatsHTML, setMetricsModel } from './metrics.js';
19+
import { metricsLiveContext, metricsDone, metricsApplySpeed, metricsResetSpeed, metricsBeginTurn, metricsLiveUsage, turnStatsSpans, setMetricsModel } from './metrics.js';
2020
import { drainQueue } from './input.js';
2121
import { setIntent, openTurn, markWakeTurn, sealTurn, paintIntent } from './render.js';
2222
import { badgeNow } from './panels.js';
@@ -367,14 +367,23 @@ export function connect() {
367367
badgeNow();
368368
announce('Turn complete');
369369
drainQueue();
370-
// Append per-message stats to the last assistant bubble
371-
const statsHTML = turnStatsHTML(event);
372-
if (statsHTML) {
370+
// Append per-message stats to the last assistant bubble. Built via
371+
// textContent/setAttribute (never innerHTML) so server-controlled
372+
// values cannot be reinterpreted as markup even if a future field
373+
// loses its numeric coercion upstream.
374+
const statsSpans = turnStatsSpans(event);
375+
if (statsSpans.length) {
373376
const lastAssistant = lastAssistantBubble();
374377
if (lastAssistant) {
375378
const stats = document.createElement('div');
376379
stats.className = 'msg-stats';
377-
stats.innerHTML = statsHTML;
380+
statsSpans.forEach((sp, i) => {
381+
if (i > 0) stats.appendChild(document.createTextNode(' · '));
382+
const span = document.createElement('span');
383+
span.title = sp.title;
384+
span.textContent = sp.text;
385+
stats.appendChild(span);
386+
});
378387
lastAssistant.appendChild(stats);
379388
}
380389
}

0 commit comments

Comments
 (0)