mirror
/
PHPMailer
mirror of https://github.com/PHPMailer/PHPMailer.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

PHPMailer::parseAddresses(): bug fix [2] - Mbstring encoding 

The `mb_decode_mimeheader()` function uses the Mbstring internal encoding to decode.

In PHP 5.5, the default internal encoding was `ISO-8859-1`.
As of PHP 5.6, the default internal encoding was changed to use the value from the `default_charset` ini setting. Additionally, in UTF-8, the value for `default_charset` was changed to `UTF-8`.

This means that when the charset is not explicitly set, the `mb_decode_mimeheader()` function may return garbled nonsense if the charset used to _encode_ does not match the charset per PHP's `default_charset` or - in PHP 5.5 - the Mbstring internal_encoding default.

So far, this wasn't making tests fail because of some hard-coded ini settings being passed in the CI.
However, changing the default ini values creates an assumption that that configuration will be used on all servers on which the PHPMailer code will be run.
This assumption is undocumented (not in the Readme or mentioned elsewhere) and will in most cases be incorrect.

The non-default ini values change the behaviour of PHP and were the cause of test failures against PHP 5.5 which I've been seeing for some of the new tests I've been creating.
Removing the changes fixes those errors, but exposes failing tests in the existing tests for `PHPMailer::parseAddresses()`.

These undocumented _changes_ to the default PHP configuration were **required** for PHPMailer to be able to parse the addresses successfully. As this library is open source and used in a wide variety of environments, those kind of assumptions can not safely be made.

So.... the hard-coded ini settings in the CI configuration ought to be removed.

This then causes the tests for the `PHPMailer::parseAddresses()` function to start failing on PHP 5.5.
> Note: the tests are only failing on PHP 5.5 as the test case causing the failure uses a UTF-8 encoded name and as of PHP 5.6, the default encoding used in PHP is UTF-8, which matches.
> If a test case would be added with a name encoded in a different charset, the tests would also start failing on PHP 5.6+.

To fix those failures and to make the code PHP cross-version compatible, including with PHP installs configured to use a different `default_encoding`:
* We need to make sure that the Mbstring "internal encoding" is set correctly based on the Charset used for PHPMailer.
* And then need to _reset_ the internal encoding after the use of the `mb_decode_mimeheader()` to prevent any impact of this change on the wider application context in which PHPMailer may be used.

As the `PHPMailer::parseAddresses()` method is `static`, it does not have access to the (non-static) `PHPMailer::$charSet` variable.
Knowing that, I've elected to add an additional, optional variable to the `PHPMailer::parseAddresses()` method to allow for passing in the charset and have set the default value for the parameter to be in line with the default value of the `PHPMailer::$charSet` variable.

I have adjusted existing method calls to this method to explicitly pass the charset.
Both of the adjusted function calls are in the "postSend" part of the PHPMailer logic when the charset will be known and final, so can be safely passed.

I've also made minimal changes to the unit test file to allow for passing the charset in the tests.

This implementation is based on the assumption that names can be encoded in different charsets.
If the name encoding only happens when the charset is UTF-8, the new function parameter can be removed and the charset can be set to UTF-8 directly.

As I'm not completely read-in on the RFC specs for the address header being parsed and when encoding happens, I'd like a second opinion on the currently chosen implementation.

If this is the correct way to go, then additional tests need to be added to safeguard that things works correctly when a different encoding is used.
If the encoding only happens for UTF-8, the implementation can be simplified.

Update: the current implementation is correct and a `@todo` note has been added to add more tests with different encodings during a next iteration on these tests.

Refs:
* https://www.php.net/manual/en/migration56.deprecated.php#migration56.deprecated.iconv-mbstring-encoding
* https://php-legacy-docs.zend.com/manual/php5/en/ini.core#ini.default-charset
This commit is contained in:
jrfnl 2021-07-11 18:29:59 +02:00
parent 7cc67dd1dc
commit 228338fe7e
3 changed files with 49 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.github/workflows/tests.yml vendored
View File

@ -116,7 +116,7 @@ jobs:
with:
php-version: ${{ matrix.php }}
coverage: ${{ steps.set_cov.outputs.COV }}
ini-values: sendmail_path=/usr/sbin/sendmail -t -i, zend.multibyte=1, zend.script_encoding=UTF-8, default_charset=UTF-8, error_reporting=E_ALL, display_errors=On
ini-values: sendmail_path=/usr/sbin/sendmail -t -i, error_reporting=E_ALL, display_errors=On
extensions: imap, mbstring, intl, ctype, filter, hash
# Install dependencies and handle caching in one go.

12
src/PHPMailer.php
View File

@ -1188,7 +1188,7 @@ class PHPMailer
*
* @return array
*/
public static function parseAddresses($addrstr, $useimap = true)
public static function parseAddresses($addrstr, $useimap = true, $charset = self::CHARSET_ISO88591)
{
$addresses = [];
if ($useimap && function_exists('imap_rfc822_parse_adrlist')) {
@ -1209,7 +1209,10 @@ class PHPMailer
defined('MB_CASE_UPPER') &&
preg_match('/^=\?.*\?=$/', $address->personal)
) {
$origCharset = mb_internal_encoding();
mb_internal_encoding($charset);
$address->personal = mb_decode_mimeheader($address->personal);
mb_internal_encoding($origCharset);
}
$addresses[] = [
@ -1240,7 +1243,10 @@ class PHPMailer
//Check for a Mbstring constant rather than using extension_loaded, which is sometimes disabled
//If this name is encoded, decode it
if (defined('MB_CASE_UPPER') && preg_match('/^=\?.*\?=$/', $name)) {
$origCharset = mb_internal_encoding();
mb_internal_encoding($charset);
$name = mb_decode_mimeheader($name);
mb_internal_encoding($origCharset);
}
$addresses[] = [
//Remove any surrounding quotes and spaces from the name
@ -1723,7 +1729,7 @@ class PHPMailer
fwrite($mail, $header);
fwrite($mail, $body);
$result = pclose($mail);
$addrinfo = static::parseAddresses($toAddr);
$addrinfo = static::parseAddresses($toAddr, true, $this->charSet);
$this->doCallback(
($result === 0),
[[$addrinfo['address'], $addrinfo['name']]],
@ -1883,7 +1889,7 @@ class PHPMailer
if ($this->SingleTo && count($toArr) > 1) {
foreach ($toArr as $toAddr) {
$result = $this->mailPassthru($toAddr, $this->Subject, $body, $header, $params);
$addrinfo = static::parseAddresses($toAddr);
$addrinfo = static::parseAddresses($toAddr, true, $this->charSet);
$this->doCallback(
$result,
[[$addrinfo['address'], $addrinfo['name']]],

47
test/PHPMailer/ParseAddressesTest.php
View File

@ -19,6 +19,11 @@ use Yoast\PHPUnitPolyfills\TestCases\TestCase;
/**
* Test RFC822 address splitting.
*
* @todo Additional tests need to be added to verify the correct handling of inputs which
* include a different encoding than UTF8 or even mixed encoding. For more information
* on what these test cases should look like and should test, please see
* {@link https://github.com/PHPMailer/PHPMailer/pull/2449} for context.
*
* @covers \PHPMailer\PHPMailer\PHPMailer::parseAddresses
*/
final class ParseAddressesTest extends TestCase
@ -34,10 +39,16 @@ final class ParseAddressesTest extends TestCase
*
* @param string $addrstr The address list string.
* @param array $expected The expected function output.
* @param string $charset Optional. The charset to use.
*/
public function testAddressSplittingNative($addrstr, $expected)
public function testAddressSplittingNative($addrstr, $expected, $charset = null)
{
$parsed = PHPMailer::parseAddresses($addrstr, false);
if (isset($charset)) {
$parsed = PHPMailer::parseAddresses($addrstr, false, $charset);
} else {
$parsed = PHPMailer::parseAddresses($addrstr, false);
}
$expectedOutput = $expected['default'];
if (empty($expected['native+mbstring']) === false) {
$expectedOutput = $expected['native+mbstring'];
@ -59,10 +70,16 @@ final class ParseAddressesTest extends TestCase
*
* @param string $addrstr The address list string.
* @param array $expected The expected function output.
* @param string $charset Optional. The charset to use.
*/
public function testAddressSplittingImap($addrstr, $expected)
public function testAddressSplittingImap($addrstr, $expected, $charset = null)
{
$parsed = PHPMailer::parseAddresses($addrstr, true);
if (isset($charset)) {
$parsed = PHPMailer::parseAddresses($addrstr, true, $charset);
} else {
$parsed = PHPMailer::parseAddresses($addrstr, true);
}
$expectedOutput = $expected['default'];
if (empty($expected['imap+mbstring']) === false) {
$expectedOutput = $expected['imap+mbstring'];
@ -81,14 +98,20 @@ final class ParseAddressesTest extends TestCase
*
* @param string $addrstr The address list string.
* @param array $expected The expected function output.
* @param string $charset Optional. The charset to use.
*/
public function testAddressSplittingNativeNoMbstring($addrstr, $expected)
public function testAddressSplittingNativeNoMbstring($addrstr, $expected, $charset = null)
{
if (extension_loaded('mbstring')) {
$this->markTestSkipped('Test requires MbString *not* to be available');
}
$parsed = PHPMailer::parseAddresses($addrstr, false);
if (isset($charset)) {
$parsed = PHPMailer::parseAddresses($addrstr, false, $charset);
} else {
$parsed = PHPMailer::parseAddresses($addrstr, false);
}
$expectedOutput = $expected['default'];
if (empty($expected['native--mbstring']) === false) {
$expectedOutput = $expected['native--mbstring'];
@ -109,14 +132,20 @@ final class ParseAddressesTest extends TestCase
*
* @param string $addrstr The address list string.
* @param array $expected The expected function output.
* @param string $charset Optional. The charset to use.
*/
public function testAddressSplittingImapNoMbstring($addrstr, $expected)
public function testAddressSplittingImapNoMbstring($addrstr, $expected, $charset = null)
{
if (extension_loaded('mbstring')) {
$this->markTestSkipped('Test requires MbString *not* to be available');
}
$parsed = PHPMailer::parseAddresses($addrstr, true);
if (isset($charset)) {
$parsed = PHPMailer::parseAddresses($addrstr, true, $charset);
} else {
$parsed = PHPMailer::parseAddresses($addrstr, true);
}
$expectedOutput = $expected['default'];
if (empty($expected['imap--mbstring']) === false) {
$expectedOutput = $expected['imap--mbstring'];
@ -157,6 +186,7 @@ final class ParseAddressesTest extends TestCase
* - `imap` Expected output from the IMAP implementation with or without Mbstring.
* - `imap+mbstring` Expected output from the IMAP implementation with Mbstring.
* - `imap--mbstring` Expected output from the IMAP implementation without Mbstring.
* Also optionally, an additional `charset` key can be passed,
*/
public function dataAddressSplitting()
{
@ -282,6 +312,7 @@ final class ParseAddressesTest extends TestCase
],
],
],
'charset' => PHPMailer::CHARSET_UTF8,
],
// Test cases with invalid addresses.