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

Fixed mailcollector with multibyte filename (#8495) #8531

Merged
merged 12 commits into from
Jan 28, 2021
14 changes: 7 additions & 7 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,13 @@ jobs:
docker exec app bin/console glpi:database:configure --config-dir=./tests --no-interaction --reconfigure --db-name=glpi --db-host=db --db-user=root
docker exec app vendor/bin/atoum -p 'php -d memory_limit=512M' --debug --force-terminal --use-dot-report --bootstrap-file tests/bootstrap.php --no-code-coverage --fail-if-skipped-methods --max-children-number 1 -d tests/database
docker exec app bin/console glpi:database:configure --config-dir=./tests --no-interaction --reconfigure --db-name=glpi --db-host=db --db-user=root
- name: "IMAP tests"
cedric-anne marked this conversation as resolved.
Show resolved Hide resolved
if: env.skip != 'true'
run: |
for f in `ls tests/emails-tests/*.eml`; do cat $f | docker exec --user glpi --interactive dovecot getmail_maildir /home/glpi/Maildir/ ; done
docker exec app sed -e 's/127.0.0.1/dovecot/g' -i tests/imap/MailCollector.php
docker exec app sed -e 's/127.0.0.1/dovecot/g' -i tests/imap/Toolbox.php
docker exec app vendor/bin/atoum -p 'php -d memory_limit=512M' --debug --force-terminal --use-dot-report --bootstrap-file tests/bootstrap.php --no-code-coverage --fail-if-skipped-methods --max-children-number 1 -d tests/imap
- name: "Unit tests"
if: env.skip != 'true'
run: |
Expand All @@ -240,13 +247,6 @@ jobs:
run: |
for f in `ls tests/LDAP/ldif/*.ldif`; do cat $f | docker exec --interactive openldap ldapadd -x -H ldap://127.0.0.1:3890/ -D "cn=Manager,dc=glpi,dc=org" -w insecure ; done
docker exec app vendor/bin/atoum -p 'php -d memory_limit=512M' --debug --force-terminal --use-dot-report --bootstrap-file tests/bootstrap.php --no-code-coverage --fail-if-skipped-methods --max-children-number 1 -d tests/LDAP
- name: "IMAP tests"
if: env.skip != 'true'
run: |
for f in `ls tests/emails-tests/*.eml`; do cat $f | docker exec --user glpi --interactive dovecot getmail_maildir /home/glpi/Maildir/ ; done
docker exec app sed -e 's/127.0.0.1/dovecot/g' -i tests/imap/MailCollector.php
docker exec app sed -e 's/127.0.0.1/dovecot/g' -i tests/imap/Toolbox.php
docker exec app vendor/bin/atoum -p 'php -d memory_limit=512M' --debug --force-terminal --use-dot-report --bootstrap-file tests/bootstrap.php --no-code-coverage --fail-if-skipped-methods --max-children-number 1 -d tests/imap
- name: "WEB tests"
if: env.skip != 'true'
run: |
Expand Down
7 changes: 5 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"ext-zlib": "*",
"blueimp/jquery-file-upload": "^10.2",
"elvanto/litemoji": "^1.4 || ^2.0",
"glen/filename-normalizer": "^2.0",
"guzzlehttp/guzzle": "^6.5",
"guzzlehttp/psr7": "^1.6",
"htmlawed/htmlawed": "^1.2",
Expand Down Expand Up @@ -94,11 +95,13 @@
"lint": "vendor/bin/parallel-lint --exclude files --exclude marketplace --exclude plugins --exclude vendor --exclude tools/vendor .",
"post-install-cmd": [
"@php -r \"file_put_contents('.composer.hash', sha1_file('composer.lock'));\"",
"patch -f -p1 -d vendor/tecnickcom/tcpdf/ < tools/tcpdf-php8-compat.patch || echo 'Error applying patch, deprecation warnings related to TCPDF lib may pollute logs'"
"patch -f -p1 -d vendor/tecnickcom/tcpdf/ < tools/tcpdf-php8-compat.patch || echo 'Error applying patch, deprecation warnings related to TCPDF lib may pollute logs'",
"patch -f -p1 -d vendor/laminas/laminas-mail/ < tools/laminas-mail-128.patch || echo 'Error applying patch, retrieving some mail attachement could fail'"
],
"post-update-cmd": [
"@php -r \"file_put_contents('.composer.hash', sha1_file('composer.lock'));\"",
"patch -f -p1 -d vendor/tecnickcom/tcpdf/ < tools/tcpdf-php8-compat.patch || echo 'Error applying patch, deprecation warnings related to TCPDF lib may pollute logs'"
"patch -f -p1 -d vendor/tecnickcom/tcpdf/ < tools/tcpdf-php8-compat.patch || echo 'Error applying patch, deprecation warnings related to TCPDF lib may pollute logs'",
"patch -f -p1 -d vendor/laminas/laminas-mail/ < tools/laminas-mail-128.patch || echo 'Error applying patch, retrieving some mail attachement could fail'"
]
},
"repositories": {
Expand Down
139 changes: 138 additions & 1 deletion composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions inc/config.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -2077,6 +2077,8 @@ static function getLibraries($all = false) {
'check' => 'wapmorgan\\UnifiedArchive\\UnifiedArchive' ],
[ 'name' => 'paragonie/sodium_compat',
'check' => 'ParagonIE_Sodium_Compat' ],
[ 'name' => 'glen/filename-normalizer',
'check' => 'glen\\FilenameNormalizer\\Normalizer' ],
];
if (Toolbox::canUseCAS()) {
$deps[] = [
Expand Down
6 changes: 6 additions & 0 deletions inc/mailcollector.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
die("Sorry. You can't access this file directly");
}

use glen\FilenameNormalizer\Normalizer;
use LitEmoji\LitEmoji;
use Laminas\Mail\Address;
use Laminas\Mail\Header\AbstractAddressList;
Expand Down Expand Up @@ -1527,11 +1528,14 @@ private function getRecursiveAttached(\Laminas\Mail\Storage\Part $part, $path, $
&& $part->getHeaders()->has('content-disposition')
&& ($content_disp_header = $part->getHeader('content-disposition')) instanceof ContentDisposition) {
$filename = $content_disp_header->getParameter('filename') ?? '';

var_dump(sprintf('filename from content-disposition: >%s<', $filename));
}

// Try to get filename from Content-Type header
if (empty($filename)) {
$filename = $content_type_header->getParameter('name') ?? '';
var_dump(sprintf('filename from content-type: >%s<', $filename));
}

$filename_matches = [];
Expand Down Expand Up @@ -1569,6 +1573,8 @@ private function getRecursiveAttached(\Laminas\Mail\Storage\Part $part, $path, $
return false;
}

$filename = Normalizer::normalize($filename);

//try to avoid conflict between inline image and attachment
$i = 2;
while (in_array($filename, $this->files)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
Return-Path: normal@glpi-project.org
Received: from mail.glpi-project.org (localhost [127.0.0.1])
by mail.glpi-project.org (Postfix) with ESMTP id B74527E805E8
for <unittests@glpi-project.org>; Wed, 3 Apr 2019 11:48:35 +0200 (CEST)
Date: Thu, 21 Jan 2021 15:26:54 +0100 (CET)
From: "Normal User" <normal@glpi-project.org>
To: unittests@glpi-project.org
Message-ID: <aa097017-ea09-4dc2-8d71-8cb3cd64f397@mail.glpi-project.org>
Subject: 24.1 Test attachment with long multibyte filename
MIME-Version: 1.0
Content-Type: multipart/mixed;
boundary="----=_Part_1101800_1137170823.1611253212835"

------=_Part_1101800_1137170823.1611253212835
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

test

------=_Part_1101800_1137170823.1611253212835
Content-Type: application/pdf;
filename*0="=?gb2312?B?MjQuMS3plb/mlofku7blkI3vvIzlsIblr7zoh7TlhoXlrrnlp?=";
filename*1="=?gb2312?B?ITnva7moIflpLTkuK3nmoTov57nu63ooYwudHh0?="
Content-Disposition: attachment;
filename*0="=?gb2312?B?MjQuMS3plb/mlofku7blkI3vvIzlsIblr7zoh7TlhoXlrrnlp?=";
filename*1="=?gb2312?B?ITnva7moIflpLTkuK3nmoTov57nu63ooYwudHh0?="
Content-Transfer-Encoding: base64

MjQuMS1hdHRhY2hlbWVudC1tdWx0aWJ5dGUtbmFtZS13aXRoLWNvbnRpbnVhdGlvbi5lbWw=
------=_Part_1101800_1137170823.1611253212835--
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
Return-Path: normal@glpi-project.org
Received: from mail.glpi-project.org (localhost [127.0.0.1])
by mail.glpi-project.org (Postfix) with ESMTP id B74527E805E8
for <unittests@glpi-project.org>; Wed, 3 Apr 2019 11:48:35 +0200 (CEST)
Date: Thu, 21 Jan 2021 17:25:54 +0100 (CET)
From: "Normal User" <normal@glpi-project.org>
To: unittests@glpi-project.org
Message-ID: <2122674625.1101804.1611239214539@mail.glpi-project.org>
Subject: 24.2 Test attachment with short multibyte filename
MIME-Version: 1.0
Content-Type: multipart/mixed;
boundary="----=_Part_1101800_1137170823.1611239214535"

------=_Part_1101801_1973047508.1611239214535
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

test

------=_Part_1101800_1137170823.1611239214535
Content-Type: application/pdf;
filename="=?gb2312?B?MjQuMi3kuK3lm73lrZfnrKYudHh0?="
Content-Disposition: attachment;
filename="=?gb2312?B?MjQuMi3kuK3lm73lrZfnrKYudHh0?="
Content-Transfer-Encoding: base64

MjQuMi1hdHRhY2hlbWVudC1tdWx0aWJ5dGUtbmFtZS13aXRob3V0LWNvbnRpbnVhdGlvbi5lbWw=
------=_Part_1101800_1137170823.1611239214535--
28 changes: 28 additions & 0 deletions tests/emails-tests/25-attachement-with-OS-incompatible-char.eml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
Return-Path: normal@glpi-project.org
Received: from mail.glpi-project.org (localhost [127.0.0.1])
by mail.glpi-project.org (Postfix) with ESMTP id B74527E805E8
for <unittests@glpi-project.org>; Wed, 3 Apr 2019 11:48:35 +0200 (CEST)
Date: Thu, 22 Jan 2021 15:26:54 +0100 (CET)
From: "Normal User" <normal@glpi-project.org>
To: unittests@glpi-project.org
Message-ID: <50BFB6E5-3040-4535-9A10-3164C726A37B@FRY09565>
Subject: 25 - Test attachment with invalid chars for OS
MIME-Version: 1.0
Content-Type: multipart/mixed;
boundary="_002_50BFB6E5304045359A103164C726A37BFRY09565_"

--_002_50BFB6E5304045359A103164C726A37BFRY09565_
Content-Type: text/plain; charset="us-ascii"

Test body
--_002_50BFB6E5304045359A103164C726A37BFRY09565_
Content-Type: application/octet-stream; name="25-New Text ? Document.txt"
Content-Description: 25-New Text ? Document.txt
Content-Disposition: attachment; filename="25-New Text ? Document.txt"; size=265;
creation-date="Fri, 15 Jan 2021 12:52:27 GMT";
modification-date="Fri, 15 Jan 2021 12:52:27 GMT"
Content-Transfer-Encoding: base64

dGVzdA==

--_002_50BFB6E5304045359A103164C726A37BFRY09565_--
9 changes: 8 additions & 1 deletion tests/imap/MailCollector.php
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,9 @@ public function testCollect() {
'Attachement having filename using RFC5987 (multiple lines)',
'Attachement having filename using RFC5987 (single line)',
'Mono-part HTML message',
'24.1 Test attachment with long multibyte filename',
'24.2 Test attachment with short multibyte filename',
'25 - Test attachment with invalid chars for OS'
]
],
// Mails having "normal" user as observer (add_cc_to_observer = true)
Expand Down Expand Up @@ -593,12 +596,16 @@ public function testCollect() {
// Check creation of expected documents
$expected_docs = [
'00-logoteclib.png',
'01-Screenshot-2018-4-12 Observatoire - France très haut débit.png',
// Space is missing between "France" and "très" due to a bug in laminas-mail
'01-Screenshot-2018-4-12 Observatoire - Francetrès haut débit.png',
Copy link
Member

@cedric-anne cedric-anne Jan 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edgardmessias

Did you saw that ?

I guess this is only a side effect of your fix. Indeed, now the space char is located at the beginning or end of a param line, and trimming done in Laminas removes it. Before your fix, this space was certainly located in the middle of second parameter line.

Param lines are extracted with these values:

array(2) {
  [0]=>
  string(45) "01-Screenshot-2018-4-12 Observatoire - France"
  [1]=>
  string(21) "très haut débit.png"
}

EDIT: I don't have time to do a PR on laminas-mail for the moment, especially since this bug has little impact.

'01-test.JPG',
'15-image001.png',
'18-blank.gif',
'19-ʂǷèɕɩɐɫ ȼɦâʁȿ.gif',
'20-specïal chars.gif',
'24.1-长文件名,将导致内容处置标头中的连续行.txt',
'24.2-中国字符.txt',
'25-New Text - Document.txt',
];

$iterator = $DB->request(
Expand Down
Loading