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

Update HTTPs security check, when client is using HTTP just throw a warning on diagnostic #19527

Merged
merged 25 commits into from Jul 25, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Jul 14, 2022

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

Peter added 2 commits July 14, 2022 15:35
default Matomo request to http
Peter Zhang and others added 5 commits July 14, 2022 17:21
plugins/CoreUpdater/Updater.php Outdated Show resolved Hide resolved
plugins/Installation/lang/en.json Outdated Show resolved Hide resolved
plugins/Marketplace/Api/Client.php Outdated Show resolved Hide resolved
Peter Zhang and others added 5 commits July 19, 2022 09:29
@peterhashair peterhashair added the Needs Review PRs that need a code review label Jul 19, 2022
Peter Zhang and others added 3 commits July 19, 2022 18:07
Copy link
Contributor

@justinvelluppillai justinvelluppillai left a 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.

@justinvelluppillai justinvelluppillai removed the Needs Review PRs that need a code review label Jul 19, 2022
@peterhashair peterhashair added the Needs Review PRs that need a code review label Jul 20, 2022
Peter Zhang and others added 3 commits July 21, 2022 09:58
Co-authored-by: Justin Velluppillai <justin@innocraft.com>
remove https
fix phpcs
Copy link
Member

@sgiehl sgiehl left a 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. 🤔

plugins/Installation/lang/en.json Outdated Show resolved Hide resolved
@@ -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">',
Copy link
Member

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?

@justinvelluppillai
Copy link
Contributor

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

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
Copy link
Member

@sgiehl sgiehl left a 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

@@ -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"
Copy link
Member

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

@peterhashair peterhashair merged commit 797b606 into next_release Jul 25, 2022
@peterhashair peterhashair deleted the 19081-2 branch July 25, 2022 00:07
@sgiehl sgiehl modified the milestone: 4.10.0 May 16, 2023
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants