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

Fold header lines with the correct length #25

Merged
merged 3 commits into from
Aug 30, 2022
Merged

Fold header lines with the correct length #25

merged 3 commits into from
Aug 30, 2022

Conversation

danielabyan
Copy link
Contributor

Q A
Documentation no
Bugfix yes
BC Break yes
New Feature no
RFC yes
QA no

Description

According to the RFC 2822 specification (clause 2.1.1), the header length should be no more than 78 characters.

Each line of characters MUST be no more than 998 characters, and SHOULD be no more than 78 characters, excluding the CRLF.

According to the specification, the length of the ENTIRE LINE should not exceed 78 characters. Currently, the laminas-mime library only counts the length of a value of the header.

Reproducing the problem

$message = new Message();
$message->setEncoding('UTF-8');
$message->addFrom('matthew@example.org', 'Matthew Somelli');
$message->addTo('foobar@example.com');
$message->setSubject('xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx');
$message->setBody('This is the message body.');

echo $message->toString();

This code will return the following result.

Date: Thu, 21 Oct 2021 10:37:01 +0000
From: =?UTF-8?Q?Matthew=20Somelli?= matthew@example.org
To: foobar@example.com
Subject: =?UTF-8?Q?xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20?=
=?UTF-8?Q?xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx?=

This is the message body.

As you can see from the example, the header is longer than 78 characters.

New code behavior

This commit has fixed this issue. In this pool request, the code above will return the following result.

Date: Thu, 21 Oct 2021 10:38:56 +0000
From: =?UTF-8?Q?Matthew=20Somelli?= matthew@example.org
To: foobar@example.com
Subject: =?UTF-8?Q?xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20?=
=?UTF-8?Q?xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx?=

This is the message body.

Now, the title does not exceed 78 characters.

Note

I will wait until this pool request is merged to open my pool request on laminas-mail

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

This kind of change requires careful test additions.

@glensc
Copy link
Contributor

glensc commented Oct 21, 2021

Perhaps it's just better to add a new api that can take headerline or headerkey,headervalue pair as input?

or maybe that is not needed if the caller just passes headerline instead of headervalue as input?
it may break headerkeys that are longer than 998 bytes, but perhaps caller should handle (throw) such edge cases themselves?

@danielabyan
Copy link
Contributor Author

@glensc I was considering passing the whole string, but there is a problem here. If we pass the entire line, the header name will be encoded as well. But according to the RFC 2822, only the value (body) of the header must be encoded.
For example, this code

$mime = Mime\Mime::encodeQuotedPrintableHeader(
    'Subject: xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx xxxxx',
    'UTF-8',
    78
);

var_dump($mime);

Will return a result like this

=?UTF-8?Q?Subject:=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20?=
 =?UTF-8?Q?xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx=20xxxxx?="

As you can see, in this case, the whole line breaks. So, I made the changes to be minimal and not to break past behavior.

@danielabyan
Copy link
Contributor Author

@Ocramius Do we have any updates? I just need this change to improve my mailing since my emails have long lines than recommended in RFC.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@glensc can I have a second opinion? Code-wise, it makes sense, but I don't know much about RFC 2822

test/MimeTest.php Outdated Show resolved Hide resolved
src/Mime.php Show resolved Hide resolved
src/Mime.php Outdated Show resolved Hide resolved
@danielabyan
Copy link
Contributor Author

@Ocramius Sorry for writing a lot about this, but can we release it if everything is fine with this request?

@Ocramius
Copy link
Member

Given my low RFC-fu here, perhaps either @Xerkus or @glensc can give a green light here.

Code-wise, it is fine for me.

@Xerkus
Copy link
Member

Xerkus commented Nov 22, 2021

Makes sense to me. Spec does speak about line length irrespective to the structured content of the line.

According to the RFC 2822 specification (clause 2.1.1),
the header length should be no more than 78 characters.
   "Each line of characters MUST be no more than 998
   characters, and SHOULD be no more than 78 characters,
   excluding the CRLF."
According to the specification, the length of the ENTIRE
LINE should not exceed 78 characters. Currently, the
`laminas-mail` library only counts the length of a value
of the header. This commit has fixed this issue.

Signed-off-by: danielabyan <danielabyan@gmail.com>
@danielabyan
Copy link
Contributor Author

I have made all the changes in the code to meet the requirements of the code sniffer. But now I have a problem with composer. Please check this problem.
image
As you can see the problem lies in the new security policy in Composer 2.2.
This problem can be fixed if I add this configuration to the "composer.json" file

   ...
    "config": {
        "sort-packages": true,
        "allow-plugins": {
            "dealerdirect/phpcodesniffer-composer-installer": true
        }
    },
   ...

@Ocramius can I do this in this pull request or should I open a separate pull request?

Thank you.

@Ocramius
Copy link
Member

@danielabyan from my point of view, you can do it here 👍

Changes have been made to meet the requirements of
composer 2.2. Composer 2.2 introduced a new security
layer to prevent executing arbitrary code when the
Composer runs. We have a library
`dealerdirect/phpcodesniffer-composer-installer` which
is executed during the installation of the project.
In this commit, this library was allowed to run during
the installation of the project.

Signed-off-by: danielabyan <danielabyan@gmail.com>
composer.json Outdated Show resolved Hide resolved
@danielabyan
Copy link
Contributor Author

@Ocramius I fixed all the bugs and issues that were in this pool request. Please check my work, and if everything is correct, please create a new release, as some mail services refuse to deliver my emails due to RFC complaints. Thanks a lot.

In the old version of PHPUnit there is a bug that
does not allow you to build the project. An error
`Schemas parser error : Failed to locate the main schema
resource at 'vendor/phpunit/phpunit/phpunit.xsd'` is
thrown during the test execution. This bug has been
fixed in version `9.5`. Therefore, the version of the
library has been updated.

Signed-off-by: danielabyan <danielabyan@gmail.com>
@danielabyan danielabyan requested review from Ocramius and Xerkus and removed request for Ocramius and Xerkus August 29, 2022 11:59
@danielabyan danielabyan requested review from Xerkus and Ocramius and removed request for Ocramius and Xerkus August 29, 2022 11:59
@Ocramius Ocramius self-assigned this Aug 30, 2022
@Ocramius Ocramius added the Bug Something isn't working label Aug 30, 2022
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Thanks @danielabyan!

@Ocramius Ocramius merged commit 62a899a into laminas:2.10.x Aug 30, 2022
@Ocramius Ocramius changed the title Fold a header line with the correct length Fold header lines with the correct length Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants