Steve Hanlon
2294c8eda7
Address findings from security audit: wrap inline JSON.parse in try/catch to prevent consent flow failure on corrupted cookies, allowlist admin tab parameter, clamp consent_expiry server-side, escape all server values in admin JS table builder, fix focus trap listener cleanup, add aria-labelledby and banner focus for screen readers, skip frontend loading during AJAX. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
107 lines
4.8 KiB
Markdown
107 lines
4.8 KiB
Markdown
# ATT Consent — Security & Standards Audit
|
||
|
||
Audit date: 2026-02-18
|
||
|
||
## Summary
|
||
|
||
| Severity | Count | Status |
|
||
|----------|-------|--------|
|
||
| Critical | 0 | — |
|
||
| High | 2 | Documented (by-design / future consideration) |
|
||
| Medium | 4 | Fix in progress |
|
||
| Low | 7 | Fix in progress (priority items) |
|
||
|
||
---
|
||
|
||
## High Severity
|
||
|
||
### H1. Admin Script Snippets Rendered Raw to Frontend
|
||
|
||
**File:** `includes/class-scripts-manager.php:106`, `includes/class-frontend.php:178,185`
|
||
**Status:** By design — no action required
|
||
**Detail:** The `snippet` field is stored raw and rendered to all visitors. This is the standard WordPress pattern for code-injection features (Theme Editor, HFCM). Protected by `manage_options` capability + nonce verification on save.
|
||
|
||
### H2. Inline Scripts Incompatible with CSP
|
||
|
||
**File:** `includes/class-frontend.php:68-127`
|
||
**Status:** Future consideration
|
||
**Detail:** The consent defaults and tracking scripts are output as raw inline `<script>` tags without a `nonce` attribute. Sites enforcing `Content-Security-Policy: script-src 'self'` will break. Consider adding a filter for nonce injection if CSP support is needed.
|
||
|
||
---
|
||
|
||
## Medium Severity
|
||
|
||
### M1. Uncaught JSON.parse in Inline Head Script
|
||
|
||
**File:** `includes/class-frontend.php:83`
|
||
**Status:** Fixed
|
||
**Detail:** The inline `<head>` script parses the consent cookie with `JSON.parse()` without try/catch. If the cookie is corrupted or tampered, the exception kills the entire consent flow. The enqueued `consent-manager.js` already handles this correctly with try/catch.
|
||
**Fix:** Wrap in try/catch.
|
||
|
||
### M2. Active Tab Not Validated Against Allowlist
|
||
|
||
**File:** `includes/class-admin.php:105`
|
||
**Status:** Fixed
|
||
**Detail:** `$active_tab` is sanitized with `sanitize_key()` but not validated against the known `$tabs` array keys before being used in an `include` path. While `sanitize_key()` prevents directory traversal and `file_exists()` provides a guard, an allowlist check is better defense-in-depth.
|
||
**Fix:** Validate `$active_tab` against `$tabs` array keys.
|
||
|
||
### M4. Admin JS Builds HTML with Unescaped Server Values
|
||
|
||
**File:** `admin/js/admin.js:141-153`
|
||
**Status:** Fixed
|
||
**Detail:** `refreshScriptsTable()` injects `s.category`, `s.status`, `s.placement`, and `s.priority` into HTML without escaping. Server-side validation makes exploitation unlikely, but client-side escaping should be applied for defense-in-depth.
|
||
**Fix:** Apply `escapeHtml()` to all interpolated values.
|
||
|
||
### M5. Standalone Config Values Concatenated into innerHTML
|
||
|
||
**File:** `standalone/att-consent.js:360-412`
|
||
**Status:** Documented — fix if standalone is distributed
|
||
**Detail:** Banner heading, button labels, and category descriptions from `window.attConsentConfig` are concatenated directly into innerHTML. If a third party can control the config object, they can inject HTML/JS. `bannerMessage` intentionally supports HTML; other fields should be escaped.
|
||
|
||
---
|
||
|
||
## Low Severity
|
||
|
||
### L1. Dialogs Should Use aria-labelledby Instead of aria-label
|
||
|
||
**File:** `includes/class-frontend.php:238,252`
|
||
**Status:** Fixed
|
||
**Detail:** Both banner and modal use `aria-label` despite containing visible headings. Best practice (WCAG 2.1) is `aria-labelledby` pointing to the heading's `id`.
|
||
|
||
### L2. Focus Trap Listener Not Removed on Non-Escape Close
|
||
|
||
**File:** `public/js/banner.js:160-199`
|
||
**Status:** Fixed
|
||
**Detail:** The `keydown` listener added by `trapFocus()` is only removed on Escape. Closing via Cancel button or overlay click leaves the listener attached. Repeated opens stack duplicate listeners.
|
||
|
||
### L3. No rel="noopener" Enforcement on Banner Links
|
||
|
||
**File:** `includes/class-frontend.php:242`
|
||
**Status:** Documented — low risk
|
||
**Detail:** `wp_kses_post()` allows `<a target="_blank">` without requiring `rel="noopener"`. Minor tab-napping vector if admin adds external links.
|
||
|
||
### L4. No Format Validation on GA4/GTM IDs
|
||
|
||
**File:** `includes/class-admin.php:162-163`
|
||
**Status:** Documented — cosmetic
|
||
**Detail:** Accepts any text for measurement/container IDs. Invalid values cause silent tracking failures but no security risk.
|
||
|
||
### L5. consent_expiry Not Range-Clamped Server-Side
|
||
|
||
**File:** `includes/class-admin.php:166`
|
||
**Status:** Fixed
|
||
**Detail:** `absint()` prevents negatives but no upper bound. HTML input has `max="730"` but server doesn't enforce it.
|
||
**Fix:** Clamp to 1–730.
|
||
|
||
### L6. Banner Doesn't Receive Focus on First Display
|
||
|
||
**File:** `public/js/banner.js:69-102`
|
||
**Status:** Fixed
|
||
**Detail:** Screen reader users may not be aware the consent banner appeared. Focus should move to the banner or it should be announced via a live region.
|
||
|
||
### L7. Frontend Class Loaded During AJAX Unnecessarily
|
||
|
||
**File:** `includes/class-att-consent.php:107-110`
|
||
**Status:** Fixed
|
||
**Detail:** The frontend class is instantiated during `wp_doing_ajax()` but its hooks (`wp_head`, `wp_footer`) never fire during AJAX. Wasteful but harmless.
|