@Findus23 opened this Issue on June 7th 2018 Member

Another beginner friendly bug:

When I click on the ISP Rostelecom it wan't to go to http://www.rostelecom/ as it thinks the providername is an URL ending with com. As many ISPs are called telecom I guess this could happen quite often:
https://github.com/matomo-org/matomo/blob/9243b9a7b6fae8237596f76cda1fe8b6816463af/plugins/Provider/Provider.php#L82-L84
https://github.com/matomo-org/matomo/blob/9243b9a7b6fae8237596f76cda1fe8b6816463af/plugins/Provider/Provider.php#L118-L126

PS: This code was added in def7586305ef208ec0456e5840cb239a25ff9124 11 years ago!

@sunsplat commented on June 21st 2018

I would like to work on this!

@Findus23 where do you click on the ISP in the web interface?

@Findus23 commented on June 22nd 2018 Member
@justju commented on June 22nd 2018 Contributor

Hi,

I don't think it's the com-ending. This seems to happen with providers, that only have no spaces.

test

@sunsplat commented on June 26th 2018

I found the issue here:

function getHostnameUrl($in)
{
    if ($in == DataTable::LABEL_SUMMARY_ROW || empty($in) || strtolower($in) === 'ip') {
        return null;
    }

    // if the name looks like it can be used in a URL, use it in one, otherwise link to startpage
    if (preg_match("/^[-a-zA-Z0-9_.]+$/", $in)) {
        return "http://www." . $in . "/";
    } else {
        return "https://startpage.com/do/search?q=" . urlencode($in);
    }
}

Removing:

if (preg_match("/^[-a-zA-Z0-9_.]+$/", $in)) {
        return "http://www." . $in . "/";
    } else {

will solve the problem, but I don't know if this is good long-term solution.

I think it will take refactoring a bit to get the IP passed in and using gethostbyaddr() to get the url for the provider. This way, it should work no matter the url extension.

@Findus23 Is there a way I can test my change by adding test providers? Are providers saved to the db somewhere?

@sunsplat commented on June 28th 2018

Pull Request made for change: https://github.com/matomo-org/matomo/pull/13110

@justju commented on June 28th 2018 Contributor

How about changing the regex to be more specific, e.g.:
^[-a-zA-Z0-9_.]+\.[-a-zA-Z0-9_]+$

This should match if there is at least a Second-level domain followed by a dot and a Top-level domain after.

@sgiehl commented on June 28th 2018 Member

Guess the easiest way would be to use the hostname validator to check if it's a valid hostname

This Issue was closed on July 1st 2018
Powered by GitHub Issue Mirror