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

Jump to bottom

Check if config loaded in getHostname() #17526

Merged
merged 4 commits into from Jun 11, 2021
Merged

Check if config loaded in getHostname() #17526

merged 4 commits into from Jun 11, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented May 6, 2021

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 flamisz self-assigned this May 6, 2021
@flamisz flamisz added the Needs Review PRs that need a code review label May 6, 2021
@flamisz flamisz added this to the 4.3.0 milestone May 6, 2021
sgiehl
sgiehl reviewed May 6, 2021
View reviewed changes
core/Config.php Outdated Show resolved Hide resolved
@flamisz
Copy link
Contributor Author

flamisz commented May 6, 2021

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

@mattab mattab modified the milestones: 4.3.0, 4.4.0 May 26, 2021
sgiehl
sgiehl reviewed May 31, 2021
View reviewed changes
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.

The code itself looks ok. But not sure if it fulfills the requirements. ping @tsteur
When visiting the diagnostic page over a trusted host, the hostname is shown in the archiving command. Coming from an untrusted host for me it now always uses the IP address as matomo-domain, even though some trusted hosts are configured...

@tsteur
Copy link
Member

tsteur commented May 31, 2021

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

sgiehl commented Jun 1, 2021

@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:

matomo/core/Url.php

Lines 298 to 312 in 0d97901

public static function getHost($checkIfTrusted = true)
{
$host = self::getHostFromServerVariable();
if (strlen($host) && (!$checkIfTrusted || self::isValidHost($host))) {
return $host;
}
// HTTP/1.0 request doesn't include Host: header
if (isset($_SERVER['SERVER_ADDR'])) {
return $_SERVER['SERVER_ADDR'];
}
return false;
}

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

tsteur commented Jun 1, 2021

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

sgiehl commented Jun 2, 2021

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

@tsteur
Copy link
Member

tsteur commented Jun 2, 2021

@flamisz could you look into this?

@flamisz
Copy link
Contributor Author

flamisz commented Jun 2, 2021

@flamisz could you look into this?

yeah, sure

@flamisz
Copy link
Contributor Author

flamisz commented Jun 2, 2021

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

@tsteur
Copy link
Member

tsteur commented Jun 2, 2021

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

@flamisz
Copy link
Contributor Author

flamisz commented Jun 2, 2021

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

tsteur commented Jun 2, 2021

@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
Copy link
Contributor Author

flamisz commented Jun 2, 2021

@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
Copy link
Contributor Author

flamisz commented Jun 2, 2021

@tsteur @sgiehl I updated the PR

sgiehl
sgiehl approved these changes Jun 11, 2021
View reviewed changes
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.

Seems to work as expected now. I've rebased the branch, if all tests are still working it should be good to merge

@sgiehl sgiehl merged commit 4bf0ab9 into 4.x-dev Jun 11, 2021
@sgiehl sgiehl deleted the gethostnamecheckconfig branch June 11, 2021 09:03
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsteur tsteur tsteur left review comments

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
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
Milestone
4.4.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@flamisz @tsteur @sgiehl @mattab