-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
getLibXmlLoaderOptions() globally affects external XML reading (e.g. Soap) #74
Comments
FYI it's because of these lines: For anyone that's looking for a workaround for your SOAP calls or other external xml reading, call |
Can confirm. Our Symfony app breaks after using PhpSpreadsheet because it fails to properly read XML files. |
@amcsi thx for mentioning a work around, works fine. |
It would be appreciated it someone can come up with a proper fix that is covered by unit tests. Without digging into it I suppose something like remembering previous global settings and re-set them afterwards might be the way to go. What do you think ? |
I did a bit or research on this. Apparently, the incriminated piece of code calls the It seems that that call should be left in the codebase, so maybe the best workaround would be to make an XML reader "factory" for each XML library (we have all of them here: SimpleXML, DOMDocument, and XMLReader) that wraps the code required to open the XML file. Here is an example taken from the last link: $oldValue = libxml_disable_entity_loader(true);
$dom = new DOMDocument();
$dom->loadXML($xml);
libxml_disable_entity_loader($oldValue);
Edit: Just wanted to clarify. The change wouldn't be that big per se, but of the four classes involved, only one is covered by unit tests. I'm not sure if this is something that could be changed with confidence without having all the necessary tests in place. |
Unfortunately unit tests are lacking and there isn't much we can do except to add new one progressively. Your suggestion sounds good to me as long as we are sure that those options only have effect when loading the document, and not later when using the objects. If that would be the case, that would mean we would have to keep "our" options until the end of usage of the reader/writer, which may be hard to do. Is there are a to confirm or infirm that ? Also on a side note and slightly out of scope, but I wouldn't mind getting rid of SimpleXML in favor of DOMDocument. If it's not too much work, not too much risky, and could simplify our code base (future factories), then that could/should be done. |
@agopaul Would you really have to unit test every usage? I would think that you could just create a wrapper class around DOMDocument/SimpleXML, use and unit test those. You already assume that DOMDocument and SimpleXML work as expected due to being external classes, so if you just created a wrapper, you could get away with just testing the differences. |
Theoretically, that is true, but there is a marginal risk of breaking something due to unexpected behavior of the XML libraries involved. Also, I'm not 100% sure that a wrapper would do the job: like @PowerKiKi said, we don't know if the options are only used while loading the XML or if they are also used in other parts of the libraries. Maybe I'll do some testing on the weekend. In the meantime, if this is breaking your application, a quick workaround would be using the |
Coincidently I ran into an error due to this very issue just yesterday when a test suite on a project that I'm working on was failing unexpectedly and sure enough, it was because of this function calls. Anyway, the whole point of disabling the external entities loader via libxml_disable_entity_loader() is to prevent XXE/XEE attack vectors. Digging inside the codebase I've found that @MarkBaker actually fixed the XXE/XEE vulnerability in another way after the commit that added the calls to the So there are 2 fixes for the same problem. A few considerations on @MarkBaker's solution:
I looked up how the Zend Framework team fixed this and they created a library that parses XML safely and they adopted both approaches ( Links:
In other words, I think it's safe to say that these I think this is probably a safe change and we wouldn't need to have a complete test coverage of the readers to do it. I can work on a PR if needed. |
Thanks for your in-depth analysis. If I understand things correctly we can remove |
…not necessary. Prevent setLibXmlLoaderOptions() and getLibXmlLoaderOptions() to call the libxml_disable_entity_loader() function which alter the global state of libxml. See the discussion here PHPOffice#74
Settings: deleted libxml_disable_entity_loader() calls since they're not necessary. Prevent setLibXmlLoaderOptions() and getLibXmlLoaderOptions() to call the libxml_disable_entity_loader() function which alter the global state of libxml. See the discussion here #74
Closed via #113 |
It appears that when
getLibXmlLoaderOptions()
is called, it globally affects external xml reading.This is bad, because just reading excel files with this library has a global side effect, and it breaks Soap.
I created a PR with a failing test: #73
The text was updated successfully, but these errors were encountered: