# Code Review: MarketPulse Dashboard
**Date:** May 23, 2026  
**Scope:** `dash.html`, `api/finnhub-key.php`

---

## Critical Issues

### 1. **⚠️ SECURITY: API Key in URL (Unavoidable — Mitigate Risk) [NOT FIXED — API CONSTRAINT]**
**File:** `dash.html` lines 935–950 (fhFetch function)  
**Issue:** API key must be appended to fetch URL because both Twelve Data and Finnhub APIs require it as a query parameter, not as a header.
```javascript
// Twelve Data requires: &apikey=KEY
// Finnhub requires: &token=KEY
```

**Risk:** URL is visible in:
- Browser DevTools Network tab
- Server access logs
- Browser history
- Error messages if logged

**Mitigation (Current):**
- ✅ Never log the full fetchUrl to console (don't pass it to debugLog)
- ✅ Debug console disabled by default
- ✅ API keys stored server-side with 0600 permissions
- ✅ Short TTL on cached quotes (5 min)

**Recommendation:**
This is a fundamental limitation of the Twelve Data and Finnhub public APIs. For production, consider:
1. **API Proxy**: Route all API calls through your server, authenticate client via session token
2. **API Gateway**: Use AWS API Gateway, Cloudflare Workers, or similar to proxy + rate-limit
3. **OAuth Flow**: Implement server-side OAuth token management (if APIs support it)

For local/dev use, this is acceptable risk.

---

### 2. **⚠️ SECURITY: Double Ending PHP Tag (HIGH)**
**File:** `api/finnhub-key.php` line 136  
**Issue:** File ends with `?>` followed by another `?>`:
```php
}
?>
?>  // ← DUPLICATE
```

**Risk:** Malformed PHP can cause unexpected behavior; good practice is to omit closing `?>` in PHP files.

**Fix:** Remove the last `?>` line.

---

### 3. **🔴 BUG: Missing `prefs` Object Initialization (CRITICAL)**
**File:** `dash.html` lines 1784+  
**Issue:** DOMContentLoaded references `prefs` object without ensuring it's loaded first:
```javascript
window.addEventListener('DOMContentLoaded', async () => {
  API_PROVIDER = prefs.apiProvider || 'twelve';  // ← prefs might be undefined
```

The `loadPrefs()` function should be called before accessing `prefs`, but there's no guarantee it's been called:

```javascript
function loadPrefs() {
  prefs = storageJSON('mp_prefs2', {...});  // defined later
}
```

**Fix:** Ensure `loadPrefs()` is called at the very start of DOMContentLoaded:
```javascript
window.addEventListener('DOMContentLoaded', async () => {
  loadPrefs();  // ← FIRST
  API_PROVIDER = prefs.apiProvider || 'twelve';
  // ... rest of initialization
```

---

### 4. **🔴 BUG: Race Condition on Initialization (HIGH)**
**File:** `dash.html` DOMContentLoaded handler  
**Issue:** Multiple async calls without coordination:
```javascript
window.addEventListener('DOMContentLoaded', async () => {
  // ... (no await)
  const serverKey = await serverKeyGet(API_PROVIDER);
  // ... DOM reads/writes may fire before this completes
  loadPrefs();  // called AFTER trying to use prefs
  refreshAll();  // may fire before prefs is loaded
```

**Impact:** Race condition between async key fetch and DOM initialization.

**Fix:**
```javascript
window.addEventListener('DOMContentLoaded', async () => {
  loadPrefs();  // Load first (synchronous)
  
  API_PROVIDER = prefs.apiProvider || 'twelve';
  document.getElementById('apiProviderSelect').value = API_PROVIDER;
  updateProviderInfo();
  
  // THEN fetch server key
  const serverKey = await serverKeyGet(API_PROVIDER);
  if (serverKey) {
    API_KEY = serverKey;
    setProviderKey(API_PROVIDER, serverKey);
  } else {
    API_KEY = getProviderKey(API_PROVIDER) || '';
  }
  
  // Finally refresh
  if (API_KEY) refreshAll();
});
```

---

## Medium Issues

### 5. **⚠️ SECURITY: CORS Wildcard Access (MEDIUM)**
**File:** `api/finnhub-key.php` line 17  
**Issue:**
```php
header('Access-Control-Allow-Origin: *');
```

**Risk:** Any domain can call this endpoint; key storage is accessible cross-origin.

**Fix:** Restrict to specific origins:
```php
$allowed = ['https://fid.intellihometech.com', 'http://localhost'];
$origin = $_SERVER['HTTP_ORIGIN'] ?? '';
if (in_array($origin, $allowed)) {
  header("Access-Control-Allow-Origin: $origin");
}
```

---

### 6. **⚠️ SECURITY: Missing CSRF Protection (MEDIUM)**
**File:** `api/finnhub-key.php`  
**Issue:** No CSRF token validation on POST/DELETE operations. An attacker's website could submit forms to modify API keys.

**Fix:** Validate a CSRF token from session:
```php
if ($_POST || $_SERVER['REQUEST_METHOD'] === 'POST') {
  if (!isset($_POST['csrf']) || $_POST['csrf'] !== $_SESSION['csrf_token']) {
    http_response_code(403);
    exit('CSRF validation failed');
  }
}
```

---

### 7. **⚠️ BUG: Incomplete Error Handling in getQuote() (MEDIUM)**
**File:** `dash.html` lines 974–1013  
**Issue:** Division by zero not caught in Twelve Data normalization:
```javascript
if (API_PROVIDER === 'twelve') {
  quote = {
    d: parseFloat(d.close) - parseFloat(d.previous_close),
    dp: ((parseFloat(d.close) - parseFloat(d.previous_close)) / parseFloat(d.previous_close) * 100)  // ← /0 if previous_close is 0
  };
}
```

**Fix:**
```javascript
const prevClose = parseFloat(d.previous_close) || 1;  // fallback to 1 to avoid /0
quote.dp = ((parseFloat(d.close) - prevClose) / prevClose * 100);
```

---

### 8. **⚠️ PERFORMANCE: No Request Timeout (MEDIUM)**
**File:** `dash.html` fhFetch() function (lines 935–973)  
**Issue:** Fetch calls have no timeout; slow API responses hang indefinitely.

**Fix:** Add AbortController timeout:
```javascript
async function fhFetch(url) {
  const controller = new AbortController();
  const timeout = setTimeout(() => controller.abort(), 10000);  // 10s timeout
  
  try {
    const res = await fetch(fetchUrl, { signal: controller.signal });
    // ...
  } finally {
    clearTimeout(timeout);
  }
}
```

---

### 9. **⚠️ BUG: Missing Null Check Before DOM Operations (MEDIUM)**
**File:** `dash.html` line 1043 (setStatusMessage)  
**Issue:**
```javascript
function setStatusMessage(message, color = 'var(--bs-danger)') {
  const toast = document.getElementById('errorToast');
  const toastMessage = document.getElementById('toastMessage');
  if (!toast || !toastMessage) return;  // ← guard is here
  
  toastMessage.innerHTML = message;  // ← but innerHTML allows XSS
}
```

**Risk:** If `message` contains unsanitized user input (e.g., from API error response), it enables XSS.

**Fix:** Use `textContent` instead of `innerHTML`:
```javascript
toastMessage.textContent = message;  // Safe: no HTML parsing
```

---

## Low Issues

### 10. **💡 PERFORMANCE: Cache Never Cleared (LOW)**
**File:** `dash.html` line 1013 (quoteCache)  
**Issue:** Quote cache grows unbounded; no size limit or LRU cleanup.

**Fix:** Add max cache size:
```javascript
const MAX_CACHE_SIZE = 200;
if (Object.keys(quoteCache).length > MAX_CACHE_SIZE) {
  const keys = Object.keys(quoteCache);
  delete quoteCache[keys[0]];  // Remove oldest
}
```

---

### 11. **💡 MAINTAINABILITY: Magic Numbers (LOW)**
**File:** Multiple locations  
**Issues:**
- Cache timeout: `300000` ms (5 min) — should be constant
- Timeout: `10000` ms (if added) — unclear duration
- Decimal precision: `dp()` function called repeatedly

**Fix:**
```javascript
const CACHE_TTL_MS = 5 * 60 * 1000;     // 5 minutes
const REQUEST_TIMEOUT_MS = 10 * 1000;   // 10 seconds
const DEFAULT_DECIMAL_PLACES = 2;
```

---

### 12. **💡 UX: Error Messages Not User-Friendly (LOW)**
**File:** `dash.html` testApiKey() line 1668–1670  
**Issue:** Error messages show raw exceptions:
```javascript
el.innerHTML = `<div style="color:var(--bs-danger)">✗ API key invalid or ${symbol} not found</div><pre>${e.message}</pre>`;
```

If API returns HTML error or raw SQL, it gets displayed as-is.

**Fix:** Sanitize and categorize errors:
```javascript
el.textContent = `✗ API key invalid or ${symbol} not found`;
debugLog(`Debug: ${escHTML(e.message)}`);
```

---

### 13. **💡 CODE QUALITY: Unused Variables (LOW)**
**File:** `dash.html` line 1669  
**Issue:** `testSymIco` element is written twice unnecessarily:
```javascript
if (document.getElementById('testSymIco')) document.getElementById('testSymIco').className = 'bi bi-hourglass-split';
// later...
if (document.getElementById('testSymIco')) document.getElementById('testSymIco').className = 'bi bi-check-circle-fill';
```

**Fix:** Cache the element:
```javascript
const ico = document.getElementById('testSymIco');
if (ico) ico.className = 'bi bi-hourglass-split';
```

---

## Recommendations Summary

| Severity | Count | Action |
|----------|-------|--------|
| 🔴 CRITICAL | 2 | Fix immediately (prefs init, race condition) |
| ⚠️ HIGH | 2 | Fix before production (API key in URL, PHP syntax) |
| ⚠️ MEDIUM | 5 | Fix soon (CORS, CSRF, error handling, timeout, XSS) |
| 💡 LOW | 4 | Refactor when convenient (cache, magic numbers, UX, code quality) |

---

## Testing Checklist

- [ ] Test with network throttled to verify timeout behavior
- [ ] Test API error responses (invalid key, rate limit, malformed JSON)
- [ ] Test switching providers with invalid/missing keys
- [ ] Verify API key never appears in browser console/DevTools Network tab
- [ ] Test page refresh during fetch to verify race conditions don't occur
- [ ] Load page without stored keys — verify overlay appears
- [ ] Test with disabled JavaScript to verify graceful degradation