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
Update HTTPs security check, when client is using HTTP just throw a warning on diagnostic #19527
Conversation
default Matomo request to http
update checks
Co-authored-by: Justin Velluppillai <justin@innocraft.com>
update lang and links
update phpcs
update checks
Co-authored-by: Stefan Giehl <stefan@matomo.org>
revert code style changes
update checks and changes
update screenshots
Co-authored-by: Justin Velluppillai <justin@innocraft.com>
update link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in the right direction @peterhashair . The other thing we want here is to not change the matomo URLs yet - we will do that part in a next step in a future release once users have had a chance to fix their HTTPS or use the new force_matomo_http_request
setting. This PR should result in no change to the updating behaviour from 4.10.0
. Let me know if this needs any clarification.
update https to http as default
Co-authored-by: Justin Velluppillai <justin@innocraft.com>
remove https
fix phpcs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justinvelluppillai Do I understand it correct that the purpose of this PR is to show the user a warning if ssl is not supported to either enable ssl support or enable a config option, that currently would only display another warning?
I guess that is meant to avoid possible issues for users when switching to ssl later, but actually that would only have a big difference if we wait a certain amount of time until we switch to ssl. Otherwise, if it would be to quick, users might directly update to a version where ssl is used and may have never seen the warning before. Guess we would need to do that with Matomo 5 then, where, on the other side, this "breaking" change could be directly implemented, without any prior notice. 🤔
@@ -30,19 +30,24 @@ public function __construct(Translator $translator) | |||
|
|||
public function execute() | |||
{ | |||
$faqLinks = [ | |||
'<a href="https://matomo.org/faq/faq-how-to-disable-https-for-matomo-org-and-api-matomo-org-requests" rel="noreferrer noopener" target="_blank">', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's on purpose that this url isn't yet available?
Yes that's right, we want to release this as stage 1 which will just warn that soon HTTPS will be default, and then we wait a few months before making it default. Even if we could make it default already in Matomo 5.0.0 we want to give users some warning first, so the change to make it default could be Matomo 5.0.0 or 5.1.0 or even later. Of course some users might still update multiple versions and not see the warning but we will still show a system check at that point to help them. |
remove translations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left another minor comment. Otherwise looks good if related tests are passing
plugins/Installation/lang/en.json
Outdated
@@ -159,6 +158,6 @@ | |||
"EmailPrivacyNotice": "Your email address will only be used to send you the newsletter. It is shared with Mad Mimi to do so, but the third-party provider may change. We will not share your email with anyone else or use your it for any other purpose. Unsubscribe at any time. The %1$sprivacy policy%2$s has more info.", | |||
"PerformanceSettingsDesc1": "Your Matomo is set up and ready to track and report on your website's traffic. Set up %1$sCLI archiving%2$s if you find it to be slow. This generates reports in the background, rather than on demand.", | |||
"PerformanceSettingsDesc2": "This requires adding a Matomo command to Cron which can't be done automatically by the installer. %1$sRead our FAQ to learn to set it up yourself.%2$s", | |||
"MatomoSslRequestConfigInfo": "The force_matomo_ssl_request in the general config is turned off, we highly recommend switching it on" | |||
"MatomoHttpRequestConfigInfo": "The force_matomo_http_request in the general config is turned on, we highly recommend switching it off for security reasons. For more details please read our %1$sFAQ%2$s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it sound better to use something like The option force_matomo_ssl_request...
or The flag force_matomo_ssl_request...
update lang
Description:
Fixes: #19081
Part 2: #19549
Update HTTPS security check, when the client is using HTTP just throw a warning on diagnostic
reopen this issue, as we discussed, not to force users to use HTTPS at this stage, only a warning message. In the next stage, we will force HTTPS connections. Ref Here: https://github.com/matomo-org/matomo-security/issues/195
Review