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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions src/Header/HeaderValue.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,18 @@ private function __construct()
public static function filter($value)
{
$result = '';
$tot = strlen($value);
$total = strlen($value);

// Filter for CR and LF characters, leaving CRLF + WSP sequences for
// Long Header Fields (section 2.2.3 of RFC 2822)
for ($i = 0; $i < $tot; $i += 1) {
for ($i = 0; $i < $total; $i += 1) {
$ord = ord($value[$i]);
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

continue;
}

if ($ord === 13) {
if ($i + 2 >= $tot) {
if ($i + 2 >= $total) {
continue;
}

Expand Down Expand Up @@ -72,17 +70,17 @@ public static function filter($value)
*/
public static function isValid($value)
{
$tot = strlen($value);
for ($i = 0; $i < $tot; $i += 1) {
$total = strlen($value);
for ($i = 0; $i < $total; $i += 1) {
$ord = ord($value[$i]);
if (($ord < 32 || $ord > 126)
&& $ord !== 13
) {

// bare LF means we aren't valid
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

return false;
}

if ($ord === 13) {
if ($i + 2 >= $tot) {
if ($i + 2 >= $total) {
return false;
}

Expand All @@ -93,6 +91,7 @@ public static function isValid($value)
return false;
}

// skip over the LF following this
$i += 2;
}
}
Expand Down
18 changes: 15 additions & 3 deletions test/Header/HeaderValueTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
namespace ZendTest\Mail\Header;

use PHPUnit_Framework_TestCase as TestCase;
use ReflectionClass;
use Zend\Mail\Header\HeaderValue;

class HeaderValueTest extends TestCase
Expand All @@ -30,7 +31,8 @@ public function getFilterValues()
["This is a\r\r test", "This is a test"],
["This is a \r\r\n test", "This is a \r\n test"],
["This is a \r\n\r\ntest", "This is a test"],
["This is a \r\n\n\r\n test", "This is a \r\n test"]
["This is a \r\n\n\r\n test", "This is a \r\n test"],
["This is a test\r\n", "This is a test"],
];
}

Expand All @@ -56,7 +58,11 @@ public function validateValues()
["This is a\r\r test", 'assertFalse'],
["This is a \r\r\n test", 'assertFalse'],
["This is a \r\n\r\ntest", 'assertFalse'],
["This is a \r\n\n\r\n test", 'assertFalse']
["This is a \r\n\n\r\n test", 'assertFalse'],
["This\tis\ta test", 'assertTrue'],
["This is\ta \r\n test", 'assertTrue'],
Copy link
Member

Choose a reason for hiding this comment

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

Missing test with the sequence \r\n\t which should be valid

["This\tis\ta\ntest", 'assertFalse'],
["This is a \r\t\n \r\n test", 'assertFalse'],
];
}

Expand All @@ -81,7 +87,7 @@ public function assertValues()
["This is a\r\r test"],
["This is a \r\r\n test"],
["This is a \r\n\r\ntest"],
["This is a \r\n\n\r\n test"]
["This is a \r\n\n\r\n test"],
];
}

Expand All @@ -94,4 +100,10 @@ public function testAssertValidRaisesExceptionForInvalidValues($value)
$this->setExpectedException('Zend\Mail\Header\Exception\RuntimeException', 'Invalid');
HeaderValue::assertValid($value);
}

public function testCannotBeConstructed()
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this test. It's not relevant.

{
$class = new ReflectionClass('Zend\Mail\Header\HeaderValue');
$this->assertFalse($class->isInstantiable());
}
}