fix(ext/popup): close 3 critical regressions from slice-2 code review
- C1: escapeHtml now escapes " and ' so values stored in data-field-value attributes (concealed rows, copyable rows) round-trip correctly. Prior impl silently truncated passwords containing quotes. +3 regression tests. - C2: centralize view-teardown. login.ts exports teardown() that stops the TOTP ticker and removes the active keydown handler; item-detail.ts and item-form.ts dispatchers call it before rendering the next view; each button handler also calls teardown() locally for belt-and-suspenders. - C3: restore alpha's keyboard shortcuts on login detail view: c (copy username), p (copy password), t (copy TOTP), f (autofill), e (edit), d (trash), plus Escape (back). All gated by the is-editable-target guard so they don't eat keystrokes inside form fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -115,3 +115,35 @@ describe('wireFieldHandlers', () => {
|
|||||||
expect(writeText).toHaveBeenCalledWith('alice@example.com');
|
expect(writeText).toHaveBeenCalledWith('alice@example.com');
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('concealed-row round-trip with special characters', () => {
|
||||||
|
it('reveals a value containing double quotes correctly', () => {
|
||||||
|
document.body.innerHTML = renderConcealedRow({ id: 'pw', label: 'p', value: 'a"b"c' });
|
||||||
|
wireFieldHandlers(document.body);
|
||||||
|
const btn = document.querySelector('[data-field-action="reveal"]') as HTMLButtonElement;
|
||||||
|
btn.click();
|
||||||
|
const valueEl = document.querySelector('[data-field-role="value"]') as HTMLElement;
|
||||||
|
expect(valueEl.textContent).toBe('a"b"c');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('reveals a value containing single quotes correctly', () => {
|
||||||
|
document.body.innerHTML = renderConcealedRow({ id: 'pw', label: 'p', value: "it's & ok" });
|
||||||
|
wireFieldHandlers(document.body);
|
||||||
|
const btn = document.querySelector('[data-field-action="reveal"]') as HTMLButtonElement;
|
||||||
|
btn.click();
|
||||||
|
const valueEl = document.querySelector('[data-field-role="value"]') as HTMLElement;
|
||||||
|
expect(valueEl.textContent).toBe("it's & ok");
|
||||||
|
});
|
||||||
|
|
||||||
|
it('copies a value containing double quotes correctly', async () => {
|
||||||
|
const writeText = vi.fn().mockResolvedValue(undefined);
|
||||||
|
Object.defineProperty(navigator, 'clipboard', {
|
||||||
|
configurable: true,
|
||||||
|
value: { writeText },
|
||||||
|
});
|
||||||
|
document.body.innerHTML = renderRow({ label: 'p', value: 'a"b"c', copyable: true });
|
||||||
|
wireFieldHandlers(document.body);
|
||||||
|
(document.querySelector('[data-field-action="copy"]') as HTMLButtonElement).click();
|
||||||
|
expect(writeText).toHaveBeenCalledWith('a"b"c');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -7,6 +7,11 @@ import { getState } from '../popup';
|
|||||||
import * as login from './types/login';
|
import * as login from './types/login';
|
||||||
|
|
||||||
export async function renderItemDetail(app: HTMLElement): Promise<void> {
|
export async function renderItemDetail(app: HTMLElement): Promise<void> {
|
||||||
|
// Tear down any tickers/handlers from a previous detail render before
|
||||||
|
// the next one boots up. Each type module owns its own teardown; we
|
||||||
|
// call all of them since the dispatcher doesn't know which was active.
|
||||||
|
login.teardown();
|
||||||
|
|
||||||
const item = getState().selectedItem;
|
const item = getState().selectedItem;
|
||||||
if (!item) { navigate('list'); return; }
|
if (!item) { navigate('list'); return; }
|
||||||
|
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import type { Item, ItemType } from '../../shared/types';
|
|||||||
import * as login from './types/login';
|
import * as login from './types/login';
|
||||||
|
|
||||||
export function renderItemForm(app: HTMLElement, mode: 'add' | 'edit'): void {
|
export function renderItemForm(app: HTMLElement, mode: 'add' | 'edit'): void {
|
||||||
|
login.teardown(); // detail-view's ticker/listener don't leak into form
|
||||||
const state = getState();
|
const state = getState();
|
||||||
const existing = mode === 'edit' ? state.selectedItem : null;
|
const existing = mode === 'edit' ? state.selectedItem : null;
|
||||||
const type: ItemType = existing?.type ?? state.newType ?? 'login';
|
const type: ItemType = existing?.type ?? state.newType ?? 'login';
|
||||||
|
|||||||
@@ -12,6 +12,16 @@ import {
|
|||||||
wireFieldHandlers,
|
wireFieldHandlers,
|
||||||
} from '../fields';
|
} from '../fields';
|
||||||
|
|
||||||
|
/// Called by the dispatcher before each render. Stops any in-flight
|
||||||
|
/// tickers / intervals / listeners the previous view may have attached.
|
||||||
|
export function teardown(): void {
|
||||||
|
stopTotpTicker();
|
||||||
|
if (activeKeyHandler) {
|
||||||
|
document.removeEventListener('keydown', activeKeyHandler);
|
||||||
|
activeKeyHandler = null;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// ----------------------------------------------------------------------
|
// ----------------------------------------------------------------------
|
||||||
// Detail view
|
// Detail view
|
||||||
// ----------------------------------------------------------------------
|
// ----------------------------------------------------------------------
|
||||||
@@ -58,10 +68,17 @@ export async function renderDetail(app: HTMLElement, item: Item): Promise<void>
|
|||||||
|
|
||||||
wireFieldHandlers(app);
|
wireFieldHandlers(app);
|
||||||
|
|
||||||
document.getElementById('back-btn')?.addEventListener('click', () => navigate('list'));
|
document.getElementById('back-btn')?.addEventListener('click', () => {
|
||||||
document.getElementById('edit-btn')?.addEventListener('click', () => navigate('edit'));
|
teardown();
|
||||||
|
navigate('list');
|
||||||
|
});
|
||||||
|
document.getElementById('edit-btn')?.addEventListener('click', () => {
|
||||||
|
teardown();
|
||||||
|
navigate('edit');
|
||||||
|
});
|
||||||
document.getElementById('trash-btn')?.addEventListener('click', async () => {
|
document.getElementById('trash-btn')?.addEventListener('click', async () => {
|
||||||
if (!confirm(`Move "${item.title}" to trash?`)) return;
|
if (!confirm(`Move "${item.title}" to trash?`)) return;
|
||||||
|
teardown();
|
||||||
const resp = await sendMessage({ type: 'delete_item', id: item.id });
|
const resp = await sendMessage({ type: 'delete_item', id: item.id });
|
||||||
if (!resp.ok) { setState({ error: resp.error }); return; }
|
if (!resp.ok) { setState({ error: resp.error }); return; }
|
||||||
const listResp = await sendMessage({ type: 'list_items' });
|
const listResp = await sendMessage({ type: 'list_items' });
|
||||||
@@ -77,23 +94,77 @@ export async function renderDetail(app: HTMLElement, item: Item): Promise<void>
|
|||||||
type: 'fill_credentials', id: item.id, capturedTabId, capturedUrl,
|
type: 'fill_credentials', id: item.id, capturedTabId, capturedUrl,
|
||||||
});
|
});
|
||||||
if (!resp.ok) setState({ error: resp.error });
|
if (!resp.ok) setState({ error: resp.error });
|
||||||
else window.close();
|
else { teardown(); window.close(); }
|
||||||
});
|
});
|
||||||
|
|
||||||
if (hasTotp) startTotpTicker(item.id);
|
if (hasTotp) startTotpTicker(item.id);
|
||||||
|
|
||||||
const handler = (e: KeyboardEvent) => {
|
const handler = async (e: KeyboardEvent) => {
|
||||||
|
// Bail if the user is typing in an editable field — don't steal printable keystrokes.
|
||||||
const t = e.target;
|
const t = e.target;
|
||||||
if (t instanceof HTMLElement) {
|
if (t instanceof HTMLElement) {
|
||||||
const tag = t.tagName;
|
const tag = t.tagName;
|
||||||
if (tag === 'INPUT' || tag === 'TEXTAREA' || tag === 'SELECT' || t.isContentEditable) return;
|
if (tag === 'INPUT' || tag === 'TEXTAREA' || tag === 'SELECT' || t.isContentEditable) return;
|
||||||
}
|
}
|
||||||
if (e.key === 'Escape') {
|
|
||||||
document.removeEventListener('keydown', handler);
|
switch (e.key) {
|
||||||
stopTotpTicker();
|
case 'Escape':
|
||||||
|
teardown();
|
||||||
navigate('list');
|
navigate('list');
|
||||||
|
break;
|
||||||
|
|
||||||
|
case 'c':
|
||||||
|
if (username) {
|
||||||
|
try { await navigator.clipboard.writeText(username); } catch { /* no-op */ }
|
||||||
|
}
|
||||||
|
break;
|
||||||
|
|
||||||
|
case 'p':
|
||||||
|
try { await navigator.clipboard.writeText(password); } catch { /* no-op */ }
|
||||||
|
break;
|
||||||
|
|
||||||
|
case 't':
|
||||||
|
if (hasTotp) {
|
||||||
|
const codeEl = document.getElementById('totp-code');
|
||||||
|
const code = codeEl?.textContent?.trim();
|
||||||
|
if (code && code !== '…') {
|
||||||
|
try { await navigator.clipboard.writeText(code); } catch { /* no-op */ }
|
||||||
|
}
|
||||||
|
}
|
||||||
|
break;
|
||||||
|
|
||||||
|
case 'f': {
|
||||||
|
const { capturedTabId, capturedUrl } = getState();
|
||||||
|
if (capturedTabId === null) { setState({ error: 'No active tab captured' }); break; }
|
||||||
|
const resp = await sendMessage({
|
||||||
|
type: 'fill_credentials', id: item.id, capturedTabId, capturedUrl,
|
||||||
|
});
|
||||||
|
if (!resp.ok) setState({ error: resp.error });
|
||||||
|
else { teardown(); window.close(); }
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
|
||||||
|
case 'e':
|
||||||
|
teardown();
|
||||||
|
navigate('edit');
|
||||||
|
break;
|
||||||
|
|
||||||
|
case 'd':
|
||||||
|
e.preventDefault();
|
||||||
|
if (confirm(`Move "${item.title}" to trash?`)) {
|
||||||
|
teardown();
|
||||||
|
const resp = await sendMessage({ type: 'delete_item', id: item.id });
|
||||||
|
if (!resp.ok) { setState({ error: resp.error }); return; }
|
||||||
|
const listResp = await sendMessage({ type: 'list_items' });
|
||||||
|
if (listResp.ok) {
|
||||||
|
const data = listResp.data as { items: Array<[ItemId, ManifestEntry]> };
|
||||||
|
navigate('list', { entries: data.items, selectedId: null, selectedItem: null });
|
||||||
|
} else navigate('list');
|
||||||
|
}
|
||||||
|
break;
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
activeKeyHandler = handler;
|
||||||
document.addEventListener('keydown', handler);
|
document.addEventListener('keydown', handler);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -102,6 +173,7 @@ export async function renderDetail(app: HTMLElement, item: Item): Promise<void>
|
|||||||
// ----------------------------------------------------------------------
|
// ----------------------------------------------------------------------
|
||||||
|
|
||||||
let totpTickerId: ReturnType<typeof setInterval> | null = null;
|
let totpTickerId: ReturnType<typeof setInterval> | null = null;
|
||||||
|
let activeKeyHandler: ((e: KeyboardEvent) => void) | null = null;
|
||||||
function stopTotpTicker(): void {
|
function stopTotpTicker(): void {
|
||||||
if (totpTickerId !== null) { clearInterval(totpTickerId); totpTickerId = null; }
|
if (totpTickerId !== null) { clearInterval(totpTickerId); totpTickerId = null; }
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -13,9 +13,12 @@ import { renderSettings } from './components/settings';
|
|||||||
|
|
||||||
// --- Escape HTML to prevent XSS ---
|
// --- Escape HTML to prevent XSS ---
|
||||||
export function escapeHtml(str: string): string {
|
export function escapeHtml(str: string): string {
|
||||||
const div = document.createElement('div');
|
return str
|
||||||
div.textContent = str;
|
.replace(/&/g, '&')
|
||||||
return div.innerHTML;
|
.replace(/</g, '<')
|
||||||
|
.replace(/>/g, '>')
|
||||||
|
.replace(/"/g, '"')
|
||||||
|
.replace(/'/g, ''');
|
||||||
}
|
}
|
||||||
|
|
||||||
// --- State ---
|
// --- State ---
|
||||||
|
|||||||
Reference in New Issue
Block a user