fix(security): close XSS in control-pane board controls

The interactive claim/move buttons concatenated work-item ids into inline
onclick JS with only single-quote escaping — a crafted id (ids/titles come from
GitHub sync and manual upserts, not a strict allowlist) could break out and
inject script, even on the localhost-only server.

Fix: emit the id/lane in HTML-escaped data-* attributes (escapeHtml encodes
&<>"'), attach delegated click listeners that read them via getAttribute, and
pass the raw value as a JS string arg — never concatenated into code. Adds a
regression assertion that no inline onclick handlers with interpolated ids
remain. Flagged by automated security review.

Full suite 2845/2845; lint green.
This commit is contained in:
Affaan Mustafa 2026-06-18 18:25:28 -04:00
parent 607ab02b1f
commit a03d63cba0
2 changed files with 23 additions and 6 deletions

View File

@ -519,14 +519,16 @@ function renderControlPaneHtml() {
const blocker = item.blocker || (item.metadata && item.metadata.blocker) || ''; const blocker = item.blocker || (item.metadata && item.metadata.blocker) || '';
const assigneeKind = item.assigneeKind || 'unassigned'; const assigneeKind = item.assigneeKind || 'unassigned';
const owner = item.assignee || item.owner || (assigneeKind === 'unassigned' ? 'unassigned (JIT)' : item.source) || 'unassigned'; const owner = item.assignee || item.owner || (assigneeKind === 'unassigned' ? 'unassigned (JIT)' : item.source) || 'unassigned';
const idJs = "'" + String(item.id).replace(/'/g, "\\'") + "'"; // Ids/lanes go into HTML-escaped data-* attributes and are read back via
const moveButtons = ['ready', 'running', 'blocked', 'done'].map(lane => { // dataset in delegated listeners below — never concatenated into inline
const call = 'eccMoveItem(' + idJs + ", '" + lane + "')"; // JS handlers — so a crafted work-item id cannot inject script (XSS).
return '<button type="button" onclick="' + call + '">' + escapeHtml(lane) + '</button>'; const idAttr = escapeHtml(item.id);
}).join(''); const moveButtons = ['ready', 'running', 'blocked', 'done'].map(lane =>
'<button type="button" data-wi-action="move" data-wi-id="' + idAttr + '" data-wi-lane="' + lane + '">' + escapeHtml(lane) + '</button>'
).join('');
const controls = state.allowActions const controls = state.allowActions
? '<div class="row">' ? '<div class="row">'
+ (assigneeKind === 'unassigned' ? '<button type="button" onclick="eccClaimItem(' + idJs + ')">Claim</button>' : '') + (assigneeKind === 'unassigned' ? '<button type="button" data-wi-action="claim" data-wi-id="' + idAttr + '">Claim</button>' : '')
+ moveButtons + moveButtons
+ '</div>' + '</div>'
: ''; : '';
@ -539,6 +541,17 @@ function renderControlPaneHtml() {
controls + controls +
'</div>'; '</div>';
}).join(''); }).join('');
document.querySelectorAll('#work-items [data-wi-action]').forEach(button => {
button.addEventListener('click', () => {
const id = button.getAttribute('data-wi-id');
if (button.getAttribute('data-wi-action') === 'claim') {
eccClaimItem(id);
} else {
eccMoveItem(id, button.getAttribute('data-wi-lane'));
}
});
});
} }
function renderKnowledge(knowledge) { function renderKnowledge(knowledge) {

View File

@ -205,6 +205,10 @@ async function runTests() {
assert.ok(html.includes('function renderWorkItems')); assert.ok(html.includes('function renderWorkItems'));
assert.ok(html.includes('function showError')); assert.ok(html.includes('function showError'));
assert.ok(html.includes('response.ok')); assert.ok(html.includes('response.ok'));
// Board controls must use escaped data-* attributes + delegated
// listeners, never ids concatenated into inline onclick JS (XSS).
assert.ok(html.includes('data-wi-action'));
assert.ok(!/onclick="ecc(Claim|Move)Item\(/.test(html), 'no inline onclick handlers with interpolated ids');
const snapshot = await fetchLocal(`${app.url}/api/snapshot?query=control`).then(response => response.json()); const snapshot = await fetchLocal(`${app.url}/api/snapshot?query=control`).then(response => response.json());
assert.strictEqual(snapshot.schemaVersion, 'ecc.control-pane.snapshot.v1'); assert.strictEqual(snapshot.schemaVersion, 'ecc.control-pane.snapshot.v1');