@mattab opened this Pull Request on May 6th 2021 Member

NOTE: Had to add the risky |raw to the item.comment, which may have security risks if some of the "Informational" diagnostics will contain random content that may be injectable by someone or the system. Maybe we should audit all informational diagnostics?

The new FAQ is: https://matomo.org/faq/troubleshooting/how-to-make-the-diagnostic-managing-processes-via-cli-to-display-ok/

Here is what it looks like:
Screenshot from 2021-05-06 17-01-20

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@sgiehl commented on May 6th 2021 Member

@mattab Actually it seems all other informational results are handled using this method:
https://github.com/matomo-org/matomo/blob/11f54ee6389bd8ac408385b0d1760936c496470c/plugins/Diagnostics/Diagnostic/DiagnosticResult.php#L62-L72
So maybe we could add some kind of html escaping there, to prevent any injection coming from server configuration variables or HTTP headers.

@mattab commented on May 6th 2021 Member

Sounds good @sgiehl - unfortunately i won't have time to finish the work on this so maybe someone can do it :see_no_evil:

@tsteur commented on May 6th 2021 Member

We can look into this eventually as part of 4.4 release

@github-actions[bot] commented on June 4th 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @tsteur @sgiehl @diosmosis @flamisz

@diosmosis commented on June 5th 2021 Member

If we escape the comment we won't be able to put the link in, correct? We could accomplish the same thing w/ another variable that's meant to not contain unsafe input (eg, item.details after item.comment).

@github-actions[bot] commented on June 13th 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @tsteur @sgiehl @diosmosis @flamisz

@sgiehl commented on June 16th 2021 Member

@diosmosis I've rebased the branch and updated it so comments for informational items are escaped by default. Even though it currently seems unneeded, there is a new parameter to disable that.

This Pull Request was closed on June 17th 2021
Powered by GitHub Issue Mirror