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

Jump to bottom

Add link to new FAQ how to make the diagnostic “Managing processes via CLI” show Ok #17527

Merged
merged 2 commits into from Jun 17, 2021
Merged

Add link to new FAQ how to make the diagnostic “Managing processes via CLI” show Ok #17527

merged 2 commits into from Jun 17, 2021

Conversation

mattab
Copy link
Member

@mattab mattab commented May 6, 2021

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

@mattab mattab added c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Needs Review PRs that need a code review labels May 6, 2021
@mattab mattab added this to the 4.4.0 milestone May 6, 2021
@sgiehl
Copy link
Member

sgiehl commented May 6, 2021

@mattab Actually it seems all other informational results are handled using this method:

matomo/plugins/Diagnostics/Diagnostic/DiagnosticResult.php

Lines 62 to 72 in 11f54ee

public static function informationalResult($label, $comment = '')
{
if ($comment === true) {
$comment = '1';
} elseif ($comment === false) {
$comment = '0';
}
$result = new self($label);
$result->addItem(new DiagnosticResultItem(self::STATUS_INFORMATIONAL, $comment));
return $result;
}

So maybe we could add some kind of html escaping there, to prevent any injection coming from server configuration variables or HTTP headers.

@mattab
Copy link
Member Author

mattab commented May 6, 2021

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

@tsteur
Copy link
Member

tsteur commented May 6, 2021

We can look into this eventually as part of 4.4 release

@mattab mattab modified the milestones: 4.4.0, 4.3.0 May 26, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jun 4, 2021

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

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jun 4, 2021
@diosmosis
Copy link
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 github-actions bot removed the Stale The label used by the Close Stale Issues action label Jun 5, 2021
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jun 13, 2021
@mattab @sgiehl
Add link to new FAQ how to make the diagnostic “Managing processes vi…
3189c91 
…a CLI” show Ok

NOTE: Had to add the |raw to the item.comment, which may have security risks if some of the "Informational" diagnostics will contain random content that may be injected by someone. 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/
@sgiehl sgiehl force-pushed the cli_process_faq branch 2 times, most recently from 6b13e1e to 873fce4 Compare June 16, 2021 13:19
@sgiehl
Copy link
Member

sgiehl commented Jun 16, 2021

@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.

@sgiehl sgiehl removed the Stale The label used by the Close Stale Issues action label Jun 16, 2021
diosmosis
diosmosis reviewed Jun 16, 2021
View reviewed changes
plugins/Diagnostics/Diagnostic/CronArchivingCheck.php
@@ -77,7 +77,8 @@ public function execute()
}
$comment .= $this->translator->translate('Installation_NotSupported')
. ' ' . $this->translator->translate('Goals_Optional')
. ' (' . $this->translator->translate('General_Reasons') . ': ' . $reasonText . ')';
. ' (' . $this->translator->translate('General_Reasons') . ': ' . $reasonText . ')'
. $this->translator->translate('General_LearnMore', [' <a target="_blank" href="https://matomo.org/faq/troubleshooting/how-to-make-the-diagnostic-managing-processes-via-cli-to-display-ok/">', '</a>']);
$status = DiagnosticResult::STATUS_INFORMATIONAL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgiehl So, below, we still need to set $escapeComment to false, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually not, as it doesn't use the informationalResult method, but created the DiagnosticResultItem item directly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@diosmosis diosmosis merged commit 216aa65 into 4.x-dev Jun 17, 2021
@diosmosis diosmosis deleted the cli_process_faq branch June 17, 2021 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diosmosis diosmosis diosmosis left review comments

@sgiehl sgiehl sgiehl left review comments

Assignees
No one assigned
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Needs Review PRs that need a code review
Projects
None yet
Milestone
4.4.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mattab @sgiehl @tsteur @diosmosis