Security hardening and accessibility improvements (v1.1.0)

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>
2026-02-18 10:32:16 +00:00
parent 0a73181ea7
commit 2294c8eda7
7 changed files with 146 additions and 24 deletions
--- a/SECURITY_TASKS.md
+++ b/SECURITY_TASKS.md
@@ -0,0 +1,106 @@
+# 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.
--- a/admin/js/admin.js
+++ b/admin/js/admin.js
@@ -131,22 +131,21 @@
 				return;
 			}

-			var categoryColors = {
-				functional: 'functional',
-				analytics:  'analytics',
-				marketing:  'marketing',
-			};
-
 			scripts.forEach(function(s) {
-				var row = '<tr data-id="' + s.id + '">' +
+				var eCat = escapeHtml(s.category);
+				var eStatus = escapeHtml(s.status);
+				var ePlacement = escapeHtml(s.placement);
+				var ePriority = escapeHtml(String(s.priority));
+				var eId = escapeHtml(String(s.id));
+				var row = '<tr data-id="' + eId + '">' +
 					'<td class="column-name">' + escapeHtml(s.name) + '</td>' +
-					'<td class="column-category"><span class="att-cc-badge att-cc-badge--' + s.category + '">' + capitalize(s.category) + '</span></td>' +
-					'<td class="column-placement">' + capitalize(s.placement) + '</td>' +
-					'<td class="column-priority">' + s.priority + '</td>' +
-					'<td class="column-status"><span class="att-cc-status att-cc-status--' + s.status + '">' + capitalize(s.status) + '</span></td>' +
+					'<td class="column-category"><span class="att-cc-badge att-cc-badge--' + eCat + '">' + capitalize(eCat) + '</span></td>' +
+					'<td class="column-placement">' + capitalize(ePlacement) + '</td>' +
+					'<td class="column-priority">' + ePriority + '</td>' +
+					'<td class="column-status"><span class="att-cc-status att-cc-status--' + eStatus + '">' + capitalize(eStatus) + '</span></td>' +
 					'<td class="column-actions">' +
-						'<button type="button" class="button button-small att-cc-edit-script" data-id="' + s.id + '">Edit</button> ' +
-						'<button type="button" class="button button-small button-link-delete att-cc-delete-script" data-id="' + s.id + '">Delete</button>' +
+						'<button type="button" class="button button-small att-cc-edit-script" data-id="' + eId + '">Edit</button> ' +
+						'<button type="button" class="button button-small button-link-delete att-cc-delete-script" data-id="' + eId + '">Delete</button>' +
 					'</td>' +
 				'</tr>';
 				$tbody.append(row);
--- a/att-consent.php
+++ b/att-consent.php
@@ -3,7 +3,7 @@
 * Plugin Name: ATT Consent
 * Plugin URI:  https://github.com/att-consent/att-consent
 * Description: Google Consent Mode v2 cookie consent with session attribution preservation, custom script management, and full gtag.js/GTM support.
- * Version:     1.0.0
+ * Version:     1.1.0
 * Requires at least: 6.0
 * Requires PHP: 7.4
 * Author:      ATT Consent
@@ -18,7 +18,7 @@ if ( ! defined( 'ABSPATH' ) ) {
 	exit;
 }

-define( 'ATT_CC_VERSION', '1.0.0' );
+define( 'ATT_CC_VERSION', '1.1.0' );
 define( 'ATT_CC_PLUGIN_DIR', plugin_dir_path( __FILE__ ) );
 define( 'ATT_CC_PLUGIN_URL', plugin_dir_url( __FILE__ ) );
 define( 'ATT_CC_PLUGIN_BASENAME', plugin_basename( __FILE__ ) );
--- a/includes/class-admin.php
+++ b/includes/class-admin.php
@@ -102,7 +102,6 @@ class ATT_Consent_Admin {
 			return;
 		}

-		$active_tab = isset( $_GET['tab'] ) ? sanitize_key( $_GET['tab'] ) : 'general'; // phpcs:ignore WordPress.Security.NonceVerification.Recommended
 		$tabs       = array(
 			'general'    => __( 'General', 'att-consent' ),
 			'appearance' => __( 'Appearance', 'att-consent' ),
@@ -110,6 +109,10 @@ class ATT_Consent_Admin {
 			'scripts'    => __( 'Custom Scripts', 'att-consent' ),
 			'advanced'   => __( 'Advanced', 'att-consent' ),
 		);
+		$active_tab = isset( $_GET['tab'] ) ? sanitize_key( $_GET['tab'] ) : 'general'; // phpcs:ignore WordPress.Security.NonceVerification.Recommended
+		if ( ! array_key_exists( $active_tab, $tabs ) ) {
+			$active_tab = 'general';
+		}

 		$settings = ATT_Consent::get_settings();
 		?>
@@ -163,7 +166,7 @@ class ATT_Consent_Admin {
 				$settings['gtm_container_id']   = sanitize_text_field( $_POST['gtm_container_id'] ?? '' );
 				$settings['consent_mode']       = in_array( $_POST['consent_mode'] ?? '', array( 'advanced', 'basic' ), true ) ? $_POST['consent_mode'] : 'advanced';
 				$settings['banner_position']    = in_array( $_POST['banner_position'] ?? '', array( 'bottom', 'top', 'center' ), true ) ? $_POST['banner_position'] : 'bottom';
-				$settings['consent_expiry']     = absint( $_POST['consent_expiry'] ?? 365 );
+				$settings['consent_expiry']     = min( 730, max( 1, absint( $_POST['consent_expiry'] ?? 365 ) ) );
 				$settings['floating_widget'] = in_array(
 					$_POST['floating_widget'] ?? '',
 					array( 'bottom-right', 'right', 'none' ),
--- a/includes/class-att-consent.php
+++ b/includes/class-att-consent.php
@@ -104,7 +104,7 @@ class ATT_Consent {
 			new ATT_Consent_Admin();
 		}

-		if ( ! is_admin() || wp_doing_ajax() ) {
+		if ( ! is_admin() ) {
 			require_once ATT_CC_PLUGIN_DIR . 'includes/class-frontend.php';
 			new ATT_Consent_Frontend();
 		}
--- a/includes/class-frontend.php
+++ b/includes/class-frontend.php
@@ -80,7 +80,9 @@ g('js',new Date());
 if(c.ads_data_redaction){g('set','ads_data_redaction',true);}
 if(c.url_passthrough){g('set','url_passthrough',true);}
 var ck=document.cookie.match(/(?:^|; )att_cc_consent=([^;]*)/);
-var st=ck?JSON.parse(decodeURIComponent(ck[1])):null;
+var st=null;
+if(ck){try{st=JSON.parse(decodeURIComponent(ck[1]));}catch(e){}}
+
 if(st){
 g('consent','default',{
 ad_storage:st.marketing?'granted':'denied',
@@ -235,10 +237,10 @@ j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
 			esc_attr( $s['btn_preferences_text'] )
 		);

-		$banner_html = '<div id="att-cc-banner" class="' . esc_attr( $position_class ) . '" role="dialog" aria-label="' . esc_attr__( 'Cookie consent', 'att-consent' ) . '" aria-hidden="false" style="' . $style_vars . '">
+		$banner_html = '<div id="att-cc-banner" class="' . esc_attr( $position_class ) . '" role="dialog" aria-labelledby="att-cc-banner-heading" aria-hidden="false" tabindex="-1" style="' . $style_vars . '">
 	<div class="att-cc-banner__inner">
 		<div class="att-cc-banner__content">
-			<h2 class="att-cc-banner__heading">' . esc_html( $s['banner_heading'] ) . '</h2>
+			<h2 id="att-cc-banner-heading" class="att-cc-banner__heading">' . esc_html( $s['banner_heading'] ) . '</h2>
 			<p class="att-cc-banner__message">' . wp_kses_post( $s['banner_message'] ) . '</p>
 		</div>
 		<div class="att-cc-banner__actions">
@@ -249,10 +251,10 @@ j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
 	</div>
 </div>';

-		$modal_html = '<div id="att-cc-modal" role="dialog" aria-label="' . esc_attr__( 'Cookie preferences', 'att-consent' ) . '" aria-hidden="true" aria-modal="true" style="' . $style_vars . '">
+		$modal_html = '<div id="att-cc-modal" role="dialog" aria-labelledby="att-cc-modal-heading" aria-hidden="true" aria-modal="true" style="' . $style_vars . '">
 	<div class="att-cc-modal__overlay"></div>
 	<div class="att-cc-modal__dialog">
-		<h2 class="att-cc-modal__heading">' . esc_html__( 'Manage Cookie Preferences', 'att-consent' ) . '</h2>
+		<h2 id="att-cc-modal-heading" class="att-cc-modal__heading">' . esc_html__( 'Manage Cookie Preferences', 'att-consent' ) . '</h2>

 		<div class="att-cc-modal__category">
 			<div class="att-cc-modal__cat-header">
--- a/public/js/banner.js
+++ b/public/js/banner.js
@@ -72,6 +72,9 @@
 		hideBanner();
 		showWidget();
 	} else {
+		// Move focus to banner so screen readers announce it.
+		banner.focus();
+
 		// --- Banner buttons (only needed for first-time visitors) ---

 		var acceptBtn = banner.querySelector('[data-att-cc="accept-all"]');
@@ -103,6 +106,9 @@

 	// --- Helper functions ---

+	// Tracks the current focus trap handler for cleanup.
+	var activeTrapHandler = null;
+
 	function hideBanner() {
 		banner.setAttribute('aria-hidden', 'true');
 		banner.classList.add('att-cc-hidden');
@@ -134,6 +140,12 @@
 		modal.classList.remove('att-cc-modal--open');
 		document.body.classList.remove('att-cc-modal-active');

+		// Clean up focus trap listener.
+		if (activeTrapHandler) {
+			modal.removeEventListener('keydown', activeTrapHandler);
+			activeTrapHandler = null;
+		}
+
 		// Return focus to the widget if visible, otherwise the banner prefs button.
 		if (widget && widget.style.display !== 'none') {
 			widget.focus();
@@ -174,7 +186,6 @@
 		function handleKeydown(e) {
 			if (e.key === 'Escape') {
 				closeModal();
-				element.removeEventListener('keydown', handleKeydown);
 				return;
 			}

@@ -195,6 +206,7 @@
 			}
 		}

+		activeTrapHandler = handleKeydown;
 		element.addEventListener('keydown', handleKeydown);
 	}
 })();