-
Notifications
You must be signed in to change notification settings - Fork 15
Add a way to customize the load command to run loadHTML or pass addition... #6
Conversation
For reference #5 |
A different solution would be to provide 2 additional parameters like <?php
$libXMLOptions = LIBXML_HTML_NOIMPLIED | LIBXML_HTML_NODEFDTD | LIBXML_NONET; // default LIBXML_NONET
$loadHTML = true // default false;
XmlSecurity::scan($xml, $dom, $libXMLOptions, $loadHTML); But I think the callback is the most versatile solution since the whole library is essentially just here to securely wrap around an XML load call and if the loadXML and loadHTML receive additional arguments the code does not have to be changed. Also anything above 3 parameters is usually not a good idea for readability |
Any updates on this? Please review |
The one issue I see with this is the fact that inside @Raydiation we need a solution that does not bypass that particular aspect of the code. |
Well the only other solution I can see then is to go for XmlSecurity::scan($xml, $dom, $libXmlOptions, $useLoadHtml); and use $libXmlOptions | LIBXML_NONET inside the method. Would that solution work for you? |
@Raydiation another possibility is a new |
Right, makes sense :) Will do |
Ok, usage is now: $libXmlConstants = LIBXML_HTML_NOIMPLIED | LIBXML_HTML_NODEFDTD;
XmlSecurity::scan($xml, $dom, $libXmlConstants);
XmlSecurity::scanHtml($xml, $dom, $libXmlConstants); The third parameter is optional and will always be binary or'd with LIBXML_NONET |
I've updated the travis config file to update to libxml 2.7.8 to be able to use the LIBXML_HTML_NOIMPLIED | LIBXML_HTML_NODEFDTD headers in the tests if thats ok |
…ional parameters to loadXML remove callable type hint because its not available in php 5.3 remove options from test to not break when using loadHTML on php 5.3 where only one parameter is expected actually cause test to fail if code is removed add comment and clean up test add scanHtml method and allow to pass in libxml headers try to fix coding style fixes update libxml on travis to not fail the tests remove unnecessary if
BTW: PR is under the same license as the whole zend framework, https://github.com/zendframework/zf2/blob/master/LICENSE.txt |
@weierophinney everything ok now? can i get a 👍 ? |
matrix: | ||
allow_failures: | ||
- php: hhvm | ||
|
||
before_install: | ||
# need to update libxml to 2.7.8 to be able to run tests using the | ||
# LIBXML_HTML_NODEFDTD and LIBXML_HTML_NOIMPLIED libxml constant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if those flags are not existing at all? Just a hard failure in the test suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ATM yes, I'll add a check for libxml version and skip the test like mentioned in the phpunit docs, sec ;)
if (!extension_loaded('mysqli')) {
$this->markTestSkipped(
'The MySQLi extension is not available.'
);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -72,6 +72,31 @@ public function testScanDom() | |||
$this->assertEquals($node->nodeValue, 'test'); | |||
} | |||
|
|||
/** | |||
* @requires PHP 5.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about PHP 5.3? Can there be a test for it as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5.3 does not allow any parameters to be passed to loadHtml so LIBXML_NONET can not be passed in, see http://php.net/manual/de/domdocument.loadhtml.php
I'm unsure if this can be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, we need 5.3 compatibility here for now :-\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only option I see atm is to fall back to an xml parser in case php 5.3 and scanHtml is being used. This might yield weird results though so I'm unsure if this is the way to go. Or maybe throw an exception if its used on PHP 5.3 and tell the user to use loadXml instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ezimuel ping on this.
Or maybe throw an exception if its used on PHP 5.3 and tell the user to use loadXml instead?
Can't really do, as there are parts of the codebase loading DOM nodes that are indeed used also in PHP 5.3 context. Writing an HTML parser is not attracting either, and would just be another huge source of trouble.
Since we no longer support 5.3 in this library (currently 5.6 and up), I think the last points are now moot. As such, will review again for a 1.2.0 release. I can likely do the rebase and push back to your branch, @BernhardPosselt . |
Nah, I think we can kill it then :) |
@BernhardPosselt It's a great feature! I've already pulled your code and created #18 to test it - all is green, so it'll be in a 1.2.0 release in a few minutes. 😄 Thanks for the great work! |
Add a way to customize the load command to run
loadHTML
or pass additional parameters toloadXML
Example:
@ezimuel