From fbab688f51d0ea977fcddb095d3418f9f038573c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 10 Jun 2026 17:17:08 -0700 Subject: [PATCH] fix(frontend): use shared quote-safe escapeHtml in apikeys and users escapeHtml was implemented three times with diverging security semantics: the shared frontend/src/utils.ts copy escapes &, <, >, " and ', while the local copies in apikeys.ts and users/utils.ts used a DOM round-trip (div.textContent -> div.innerHTML) that leaves both quote characters unescaped. Those weak copies were interpolated inside double-quoted attributes (data-key-id in apikeys.ts, aria-label and option value across users/userList.ts, filters.ts and permissionMatrix.ts), so a value containing a double quote could break out of the attribute and inject markup. Delete both local copies: apikeys.ts now imports escapeHtml from ./utils and users/utils.ts re-exports it from ../utils (same pattern already used there for formatRelativeTime/formatDate), so all users/ modules pick up the quote-safe implementation unchanged. Update the users/utils tests that previously asserted the unescaped quote behavior, and add regression tests replicating the real scenarios: a key id containing a double quote rendered through renderApiKeysList, and a quoted value in attribute position through the users escapeHtml. Both fail against the pre-fix code. Closes #1195 --- frontend/src/__tests__/apikeys.test.ts | 27 +++++++++++++++++++++++ frontend/src/__tests__/users.test.ts | 30 ++++++++++++++++++++------ frontend/src/apikeys.ts | 11 +--------- frontend/src/users/utils.ts | 13 +++++------ 4 files changed, 59 insertions(+), 22 deletions(-) diff --git a/frontend/src/__tests__/apikeys.test.ts b/frontend/src/__tests__/apikeys.test.ts index 978bdd30c..3cc12f54b 100644 --- a/frontend/src/__tests__/apikeys.test.ts +++ b/frontend/src/__tests__/apikeys.test.ts @@ -289,6 +289,33 @@ describe('API Keys Module', () => { expect(api.revokeApiKey).toHaveBeenCalledWith('key-1'); }); + // Regression for ARCH-07 (issue #1195): the previous local escapeHtml + // copy (div.textContent -> div.innerHTML round-trip) did not escape + // quote characters, so a key id containing a double quote interpolated + // into data-key-id="..." broke out of the attribute and injected a new + // attribute (here onmouseover) into the button markup. + test('issue #1195: double quote in key id cannot inject attributes', async () => { + const hostileId = 'key-1" onmouseover="alert(1)'; + const mockKeys = [ + { + id: hostileId, + name: 'Hostile Key', + key_prefix: 'abc123', + is_active: true, + created_at: '2024-01-15T10:00:00Z' + } + ]; + + (api.getApiKeys as jest.Mock).mockResolvedValue({ api_keys: mockKeys }); + + await loadApiKeys(); + + const deleteBtn = document.querySelector('.delete-key-btn'); + expect(deleteBtn).not.toBeNull(); + expect(deleteBtn?.getAttribute('onmouseover')).toBeNull(); + expect(deleteBtn?.getAttribute('data-key-id')).toBe(hostileId); + }); + test('adds event listener to delete buttons', async () => { const mockKeys = [ { diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index f623080fd..8fa65e3f5 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -122,7 +122,7 @@ describe('users/utils', () => { describe('escapeHtml', () => { it('should escape HTML special characters', () => { expect(userUtils.escapeHtml('')).toBe( - '<script>alert("xss")</script>' + '<script>alert("xss")</script>' ); }); @@ -134,14 +134,28 @@ describe('users/utils', () => { expect(userUtils.escapeHtml('Tom & Jerry')).toBe('Tom & Jerry'); }); + // Regression for ARCH-07 (issue #1195): the previous local DOM + // round-trip implementation left both quote characters unescaped, + // so values interpolated inside double-quoted attributes (e.g. the + // aria-label carrying the user email in userList.ts) could break + // out of the attribute and inject markup. it('should escape single quotes', () => { const result = userUtils.escapeHtml("It's a test"); - expect(result).toBe("It's a test"); + expect(result).toBe('It's a test'); }); it('should escape double quotes', () => { const result = userUtils.escapeHtml('Say "Hello"'); - expect(result).toBe('Say "Hello"'); + expect(result).toBe('Say "Hello"'); + }); + + it('issue #1195: quote in attribute position cannot break out of the attribute', () => { + const malicious = 'a@b.com" onmouseover="alert(1)'; + const div = document.createElement('div'); + div.innerHTML = ``; + const btn = div.querySelector('button'); + expect(btn?.getAttribute('onmouseover')).toBeNull(); + expect(btn?.getAttribute('aria-label')).toBe(malicious); }); it('should handle empty string', () => { @@ -150,7 +164,7 @@ describe('users/utils', () => { it('should handle multiple special characters', () => { expect(userUtils.escapeHtml('
&
')).toBe( - '<div class="test">&</div>' + '<div class="test">&</div>' ); }); }); @@ -668,11 +682,15 @@ describe('users/filters', () => { userFilters.updateGroupFilterDropdown(); const select = document.getElementById('user-group-filter') as HTMLSelectElement; + // No element may be injected out of the value attribute. + expect(select.querySelector('img')).toBeNull(); // The raw payload must not appear verbatim inside the rendered HTML. expect(select.innerHTML).not.toContain('">'); }); }); }); diff --git a/frontend/src/apikeys.ts b/frontend/src/apikeys.ts index fb4633be8..ee358e9b7 100644 --- a/frontend/src/apikeys.ts +++ b/frontend/src/apikeys.ts @@ -4,7 +4,7 @@ import * as api from './api'; import type { APIKeyInfo, CreateAPIKeyResponse } from './types'; -import { formatDateTime, formatRelativeTime } from './utils'; +import { escapeHtml, formatDateTime, formatRelativeTime } from './utils'; import { confirmDialog } from './confirmDialog'; import { showToast } from './toast'; import { openModal, closeModal } from './modal'; @@ -389,12 +389,3 @@ function showError(message: string): void { showToast({ message, kind: 'error' }); } } - -/** - * Escape HTML to prevent XSS - */ -function escapeHtml(text: string): string { - const div = document.createElement('div'); - div.textContent = text; - return div.innerHTML; -} diff --git a/frontend/src/users/utils.ts b/frontend/src/users/utils.ts index afee4627d..0e78640b8 100644 --- a/frontend/src/users/utils.ts +++ b/frontend/src/users/utils.ts @@ -10,13 +10,14 @@ export { formatRelativeTime, formatDate } from '../utils'; /** - * Escape HTML to prevent XSS + * Escape HTML to prevent XSS. Re-exported from the shared utils module: + * the previous local DOM round-trip (div.textContent -> div.innerHTML) + * did not escape quote characters, so values interpolated inside + * double-quoted attributes could break out of the attribute. The shared + * implementation escapes &, <, >, " and ' and is safe in both text and + * attribute contexts. */ -export function escapeHtml(text: string): string { - const div = document.createElement('div'); - div.textContent = text; - return div.innerHTML; -} +export { escapeHtml } from '../utils'; import { showToast } from '../toast';