Make voku packages optional #3954
Comments
|
I know it's not optimal, but you can always use Composer to delete the Voku packages. We cannot make this optional - it exposes our users to security issues. For that reason, I am closing the PR associated with this. We could replace it with another package - we tried another earlier but people complained about aspects of it as well. I am working on a different approach which I think could eliminate the need for a sanitizer package; I will leave this issue open while I work on it. |
Fix PHPOffice#3954. In response to a Security Incident, a package was added to the project to sanitize the Html for comments attached to a cell. We have tried at least 2 different packages for this purpose, and users have raised legitimate concerns about both. I believe that adding a sanitizer package, although it addresses the problem, was overkill. Cell comments are a RichText object, and Html Writer already handled RichText cell *values*. Values did not figure in the Incident, and they surely would have done so if they were a problem because it is a lot easier to set up test cases for values than for comments. RichText values were not a problem because they were sanitized with `htmlspecialchars`; if comments were to use the same code that RichText was already using, it would likewise be safely sanitized. As an added bonus, the existing code for comments only uses the plaintext value, but the values code would allow the comments to be styled, just as they are for Xlsx. This is a breaking change - I don't think it will affect a lot of users, but there may be some. It is worth noting that the comment block before function `writeComment` has a link explaining what is being done. That link mentions only styling elements, not other possibilities like hyperlinks. At any rate, if people are putting styling tags (e.g. `<b>`) in their comments for this purpose, those will no longer work; the styling needs to be applied to the RichText elements. That will keep the user code the same regardless of the format of the intended output, which is certainly a good thing, but it is a break. If people are trying to insert non-styling tage (e.g. `<a>`), those will no longer work, just as there is no way to do it for Xlsx (although Excel itself may convert the raw text to a hyperlink, but that's out of scope, at least for now). I also note that, even with the current code, I haven't yet found a way to keep the Html comment in place long enough to actually click on any hyperlinks. The main package being dropped brings several other packages along with it, so the project as a whole will have a slightly lighter footprint than before. The existing `XssVulnerability` test cases are all preserved. Although the final result of the sanitizing changes, it can easily be seen that the results are all harmless.
|
Only the users that use the HTML writer. Maybe disable the entire HTML writer if the voku package is not included? Then it will not be a security risk. I don't need it, and we cannot accept the ini_set() in bootstrap.php. |
|
Can't it just be documented as the responsibility of the caller to sanitize the html if and however they see fit instead of pulling in who knows what for dependencies? |
|
No, it can't just be documented. The absence of any sanitization resulted in a CVE against the product, and that required action. |
|
But what do you think about the idea I offered? |
|
I think the merged PR renders your suggestion moot. There is no longer a need for the users to do anything with their Html. |
This is:
What is the expected behavior?
PhpSpreadsheet is about handling spreadsheets like Excel and LibreCalc files. We don't use the HTML support part of the package at all. While it is great that there is a HTML support, it should be optional.
Move the voku package to the "suggested" part of the composer.json.
The reason for this being a problem is that it in turn gets "voku/portable-utf8" which has a bootstrap.php running a ini_set(). We don't want a open source package to run ini_set on our application without our knowledge. voku/portable-utf8 did take down our system some time ago, so we have a company restriction against using that package. It is too bad if we cannot use phpspreadsheet because of a HTML support that we don't really use.
Does an issue affect all spreadsheet file formats? If not, which formats are affected?
Yes, it affect all formats, but it should only affect the HTML format.
Which versions of PhpSpreadsheet and PHP are affected?
2.*
The text was updated successfully, but these errors were encountered: