hermes-config/skills/software-development/codebase-audit/references/common-web-app-bugs.md

Common Web App Bugs Found in Audits

XSS via innerHTML Template Literals

Pattern: ${userData} inside backtick strings assigned to .innerHTML

Files affected: customers.js, appointments.js, repair-orders.js, quote-tab-manager.js, financial-dashboard.js, invoice-manager.js, dashboard.js

Fix: Import escapeHtml from a shared sanitize module and wrap every user/Firestore-derived value:

import { escapeHtml } from './shared/sanitize.js';

// Before:
`<p>${customer.name}</p>`
// After:
`<p>${escapeHtml(customer.name)}</p>`

Watch for: Template literals inside .map().join('') chains that build HTML tables/lists, data attribute injectors like data-customer='${JSON.stringify(obj)}', and numeric fields that could still have injection via toString().

Stored XSS via Inline onclick

Pattern: onclick="fn('${userData}')" in HTML template literals

Fix: Remove inline onclick, use addEventListener with data-* attributes:

// Before:
`<button onclick="openModal('${ro.id}', '${ro.status}')">Edit</button>`

// After:
`<button class="edit-btn" data-ro-id="${escapeHtml(ro.id)}" data-status="${escapeHtml(ro.status)}">Edit</button>`
// In JS:
document.querySelectorAll('.edit-btn').forEach(btn => {
    btn.addEventListener('click', () => {
        openModal(btn.dataset.roId, btn.dataset.status);
    });
});

Null Crash on Firestore Timestamp Fields

Pattern: doc.data().timestampField.toDate() without null check

Fix: Optional chaining with fallback:

// Before:
writeupTime: doc.data().writeupTime.toDate(),
promisedTime: doc.data().promisedTime.toDate(),

// After:
writeupTime: doc.data().writeupTime?.toDate?.() ?? new Date(),
promisedTime: doc.data().promisedTime?.toDate?.() ?? new Date()

Also check .toUpperCase(), .toLocaleString(), and .toDateString() calls on fields that might be undefined.

Timer Nuking

Pattern: Aggressive loop clearing ALL browser timers:

const highestTimeoutId = setTimeout(() => {}, 0);
for (let i = 0; i < highestTimeoutId; i++) {
    clearTimeout(i);
}

Fix: Track your own timeouts:

let activeTimeouts = [];
let activeIntervals = [];

function trackedSetTimeout(fn, ms) {
    const id = setTimeout(() => {
        activeTimeouts = activeTimeouts.filter(t => t !== id);
        fn();
    }, ms);
    activeTimeouts.push(id);
    return id;
}

function clearNamedTimers() {
    for (const id of activeTimeouts) clearTimeout(id);
    for (const id of activeIntervals) clearInterval(id);
    activeTimeouts = [];
    activeIntervals = [];
}

Infinite Retry With No Backoff

Pattern: setTimeout(() => loadData(), 3000) in catch block with no retry limit

Fix: Bounded retry with exponential backoff:

let retryCount = 0;
const MAX_RETRIES = 3;

// Inside catch block:
retryCount++;
if (retryCount <= MAX_RETRIES) {
    const delay = Math.min(3000 * Math.pow(2, retryCount - 1), 30000);
    setTimeout(() => loadData(), delay);
} else {
    showNotification('Failed after multiple attempts. Please refresh.', true);
}

Duplicate Variable in Closure (Shadowing Bug)

Pattern: A variable name used both at function scope and inside an if block:

const roIndex = repairOrders.findIndex(ro => ro.id === roId);
if (roIndexStr !== -1) {
    const roIndex = roIndexStr;  // SHADOWS outer roIndex!
    repairOrders.splice(roIndex, 1);
}
// Outside if: roIndex is still -1 (original value)
let actualIndex = roIndex;  // BUG: uses -1

Fix: Rename the inner variable or use a single variable throughout:

let roArrayIndex = repairOrders.findIndex(ro => ro.id === roId);
if (roArrayIndex === -1) {
    roArrayIndex = repairOrders.findIndex(ro => ro.roNumber === roId);
}

Accumulating Global Click Listeners

Pattern: document.addEventListener('click', ...) inside a function called on every render:

function renderList(items) {
    container.innerHTML = items.map(createCard).join('');
    document.addEventListener('click', handleCardClick);  // NEW LISTENER EVERY RENDER!
}

Fix: Use a flag or event delegation on the container instead:

let listenersAttached = false;

function renderList(items) {
    container.innerHTML = items.map(createCard).join('');
    if (!listenersAttached) {
        container.addEventListener('click', handleCardClick);
        listenersAttached = true;
    }
}

Or better, delegate on the container and let event bubbling handle dynamically added children without re-attaching.

Cache-Control Locking Users Out of Updates

Pattern: firebase.json with max-age=31536000 (1 year) on JS/CSS

Fix: Significantly reduce cache time and add must-revalidate:

{
    "source": "**/*.@(js|css)",
    "headers": [{
        "key": "Cache-Control",
        "value": "max-age=3600, must-revalidate"
    }]
},
{
    "source": "**/*.html",
    "headers": [{
        "key": "Cache-Control",
        "value": "no-cache, must-revalidate"
    }]
}

For production, use content fingerprinting in filenames (e.g., app.a1b2c3.js) so you can safely use long cache durations without staleness.