Fix error mb_convert_encoding(): Argument #3 ($from_encoding) contains invalid encoding "windows-1256"

[DoobleD]

Released under the GNU Affero General Public License (AGPL), version 3.

What does this implement/fix? Explain your changes.

Fixes the error below happening all the time when running Z-Push with PHP 8 (mb_convert_encoding now throws an error when $to_encoding or $from_encoding is an invalid encodig):

[error] 690771#690771: *3590124 FastCGI sent in stderr: "PHP message: PHP Fatal error:  Uncaught ValueError: mb_convert_encoding(): Argument #3 ($from_encoding) contains invalid encoding "windows-1256" in /opt/z-push/lib/utils/utils.php:1281
Stack trace:
#0 /opt/z-push/lib/utils/utils.php(1281): mb_convert_encoding()
#1 /opt/z-push/lib/utils/utils.php(1406): Utils::convertRawHeader2Utf8()
#2 /opt/z-push/backend/imap/imap.php(1154): Utils::CheckAndFixEncodingInHeaders()
#3 /opt/z-push/lib/default/diffbackend/exportchangesdiff.php(160): BackendIMAP->GetMessage()
#4 /opt/z-push/lib/request/sync.php(1199): ExportChangesDiff->Synchronize()
#5 /opt/z-push/lib/request/sync.php(956): Sync->syncFolder()
#6 /opt/z-push/lib/request/requestprocessor.php(116): Sync->Handle()
#7 /opt/z-push/index.php(107): RequestProcessor::HandleRequest()
#8 {main}
  thrown in /opt/z-push/lib/utils/utils.php on line 1281" while reading response header from upstream, ...

The fix is copied from nextcloud/mail#5593. More information there.

Does this close any currently open issues?

At the time of this PR I haven't found any open issue about this error.

Any relevant logs, error output, etc?

See error log above.

Where has this been tested?

Server (please complete the following information):

• OS: Debian bullseye
• PHP Version: 8.2
• Backend for: IMAP
• and Version: 2.7.0

Smartphone (please complete the following information):
The log above is found for POST /Microsoft-Server-ActiveSync queries with at least these DeviceType:

• iPhone
• Outlook
• Android
• SamsungDevice
  This is probably not exhaustive.

I'm not familiar at all with Z-Push's code base, feel free to modify however you think is best, or close and open an other PR with a better fix.

[matidau]

Thanks for the PR!

Haven't had a chance to test it yet but the solution looks solid.

Can you add a statement releasing this under the AGPLv3?
https://github.com/Z-Hub/Z-Push/blob/develop/CONTRIBUTING.md#releasing-contributions

[DoobleD]

Thank you @matidau, happy to add the statement, it's done (top of the PR comment). :)

[michlann]

Hmmm... what happens when iconv() encounters characters illegal in the source encoding? That makes it return FALSE. Is that a fatal error, too?

What happens when iconv() encounters an encoding it doesn't handle?

[matidau]

Before I dive in how did the testing go? I'm guessing that you encountered iconv returning false?

We could add //IGNORE to the charset string, but this will cause the characters to drop. Not sure this is desirable behaviour.

https://www.php.net/manual/en/function.iconv.php

If the string //IGNORE is appended, characters that cannot be represented in the target charset are silently discarded. Otherwise, E_NOTICE is generated and the function will return false.

There is also //TRANSLIT but this still has a chance of returning false, depending on the implementation. If this is used we would need to then check the result and call it again with //IGNORE to ensure that it always works.

Ideally we would make a config variable so this can be defined per system, this makes it a less simple bug fix though, possibly more a feature, so would prefer to handle in another PR.

[michlann]

I haven't had neither time nor material (i.e. stuff with illegal characters or weird encodings) at hand to test anything, sorry.

I see iconv() is merged, that is most certainly already better than it was before. And I see #27 open for further improvement. Good for me - if I have anything to say ;-).

Thanks!