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

Add a small PHP 8.0 fix #3

Merged
merged 8 commits into from
Nov 18, 2020
Merged

Add a small PHP 8.0 fix #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0998c41
Update Security.php
xvilo Nov 17, 2020
55716b8
Update minimum php requirement and phpunit
xvilo Nov 18, 2020
1a267d5
Update travis.yml to use php 8
xvilo Nov 18, 2020
c1b12d1
Add OS option
xvilo Nov 18, 2020
e393636
qa: update CI test matrix
weierophinney Nov 18, 2020
d0b105b
qa: use tilde version range for PHP constraint
weierophinney Nov 18, 2020
895415a
qa: update tests to make use of new PHP features
weierophinney Nov 18, 2020
ab35cfd
qa: move CS checks to PHP 7.3
weierophinney Nov 18, 2020
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/.phpunit.result.cache
/clover.xml
/composer.lock
/coveralls-upload.json
Expand Down
33 changes: 13 additions & 20 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,41 +9,34 @@ env:
- COMPOSER_ARGS="--no-interaction"
- COVERAGE_DEPS="php-coveralls/php-coveralls"

matrix:
os:
- linux

jobs:
fast_finish: true
include:
- php: 5.6
env:
- DEPS=lowest
- php: 5.6
env:
- DEPS=latest
- php: 7
env:
- DEPS=lowest
- php: 7
env:
- DEPS=latest
- php: 7.1
- php: 7.3
env:
- DEPS=lowest
- php: 7.1
- php: 7.3
env:
- DEPS=latest
- CS_CHECK=true
- TEST_COVERAGE=true
- php: 7.2
- php: 7.4
env:
- DEPS=lowest
- php: 7.2
- php: 7.4
env:
- DEPS=latest
- php: 7.3
- TEST_COVERAGE=true
- php: nightly
env:
- DEPS=lowest
- php: 7.3
- COMPOSER_ARGS="--no-interaction --ignore-platform-reqs"
- php: nightly
env:
- DEPS=latest
- COMPOSER_ARGS="--no-interaction --ignore-platform-reqs"

before_install:
- if [[ $TEST_COVERAGE != 'true' ]]; then phpenv config-rm xdebug.ini || return 0 ; fi
Expand Down
5 changes: 3 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@
}
},
"require": {
"php": "^5.6 || ^7.0",
"php": "^7.3 || ~8.0.0",
"laminas/laminas-zendframework-bridge": "^1.0"
},
"require-dev": {
"laminas/laminas-coding-standard": "~1.0.0",
"phpunit/phpunit": "^5.7.27 || ^6.5.8 || ^7.1.4"
"phpunit/phpunit": "^9.4",
Copy link
Member

Choose a reason for hiding this comment

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

By bumping phpunit, you definitely have to change almost all tests contain setUp and tearDown aswell as have to check for some assertion methods.

Did you execute tests locally? You might want to fix those tests even without travis. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did run the tests on PHP 8 and via ./vendor/bin/phpunit so not sure what happened there if they would be incompatible. I will double-check and see if I did something wrong 👍

Copy link
Member

Choose a reason for hiding this comment

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

@boesing There are two test cases in the suite, and neither use setUp or tearDown, so tests actually do run!

@xvilo I'm doing updates and testing manually, and will push to your branch for final review shortly. Thanks for getting this started!

"ext-iconv": "*"
},
"autoload": {
"psr-4": {
Expand Down
21 changes: 11 additions & 10 deletions phpunit.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
<?xml version="1.0" encoding="UTF-8"?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd"
bootstrap="vendor/autoload.php"
colors="true">
<phpunit
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd"
Copy link
Member

Choose a reason for hiding this comment

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

We can use the local XML schema definition, otherwise we have to update it with each version..

bootstrap="vendor/autoload.php"
colors="true">
<coverage processUncoveredFiles="true">
<include>
<directory suffix=".php">./src</directory>
</include>
</coverage>

<testsuites>
<testsuite name="laminas-xml Test Suite">
<directory>./test</directory>
</testsuite>
</testsuites>

<filter>
<whitelist processUncoveredFilesFromWhitelist="true">
<directory suffix=".php">./src</directory>
</whitelist>
</filter>
</phpunit>
12 changes: 9 additions & 3 deletions src/Security.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ private static function scanString($xml, DOMDocument $dom = null, $libXmlConstan
}

if (! self::isPhpFpm()) {
$loadEntities = libxml_disable_entity_loader(true);
if (\PHP_VERSION_ID < 80000) {
$loadEntities = libxml_disable_entity_loader(true);
}
$useInternalXmlErrors = libxml_use_internal_errors(true);
}

Expand All @@ -75,7 +77,9 @@ private static function scanString($xml, DOMDocument $dom = null, $libXmlConstan
if (! $result) {
// Entity load to previous setting
if (! self::isPhpFpm()) {
libxml_disable_entity_loader($loadEntities);
if (\PHP_VERSION_ID < 80000) {
libxml_disable_entity_loader($loadEntities);
}
libxml_use_internal_errors($useInternalXmlErrors);
}
return false;
Expand All @@ -94,7 +98,9 @@ private static function scanString($xml, DOMDocument $dom = null, $libXmlConstan

// Entity load to previous setting
if (! self::isPhpFpm()) {
libxml_disable_entity_loader($loadEntities);
if (\PHP_VERSION_ID < 80000) {
libxml_disable_entity_loader($loadEntities);
}
libxml_use_internal_errors($useInternalXmlErrors);
}

Expand Down
72 changes: 41 additions & 31 deletions test/MultibyteTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
namespace LaminasTest\Xml;

use Laminas\Xml\Exception;
use Laminas\Xml\Exception\RuntimeException;
use Laminas\Xml\Security;
use PHPUnit\Framework\TestCase;
use ReflectionMethod;

Expand All @@ -17,7 +19,10 @@
*/
class MultibyteTest extends TestCase
{
public function multibyteEncodings()
/**
* @psalm-return array<array-key, array{0: string, 1: string, 2: int}>
*/
public function multibyteEncodings(): array
{
return [
'UTF-16LE' => ['UTF-16LE', pack('CC', 0xff, 0xfe), 3],
Expand All @@ -27,76 +32,79 @@ public function multibyteEncodings()
];
}

public function getXmlWithXXE()
public function getXmlWithXXE(): string
{
return <<<XML
<?xml version="1.0" encoding="{ENCODING}"?>
<!DOCTYPE methodCall [
<!ENTITY pocdata SYSTEM "file:///etc/passwd">
]>
<methodCall>
<methodName>retrieved: &pocdata;</methodName>
</methodCall>
XML;
<?xml version="1.0" encoding="{ENCODING}"?>
<!DOCTYPE methodCall [
<!ENTITY pocdata SYSTEM "file:///etc/passwd">
]>
<methodCall>
<methodName>retrieved: &pocdata;</methodName>
</methodCall>
XML;
Comment on lines +38 to +45
Copy link
Member

Choose a reason for hiding this comment

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

Just a note, in case anybody wonders why I did this: PHP 7.3 adds the ability to indent heredocs and nowdocs. In each case, any leading whitespace before the closing marker is considered indentation, and stripped from each line of the string. This makes them far more readable when declared in indented code, which was the whole point of the change.

}

/**
* Invoke Laminas\Xml\Security::heuristicScan with the provided XML.
*
* @param string $xml
* @return void
* @throws Exception\RuntimeException
*/
public function invokeHeuristicScan($xml)
public function invokeHeuristicScan(string $xml): void
{
$r = new ReflectionMethod('Laminas\Xml\Security', 'heuristicScan');
$r = new ReflectionMethod(Security::class, 'heuristicScan');
$r->setAccessible(true);
return $r->invoke(null, $xml);
$r->invoke(null, $xml);
}

/**
* @dataProvider multibyteEncodings
* @group heuristicDetection
*/
public function testDetectsMultibyteXXEVectorsUnderFPMWithEncodedStringMissingBOM($encoding, $bom, $bomLength)
{
public function testDetectsMultibyteXXEVectorsUnderFPMWithEncodedStringMissingBOM(
string $encoding,
string $bom,
int $bomLength
): void {
$xml = $this->getXmlWithXXE();
$xml = str_replace('{ENCODING}', $encoding, $xml);
$xml = iconv('UTF-8', $encoding, $xml);
$this->assertNotSame(0, strncmp($xml, $bom, $bomLength));
$this->expectException('Laminas\Xml\Exception\RuntimeException');
$this->expectException(RuntimeException::class);
$this->expectExceptionMessage('ENTITY');
$this->invokeHeuristicScan($xml);
}

/**
* @dataProvider multibyteEncodings
*/
public function testDetectsMultibyteXXEVectorsUnderFPMWithEncodedStringUsingBOM($encoding, $bom)
{
public function testDetectsMultibyteXXEVectorsUnderFPMWithEncodedStringUsingBOM(
string $encoding,
string $bom
): void {
$xml = $this->getXmlWithXXE();
$xml = str_replace('{ENCODING}', $encoding, $xml);
$orig = iconv('UTF-8', $encoding, $xml);
$xml = $bom . $orig;
$this->expectException('Laminas\Xml\Exception\RuntimeException');
$this->expectException(RuntimeException::class);
$this->expectExceptionMessage('ENTITY');
$this->invokeHeuristicScan($xml);
}

public function getXmlWithoutXXE()
public function getXmlWithoutXXE(): string
{
return <<<XML
<?xml version="1.0" encoding="{ENCODING}"?>
<methodCall>
<methodName>retrieved: &pocdata;</methodName>
</methodCall>
XML;
<?xml version="1.0" encoding="{ENCODING}"?>
<methodCall>
<methodName>retrieved: &pocdata;</methodName>
</methodCall>
XML;
}

/**
* @dataProvider multibyteEncodings
*/
public function testDoesNotFlagValidMultibyteXmlAsInvalidUnderFPM($encoding)
public function testDoesNotFlagValidMultibyteXmlAsInvalidUnderFPM(string $encoding): void
{
$xml = $this->getXmlWithoutXXE();
$xml = str_replace('{ENCODING}', $encoding, $xml);
Expand All @@ -113,13 +121,15 @@ public function testDoesNotFlagValidMultibyteXmlAsInvalidUnderFPM($encoding)
* @dataProvider multibyteEncodings
* @group mixedEncoding
*/
public function testDetectsXXEWhenXMLDocumentEncodingDiffersFromFileEncoding($encoding, $bom)
{
public function testDetectsXXEWhenXMLDocumentEncodingDiffersFromFileEncoding(
string $encoding,
string $bom
): void {
$xml = $this->getXmlWithXXE();
$xml = str_replace('{ENCODING}', 'UTF-8', $xml);
$xml = iconv('UTF-8', $encoding, $xml);
$xml = $bom . $xml;
$this->expectException('Laminas\Xml\Exception\RuntimeException');
$this->expectException(RuntimeException::class);
$this->expectExceptionMessage('ENTITY');
$this->invokeHeuristicScan($xml);
}
Expand Down
Loading