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

Support for encrypted file attachments to outgoing email messages #4

Open
fabacab opened this issue Feb 20, 2016 · 11 comments
Open

Support for encrypted file attachments to outgoing email messages #4

fabacab opened this issue Feb 20, 2016 · 11 comments

Comments

@fabacab
Copy link
Owner

fabacab commented Feb 20, 2016

This will require implementing RFC 3156.

See also singpolyma/openpgp-php#21.

/via https://wordpress.org/support/topic/encrypt-e-mail-attachments?replies=1

@DanielRuf
Copy link

PHPMailer should be sufficient for this or the core functions of WordPress.

But for mail attachments PHPMailer is strongly recommended. Also see http:https://stackoverflow.com/questions/12301358/send-attachments-with-php-mail

Implementing this with mail() is very hard and error prone.

I have an example code (which uses php-gpg but will possibly migrate it to openpgp-php in the future) there: https://github.com/DanielRuf/honeypot-scripts/blob/7a8b0daf5d57f27ab9ab164e6672af490d07b472/upload_mail.php#L30-L41

Theoretically just one line to add a file attachment to a mail.

Generally this works with both libraries, the old one and the new one, you just have to encrypt the file as string and use the appropriate PHPMailer function.

@fabacab
Copy link
Owner Author

fabacab commented Feb 20, 2016

WordPress uses PHPMailer under the hood since version 2.2, so perhaps there is already an API available for this. If so, and if it is indeed a simple matter of attaching an encrypted string to an outgoing email, this should not be too hard. The harder part would be figuring out how to provide a consistent public UX for end users and developers to make use of the encryption feature, but that is a problem I actually know enough to solve.

I will have a closer look this weekend at your example code, PHPMailer, and how WordPress uses it to see if integration is feasible.

Thank you for the suggestions!

@DanielRuf
Copy link

UX is definitely a challenge which should be solved. When the UX is good, more people will use it.

Fid not take a look at the exact API methods of WordPress for sending mails with attachments but theoretically this should be very simple.

I will take a look at these specific API methods later.

@fabacab
Copy link
Owner Author

fabacab commented Feb 21, 2016

Oops, did not mean to close this from the commit. My bad.

@fabacab fabacab reopened this Feb 21, 2016
@DanielRuf
Copy link

DanielRuf commented Feb 21, 2016

Sorry, forgot that. Seems to be quite easy.

You just have to filter it and process the 5th parameter ((array)attachments) with the encryption function.

$attachments should be an array. If it is not, it will be converted to one by the wp_mail function after the filter.

https://developer.wordpress.org/reference/functions/wp_mail/
https://codex.wordpress.org/Plugin_API/Filter_Reference/wp_mail

So you just have to iterate over the $args['attachments'] array

public static function wp_mail ($args) {

The attachments argument contains the path to the file, so you can read the file to a string, encrypt it and use this with PHPMailer::AddStringAttachment()

http:https://wp-a2z.com/oik_api/phpmaileraddstringattachment/

$attachments = array( WP_CONTENT_DIR . '/uploads/file_to_attach.zip' );

http:https://stackoverflow.com/questions/11164167/phpmailer-attachment-doing-it-without-a-physical-file

https://core.trac.wordpress.org/browser/tags/4.4.2/src/wp-includes/pluggable.php#L235

if ( !empty( $attachments ) ) {
        foreach ( $attachments as $attachment ) {
            try {
                $phpmailer->AddAttachment($attachment);
            } catch ( phpmailerException $e ) {
                continue;
            }
        }
    }

...
$phpmailer->ClearAttachments();

@fabacab
Copy link
Owner Author

fabacab commented Feb 21, 2016

If I understand the WordPress support forum post you linked to correctly, it suggests modifying WordPress core functions? This is not acceptable for a plugin to do. Also, the wp_mail() function is "pluggable" and so I can outright replace it, but the whole concept of pluggable functions has been deprecated for a long time in WordPress, which is why I used the wp_mail filter hook instead.

Unfortunately, a filter hook isn't enough here if we need to call PHPMailer::addStringAttachment(). I'm not sure WordPress has any way to do this.

There is also a WordPress Core issue citing this precise scenario as a limitation, see ticket:29513 on the WP core Trac:

If a plug-in is sending an e-mail, the class PHPMailer has a lot of useful methods (e.g. addStringAttachment()), but these are not available when using wp_mail(), which is a requirement to work with numerous other plug-ins owing to the hooks it triggers.

So it looks like this will be easy once WordPress core is updated to allow addStringAttachment() to be used from wp_mail but this is not the case yet. You agree?

@DanielRuf
Copy link

Sorry, forget the link, I pasted the wrong link here.

You should use the filter_hook, but I am not sure how to replace this. Seems you would have to copy wp_mail and modify it in your plugin to use PHPMailer::addStringAttachment(). But this may create problems when the code in the core changes or is different between WordPress versions and the plugin code does not use the latets code.

This would be the manual way (recreate the functionality using a PHPMailer instance with the same arguments) but possibly not directly possible and the right way. The WordPress API should be preferred.

So it looks like this will be easy once WordPress core is updated to allow addStringAttachment() to be used from wp_mail but this is not the case yet. You agree?

Right, I agree. This seems to be a bit more tricky. This https://core.trac.wordpress.org/ticket/29513 would probably ease this.

Here is an example without modifying core files:
http:https://dancameron.org/code/adding-string-attachments-addstringattachment-wp_mail/
https://gist.github.com/dancameron/1a0202e3bc784f567440
https://gist.github.com/dancameron/048ee1ac3b476858cd54

Did not try this but looks more promising but like already mentioned is more like a hack than a real solution as it does not use the WordPress API directly and uses the PHPMailer class directly.

Anyhow, I thought these may be useful maybe but I would also vote for the trac ticket as this is definitely useful.

@fabacab
Copy link
Owner Author

fabacab commented Feb 22, 2016

That example is clever, I will try playing with that some to see if it works well for me. Good find, and again, thank you.

@DanielRuf
Copy link

Hm, what if we create a temporary file with the encrypted attachment using file_get_contents and the openpgp-php functions, using this instead of the original one and unlinking this temporary file afterwards? Would be one possible and working workaround.

@DanielRuf
Copy link

For example tmpfile(), php:https://memory and php:https://temp

@fabacab
Copy link
Owner Author

fabacab commented Mar 1, 2016

@DanielRuf I don't think we'll need to use temporary files because when wp_mail() deals with attachments it always expects an array of file paths, according to the wp_mail() docs:

The filenames in the $attachments attribute have to be filesystem paths.

So that means we can handle the $attachments array similarly. Commit e34412f (in the issue-4 topic branch) is able to successfully use PHPMailer::addStringAttachments() by passing data in PHPMailer's error properties like the trick you found suggests doing, so this works, but it turns out there are additional complications.

To send PGP-encrypted messages with attachments, my understanding is that we have to use PGP/MIME, defined in RFC 3156, but PHPMailer does not support this. There is a subclass in @ravisorg's PHPMailer fork here that does (seem to) implement PGP/MIME but it uses the PHP GnuPG extension instead of @singpolyma's OpenPGP-PHP for its PGP functionality. This means we'll need to either further subclass @ravisorg's implementation or contribute a patch upstream to make it optionally use OpenPGP-PHP in addition to the gnupg_* functions. That is assuming, of course, that their PHPMailerPGP class actually does what we need. :\ (I will ask them about it, I think PHPMailer/PHPMailer#505 is the best place.)

In the mean time, I will ask @stefannagy back on the WordPress forums to help test the issue-4 branch. It seems to work well for me, at least with text-only attachments, but I do think there is a problem with image attachments. It seems if I try to attach a PNG or some other image, then the file gets sent as an ASCII-armored text attachment (which is expected given the code), but then when I use my gpg command line to decrypt it, the PNG image data is corrupted somehow and won't show up as an image. Not sure why that's happening, it could be a problem in transit due to PHPMailer's handling, but I'm not sure and haven't looked deeply yet.

So that's where we are with this for now. Thanks again for finding that neat WordPress trick.

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

2 participants