Fixes #6141 - Warnings when print_r() is disabled for security reasons by jimmyandrade · Pull Request #6176 · Automattic/jetpack

jimmyandrade · 2017-01-26T14:10:29Z

Fixes #6141 - Warnings when print_r() is disabled for security reasons

Changes proposed in this Pull Request:

Now, before using print_r() to print $all_values data on Feedback post content, Jetpack will check if print_r function was not disabled.

Fixes a warning when a visitor submit a contact form in a website hosted with print_r disabled. (Jetpack Contact Form uses print_r to store all values in Feedback post content so that search will pick up this data).

georgestephanis

Honestly, this is way too complicated of a solution for a rare server configuration. If anything, I'd suggest possibly doing something like ...

$all_values_string = apply_filters( 'grunion_print_r_all_values', @print_r( $all_values, true ), $all_values );

and then dropping $all_values into the line below instead. The @ should silence any warnings you may get, and the filter would let you rebuild the string for searchability manually as the code is intended to behave.

But in either case, I'm not sure if it's a good idea for us to set a precedent of working around disabling basic functions of a language.

georgestephanis · 2017-01-26T16:51:00Z

modules/contact-form/grunion-contact-form.php

+		}
+
+		$post_content_string =  $comment_content . "\n<!--more-->\n" . "AUTHOR: {$comment_author}\n
+		AUTHOR EMAIL: {$comment_author_email}\n


The prior string used to insert just a \n between lines, the new code does that and then also does an actual new line (I've not examined it closely enough to identify if it's \r\n or just another \n) followed by tabs. This would change it so that the formatting is not consistent with the prior entries. :\

While it is more readable in code, it's less reliable for any code trying to parse out the content (not the best way to do things, but the things I've seen ...)

Another thing I had not paid attention to.
To keep the code readable while avoiding inconsistency in formatting, I switched to:

$post_content_string = $comment_content . "\n\n" . "AUTHOR: {$comment_author}\n" . "AUTHOR EMAIL: {$comment_author_email}\n" . "AUTHOR URL: {$comment_author_url}\n" . "SUBJECT: {$subject}\n" . "IP: {$comment_author_IP}\n" . $all_values_string;

georgestephanis · 2017-01-26T16:52:15Z

modules/contact-form/grunion-contact-form.php

 		 * with this work around. */
 		add_filter( 'wp_insert_post_data', array( $plugin, 'insert_feedback_filter' ), 10, 2 );

+		$disabled_functions = explode( ',', ini_get( 'disable_functions' ) );


Not the best way to do this. Any item in disable_functions can be detected as disabled by testing it against function_exists() as per https://secure.php.net/manual/en/function.function-exists.php#67947

We won't be able to redefine it, but it's a tidier way to check.

Hey @georgestephanis. Thanks for all the feedback.

Firstly, about "rare server configuration". As I said before, Locaweb is one of the most common hosting providers in Brazil, and some of accounts are configured with print_r disabled. It is possible that other people will experience this same problem.

Regarding the use of the filter, it is really a better strategy...

$all_values_string = apply_filters( 'grunion_print_all_values', @print_r( $all_values, true ), $all_values );

I did not really know that function_exists detected disabled functions, so because of your hint I've switched to:

if ( function_exists( 'print_r' ) ) { // ... }

georgestephanis · 2017-01-26T16:54:10Z

modules/contact-form/grunion-contact-form.php

+			$all_values_string = print_r( $all_values, true );
+		}
+		// If print_r is disabled, try to use var_export as an alternative solution
+		elseif ( ! in_array( 'var_export', $disabled_functions ) ) {


As per WP Coding Style, conditionals should be on the same line as the prior closing brace. So this line should be:

} elseif ( ! in_array( 'var_export', $disabled_functions ) ) {

with the comment either at the end of the line, or below and indented.

Thank you! I had not realized that.

georgestephanis · 2017-01-26T16:55:27Z

modules/contact-form/grunion-contact-form.php

+			$all_values_string = var_export( $all_values, true );
+		}
+		else {
+			$all_values_string = '';


If neither print_r or var_export are available, shouldn't this manually iterate through and build the string itself for searchability, rather than just setting it to empty?

It is true. So, I wrote something like this:

$all_values_string = "Array\n(\n"; foreach ( $all_values as $key => $value ) { $all_values_string .= "\t[$key] => $value\n"; } $all_values_string .= ')';

* Adds the @ to silence any warnings on print_r or var_export; * Adds a filter that let developers rebuild Feedback post string for searchability manually as the code is intended to behave; * Using function_exists() instead of ini_get( 'disable_functions' ) to detect if a function has been disabled; * Fixing WP Coding Style issues about conditionals and closing braces; * Added a way to iterate through Feedback data array when print_r and var_export are disabled; * Fixing an issue that could cause Feedback data formatting being not consistent, but maintaining the code readability.

jeherve

Thanks for taking the time to apply those changes! I have a few remarks that I added inline below. I hope this helps!

I also wanted to address your concern:

Firstly, about "rare server configuration". As I said before, Locaweb is one of the most common hosting providers in Brazil, and some of accounts are configured with print_r disabled. It is possible that other people will experience this same problem.

This makes me wonder if you're on an old server that hasn't been updated. The Contact Form module was introduced in Jetpack more than 4 years ago, and was available as a stand-alone plugin before that. Yet that's the first report we've had about this bug.

It might be worth asking your hosting provider if they would consider moving you to a different server where print_r isn't disabled.

modules/contact-form/grunion-contact-form.php

jeherve · 2017-01-27T11:51:03Z

modules/contact-form/grunion-contact-form.php

+		// Avoid warnings when print_r() has been disabled on hosting provider for security reasons
+		// function_exists will return false for functions disabled with the disable_functions ini directive
+		if ( function_exists( 'print_r' ) ) {
+			$all_values_string = apply_filters( 'grunion_print_all_values', @print_r( $all_values, true ), $all_values );


Could you add a docblock before the new filter? Here is an example:
https://github.com/jimmyandrade/jetpack/blob/fa1c63e9ee9b0a2d21a0855e5f582574888554be/modules/contact-form/grunion-contact-form.php#L526-L535

You can learn more about Hook documentation standards here:
https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#4-hooks-actions-and-filters

Yes, I did that. Thank you so much.

georgestephanis · 2017-01-27T15:41:10Z

Honestly, I'm not sure if the filter is even necessary. Couldn't folks just use a core filter on insert posts of the respective post type to append the fields? I'm unsure offhand if there'd a scope concern there.

jimmyandrade · 2017-01-27T16:59:24Z

@georgestephanis, I confess that I do not know the WP core filters very well. Just like you, I'm not sure either. Well... I decided to keep this filter because it might be that someone wants to customize $all_values for some reason... (I hope that's not a problem)

* Now, function_exists is being used in a more expressive way; * When both the print_r and var_export functions are disabled, printing of the variables will be in charge of the filter grunion_print_all_values; * Added a docblock before the new grunion_print_all_values filter.

jimmyandrade · 2017-01-30T16:32:30Z

Awaiting review after the latest proposed changes...

Fixes #6141 #6176

georgestephanis · 2017-01-30T19:05:51Z

@jimmyandrade Can you give #6234 a shot and see if that resolves your issue please?

jeherve · 2017-01-30T21:00:59Z

Replaced by #6234

Jimmy Andrade added 2 commits January 26, 2017 12:06

Fixes #6141: alternative solution for print_r()

3e3bab1

Fixes a warning when a visitor submit a contact form in a website hosted with print_r disabled. (Jetpack Contact Form uses print_r to store all values in Feedback post content so that search will pick up this data).

Fixes a syntax error near $post_content

a53c990

jeherve added [Feature] Forms [Pri] Low [Status] Needs Review This PR is ready for review. Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Jan 26, 2017

jeherve added this to the 2/17 - February milestone Jan 26, 2017

Fixes a syntax issue on 'post_content'

7162798

georgestephanis suggested changes Jan 26, 2017

View reviewed changes

jeherve added [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Jan 26, 2017

jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jan 27, 2017

jeherve requested changes Jan 27, 2017

View reviewed changes

jeherve added [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Jan 27, 2017

jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jan 27, 2017

georgestephanis added a commit that referenced this pull request Jan 30, 2017

Don't cause warnings if server has print_r() disabled.

8f23629

Fixes #6141 #6176

georgestephanis mentioned this pull request Jan 30, 2017

Don't cause warnings if server has print_r() disabled. #6234

Merged

jeherve closed this Jan 30, 2017

jeherve modified the milestones: 2/17 - February, Not Currently Planned Jan 30, 2017

kraftbj removed the [Status] Needs Review This PR is ready for review. label Sep 29, 2021

matticbot added the Touches WP.com Files label Sep 29, 2021

Conversation

jimmyandrade commented Jan 26, 2017

Changes proposed in this Pull Request:

Uh oh!

georgestephanis left a comment • 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!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgestephanis commented Jan 27, 2017

Uh oh!

jimmyandrade commented Jan 27, 2017

Uh oh!

jimmyandrade commented Jan 30, 2017

Uh oh!

georgestephanis commented Jan 30, 2017

Uh oh!

jeherve commented Jan 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

georgestephanis left a comment •

edited

Loading