libxml_disable_entity_loader() is deprecated in PHP 8

[alexpott]

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

Description

See https://php.watch/versions/8.0/libxml_disable_entity_loader-deprecation

This approach copies Symfony's approach - see https://github.com/symfony/symfony/blob/4ee85e8e3bcc0d4d57bdc81879e99b1883b4ae83/src/Symfony/Component/DomCrawler/Crawler.php#L225. for example.

[weierophinney]

I have a huge concern.

This functionality was put into place to mitigate XXE and XEE attacks. My question is: does the version of libxml used in PHP 8 remove this function because it no longer loads external entities by default? If so, does it provide ways to do so? And if so, how do you disable that once you have? If it doesn't ever allow loading external entities, then we have no issue. But until that is verified, there's no way I can merge this, as it raises risk for users.

[froschdesign]

@weierophinney
Please see at the PHP documentation:

However, as of libxml 2.9.0 entity substitution is disabled by default, so there is no need to disable the loading of external entities.

https://www.php.net/manual/function.libxml-disable-entity-loader.php

[weierophinney]

@froschdesign That doesn't answer the question, though: is there a mechanism available for enabling entity loading still available, and is PHP exposing it somehow? That method previously allowed enabling as well by passing a different boolean flag to it. If there's a way to enable entity loading in PHP 8 and/or prior PHP versions that use libxml 2.9, we still need to ensure it's disabled in our own code when we do anything that might trigger entity loading otherwise.

[froschdesign]

@weierophinney
Sorry, you are right, my answer was not complete.

we still need to ensure it's disabled

If I understand correctly then it must be activated via options on the constructor of DOMDocument: https://www.php.net/manual/libxml.constants.php#constant.libxml-noent
Which means that nothing needs to be done.

[weierophinney]

If I understand correctly then it must be activated via options on the constructor of DOMDocument: https://www.php.net/manual/libxml.constants.php#constant.libxml-noent
Which means that nothing needs to be done.

That is one of the most misleading constant names I've encountered. 💩 ("NOENT" to mean "enable loading entities"? who thought that was a good idea?!?!)

But verified: it's an option to pass to any of the load*() methods, and if not passed, the default behavior, which is not to load entities, is used. Since we don't pass any options to those methods currently, this works for purposes of the security measures we had in place previously.

[froschdesign]

That is one of the most misleading constant names I've encountered. 💩 ("NOENT" to mean "enable loading entities"? who thought that was a good idea?!?!)

I fully agree with this!

[weierophinney]

I recommend adding a method for this:

public static function disableEntityLoader($flag = true)
{
    if (LIBXML_VERSION < 20900) {
        return $flag;
    }
    return libxml_disable_entity_loader($flag);
}

For the pre-conditions, you would then use:

$disableEntityLoaderFlag = self::disableEntityLoader();

and post conditions would become:

self::disableEntityLoader($disableEntityLoaderFlag);

This will make the code easier to read, and easier to update when we update our minimum supported PHP versions to those that use libxml 2.9+ (as we can grep for those calls).

[froschdesign]

Please use the suggestion from @weierophinney to remove repetitions: #25 (review)

Thanks in advance!

[boesing]

I think this is solved with #29

[froschdesign]

@boesing

I think this is solved with #29

Right!