Fixes #16057 #16058
Fixes #16057 #16058
Conversation
| $origins = array( | ||
| 'http://' . $host, | ||
| 'https://' . $host, | ||
| 'http://' . $host . ':80', |
There was a problem hiding this comment.
Thanks for the PR @luismsgomes
btw always adding these ports not sure if it's a good idea or has any side effects. Wonder if we could remove the two lines that always add :80 and :443 assuming that your $host is like https://somedomain.org:443
What is Url::getCurrentHost(null); returning for you?
There was a problem hiding this comment.
Hi,
The main motivation for this PR was exactly that :80 and :443 were missing from the list of urls.
The url returned by Url::getCurrentHost(null); contains https:// and :443 in my case.
Cheers.
There was a problem hiding this comment.
@luismsgomes added few suggestions to prevent we're regressing here on anything.
Basically could even use the logic completely but only change the if (!empty($port) && $port != 80 && $port != 443) { to if (!empty($port)) {
Basically.... if no port is configured, maybe we don't want to add 80 and 443. I think this be still fine for your case. It might be fine to add it just not sure and while it's not needed be good to maybe not add it automatically
There was a problem hiding this comment.
Sorry, my last reply was incorrect: getCurrentHost() returns only the hostname, no scheme and no port in my case.
The function getOrigin() does return the scheme and the port.
There was a problem hiding this comment.
I see 👍 I'll undo my suggestions. @Findus23 I reckon it be fine to also allow the port as we can assume if no port is returned it is running on either port 80 or 443? Maybe if SSL is forced ([General]force_ssl=1), then we would not return the URL with port 80?
There was a problem hiding this comment.
Maybe if SSL is forced ([General]force_ssl=1), then we would not return the URL with port 80?
Yes, that makes sense.
There was a problem hiding this comment.
@luismsgomes can you make that change or do you need help with this?
| $origins = array( | ||
| 'http://' . $host, | ||
| 'https://' . $host, | ||
| 'http://' . $host . ':80', |
There was a problem hiding this comment.
@luismsgomes added few suggestions to prevent we're regressing here on anything.
Basically could even use the logic completely but only change the if (!empty($port) && $port != 80 && $port != 443) { to if (!empty($port)) {
Basically.... if no port is configured, maybe we don't want to add 80 and 443. I think this be still fine for your case. It might be fine to add it just not sure and while it's not needed be good to maybe not add it automatically
|
Looks generally good to me. @Findus23 @sgiehl @diosmosis any other thoughts? Generally thinking because there are some minor changes in there that could break something we might prefer to have this in Matomo 4 and not Matomo 3 where we now only fix security issues and bigger bugs. |
|
Well, this is big for me since without this patch I couldn't login ;-)
That's why I took the effort to fix it.
…On Tue, Jun 16, 2020, 21:32 Thomas Steur ***@***.***> wrote:
Looks generally good to me. @Findus23 <https://github.com/Findus23>
@sgiehl <https://github.com/sgiehl> @diosmosis
<https://github.com/diosmosis> any other thoughts?
Generally thinking because there are some minor changes in there that
could break something we might prefer to have this in Matomo 4 and not
Matomo 3 where we now only fix security issues and bigger bugs.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16058 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACFVCGJIPYOOGV56ZPZDODLRW7JD7ANCNFSM4N3YAU6A>
.
|
|
@luismsgomes sorry about that. There will be only very few releases for 3.X in the future so it might be worth patching it in the worst case. Maybe we could think of a less "risky" version for 3.X where it doesn't change as much. Eg for the case where a port is defined like in https://github.com/matomo-org/matomo/pull/16058/files#diff-e3d1b64ec2cd9e138ad8d5c1a0d09a0eR140 we are now no longer returning the URLs without port. This could be causing trouble to other users maybe (as it's always hard to tell with these things) |
|
@Findus23 can you think of any objections or do you reckon it should be fine? |
|
I know little about how this part works, so I can't comment on it. |
|
@tsteur missed this issue, looks ok to me, |
tsteur
added
the
not-in-changelog
* Fixes matomo-org#16057 * Don't return HTTP URLs is force_ssl is true.
Forgot to commit file in matomo-org#16110 which applies a fix from matomo-org#16058
* Fixes matomo-org#16057 * Don't return HTTP URLs is force_ssl is true.
Forgot to commit file in matomo-org#16110 which applies a fix from matomo-org#16058
Please see #16057 for details on what this commit is fixing.