CRM: Improve string sanitization and escaping by tbradsha · Pull Request #48390 · Automattic/jetpack

tbradsha · 2026-04-29T17:56:41Z

Proposed changes

This improves string sanitization and escaping.

Related product discussion/links

pgvhYc-BV-p2#comment-666
pgvhYc-BV-p2#comment-667

Does this pull request change what data or activity we track or use?

Testing instructions

Go to '..'

github-actions · 2026-04-29T17:57:53Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
You can use GitHub's Reviewers functionality to request a review.
When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

jp-launch-control · 2026-04-29T18:00:39Z

Code Coverage Summary

Coverage changed in 6 files. Only the first 5 are listed here.

File	Coverage	Δ%	Δ Uncovered
projects/plugins/crm/modules/woo-sync/includes/class-woo-sync-my-account-integration.php	0/290 (0.00%)	0.00%	2 ❤️‍🩹
projects/plugins/crm/admin/settings/client-portal.page.php	0/230 (0.00%)	0.00%	1 ❤️‍🩹
projects/plugins/crm/admin/settings/locale.page.php	0/209 (0.00%)	0.00%	1 ❤️‍🩹
projects/plugins/crm/admin/settings/mail.page.php	0/123 (0.00%)	0.00%	1 ❤️‍🩹
projects/plugins/crm/modules/portal/class-client-portal-render-helper.php	0/58 (0.00%)	0.00%	-1 💚

Full summary · PHP report

anomiex · 2026-04-30T16:27:54Z

 	if ( SHOW_FUNNEL_LEGEND ) {
 		funnel_html += '<br>' + legend_html;
 	}
 	funnel_element.innerHTML = funnel_html + '<br>';


Even better would be to construct the contents using DOM functions instead of assigning innerHTML. But the above seems to cover everything.

Personally I prefer innerHTML (less verbose and more performant), but a refactor would be out of scope here.

anomiex · 2026-04-30T16:30:06Z

@@ -767,7 +767,7 @@ function zeroBSCRMJS_validateSettings() {
 											resHTML =
 												window.zeroBSCRMJS_globViewLang( 'settingsValidatedSMTP' ) +


Should this be escaped too?

anomiex · 2026-04-30T16:32:31Z

+						'<div class="email-avatar avatar-' +
+							jpcrm.esc_attr( v.in_or_out ) +
+							'">' +
+							v.avatar +


Is v.avatar sanitized server-side too?

anomiex · 2026-04-30T16:34:52Z

-		billTo +
+		jpcrm.esc_attr( billTo ) +
 		'" placeholder="' +
 		zbscrm_JS_invoice_lang( 'bill_to' ) +


Should this be escaped too? If so, I see a lot of them in this file.

anomiex · 2026-04-30T16:44:39Z

 					break;
 				case 'email':
-					$new_contact_data['email'] = $v;
+					$new_contact_data['email'] = sanitize_text_field( $v );


If you're going to use WordPress's "sanitize" functions, this should probably be

Suggested change

$new_contact_data['email'] = sanitize_text_field( $v );

$new_contact_data['email'] = sanitize_email( $v );

Same everywhere else you process email addresses.

And address surrounding `phpcs:ignore` comments

Skipping any that require more attention

anomiex · 2026-05-01T16:57:16Z

-				title: window.zeroBSCRMJS_globViewLang( 'deleteMailDeliverySureTitle' ),
-				text: window.zeroBSCRMJS_globViewLang( 'deleteMailDeliverySureText' ),
+				title: jpcrm.esc_html( window.zeroBSCRMJS_globViewLang( 'deleteMailDeliverySureTitle' ) ),
+				text: jpcrm.esc_html( window.zeroBSCRMJS_globViewLang( 'deleteMailDeliverySureText' ) ),


Looks like using text rather than html doesn't need escaping.

Looks like there's also titleText to use instead of title to do the same for that field.

Looks like using text rather than html doesn't need escaping.

Can you clarify what you mean by that? If you mean jQuery's text(), I did switch to that several places in this branch.

Oh, in the SweetAlert params, I suppose.

This is about the sweetalert2 things (function swal()). https://sweetalert2.github.io/#configuration

348b38e and 70bad70

anomiex · 2026-05-01T17:15:52Z

 				confirmButtonColor: '#3085d6',
 				cancelButtonColor: '#d33',
-				confirmButtonText: window.zeroBSCRMJS_globViewLang( 'deleteMailDeliverySureConfirm' ),
+				confirmButtonText: jpcrm.esc_html(


Oddly, despite the name, confirmButtonText appears to take HTML and so still probably needs escaping.

348b38e and 70bad70

anomiex · 2026-05-04T17:53:45Z

 			e.preventDefault();
 			var image = wp.media({
-				title: '<?php esc_html_e( 'Upload Image', 'zero-bs-crm' ); ?>',
+				titleText: '<?php esc_html_e( 'Upload Image', 'zero-bs-crm' ); ?>',


This should probably drop the esc_html then. And really, probably switch to json_encode() for outputting the string to JS.

OTOH, in practice it probably doesn't much matter, because "Upload Image" or any reasonable translation is unlikely to wind up with characters that get encoded as HTML entities anyway. I see some below that were already passing esc_html to text: and no one has noticed.

Actually, I introduced a bug here (it's wp.media(), not swal()). Fixed (along with escape) in a58bfab.

Properly escaped swal() instances here: 644ef0f

anomiex · 2026-05-04T17:57:22Z

-		);
+		swal( {
+			titleText: 'Twilio Extension Needed!',
+			text: 'To SMS your contacts you need the <a target="_blank" style="font-weight:900;text-decoration:underline;" href="https://jetpackcrm.com/extension-bundles/">Twilio extension</a> (included in our Entrepreneurs Bundle)',


This one would need to stay as html, there's an HTML link in there.

On the plus side, this is the only one I saw like this. 🙂

Oops, not sure how I missed that one. It's a literal string, so should be safe to leave as unescaped at this stage: 38367c7

anomiex · 2026-05-04T18:01:50Z

-		html: '<div>' + zeroBSCRMJS_listViewLang( 'acceptareyousurequotes' ) + '</div>',
+		titleText: zeroBSCRMJS_listViewLang( 'areyousure' ),
+		html:
+			'<div>' + jpcrm.esc_html( zeroBSCRMJS_listViewLang( 'acceptareyousurequotes' ) ) + '</div>',


I wonder why this wraps it in a div while most others don't.

anomiex

Looks reasonable; I'm not too worried about the other two comments when there's so much existing debt already.

anomiex

Quick, let's merge before we see more stuff that needs fixing! 😅

tbradsha added 4 commits April 29, 2026 11:46

Escape JS output

2fb111b

Better sanitization/escaping

0c384eb

Add changelog

379283b

Linting

2d57f37

tbradsha requested a review from a team April 29, 2026 17:56

tbradsha self-assigned this Apr 29, 2026

tbradsha added the [Status] Needs Review This PR is ready for review. label Apr 29, 2026

github-actions Bot added [CRM] Givewp Module [CRM] MailPoet Module [CRM] Portal Module [CRM] WooSync Module [Plugin] CRM Issues about the Jetpack CRM plugin labels Apr 29, 2026

PHPCS

f941e59

anomiex reviewed Apr 30, 2026

View reviewed changes

tbradsha added 5 commits April 30, 2026 16:37

Escape translated text

ceaaa24

Escape contact avatar output

a75e9df

Escape zbscrm_JS_invoice_lang() calls

f926d15

Use sanitize_email() instead of sanitize_text_field() when relevant

ec04b5f

And address surrounding `phpcs:ignore` comments

Simplify redundant (int) sanitize_text_field() calls

1d01ee7

Skipping any that require more attention

github-actions Bot added [CRM] API Admin Page React-powered dashboard under the Jetpack menu labels May 1, 2026

tbradsha added 3 commits April 30, 2026 19:05

Merge branch 'trunk' into fix/crm/harden_admin_rendering

329f2e3

Make Phan and PHPCS happy

d85ae53

Update Phan baseline

4ecdca1

anomiex reviewed May 1, 2026

View reviewed changes

tbradsha added 3 commits May 1, 2026 13:22

Swal cleanup, part 1

348b38e

A few more casualties

34c4b30

Swal cleanup, part 2

70bad70

tbradsha added 2 commits May 1, 2026 14:21

Merge branch 'trunk' into fix/crm/harden_admin_rendering

a104fdd

Update Phan baseline

734d369

anomiex reviewed May 4, 2026

View reviewed changes

tbradsha added 2 commits May 4, 2026 12:11

Oops, this has HTML

38367c7

Undo wp.media() mistake and escape properly

a58bfab

anomiex previously approved these changes May 4, 2026

View reviewed changes

esc_html() and zeroBSCRM_slashOut() → wp_json_encode()

644ef0f

tbradsha dismissed anomiex’s stale review via 644ef0f May 4, 2026 18:34

anomiex approved these changes May 4, 2026

View reviewed changes

tbradsha merged commit e49a400 into trunk May 4, 2026
72 checks passed

tbradsha deleted the fix/crm/harden_admin_rendering branch May 4, 2026 18:47

github-actions Bot removed the [Status] Needs Review This PR is ready for review. label May 4, 2026

anomiex mentioned this pull request May 4, 2026

CRM: Use proper string escape functions #48496

Open

		@@ -767,7 +767,7 @@ function zeroBSCRMJS_validateSettings() {
		resHTML =
		window.zeroBSCRMJS_globViewLang( 'settingsValidatedSMTP' ) +

	$new_contact_data['email'] = sanitize_text_field( $v );
	$new_contact_data['email'] = sanitize_email( $v );

Conversation

tbradsha commented Apr 29, 2026

Proposed changes

Related product discussion/links

Does this pull request change what data or activity we track or use?

Testing instructions

Uh oh!

github-actions Bot commented Apr 29, 2026

Uh oh!

jp-launch-control Bot commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Summary

Uh oh!

anomiex Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anomiex left a comment

Choose a reason for hiding this comment

Uh oh!

anomiex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jp-launch-control Bot commented Apr 29, 2026 •

edited

Loading

anomiex Apr 30, 2026 •

edited

Loading