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.

Sign up for GitHub

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

[1.5] Prevent infinite loop on empty xml elements #159

Merged
merged 2 commits into from
Jan 9, 2019
Merged

[1.5] Prevent infinite loop on empty xml elements #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd77b16
[1.5] Prevent infinite loop on empty xml elements
DeepDiver1975 Jan 8, 2019
a367665
Release 1.5.1
DeepDiver1975 Jan 9, 2019
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
ChangeLog
=========

1.5.1 (2019-01-09)
------------------

* #161: Prevent infinite loop on empty xml elements


1.5.0 (2016-10-09)
------------------

Expand Down
33 changes: 29 additions & 4 deletions lib/Deserializer/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,20 @@ function keyValue(Reader $reader, $namespace = null) {
return [];
}

if (!$reader->read()) {
$reader->next();

return [];
}

if (Reader::END_ELEMENT === $reader->nodeType) {
$reader->next();

return [];
}

$values = [];

$reader->read();
do {

if ($reader->nodeType === Reader::ELEMENT) {
Expand All @@ -79,7 +90,9 @@ function keyValue(Reader $reader, $namespace = null) {
$values[$clark] = $reader->parseCurrentElement()['value'];
}
} else {
$reader->read();
if (!$reader->read()) {
break;
}
}
} while ($reader->nodeType !== Reader::END_ELEMENT);

Expand Down Expand Up @@ -144,7 +157,17 @@ function enum(Reader $reader, $namespace = null) {
$reader->next();
return [];
}
$reader->read();
if (!$reader->read()) {
$reader->next();

return [];
}

if (Reader::END_ELEMENT === $reader->nodeType) {
$reader->next();

return [];
}
$currentDepth = $reader->depth;

$values = [];
Expand Down Expand Up @@ -204,7 +227,9 @@ function valueObject(Reader $reader, $className, $namespace) {
$reader->next();
}
} else {
$reader->read();
if (!$reader->read()) {
break;
}
}
} while ($reader->nodeType !== Reader::END_ELEMENT);

Expand Down
26 changes: 26 additions & 0 deletions tests/Sabre/Xml/Deserializer/EnumTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,30 @@ function testDeserializeDefaultNamespace() {

}

function testEmptyEnum()
{
$service = new Service();
$service->elementMap['{urn:test}enum'] = 'Sabre\Xml\Deserializer\enum';

$xml = <<<XML
<?xml version="1.0"?>
<root xmlns="urn:test">
<inner>
<enum></enum>
</inner>
</root>
XML;

$result = $service->parse($xml);

$this->assertEquals([[
'name' => '{urn:test}inner',
'value' => [[
'name' => '{urn:test}enum',
'value' => [],
'attributes' => [],
]],
'attributes' => [],
]], $result);
}
}
43 changes: 42 additions & 1 deletion tests/Sabre/Xml/Deserializer/KeyValueTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ function testKeyValue() {
<elem4>foo</elem4>
<elem5>foo &amp; bar</elem5>
</elem3>
<elem6></elem6>
</struct>
</root>
BLA;
Expand Down Expand Up @@ -51,7 +52,8 @@ function testKeyValue() {
'value' => 'foo & bar',
'attributes' => [],
],
]
],
'elem6' => null,
],
'attributes' => [],
]
Expand Down Expand Up @@ -109,4 +111,43 @@ function testKeyValueLoop() {

}

function testEmptyKeyValue()
{
// the nested structure below is necessary to detect if one of the deserialization functions eats to much elements
$input = <<<BLA
<?xml version="1.0"?>
<root xmlns="http://sabredav.org/ns">
<inner>
<struct></struct>
</inner>
</root>
BLA;

$reader = new Reader();
$reader->elementMap = [
'{http://sabredav.org/ns}struct' => function(Reader $reader) {
return keyValue($reader, 'http://sabredav.org/ns');
},
];
$reader->xml($input);
$output = $reader->parse();

$this->assertEquals([
'name' => '{http://sabredav.org/ns}root',
'value' => [
[
'name' => '{http://sabredav.org/ns}inner',
'value' => [
[
'name' => '{http://sabredav.org/ns}struct',
'value' => [],
'attributes' => [],
],
],
'attributes' => [],
],
],
'attributes' => [],
], $output);
}
}
81 changes: 81 additions & 0 deletions tests/Sabre/Xml/ServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace Sabre\Xml;

use Sabre\Xml\Element\KeyValue;

class ServiceTest extends \PHPUnit_Framework_TestCase {

function testGetReader() {
Expand Down Expand Up @@ -123,6 +125,40 @@ function testExpect() {
);
}

/**
* @expectedException \Sabre\Xml\LibXMLException
Copy link

Choose a reason for hiding this comment

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

The support for this annotation will be removed with version 9 of PHPUnit. This would not be a problem if this package did not use the "death star version constraint" on PHPUnit.

My suggestion: use the expectException method instead and define a version for PHPUnit in the Composer configuration – version 8 is around the corner.

Copy link
Member

@staabm staabm Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

thx for noticing.

at first we should get rid of the "death star version constraint".

changes to our unit tests itself are welcome via separate pull requets

Copy link

Choose a reason for hiding this comment

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

@staabm

thx for noticing.

Right, this was only a notice and should be done in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the 1.5 branch - it is on pure maintenance mode. No idea if we should put any further man power into this.

Copy link
Member

Choose a reason for hiding this comment

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

This is the 1.5 branch - it is on pure maintenance mode. No idea if we should put any further man power into this.

the mentioned issues should be fixed on master.

in 1.5 I would only fix the "death star version constraint"

*/
function testInvalidNameSpace()
{
$xml = '<D:propfind xmlns:D="DAV:"><D:prop><bar:foo xmlns:bar=""/></D:prop></D:propfind>';

$util = new Service();
$util->elementMap = [
'{DAV:}propfind' => PropFindTestAsset::class,
];
$util->namespaceMap = [
'http://sabre.io/ns' => 's',
];
$result = $util->expect('{DAV:}propfind', $xml);
}

/**
* @dataProvider providesEmptyPropfinds
*/
function testEmptyPropfind($xml)
{
$util = new Service();
$util->elementMap = [
'{DAV:}propfind' => PropFindTestAsset::class,
];
$util->namespaceMap = [
'http://sabre.io/ns' => 's',
];
$result = $util->expect('{DAV:}propfind', $xml);
$this->assertEquals(false, $result->allProp);
$this->assertEquals([], $result->properties);
}

/**
* @depends testGetReader
*/
Expand Down Expand Up @@ -303,6 +339,16 @@ function testParseClarkNotationFail() {

}

function providesEmptyPropfinds()
{
return [
['<D:propfind xmlns:D="DAV:"><D:prop></D:prop></D:propfind>'],
['<D:propfind xmlns:D="DAV:"><D:prop xmlns:s="http://sabredav.org/ns"></D:prop></D:propfind>'],
['<D:propfind xmlns:D="DAV:"><D:prop/></D:propfind>'],
['<D:propfind xmlns:D="DAV:"><D:prop xmlns:s="http://sabredav.org/ns"/></D:propfind>'],
['<D:propfind xmlns:D="DAV:"><D:prop> </D:prop></D:propfind>'],
];
}
}

/**
Expand All @@ -326,3 +372,38 @@ class OrderStatus {
public $id;
public $label;
}


/**
* asset for testInvalidNameSpace.
*
* @internal
*/
class PropFindTestAsset implements XmlDeserializable
{
public $allProp = false;

public $properties;

static function xmlDeserialize(Reader $reader)
{
$self = new self();

$reader->pushContext();
$reader->elementMap['{DAV:}prop'] = 'Sabre\Xml\Element\Elements';

foreach (KeyValue::xmlDeserialize($reader) as $k => $v) {
switch ($k) {
case '{DAV:}prop':
$self->properties = $v;
break;
case '{DAV:}allprop':
$self->allProp = true;
}
}

$reader->popContext();

return $self;
}
}