Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Fix HeaderValue throwing an exception on legal characters #28

Merged
merged 1 commit into from
May 5, 2016
Merged

Fix HeaderValue throwing an exception on legal characters #28

merged 1 commit into from
May 5, 2016

Conversation

ameliaikeda
Copy link
Contributor

In RFC 2822, section 2.2, there are different conditions used for header names vs header values.

Header names are restricted to only printable ascii characters (with folding allowed), while header values are allowed any us-ascii character (and "\r\n ") except a bare LF or CR.

This is a major problem, because Apache and Outlook/Office365 use tab characters to fold their subject lines. These characters are perfectly valid according to the RFC, yet throw an exception when read.

Relevant spec is below (Section 3.2.1):

NO-WS-CTL       =       %d1-8 /         ; US-ASCII control characters
                        %d11 /          ;  that do not include the
                        %d12 /          ;  carriage return, line feed,
                        %d14-31 /       ;  and white space characters
                        %d127

text            =       %d1-9 /         ; Characters excluding CR and LF
                        %d11 /
                        %d12 /
                        %d14-127 /
                        obs-text

specials        =       "(" / ")" /     ; Special characters used in
                        "<" / ">" /     ;  other parts of the syntax
                        "[" / "]" /
                        ":" / ";" /
                        "@" / "\" /
                        "," / "." /
                        DQUOTE

   No special semantics are attached to these tokens.  They are simply
   single characters.

Header names are NO-WS-CTL - specials, while header values are text with folding allowed.

@ameliaikeda
Copy link
Contributor Author

Despite adding an additional test case and adding extra elements to data providers (as well as actually testing a line that was previously never even touched), coverage never seems to go up.

I give up; I'll just keep my repo in my composer files.

@weierophinney
Copy link
Member

Don't worry too much about the coverage; we'll be evaluating the
correctness of the patch only.
On Oct 8, 2015 3:22 AM, "Amelia Ikeda" notifications@github.com wrote:

Despite adding an additional test case and adding extra elements to data
providers (as well as actually testing a line that was previously never
even touched), coverage never seems to go up.

I give up; I'll just keep my repo in my composer files.


Reply to this email directly or view it on GitHub
#28 (comment)
.

@Maks3w
Copy link
Member

Maks3w commented Oct 8, 2015

Because the number of relevant lines has decreased then the percentage given to any covered/uncovered line is higher (i.e 2 lines in a pack of 10 has more percentage than 2 lines in a pack of 100)

@ameliaikeda
Copy link
Contributor Author

Ah, okay.

I'll see if I can get a copy of an email that breaks in pretty horrifying ways when reading it and add it in, too. This is the very last issue in a migration from zf2 v2.1.3 to latest + php 5.6, and is essentially a hard blocker :/

@Maks3w
Copy link
Member

Maks3w commented Oct 24, 2015

From http://tools.ietf.org/html/rfc5322#section-2.2

2.2. Header Fields
2.2.1. Unstructured Header Field Bodies

[...] specified in section 3.2.5 as any printable
US-ASCII characters plus white space characters) with no further
restrictions. These are referred to as unstructured field bodies.
Semantically, unstructured field bodies are simply to be treated as a
single line of characters with no further processing (except for
"folding" and "unfolding" as described in section 2.2.3).

2.2.2. Structured Header Field Bodies

Some field bodies in this specification have a syntax that is more
restrictive

For the purpose of general validation / filtering we opt for choose the "unstructured header" syntax because is the most unrestricted.

From http://tools.ietf.org/html/rfc5322#section-3.1

In some of the definitions, there will be non-terminals whose names
start with "obs-" [...]
when interpreting messages, these tokens MUST be honored as part of
the legal syntax

ABNF resume with errata (1905, 1908) applied

unstructured  =   (*([FWS] VCHAR) *WSP) / obs-unstruct
FWS           =   ([*WSP CRLF] 1*WSP) /  obs-FWS
obs-FWS       =   1*([CRLF] WSP)
obs-unstruct  =   *( (*CR 1*(obs-utext / FWS)) / 1*LF ) *CR
obs-utext     =   %d0 / obs-NO-WS-CTL / VCHAR
obs-NO-WS-CTL =   %d1-8 /  %d11 / %d12 / %d14-31 / %d127
VCHAR         =   %x21-7E ; visible (printing) characters (decimal 33 / 126)
WSP           =   SP / HTAB ; white space (decimal 32 / 9)

@Maks3w
Copy link
Member

Maks3w commented Oct 24, 2015

Header bodies conclusion:

  • May be 0 length (0 characters)
  • May be composed of only FWS sequences
  • Use of WSP characters (char 9 / 32) is unrestricted
  • Use of obs-utext characters (char 0-8, 11-12, 14-31, 33-127) is unrestricted

So the following character range may appear without any restriction:

[0-9, 11-12, 14-127]

and the characters 10, 13 must be together as 10 13 and followed of at least one WSP (9, 32)

@Maks3w
Copy link
Member

Maks3w commented Oct 24, 2015

@ameliaikeda This PR is invalid. You didn't fixed your original issue with TAB characters folding.

Please add tests for that scenario.

@Maks3w Maks3w added this to the 2.4.3 milestone Oct 24, 2015
if (($ord < 32 || $ord > 126)
&& $ord !== 13
) {
if ($ord === 10 || $ord > 126) {
Copy link
Member

Choose a reason for hiding this comment

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

should be > 127

@jomofcw
Copy link

jomofcw commented Nov 4, 2015

Hello,

Not sure about how to tell it or to report i, but i've got a problem relative to this using a character like "é" in the name. It worked well in ZF1 but doesn't in ZF2. Thanks for the coming fix.

@ameliaikeda
Copy link
Contributor Author

Just got back to this; I'll have updates either later today or tomorrow night.

Thanks for the feedback!

@Maks3w
Copy link
Member

Maks3w commented Nov 22, 2015

Any chance for to see this finished?

@mikegioia
Copy link

I know this is about a month old, but is there anyway to get this resolved? I'm running into the same issue in my application ._.

@dayrontbs12001
Copy link

I'm running into the same issue in my application. Getting the exception when using the character "è" on ReplyTo header ...

@avasa
Copy link

avasa commented Feb 8, 2016

Can lines 37 and 78 on src/Header/HeaderValue.php be merged to resolve this issue?

@stevleibelt
Copy link

I'm also running into this issue :-(.

weierophinney added a commit to weierophinney/zend-mail that referenced this pull request May 5, 2016
Fix HeaderValue throwing an exception on legal characters
weierophinney added a commit to weierophinney/zend-mail that referenced this pull request May 5, 2016
- Changes the ASCII out-of-range checks to use `> 127` instead of `>
  126`, as 127 is the last ASCII character.
- Removed the `testCannotBeConstructed()` test case, as irrelevant.
- Added a data provider to test that wrapping header lines using
  `\r\n\t` is valid. It failed, and, as such, I updated the `isValid()`
  code to check for both ASCII 32 (SP) and 9 (TAB) when checking for
  line wrapping.
@weierophinney weierophinney mentioned this pull request May 5, 2016
@weierophinney weierophinney merged commit 40fbcbe into zendframework:master May 5, 2016
weierophinney added a commit that referenced this pull request May 5, 2016
weierophinney added a commit that referenced this pull request May 5, 2016
Forward port #28
Forward port #87
weierophinney added a commit that referenced this pull request May 5, 2016
@jomofcw
Copy link

jomofcw commented Apr 17, 2018

Hello !

I still have the problem :/.
I've just upgraded to the latest stable version of zend-mail.
Any chance to see it fix, please ?

@froschdesign
Copy link
Member

@jomofcw
The current problem was fixed with version 2.7.1. Please recheck you version and if you still have problems, open a new issue report and add a detailed description or an unit test.
Thanks!

@jomofcw
Copy link

jomofcw commented Apr 17, 2018

@froschdesign Hello !
Thanks for your quick answer.
Here are all the version of Zend component I use in my project (managed through composer) :

zendframework/zend-cache               2.7.2  provides a generic way to cache any data
zendframework/zend-component-installer 2.1.1  Composer plugin for automating component registration in zend-mvc and Expressive applications
zendframework/zend-eventmanager        3.2.0  Trigger and listen to events within a PHP application
zendframework/zend-json                3.1.0  provides convenience methods for serializing native PHP to JSON and decoding JSON to native PHP
zendframework/zend-loader              2.5.1
zendframework/zend-mail                2.9.0  provides generalized functionality to compose and send both text and MIME-compliant multipart e-mail messages
zendframework/zend-mime                2.7.0  Create and parse MIME messages and parts
zendframework/zend-serializer          2.8.1  provides an adapter based interface to simply generate storable representation of PHP types by different facilities, and recover
zendframework/zend-servicemanager      3.3.2  Factory-Driven Dependency Injection Container
zendframework/zend-stdlib              3.1.0
zendframework/zend-validator           2.10.2 provides a set of commonly needed validators

zend-mail seems to be in version 2.9.0, right ?
Is that the last/correct version for this module ?
If yes according to you, so I'll open a new issue.
If not, how can I upgrade it using composer, please ? Because I thinked i didi it but it doesn't worked if the answer is "no" :/.

@froschdesign
Copy link
Member

2.9.0 is the latest version and also on your system.
Create a new issue report and don't forget the description or add an unit test to illustrate your problem.


Btw. here can you find all releases: https://github.com/zendframework/zend-mail/releases

@jomofcw
Copy link

jomofcw commented Apr 17, 2018

Thanks @froschdesign .
So, here we go : #203

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants