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
If available, use SERVER_PORT variable as well as SERVER_NAME. #16186
Conversation
@diosmosis do we need to adjust other places like https://github.com/matomo-org/matomo/pull/16169/files#diff-ab05063f21f4243a15f897801ef47a7cR211 and https://github.com/matomo-org/matomo/pull/16169/files#diff-719a15b635df457515639ff08ee921b3R826 too? |
@diosmosis some tests seem to be failing now: eg https://travis-ci.org/github/matomo-org/matomo/jobs/705628957#L669 Otherwise all good to merge |
&& $_SERVER['SERVER_PORT'] != 443 | ||
) { | ||
$k_path_url .= ':' . $_SERVER['SERVER_PORT']; | ||
} |
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.
Any reason for not directly using
$k_path_url .= Url::getHostFromServerNameVar();
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 thought this was part of a library so I didn't want it to depend on Matomo, but looking at the path above, I guess it's actually Matomo code?
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.
yes, it's our configuration for tcpdf. Even though I think some parts of that config could maybe meanwhile be removed.
Updated and fixed the tests. |
Otherwise an install on a non-standard port will not pass the trusted hosts check.