Clean usage data before sending#2653
Conversation
📝 WalkthroughWalkthroughsend_snapshot() now builds a local snapshot, runs clean_before_send(&$data) to remove/mask sensitive and long-text fields, logs the sanitized snapshot, and sends it. snapshot() no longer includes admin_email. fields() and actions() preserve decoded/unserialized data instead of re-encoding. Changes
Sequence Diagram(s)sequenceDiagram
participant FrmUsage as FrmUsage (model)
participant SnapshotGen as snapshot()
participant Sanitizer as clean_before_send()
participant Remote as External API
FrmUsage->>SnapshotGen: build snapshot()
SnapshotGen-->>FrmUsage: snapshot data
FrmUsage->>Sanitizer: clean_before_send(snapshot)
Sanitizer-->>FrmUsage: sanitized snapshot
FrmUsage->>Remote: send JSON-encoded sanitized snapshot
Remote-->>FrmUsage: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/models/FrmUsage.php (1)
16-40: Remove debug logging and harden JSON/body creation + request error handling.
Line 23error_log( print_r( $snapshot, true ) );shouldn’t ship (log noise + still potentially sensitive even after sanitization). Also consider usingwp_json_encode+ bailing on encoding failure, and handlingwp_remote_requesterrors.$snapshot = $this->snapshot(); $this->clean_before_send( $snapshot ); - error_log( print_r( $snapshot, true ) ); $ep = 'aHR0cHM6Ly91c2FnZTIuZm9ybWlkYWJsZWZvcm1zLmNvbS9zbmFwc2hvdA=='; // $ep = base64_encode( 'http://localhost:4567/snapshot' ); // Uncomment for testing - $body = json_encode( $snapshot ); + $body = wp_json_encode( $snapshot ); + if ( false === $body ) { + return; + }Optionally capture/inspect the response:
- wp_remote_request( base64_decode( $ep ), $post ); + $response = wp_remote_request( base64_decode( $ep ), $post ); + // Optionally handle is_wp_error( $response ) here.
🧹 Nitpick comments (2)
classes/models/FrmUsage.php (2)
576-596: actions(): good move to maybe_json_decode settings; consider normalizing non-array fallbacks.
If$action->post_contentisn’t valid JSON,maybe_json_decodereturns the original string; downstream sanitization expects arrays in some places. Consider forcing an array fallback:- 'settings' => FrmAppHelper::maybe_json_decode( $action->post_content ), + 'settings' => (array) FrmAppHelper::maybe_json_decode( $action->post_content ),
556-569: fields(): remove unnecessary reassignment after unserialize_or_decodeLine 565 passes
$field->field_optionsby reference tounserialize_or_decode(), which mutates it in-place and returns void. Line 566's reassignment is redundant. Simplify to:- foreach ( $fields as $k => $field ) { - FrmAppHelper::unserialize_or_decode( $field->field_options ); - $fields[ $k ]->field_options = $field->field_options; + foreach ( $fields as $k => $field ) { + FrmAppHelper::unserialize_or_decode( $fields[ $k ]->field_options );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmUsage.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmUsage.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5084)maybe_json_decode(3720-3741)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Cypress
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
classes/models/FrmUsage.php (1)
118-223:clean_before_send()still leaks sensitive fields, mishandles long text, and can fatally error on objects (PHP 8+)Most of the issues called out in the earlier review are still present in this version:
Skip keys and
_urlkeys are not actually removed.
- In the loop you
continuewhenin_array( $key, $skip_keys, true )or when the key ends with_url, but you neverunset()those entries.- As a result, highly sensitive fields like
api_key,business_email,account_num,routing_num,url, and*_urlkeys are still present in$dataand will be sent in the payload, which contradicts the PR description (“remove keys that end with_url” and “remove several other keys before sending”).- These should be removed from the array, not just skipped for further processing.
description(and some_msgfields) never get the{{long_text}}placeholder.
descriptionis in both$skip_keysand$long_text_keys. Because the skip check runs first,descriptionvalues are left untouched.- Similarly, some
_msgkeys (e.g.success_msg,redirect_msg,reg_email_msg,unique_msg) are also in$skip_keys, so they never hit the long-text branch either.- This violates the stated behavior of replacing long text (e.g.
descriptionand_msgfields) with{{long_text}}.Objects in the snapshot (e.g. from
fields()) are not handled and will cause a PHP 8+TypeError.
snapshot()['fields']is now an array ofstdClassrows, each withfield_optionsas an array. In the recursive call on$snapshot['fields'], the innerforeachsees each row as an object.- For those entries:
is_array( $value )is false, so recursion stops.- Later,
strpos( $value, '@' )is called with$valueasstdClass, which raisesTypeError: strpos(): Argument #1 ($haystack) must be of type string, stdClass givenon PHP 8+.- That’s a hard fatal, and it also means you never sanitize nested
field_optionsat all.
strpos( $value, '@' )is unsafe without a type check.
$valuecan be non-string (arrays, objects, integers, booleans, etc.) across the snapshot structure.- On PHP 8+, calling
strpos()on a non-string haystack throws aTypeError. This must be guarded withis_string( $value ).The function assumes
$datais always an array.
- Filters like
frm_usage_snapshot,frm_usage_skip_keys, andfrm_usage_long_text_keyscan change structure. A defensiveif ( ! is_array( $data ) ) { return; }at the top makes this robust.Docblock nits.
@since x.xshould be replaced with a real version before release.@paramm array $long_text_keysshould be@param array $long_text_keys.To address all of the above, I recommend restructuring the sanitizer roughly like this:
private function clean_before_send( &$data ) { - $skip_keys = array( + if ( ! is_array( $data ) ) { + return; + } + + $skip_keys = array( // ... - // Form settings. - 'form_id', - 'description', + // Form settings. + 'form_id', // ... ); @@ - foreach ( $data as $key => &$value ) { - if ( ! $value || in_array( $key, $skip_keys, true ) || '_url' === substr( $key, -4 ) ) { - continue; - } - - if ( is_array( $value ) ) { - $this->clean_before_send( $value ); - continue; - } - - if ( in_array( $key, $long_text_keys, true ) || '_msg' === substr( $key, -4 ) ) { - // Replace it with a placeholder. - $value = '{{long_text}}'; - continue; - } - - // If key ends with `_email`, or value contains `@`, this might contain an email address. - if ( '_email' === substr( $key, -6 ) || false !== strpos( $value, '@' ) ) { - // Replace it with a placeholder. - $value = '{{contain_email}}'; - } - }//end foreach + foreach ( $data as $key => &$value ) { + $key_str = (string) $key; + + // 1) Remove sensitive keys entirely (including *_url). + if ( in_array( $key_str, $skip_keys, true ) || '_url' === substr( $key_str, -4 ) ) { + unset( $data[ $key ] ); + continue; + } + + // 2) Recurse into arrays. + if ( is_array( $value ) ) { + $this->clean_before_send( $value ); + continue; + } + + // 3) Recurse into objects (e.g. stdClass rows from DB). + if ( is_object( $value ) ) { + $props = get_object_vars( $value ); + $this->clean_before_send( $props ); + foreach ( $props as $prop_name => $prop_value ) { + $value->$prop_name = $prop_value; + } + continue; + } + + // 4) Skip empty scalars. + if ( '' === $value || null === $value ) { + continue; + } + + // 5) Long-text placeholders (eg description, *_msg). + if ( in_array( $key_str, $long_text_keys, true ) || '_msg' === substr( $key_str, -4 ) ) { + $value = '{{long_text}}'; + continue; + } + + // 6) Email-like content → placeholder. + if ( '_email' === substr( $key_str, -6 ) || ( is_string( $value ) && false !== strpos( $value, '@' ) ) ) { + $value = '{{contain_email}}'; + } + } + unset( $value ); }Key points of this change:
- Sensitive keys and
_urlsuffixes are actually removed from the outgoing data.descriptionis no longer skipped entirely and will be replaced with{{long_text}}via$long_text_keys.- Objects (like field rows) are safely sanitized without causing
TypeErroron PHP 8+.strpos()is guarded withis_string().Please verify this behavior under PHP 8+ by running usage tracking in an environment with forms and actions that exercise nested
fields/actionsdata to confirm there are no TypeErrors and thatapi_key, emails, and_urlkeys are absent from the final payload.
🧹 Nitpick comments (1)
classes/models/FrmUsage.php (1)
21-28: Remove or conditionally guarderror_log( print_r( $snapshot, true ) )insend_snapshot()Logging the (supposedly sanitized) snapshot on every send will spam PHP error logs and, given the current sanitizer gaps, can still leak sensitive data into server logs. This should either be removed or wrapped in a debug guard (e.g. behind
WP_DEBUGor a dedicated logging helper) before shipping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmUsage.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmUsage.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5084)maybe_json_decode(3720-3741)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (2)
classes/models/FrmUsage.php (2)
562-569:fields()change to keep decodedfield_optionslooks good, but depends on sanitizer handling objectsReturning
field_optionsas already-unserialized/decoded data on eachstdClassrow is a good move and aligns with the goal of sending arrays instead of JSON strings. However, note thatsnapshot()['fields']is now an array of objects with nested arrays, so the effectiveness of PII/long-text scrubbing depends onclean_before_send()correctly recursing into those objects (see previous comment). Onceclean_before_send()supports objects, this change is solid.
584-592: UsingFrmAppHelper::maybe_json_decode()for actionsettingsis appropriateDecoding
post_contentviaFrmAppHelper::maybe_json_decode()so thatsettingsis an array (when JSON) is consistent with the new “send arrays instead of JSON” approach and makes it possible forclean_before_send()to traverse and scrub action settings. Just ensure the external usage endpoint has been updated to expect an array here (per your PR description), which it sounds like you’ve already coordinated.Please double-check end-to-end that the updated usage service correctly handles
settingsas an array and no longer expects JSON strings.
Clean usage data before sending
2 participants
Related issue: https://github.com/Strategy11/formidable-pro/issues/6112
This PR does:
@or key ends with_emailwill be replaced by{{contain_email}}.descriptionand key ends with_msg) is replaced by{{long_text}}. We clean more of them at the usage site._urlare removed.This requires a change in the usage site to support the array instead of the old JSON data: https://github.com/Strategy11/ff-usage-2/pull/6
Summary by CodeRabbit
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.