isBase64 function returning incorrect

[thinkingserious]

Issue Summary

via @mikepage

I had to remove the base64_encode function in this line of code in order to keep the attachments readable.

$email->addAttachment(base64_encode(file_get_contents($attachment['tmp_filename'])), 'application/pdf', $attachment['filename'], 'attachment');

[Tigerman55]

I'm still experiencing this issue right now.

[thinkingserious]

Thanks for the confirmation @Tigerman55!

For this issue to gain priority in our backlog, we need additional +1's or a PR. When we receive a PR, that provides the biggest jump in priority.

[fernandoval]

Same issue here.

I just removed base64_encode() function from my code. See:

// Removing base64_encode() function due to isBase64 bug
// $fileEncoded = base64_encode(file_get_contents($path));
$fileEncoded = file_get_contents($path);
$mail->addAttachment($fileEncoded, $type, $name);

This is a temporary solution until the bug resolved.

[thinkingserious]

Thanks @fernandoval!

[martijnmelchers]

@thinkingserious Seems that the issue was a little harder than expected 😓 anyone has an idea of what we could do to fix this issue?

[thinkingserious]

Indeed @martijnmelchers,

I've put this on our backlog for further investigation.

[eddturtle]

We hit this issue too, we had in our composer "sendgrid/sendgrid": "~7.0" and suddenly in a push all our email attachments broke (double base 64 encoding), so we've forced 7.0.0 until we can look into the issue. Would be great to get a fix or release it as a breaking change.

[thinkingserious]

Thanks for reporting this @eddturtle, we have this on our backlog for a fix and your vote helps it gain priority.

[thinkingserious]

@martijnmelchers,

Maybe we add a parameter to the function to optionally enable this feature. Then, when we receive non-base encoded content, we can throw an error and inform them of how to use the auto-encoding.

[Jmky]

I made a change to isBase64 function as below and it seems to work fine for me.

private function isBase64($string) 
    {
        $decoded_data = base64_decode($string, true);
        $encoded_data = base64_encode($decoded_data);
        if ($encoded_data != $string) {
            return false;
        }
        return true;
    }

I removed

} else {
    $decoded_data = str_ireplace("\t", '', $decoded_data);
    $decoded_data = str_ireplace("\n", '', $decoded_data);
    $decoded_data = str_ireplace("\r", '', $decoded_data);
    if (!ctype_print($decoded_data)) {
        return false;
    }

The original code doesn't work because ctype_print returns false to Base64 encoded binary files even if they are strings and are stripped of \t, \n, and \r.
When isBase64 function returns false, setContent function tries to do Base64 encoding itself, resulting in an invalid file that's Base64 encoded TWICE.

I don't see a point to have ctype_print there, because coming to the 'else' section means the file has already been proven to be Base64 encoded... am I missing something here??

Although removing base64_encode from the example code as suggested works just fine, changing isBase64 function as shown above makes addAttachment function accept both Base64 encoded arguments and NOT encoded arguments as it's (probably) designed to.

I'm fairly new to both PHP and Git, so correct me if I'm wrong about anything. Thanks!

[piotrkosytorz]

This has been a fairly costly bug for us.
Since there is a requirement on the content parameter to be encoded in base64 (https://github.com/sendgrid/sendgrid-php/blame/master/lib/mail/Attachment.php#L38), what was the reason to implement this magic (https://github.com/sendgrid/sendgrid-php/blame/master/lib/mail/Attachment.php#L82) in the first place?
To me it introduces inconsistency.

[thinkingserious]

Hello Everyone!

I just finished testing v7.2.1 and it appears to solve this bug. My apologies for all the trouble this caused and thank you for helping me track it down and get a fix in.

If you continue to experience any issues with this functionality, please feel free to continue the conversation here.

With Best Regards,

Elmer

BTW, the failing test appears to be unrelated to this issue.