[2.3] Fix libxml_use_internal_errors and libxml_disable_entity_loader usage #10493
Conversation
|
Added #9483 as solved issue. For testers, these changes should fix your issue : |
|
So it will still not load the xml or schema, but just surpress the error message, or? |
|
Hi @Tobion. No, it will load the XML. It's about restoring global XML settings after an error occured to avoid trigger https://bugs.php.net/bug.php?id=62577. What I think is that |
|
I see. What about using |
|
I think QtFileLoader could use XmlUtils to reuse some code. Config Component is required for this class anyway. |
| @@ -59,6 +59,8 @@ public function testHtmlIds($css, array $elementsId) | |||
| $this->assertTrue(in_array($element->attributes()->id, $elementsId)); | |||
| } | |||
| } | |||
| libxml_clear_errors(); | |||
| libxml_use_internal_errors(false); | |||
There was a problem hiding this comment.
Shouldn't this be set to the previous value like in other tests?
|
I've reproduced the issue and added a test. A method call I'm gonna check other calls and ensure that empty XML will be discard before an exception is thrown |
|
Updating the issue to WIP, add tasks |
|
PR updated, I've addressed comments. @Tobion I've updated QtFileLoader as well as XliffFileLoader so they use XmlUtils. XmlUtils throws an exception if a document type is present in the XML whereas QtFileLoader did not. Should we throw an exception in case a XML containing a document type is passed to QtFileLoader ? In case we should not, I must revert changes Outside of that check, I think things are done |
|
|
||
| try { | ||
| XmlUtils::loadFile(__DIR__.'/../Fixtures/foo.xml'); | ||
| $this->fail('An exception should have been raised'); |
There was a problem hiding this comment.
This won't work because you are catching all exceptions, and so catching the failure
|
@romainneutron IMO QtFileLoader should also forbig doc types definitions. As far as I know, they have been disabled for the other loaders because of potential security issues. See http://phpsecurity.readthedocs.org/en/latest/Injection-Attacks.html#xml-external-entity-injection |
| $internalErrors = libxml_use_internal_errors(true); | ||
| $disableEntities = libxml_disable_entity_loader(true); | ||
| libxml_clear_errors(); | ||
|
|
||
| $dom = new \DOMDocument(); | ||
| $dom->validateOnParse = true; | ||
| if (!$dom->loadXML(file_get_contents($file), LIBXML_NONET | (defined('LIBXML_COMPACT') ? LIBXML_COMPACT : 0))) { | ||
| if (!$dom->loadXML($content, LIBXML_NONET | (defined('LIBXML_COMPACT') ? LIBXML_COMPACT : 0))) { |
There was a problem hiding this comment.
you fixed the root cause with empty xml. but should it not still use either @ or try..catch in case there are other circumstances where an error is raised. the idea was that libxml_disable_entity_loader is reset to its previous state in any way, wasn't it?
There was a problem hiding this comment.
Yes, the ides was that libxml_disable_entity_loader must be reset in its previous state in any way.
We actually don't need to silently discard any error on DomDocument::loadXML : A warning is generated only if the passed content is equal to '' when cast to string. In any other case, no error/warning are generated because these errors are managed by libxml (we set libxml_use_internal_errors to true)
No exceptions / errors / warning could be thrown or generated
|
I've addressed comments, this PR is updated |
|
+1 |
…tity_loader usage (romainneutron) This PR was merged into the 2.3 branch. Discussion ---------- [2.3] Fix libxml_use_internal_errors and libxml_disable_entity_loader usage | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #9731, #9483 | License | MIT This should fix #9731 and #9483 that seems to be triggered when `libxml_disable_entity_loader` has been called with `true` (see https://bugs.php.net/bug.php?id=62577) As `libxml_disable_entity_loader` is non thread safe (see https://bugs.php.net/bug.php?id=64938) and as we have some calls that might leave the setting to `true`, I think the bug should be fixed. I've checked the use of both `libxml_use_internal_errors` and `libxml_disable_entity_loader` among symfony code. You can see I prefered to skip DomDocument::loadXML warnings using the `@` instead of using `LIBXML_NOERROR | LIBXML_NO_WARNING` because we can log these errors whereas libxml errors would be furtives. - [x] Check calls to DOMDocument::load - [x] Check calls to DOMDocument::loadXML - [x] Check calls to DOMDocument::loadHTML - [x] Check calls to DOMDocument::loadHTMLFile - [x] Add more tests Commits ------- 489b8ae Fix libxml_use_internal_errors and libxml_disable_entity_loader usage
|
Is it possible to merge into 2.4, please? |
|
@maphe older branches are merged into newer ones regurarly, so it will be merged into 2.4 and master at some point. |
|
thanks @jakzal what's the period approximately? |
|
few days. up to a week usually. |
|
OK perfect. Thank you |
…sable_entity_loader usage (ccorliss) This PR was submitted for the 2.4 branch but it was merged into the 2.3 branch instead (closes #11259). Discussion ---------- [Config] Fixed failed config schema loads due to libxml_disable_entity_loader usage | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11258 | License | MIT | Doc PR | N/A PR #10493 helped this issue, but it can still affect users that: 1. Have libxml_disable_entity_loader set to true by default. 2. Experience libxml_disable_entity_loader php bug https://bugs.php.net/bug.php?id=64938 I used the same approach used in the DI xml validation. https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php#L452 Commits ------- de2bef5 Fixed failed config schema loads due to libxml_disable_entity_loader usage.
This should fix #9731 and #9483 that seems to be triggered when
libxml_disable_entity_loaderhas been called withtrue(see https://bugs.php.net/bug.php?id=62577)As
libxml_disable_entity_loaderis non thread safe (see https://bugs.php.net/bug.php?id=64938) and as we have some calls that might leave the setting totrue, I think the bug should be fixed.I've checked the use of both
libxml_use_internal_errorsandlibxml_disable_entity_loaderamong symfony code.You can see I prefered to skip DomDocument::loadXML warnings using the
@instead of usingLIBXML_NOERROR | LIBXML_NO_WARNINGbecause we can log these errors whereas libxml errors would be furtives.