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>
4.8 KiB
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.