fix(recaptcha): bypass on Check Payments path in modal checkout

[chickenn00dle]

All Submissions:

• Have you followed the Newspack Contributing guideline?
• Does your code follow the WordPress' coding standards and VIP Go coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

When a reader chooses Check Payments in the modal checkout, the "Complete payment" button could freeze indefinitely on sites running reCAPTCHA v2 invisible. The cheque gateway has no client-side tokenization to serialize the submit, so the v2 widget could race with checkout fragment refreshes and never produce a token — leaving the form stuck.

This change bypasses reCAPTCHA on the cheque path in the modal checkout, both client-side (the widget is never executed) and server-side (token verification is skipped). Other gateways are unaffected and continue to be protected by reCAPTCHA. The cheque path remains low-risk: no charge is processed, orders are created on hold, and reader account creation/sign-in remain protected by their own captcha layers.

Closes NPPM-2619.

How to test the changes in this Pull Request:

1. On a site with reCAPTCHA v2 invisible enabled and the Check Payments WooCommerce gateway active, open a modal checkout for any product (donation, subscription, or one-time purchase).
2. Fill in billing details and proceed to the payment step.
3. Select Check Payments as the payment method. Inspect the form.checkout element in DevTools and confirm data-skip-recaptcha="1" is present.
4. Click Complete payment. The order should submit immediately and land on the thank-you screen with a new on-hold order in WooCommerce.
5. On the same modal, switch back to a card gateway (Stripe / WooPayments). Confirm data-skip-recaptcha is removed from the form.
6. Submit a card order and verify reCAPTCHA still fires (no regression for card paths).
7. Toggle "Edit billing details" mid-checkout. Confirm data-skip-recaptcha stays set during the edit step regardless of the previously selected payment method, then resolves to the correct value when returning to the payment step.
8. Outside modal checkout (standard /checkout/ page), submit a cheque order and confirm reCAPTCHA still fires — the bypass is scoped to modal checkout only.

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes, as applicable?
• Have you successfully ran tests with your changes locally?

[Copilot]

Pull request overview

This PR addresses a modal-checkout deadlock where reCAPTCHA v2 invisible can prevent submission when the Check Payments (cheque) gateway is selected, by bypassing reCAPTCHA execution/verification for that specific path in the modal checkout flow.

Changes:

• Add client-side logic to toggle data-skip-recaptcha="1" on form.checkout when payment_method=cheque is selected in modal checkout.
• Add server-side logic to skip Newspack reCAPTCHA verification for modal checkout submissions using the cheque gateway.
• Document/extend the existing “validation-only” behavior used during the modal’s billing-details edit step.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/modal-checkout/index.js	Toggles data-skip-recaptcha based on selected payment method (cheque vs others) in the modal checkout UI.
includes/class-modal-checkout.php	Skips reCAPTCHA verification for modal checkout validation-only requests and for modal cheque submissions.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[adekbadek]

Two things need attention before merge: on v2 invisible the fix doesn't actually unblock the cheque submit (browser-verified end-to-end), and the server-side bypass opens an unprotected spam path on the cheque gateway. Details in the inlines.

[adekbadek]

Blocker: v2-invisible + cheque: data-skip-recaptcha makes Place order a dead button — On a site running v2 invisible reCAPTCHA (the NPPM-2619 environment), this attribute toggle doesn't unblock the cheque submit — it just changes the failure mode. The newspack-plugin reCAPTCHA setup renders a visible clone (#place_order_clone) and CSS-hides the real #place_order inside modal checkout (newspack-plugin/src/other-scripts/recaptcha/style.scss:29-36). The clone's click handler in newspack-plugin/src/other-scripts/recaptcha/index.js:96-107 calls e.preventDefault(); e.stopImmediatePropagation(); handleSubmit(e);. handleSubmit only triggers a submit path when reCAPTCHA needs to run — with data-skip-recaptcha=1 set, it takes the else branch (removes data-recaptcha-validated) and falls through. No grecaptcha.execute(), no form.submit(), no AJAX — just a click that registers nothing.

Verified structurally on this env:

1. wp eval over apply_filters( 'woocommerce_order_button_html', $html ) produced <button type="submit" ... id="place_order_clone" id="place_order" ...> (two id attrs — pre-existing duplication in Recaptcha::order_button_html).
2. Fed to a real HTML parser, the first id wins; document.getElementById('place_order_clone') resolves to the clone and the click handler attaches.
3. CSS hides #place_order so the user only sees the clone.
4. The clone's click handler then bails as described above when data-skip-recaptcha=1.

A fix needs to either (a) make the clone-click handler fall through to the default form-submit when data-skip-recaptcha is set; (b) skip rendering the v2 widget entirely for the cheque path (tear it down on payment_method_selected); or (c) submit the form explicitly from the JS toggle when cheque is chosen. Recommend (a) — keeps the rest of the contract intact and is the smallest change.

[chickenn00dle]

Addressed in ffe69aa. Added a capture-phase click listener on document for #place_order_clone that intercepts ahead of the clone's element-level listener when data-skip-recaptcha is set, calls stopImmediatePropagation, and force-submits via form.requestSubmit(). The bypass now actually unblocks the submit instead of just changing the failure mode.

[adekbadek]

Suggestion: Hardcoded 'cheque' — other tokenization-less gateways share the same root cause — The PR description identifies the root cause as "the cheque gateway has no client-side tokenization to serialize the submit" — but that property is shared by other offline gateways already in $supported_gateways (bacs, cod). A publisher using Direct bank transfer or Cash on delivery will hit the same v2-invisible race. Consider matching against a small allowlist (['cheque', 'bacs', 'cod']) mirrored in JS — or extracting a single source of truth (a RECAPTCHA_BYPASS_GATEWAYS constant exposed through the existing newspackBlocksModalCheckout localized object, plus a newspack_modal_checkout_recaptcha_bypass_gateways filter for custom offline gateways). That also keeps the PHP and JS halves from drifting silently if the gateway list ever changes.

[chickenn00dle]

Addressed in ffe69aa. Replaced the hardcoded 'cheque' with a bacs/cheque/cod allowlist exposed via a new newspack_blocks_modal_checkout_recaptcha_bypass_gateways filter, and mirrored it to JS through the existing newspackBlocksModalCheckout localized data (recaptcha_bypass_gateways). PHP is the single source of truth so the two halves can't drift.

[adekbadek]

Suggestion: Edit-mode detection by DOM probing is brittle — ! $form.find( '[name="is_validation_only"]' ).length infers edit-mode by probing the hidden input that setEditingDetails(true) injects. Works today, but couples this branch to an implementation detail of a function ~400 lines away. The form already triggers editing_details (line 577); listening for it and caching the state in a closure variable would be cleaner — and would remove the need for the explicit handlePaymentMethodSelect() re-invocation at line 575.

[chickenn00dle]

Addressed in ffe69aa. handlePaymentMethodSelect now defers to an isEditingDetails closure that's updated by a top-level editing_details event listener. The DOM probe for is_validation_only is gone, and the explicit handlePaymentMethodSelect() re-invocation inside setEditingDetails(false) was removed — the event listener re-applies the payment-method toggle on the edit→non-edit transition.

[adekbadek]

Suggestion: Function docblock is stale — The function's docblock above (/** Prevent reCAPTCHA from being verified for AJAX checkout (e.g. Apple Pay). */) no longer describes what it does — with this PR it also covers the validation-only billing edit step and the cheque path. Consider something like "Selectively bypass reCAPTCHA verification in the modal checkout for paths that can't safely serialize with the v2 widget".

[chickenn00dle]

Addressed in ffe69aa. Rewrote the docblock to describe what the function actually covers — validation-only billing edit, offline gateways with no client-side tokenization, and AJAX checkout — and called out the scoping (modal-only + authenticated reader) as the security invariant.

[adekbadek]

Blocker: Abuse vector: unauthenticated spam path via crafted POST — is_modal_checkout() is satisfied by any caller including modal_checkout=1 (a public marker, not a signed token), so a crafted POST to /checkout/ with modal_checkout=1 and payment_method=cheque bypasses reCAPTCHA entirely. No money moves and orders land on-hold, but with Reader Activation enabled each request can create a customer/reader record — an attacker can spam the users table with arbitrary emails, and the on-hold order queue with junk. The bypass intentionally disables reCAPTCHA on a gateway it was protecting, with no compensating control. Mitigations to consider: a per-IP rate limit on the cheque path, a signed nonce on the modal-checkout marker, or scoping the bypass to authenticated/logged-in readers when the underlying race only affects v2 invisible.

[chickenn00dle]

Addressed in ffe69aa. Gated all bypasses on is_user_logged_in(). Reader Activation auto-registers and logs in the reader on the billing step, so the real modal flow still hits the bypass; an unauthenticated crafted POST to /checkout/ with modal_checkout=1 + payment_method=cheque now falls through to the standard reCAPTCHA verification. The test added in 6 locks this invariant.

[adekbadek]

Suggestion: Small PHP unit test would lock the scoping invariant — The filter callback is pure and trivial to unit-test: set $_POST/$_REQUEST, call Modal_Checkout::recaptcha_verify_captcha( true, $url, 'checkout' ), assert the four cases — non-modal POST → unchanged; modal + is_validation_only → false; modal + payment_method=cheque → false; modal + any other gateway → unchanged. Cheap insurance on the scoping invariant promised in the inline security comment.

[chickenn00dle]

Addressed in ffe69aa. Added tests/test-modal-checkout-recaptcha.php covering: non-checkout context, non-modal request, unauthenticated modal, authenticated validation-only, each bypass gateway (cheque/bacs/cod) via dataProvider, a non-bypass gateway, modal with no bypass signals, and the filter customization path.

[adekbadek]

Nit: filter_input would let the phpcs:ignore go away — Other reads in this file (is_validation_only() at 1520, is_express_checkout() at 1687) use filter_input( INPUT_POST, ..., FILTER_SANITIZE_* ). Switching to filter_input( INPUT_POST, 'payment_method', FILTER_SANITIZE_FULL_SPECIAL_CHARS ) would let the phpcs:ignore WordPress.Security.NonceVerification.Missing come off (the rule doesn't fire on filter_input). Purely cosmetic.

[chickenn00dle]

Won't fix — kept the $_POST direct access with phpcs:ignore. filter_input(INPUT_POST, ...) returns null under PHP CLI (it reads from the original HTTP request, not the current $_POST), so switching would mean the bypass branch can't be exercised by the unit test added in 6. Left an inline comment with the rationale next to the phpcs:ignore. Nonce verification is owned by the WooCommerce checkout flow upstream of this filter.

[adekbadek]

Nit: Three overlapping listeners for handlePaymentMethodSelect — The handler is wired to input.change, document.payment_method_selected, and document.updated_checkout. payment_method_selected is the canonical Woo event and fires after Woo's own bookkeeping; the raw .change() binding is redundant on top of it. The handler is idempotent so this is harmless, but a one-line comment noting the deliberate triple-bind (or trimming the .change()) would help future readers.

[chickenn00dle]

Addressed in ffe69aa. Added a docblock on handlePaymentMethodSelect explaining why the triple-bind on input.change / payment_method_selected / updated_checkout is intentional (distinct firing scenarios; handler body is idempotent) — left as-is rather than trimming .change() to keep the initial-bind path explicit.

[chickenn00dle]

Thanks for the review @adekbadek!

I've addressed all the feedback and this should be ready for another look. That said, you make a good point about a potential abuse vector. I gated this to logged in only, but am wondering if maybe this approach is not the right way to go after all. WDYT?