@flamisz opened this Pull Request on May 6th 2021 Contributor

Description:

If the config already loaded when we use the getHostname() method, we can use the $checkIfTrusted as true when getting the host.

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
@flamisz commented on May 6th 2021 Contributor

@tsteur @sgiehl

I updated the PR using parameter instead of the try..catch. Using this getHostname() method is very rare in our codebase.

We use it in the core updater in a couple of files and in SettingsPiwik but I didn't modify it there.

@tsteur commented on May 31st 2021 Member

@sgiehl are you saying the trusted host check doesn't work? Could you maybe quickly debug why it's using the IP address for you?

In my case it worked without an issue
image

@sgiehl commented on June 1st 2021 Member

@tsteur Actually I guess the code does exactly what it should. If the current hostname used is untrusted the code falls back to $_SERVER['SERVER_ADDR'], which is the server IP:
https://github.com/matomo-org/matomo/blob/0d979018ce23e8e0187dd435cc8de99fcf3c3585/core/Url.php#L298-L312

Was just wondering if it would be more useful in this case to lookup the configured trusted hosts instead and use one of them instead of the IP. But might be unneeded, as viewing the diagnostics from an untrusted host might be uncommon.

@tsteur commented on June 1st 2021 Member

Ideally if it's not a trusted host then we wouldn't append the matomo-domain option as it wouldn't work anyway. Not sure how easy it is but there should be methods for checking if we are on a trusted host or not.

@sgiehl commented on June 2nd 2021 Member

@tsteur guess only appending the matomo-domain part if Url::isValidHost() returns true should work

@tsteur commented on June 2nd 2021 Member

@flamisz could you look into this?

@flamisz commented on June 2nd 2021 Contributor

@flamisz could you look into this?

yeah, sure

@flamisz commented on June 2nd 2021 Contributor

@tsteur @sgiehl where can I see that matomo-domain parameter? just to be able to test it.

@tsteur commented on June 2nd 2021 Member

@flamisz it's right in the line below $domain = Config::getHostname($checkIfTrusted = true);.

@flamisz commented on June 2nd 2021 Contributor

@flamisz it's right in the line below $domain = Config::getHostname($checkIfTrusted = true);.

@tsteur I meant in the UI or in any console command response. Like your screenshot, where is that from? So when I fix it I can see it works now.

@tsteur commented on June 2nd 2021 Member

@flamisz that's the system check page. You get to it by going to Administration -> Diagnostics -> System Check. It would be shown under Last Successful Archiving Completion. It's possible it won't be shown if archiving ran recently so you might need to temporarily adjust CronArchivingLastRunCheck class to force showing it.

@flamisz commented on June 2nd 2021 Contributor

@flamisz that's the system check page. You get to it by going to Administration -> Diagnostics -> System Check. It would be shown under Last Successful Archiving Completion. It's possible it won't be shown if archiving ran recently so you might need to temporarily adjust CronArchivingLastRunCheck class to force showing it.

thanks, I found it, I'm on it 😄

@flamisz commented on June 2nd 2021 Contributor

@tsteur @sgiehl I updated the PR

This Pull Request was closed on June 11th 2021
Powered by GitHub Issue Mirror