Guard the single-product tracker paths against an unparseable product payload

Security review 25 (base 788c8f2, reviewed at 612122e): one Low finding,
ledger #190, verifier-confirmed. The product detail page's add-to-cart
click path consumed gtm4wp_read_from_json()'s false return unguarded -
pushing add_to_cart with items [false] and value NaN in the shipped
sloppy-mode bundle - and the found_variation handler caught parse throws
but not a parse that succeeds with null (the attribute the server prints
when a product data filter returns null), which raised an uncaught
TypeError at the first property access. Every one of the eight
gtm4wp_read_json_from_node call sites already guards; these two direct
consumers were the siblings left behind. Both shapes are carried by
released 1.x too, whose strict dist bundle throws in both paths, so the
changelog entry is a real Fixed bullet against the stable baseline.

Both regression tests were watched red on the unfixed source first (the
traces land on the two fixed lines); the empty-payload case swaps the
describe-level parse stub for one modelling the real helper's false
return, so the test exercises the tracker rather than the stub. Jest
724 -> 726 green, npm run build + lint:js clean, PHP untouched.

The review itself found no security finding across the beta3 range;
checklist and patterns ledgers updated in this commit (RI-21 extended
with the consumer half of the reader check, FP-2/FP-5 re-derived).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: duracelltomi

.security/code-review-checklist.md

@@ -604,6 +604,16 @@ but rate the *blast radius*, which is the whole block rather than the value.
   `try`/`catch` that returns `false`. Verified, not assumed — check the reader before
   dismissing one. Re-verified 2026-08-11: all 12 `JSON.parse` call sites in `js/frontend/`
   are inside a `try`/`catch`.
+  - **The try/catch is only half the reader check (2026-08-14, #190).** A wrapped parse
+    guards the *throw* path; the class also needs the *result* path guarded, twice over:
+    the helper's `false` return must be checked by every **consumer** (grep the consumers,
+    not the helper — the guarded-sibling family in `gtm4wp-woocommerce.js` had 8
+    `gtm4wp_read_json_from_node` sites guarding and the one direct `gtm4wp_read_from_json`
+    consumer not), and a parse that **succeeds with `null`** (`wp_json_encode( null )` is the
+    string `'null'`, which a filter callback returning null produces) passes every catch and
+    throws only at the first property access. "All parse sites are inside try/catch" was this
+    entry's own recorded verdict and it was true while #190's two paths were live — a verdict
+    about the wrapper says nothing about the consumer.
 - **"Cannot fail" is a fine answer — write it down where the sink is.** The ledger reached
   **eleven** sites, not ten (#147): one more builds its literal in a separate statement, and
   its value is a `(string)`-cast scalar, so the encoder genuinely cannot fail on it. That is a
@@ -1282,6 +1292,7 @@ Reference: `PageDataLayer::confirm_pending_purchase_tracked()` (#398) writes the
 
 | Date | Action |
 |---|---|
+| 2026-08-14 (Review 25) | Reviewed `788c8f2..612122e` (7 commits — R24's fix session, the 2.0.0-beta3 release, and its first post-release hardening commits: the #462 span migration, the per-tracker media flags, the email-fold split, the reliable-purchase re-arm guard, and a dev-only dependency override). **1 Low (#190), no security finding.** Extended **RI-21** with the consumer half of the reader check: a `JSON.parse` inside try/catch guards the *throw* path only — the helper's `false` return must be guarded at every **consumer**, and a parse that succeeds with `null` (a filter returning null → the literal string `'null'`) passes every catch and fails at first property access. The entry's own "all 12 parse sites are inside try/catch" verdict was true while both of #190's paths were live — a verdict about the wrapper says nothing about the consumer; the guarded-sibling family (8 `gtm4wp_read_json_from_node` sites, 7 guard blocks) is the ledger. Verifier-CONFIRMED end to end with probes in both failure directions; recommendation probe-implemented (bad input skips the push, legit path byte-identical). **FP-2 re-derived** (oldest, rotation R21 → R25): all three legs verified, conclusion and caveat both hold. **FP-5 re-derived** (file in range): all three conditions demonstrable, beacons untouched by the range. Toolchain scope inventory reproduces #186's figures exactly (28 definitions, 0/15/2 Write/Edit); #187 snapshot/retake ran and reproduced. All core ledgers reproduce, incl. RI-25 12/12 (the two `the_permalink` re-applications match core's 2-arg signature). |
 | 2026-08-12 (Review 24) | Reviewed `1165053..788c8f2` (2 commits) — R23's second and third fix sessions in full, the ninth consecutive run reading the previous review's own fixes, and this run's job was the **adjudication R23 handed forward**: `64abf41`'s order-received gate rewrite, verified inline there because three verifier dispatches died to API overload. Re-adjudicated with the full machinery against **WooCommerce source fetched at twelve release tags**: an independent verifier's exhaustive decision-table simulation confirmed the parity claim — zero divergences beyond the two documented laxities, all three re-applied filter arities correct, and the feature-detect boundary exact at 8.5.2 (Users **class** present, **method** absent — the `method_exists` half of the guard carries it, vindicating U113's "probe the method" row). **1 Low (#189), no security finding.** Extended **RI-25** with the value half of the hook contract: when a re-applied filter is fed an **approximated** input (literal `true` for terms the mirror cannot read), "can only fail closed" is a theorem about **monotone callbacks only** — truth-table-provable, false for a strictly inverting callback — so prose absolutes over such calls need the monotone qualifier; new litmus question for every RI-25 ledger site ("exact value or approximation?"). The finding itself is docs-only, verifier-CONFIRMED with the recommendation **revised by the verifier** (the docblock names three accepted residuals, not two). **FP-5 re-derived** (rotation + file-in-range, doubly owed): all three conditions demonstrable. **#186/#187 verified live**: the widened toolchain inventory reproduced its figures (28 definitions / 0-15-2 Write/Edit across scopes), and the #187 snapshot/retake ran for this review's own read-only verifier stage and reproduced exactly. Adjudication: 2 dispatches, 0 mechanisms refuted, 1 recommendation revised, 0 dispositions refuted. |
 | 2026-08-12 (Review 23, fix session) | **Widened PA-19** from *a permission ledger* to **any ledger assembled from more than one scope** (#186). #162 fixed the scope blindness for permissions and stopped there, so one review later the identical mechanism was found unapplied next door: the inventory counted **4** agent definitions where **28** resolve (4 project + **17 user scope** + 7 from an enabled plugin), **17 of the 24 uncounted declaring `Write`/`Edit` where none of the 4 counted did** — cause: a **relative** inventory path reaches project scope only. The generalizable half is now stated outright: *fixing a scope gap in one ledger owes the same edit to every ledger of that shape* — "fix the family, not the sibling" turned on this system's own bookkeeping. Three counting traps recorded with it (relative path = project scope; `__pycache__/*.pyc` inflates file counts; a plugin's content moves under a stable name, so pin `version` + `gitCommitSha`). **RI-25's ledger corrected 11 → 12** in *both* files that carry it, after `64abf41` added a twelfth third-party filter call, applied RI-25 correctly in the code, and left the count stale — **#132's shape in the ledger written one review earlier to catch the previous instance**, which is the argument for the "the count lives in TWO files" note now on the index line. |
 | 2026-08-12 (Review 23) | Reviewed `5a1cc42..1165053` (1 commit) — R22's own fix session, the eighth consecutive run where reading the previous review's fixes paid. **1 Medium + 5 Low (#182–#187).** Added **RI-25** (⭐ re-applying somebody else's filter carries their **argument list**, not just their hook name: `WP_Hook` never pads, so a documented-signature callback reached from a short caller raises an uncaught `ArgumentCountError` — fatal, not degraded; passing *more* than upstream is free because the dispatcher slices down to each callback's `accepted_args`). Its ledger is the litmus that matters: **11** third-party hook sites in `src/`+`compat/`, **10** correct, **1** wrong — and the wrong one was the newest, added by the very commit under review. Two things worth keeping about how it was found. First, **the adjudication stage produced it**: a verifier sent to check a documentation claim about the fallback's version window returned a production defect in the same three lines, so for the first time the stage generated the review's highest-severity finding rather than filtering one. Second, **the suite structurally could not see it** — Brain Monkey intercepts `apply_filters`, so a stand-in callback with optional parameters passes either way (UC-3); the regression test declares its parameters **required**, exactly as a documented site callback would, which reproduces the real `ArgumentCountError` and was watched red on the unfixed source with the trace landing on the finding's own line. **All 3 drafted recommendations were refuted**, two of them for shipping something worse than nothing: one would have closed 7 of ~50 uncounted items while stamping its ledger row complete (#109's false-reassurance shape), the other had a demonstrated false negative *and* a false positive across five measured states. |

.security/code-review-patterns.md

@@ -120,6 +120,7 @@ Major rewrite of the plugin - please read the announcement post on gtm4wp.com be
 * Fixed: variable **subscription** products now keep their variant data in ecommerce tracking. Variations were detected by an exact `variation` product-type match, but WooCommerce Subscriptions variations report `subscription_variation`, so their `item_variant`, `item_group_id` and parent-derived `item_category`/`item_brand` were dropped (most visibly on the `purchase` event). Variations are now detected structurally (any `WC_Product_Variation`), covering subscriptions and similar extensions.
 * Fixed: the dynamic-remarketing "Product ID prefix" is now kept on variations. When a variation was selected on a variable product page, the browser swapped in the variation id and dropped the configured prefix from the `id` field used for Google/Meta catalog matching; the prefix is now re-applied to the variation's `id` (the unprefixed `item_id` is unchanged).
 * Fixed: adding a product to the cart no longer forces a full page reload on stores using WooCommerce's blockified **Add to Cart + Options** block (block themes, WooCommerce 10.9+). With e-commerce tracking enabled, the plugin printed a hidden input field into the add-to-cart form, and WooCommerce deliberately renders a classic full-page POST form instead of the interactive one when a plugin prints form fields into it - so the interactive add to cart was disabled on every product page while GTM4WP was active. The product data now travels in a hidden span's data attribute (the same pattern product lists already use), which WooCommerce's check ignores. Tracking itself is unchanged: classic (non-block) product templates keep working exactly as before, and the tracker still reads the old hidden input as a fallback so cached pages keep tracking through the upgrade. As reported on GitHub (#462).
+* Fixed: a product data payload that cannot be written as JSON no longer breaks the product page's tracking scripts. When a plugin hooked into GTM4WP's product data filters puts a value into the product array that PHP cannot convert to JSON (or replaces the array with nothing at all), the product detail page's markup carries an empty or useless payload - and the browser-side tracker then either raised a script error in the add-to-cart click and variation-selection handlers or pushed a meaningless `add_to_cart` event with no product and no value. Both paths now skip the tracking event for that product - and only that - exactly like the product list and cart trackers already did with the same broken payload.
 * Fixed: the `[add_to_cart]` shortcode button now fires an `add_to_cart` event. A standalone shortcode button is rendered outside a product loop, so it never received the hidden product-data markup that product-list items get; the GA4 item data is now attached to the button itself so a click can be tracked. Product lists are unaffected — they already carry the data.
 * Fixed: the product-page `add_to_cart` event is no longer fired when the browser blocks the add-to-cart form submit because a required field is empty (e.g. a required Product Add-ons field). The click now respects the form's HTML5 validity, so a rejected add no longer produces a false `add_to_cart`.
 * Fixed: `add_to_cart` and `remove_from_cart` now always report `quantity` as a number, and report it the same way on every surface. Three symptoms of one cause: a product form with no quantity field at all - some themes and product add-on plugins render none - emitted `quantity: null` and `value: 0`; the cart page reported a string where the mini-cart reported a number for the very same product; and a cart line set to zero fired a removal event on the cart page while the mini-cart correctly suppressed it. Every quantity now goes through one parser, so the type and the zero handling are identical everywhere. **Check your GTM setup if a trigger or variable compares `quantity` against a string.**

CHANGELOG.md

@@ -375,6 +375,15 @@ function gtm4wp_track_single_add_to_cart( trigger_element, product_form ) {
 				product_data_el.value,
 			[ 'productlink' ]
 		);
+		// #190: the helper returns false when the payload cannot be parsed - the
+		// server prints an empty attribute when wp_json_encode() refuses the
+		// product array (a site filter supplying INF/NAN or over-deep nesting)
+		// and "null" when such a filter returns null. Same guard as the eight
+		// gtm4wp_read_json_from_node call sites; without it this path pushed
+		// add_to_cart with items: [false] and value: NaN.
+		if ( ! productdata ) {
+			return false;
+		}
 		// #69: the previous guard read `isNaN( quantity )` AFTER a lookup that
 		// short-circuits to null, and isNaN( null ) is false - so a product form
 		// with no quantity field emitted quantity: null and value: 0.
@@ -953,6 +962,15 @@ function gtm4wp_woocommerce_process_pages() {
 				return true;
 			}
 
+			// #190: a parse can SUCCEED with null - the attribute is the literal
+			// string "null" when a site filter returns null and the server
+			// wp_json_encode()s it - which passes the catch above and then threw
+			// on the first property access below. Treat it as "no product data",
+			// like the catch does.
+			if ( ! current_product_detail_data ) {
+				return true;
+			}
+
 			current_product_detail_data.price = gtm4wp_make_sure_is_float(
 				current_product_detail_data.price
 			);

js/frontend/gtm4wp-woocommerce.js

@@ -547,6 +547,83 @@ describe( 'gtm4wp-woocommerce PDP span markup (#462)', () => {
 		expect( call[ 1 ][ 0 ].item_id ).toBe( 456 );
 		expect( call[ 1 ][ 0 ].item_group_id ).toBe( 10 );
 	} );
+
+	// #190: the server prints data-gtm4wp_product_data="" when wp_json_encode()
+	// refuses the payload (a site filter supplying INF/NAN or over-deep nesting)
+	// and "null" when such a filter returns null. Both single-product paths must
+	// treat that as "no product data", exactly like the eight guarded
+	// gtm4wp_read_json_from_node call sites do - not push [false]/NaN or throw.
+	it( 'skips add_to_cart and returns false on an unencodable span payload (#190)', () => {
+		document.body.innerHTML =
+			'<form class="cart" method="post" action="https://shop/p42">' +
+			'<span class="gtm4wp_single_productdata" style="display:none"></span>' +
+			'<input type="number" name="quantity" value="3" />' +
+			'<button type="button" class="single_add_to_cart_button">Add</button>' +
+			'</form>';
+		document
+			.querySelector( '.gtm4wp_single_productdata' )
+			.setAttribute( 'data-gtm4wp_product_data', '' );
+
+		// The describe-level stub throws on invalid JSON; the REAL helper
+		// (gtm4wp-ecommerce-generic.js) wraps JSON.parse and returns false for
+		// a throw AND for a falsy parse. Model the real collaborator here, or
+		// this case tests the stub instead of the tracker (TC-15 / UC-3).
+		global.gtm4wp_read_from_json = ( json ) => {
+			try {
+				const parsed = JSON.parse( json );
+				if ( parsed ) {
+					delete parsed.productlink;
+					return parsed;
+				}
+			} catch ( e ) {}
+			return false;
+		};
+
+		boot();
+		const tracked = window.gtm4wp_track_single_add_to_cart(
+			document.querySelector( '.single_add_to_cart_button' ),
+			document.querySelector( 'form.cart' )
+		);
+
+		expect( tracked ).toBe( false );
+		expect(
+			global.gtm4wp_push_ecommerce.mock.calls.find(
+				( c ) => c[ 0 ] === 'add_to_cart'
+			)
+		).toBeUndefined();
+	} );
+
+	it( 'ignores a "null" span payload in the found_variation handler (#190)', () => {
+		document.body.innerHTML =
+			'<form class="cart variations_form">' +
+			'<span class="gtm4wp_single_productdata" style="display:none"></span>' +
+			'</form>';
+		document
+			.querySelector( '.gtm4wp_single_productdata' )
+			.setAttribute( 'data-gtm4wp_product_data', 'null' );
+
+		boot();
+		// Pre-#190 this threw: JSON.parse('null') passes the catch (it does not
+		// throw) and the first property access on null raised a TypeError inside
+		// the jQuery handler.
+		expect( () =>
+			handlers.found_variation(
+				{ target: document.querySelector( 'form' ) },
+				{
+					variation_id: 456,
+					sku: 'VAR-SKU',
+					display_price: 9.99,
+					attributes: { attribute_pa_color: 'blue' },
+				}
+			)
+		).not.toThrow();
+
+		expect(
+			global.gtm4wp_push_ecommerce.mock.calls.find(
+				( c ) => c[ 0 ] === 'view_item'
+			)
+		).toBeUndefined();
+	} );
 } );
 
 describe( 'gtm4wp-woocommerce exposed add_to_cart trackers (#273)', () => {