Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Email copy's "To" -field is sanitized with sanitize_email() in save_post -hook #145

Open
ironland opened this issue Dec 13, 2018 · 2 comments
Assignees
Milestone

Comments

@ironland
Copy link
Contributor

ironland commented Dec 13, 2018

Shouldn't there be a chance to also use dynamic / placeholder values in the first field of the email copy that defines the recipient of the email? Now it gets sanitized as an email address. The affected code is in class-wplf-form.php on lines 789 - 813.

    // save email copy
    if ( isset( $_POST['wplf_email_copy_to'] ) ) {
      $email_field = $_POST['wplf_email_copy_to'];
      $to = '';

      if ( strpos( $email_field, ',' ) > 0 ) {
        // Intentional. Makes no sense if the first character is a comma, so pass it along as a single address.
        // sanitize_email() should take care of the rest.
        $email_array = explode( ',', $email_field );
        foreach ( $email_array as $email ) {
          $email = trim( $email );
          $email = sanitize_email( $email ) . ', ';
          $to .= $email;
        }
        $to = rtrim( $to, ', ' );
      } else {
        $to = sanitize_email( $email_field );
      }

      if ( ! empty( $to ) ) {
        update_post_meta( $post_id, '_wplf_email_copy_to', $to );
      } else {
        delete_post_meta( $post_id, '_wplf_email_copy_to' );
      }
    }

Even though the code above is in the plugin, in the file wplf-form-actions.php on line 62 there's a line of code that runs the replacing method to the "to" -field as well with no results, because the field is sanitized on post save. The affected code:

    // maybe replace template tags with real content
    $to = wplf_email_copy_replace_tags( $to, $form, $submission_id );
@timiwahalahti
Copy link
Member

I don't like the idea of allowing placeholders in $to field. It just leaves too many opportunities for the user to mess things up and break whole email sending. And we know that if the sending brokes, users get mad - even if it is their own fault.

There are three different filters for developers to use, if $to field really needs to be changed. Personally, I would count on developers to not mess things up. And if they do, it's somewhat better than that user itself has messed things.

So my suggestion is to remote the replacing method from $to field. It might cause a breaking change if somebody has forced to address in the database to contain placeholder. But then they can blame themselves, as it was never supported by the plugin.

Any objections?

@timiwahalahti timiwahalahti self-assigned this Jan 27, 2019
@k1sul1 k1sul1 self-assigned this Jul 27, 2020
@k1sul1
Copy link
Member

k1sul1 commented Jul 27, 2020

Easily implementable in 2.0, which is still coming. Just had some major hold ups and side projects come second.

https://github.com/libreform/wp-libre-form/blob/2.0/classes/class-plugin.php#L345-L351
https://github.com/libreform/wp-libre-form/blob/2.0/lib/helpers.php#L28-L47

Even if it was never supported by the plugin, it still looks like it's supported. Why would the placeholders be supported in all but one field? I'd use a placeholder in this particular case but I can't. That forces me to make another submission handler which I don't like.

So I'll fix that in 2.0. Which is getting finished as soon as I get SOME free time.

@timiwahalahti timiwahalahti removed their assignment Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants