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:

```javascript
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:

```javascript
// 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:

```javascript
// 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:

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

**Fix:** Track your own timeouts:

```javascript
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:

```javascript
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:

```javascript
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:

```javascript
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:

```javascript
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:

```javascript
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`:

```json
{
    "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.