relax force ssl if assuming secure protocol #13374

[geekdenz]

Description:

See commit message.

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

[geekdenz]

Do we need to add documentation for this?

[sgiehl]

we should in general try to avoid using error suppression (@). In this case it shouldn't be needed at all, as assume_secure_protocol is defined in global.ini.php and therefor the key should always be available.
Also I guess in most cases we are checking such boolish config values with == 1

[geekdenz]

Nice! Thanks @sgiehl for looking into this.

I agree with the error suppression and have corrected it. It was in another place also, which I had copied from previously and now also corrected.

However, I don't quite agree with the second point unless it is an accepted convention for this project for historical reasons.

Reasons being: PHP has the concept of falsy and truthy and I think we should be OK to take advantage of that language feature. Comparing with double equals (==) is the same check as checking 'is this variable "onesy"', so true == 1 is true while true === 1 is false. If we were checking for strict equality, we would need to check with triple equals (===).

For example to check if something is strictly false or strictly not false, one would do

<?php
false === $someVariableOrExpression
// or
false !== $someVariableOrExpression

if ($expression) echo 'prints'; // outputs 'prints' if $expression is truthy

Some functions in PHP return false or a mixed value for similar reasons.

[sgiehl]

@geekdenz In general I would agree that using === should be prefered. But when using === we need to cast the value, as the config value could be a string or integer. So something like (int) Config::getInstance()->General['assume_secure_protocol'] === 1 would work.

[sgiehl]

Btw. the reason for doing a comparison here at all was to prevent that a "invalid" config value like a random string would be counted as enabled

[tsteur]

I actually didn't know that. We're actually not doing it everywhere currently and maybe eventually might rather want to change it to trigger an exception then if it's not 0 or 1 as otherwise people won't actually realise they misconfigured it. @sgiehl can you document that we're currently checking == 1 for 0/1 configs in https://developer.matomo.org/guides/piwiks-ini-configuration ?

[tsteur]

Actually @geekdenz for this we have a method in Url::isPiwikConfiguredToAssumeSecureConnection() (which btw converts to bool so we shouldn't regress behaviour there now and generally reuse that method)

[geekdenz]

Thanks. Makes sense. I hadn't considered the case where any string could be truthy unless some special cases. Should we change it in the other place too then?

[geekdenz]

Actually, to make this less of a problem or discussion topic in the future we could have a method in the configuration class that checks if a configuration value is 1.

[Findus23]

@geekdenz Have you seen my post in #13374 (comment)?

I'm not entirely sure if this is a change we should do. After all if Matomo is behind a reverse proxy that adds SSL, then Matomo should still act as if it was using HTTPS (so e.g. create links in E-Mail with https://, etc.) and therefore these people also will need to add force_ssl=1 (which this system check reminds them to do)

[tsteur]

@Findus23 maybe, when assume secure protocol is enabled we could apply additionally same behavior as if force_ssl=1 is set? If HTTPS is supported there might not be a reason to not force it? Although it could be a regression to change this now because if someone was to POST anything to Matomo, and there's a redirect, then this can break API if the POST parameters aren't forwarded to the redirected URL (which usually isn't).

Maybe let's not merge this then and rather find a solution in the FAQ to suggest to also set force_ssl=1 when setting assume_secure_protocol=1. I'll mention this in the issue.