Skip to content
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

Html Writer Comments - Breaking Change #3957

Merged
merged 4 commits into from
Apr 14, 2024

Commits on Mar 20, 2024

  1. Html Writer Comments - Breaking Change

    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.
    oleibman committed Mar 20, 2024
    Configuration menu
    Copy the full SHA
    989a4ce View commit details
    Browse the repository at this point in the history

Commits on Mar 21, 2024

  1. Migration Aid

    Provide a means for potentially affected users to test if new logic is in effect so that they can update code beforehand.
    oleibman committed Mar 21, 2024
    Configuration menu
    Copy the full SHA
    a9ac138 View commit details
    Browse the repository at this point in the history

Commits on Mar 27, 2024

  1. Minor Correction

    Sanitize font name.
    oleibman committed Mar 27, 2024
    Configuration menu
    Copy the full SHA
    c695d5c View commit details
    Browse the repository at this point in the history
  2. Typo In Previous PR

    oleibman committed Mar 27, 2024
    Configuration menu
    Copy the full SHA
    f20688c View commit details
    Browse the repository at this point in the history