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

Qt GUI tracklist filter widget may display wrong top-level domain for tlds like .co.uk #19035

Closed
ttys3 opened this issue May 29, 2023 · 10 comments · Fixed by #19062
Closed

Qt GUI tracklist filter widget may display wrong top-level domain for tlds like .co.uk #19035

ttys3 opened this issue May 29, 2023 · 10 comments · Fixed by #19062

Comments

@ttys3
Copy link
Contributor

ttys3 commented May 29, 2023

qBittorrent & operating system versions

qBittorrent v4.5.2
Linux Fedora 38
Qt: 6.4.2
Libtorrent: 2.0.8.0
Boost: 1.78.0
OpenSSL: 3.0.8
zlib: 1.2.13

What is the problem?

found the issue during this PR: #18190

the GUI version has a bug dealing with hostname.

in QString getHost(const QString &url) (

return host.section(u'.', -2, -1);
)

it simply do host.section(u'.', -2, -1), which will cause https://www.google.co.uk become co.uk, not the right google.co.uk

Steps to reproduce

just copy the QString getHost(const QString &url) func out, and do some tests:

#include <QCoreApplication>
#include <QDebug>
#include <QtGlobal>
#include <QtNetwork>

QString getHost(const QString &url)
{
    // We want the domain + tld. Subdomains should be disregarded
    // If failed to parse the domain or IP address, original input should be returned

    const QString host = QUrl(url).host();
    if (host.isEmpty())
        return url;

    // host is in IP format
    if (!QHostAddress(host).isNull())
        return host;

    return host.section(u'.', -2, -1);
}

int main(int argc, char *argv[]) {
    QCoreApplication a(argc, argv);
    qInfo() << "tests: ";

    // QString array
    QStringList list;
    list << "https://www.google.com";
    list << "https://www.google.co.uk";
    list << "https://www.google.jp";
    list << "https://tracker.example.com/?passkey=xxxxx";

    // loop through the array
    for (int i = 0; i < list.size(); ++i) {
        qInfo() << getHost(list.at(i));
    }
// output:
//    "google.com"
//    "co.uk"
//    "google.jp"
//    "example.com"

    return QCoreApplication::exec();
}

Additional context

No response

Log(s) & preferences file(s)

No response

@ttys3 ttys3 changed the title tracklist may get wrong top-level domain for tlds like .co.uk Qt GUI tracklist filter widget may display wrong top-level domain for tlds like .co.uk May 29, 2023
@ttys3
Copy link
Contributor Author

ttys3 commented May 30, 2023

or, maybe take this comment ? #18190 (comment)

@Chocobo1
Copy link
Member

or, maybe take this comment ? #18190 (comment)

If my memory serves me right, we did try that but was quickly reverted due to users backlash, I'm not really sure.
I don't mind if someone wants to try it. 😉

@stalkerok
Copy link
Contributor

Maybe you can make it so that if this is still one tracker, then the user himself could combine it into one?

@glassez
Copy link
Member

glassez commented May 31, 2023

Maybe you can make it so that if this is still one tracker, then the user himself could combine it into one?

It looks like it could be implemented through something like tracker aliases, e.g. if given example.com=tracker1.example.com;tracker2.example.com, then they are all counted as example.com.

@ttys3
Copy link
Contributor Author

ttys3 commented Jun 2, 2023

good to see #19062

@tearfur
Copy link
Contributor

tearfur commented Jun 2, 2023

If my memory serves me right, we did try that but was quickly reverted due to users backlash, I'm not really sure.
I don't mind if someone wants to try it.

I did some tracing, here is the history of the relevant code in reverse chronological order (as far as I can tell).
It doesn't look like there was any backlash/revert of sorts, so I think it is worth a try to try use hostnames instead.

Date Commit PR Change
2023-04-03 0dcb65b #18801 Code was moved from src/gui/transferlistfilterswidget.cpp to src/gui/transferlistfilters/trackersfilterwidget.cpp
2022-03-18 802ec5a #16652 Changed from ordinary string literals to QString literals
2020-12-06 9f0429c #13912 Moved getHost() from TrackerFiltersList to anonymous namespace
2020-06-04 15b2811 #12969 Moved from using QUrl::topLevelDomain() to the current implementation. It doesn't look there was any (intentional) change in behaviour
2015-03-28 f0d5ce4 #2725 The tracker side panel was first implemented

glassez pushed a commit that referenced this issue Jun 5, 2023
Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>

PR #19062.
Closes #19035.
@stalkerok
Copy link
Contributor

Maybe you can make it so that if this is still one tracker, then the user himself could combine it into one?

It looks like it could be implemented through something like tracker aliases, e.g. if given example.com=tracker1.example.com;tracker2.example.com, then they are all counted as example.com.

@glassez, if you can find the time to implement this, that would be very nice. Thanks.

@glassez
Copy link
Member

glassez commented Oct 31, 2023

@stalkerok
Is there really a lot of such trackers (having multiple subdomains) in the wild?

@stalkerok
Copy link
Contributor

Quite a lot. For example

http://tr0.torrent4me.com/ann?uk=
http://tr1.torrent4me.com/ann?uk=
http://tr2.torrent4me.com/ann?uk=
http://tr3.torrent4me.com/ann?uk=
http://tr4.torrent4me.com/ann?uk=
http://tr5.torrent4me.com/ann?uk=

or

http://tr0.tor4me.info/ann?uk=
http://tr1.tor4me.info/ann?uk=
http://tr2.tor4me.info/ann?uk=
http://tr3.tor4me.info/ann?uk=
http://tr4.tor4me.info/ann?uk=
http://tr5.tor4me.info/ann?uk=
http://tr6.tor4me.info/ann?uk=
http://tr10.tor4me.info/ann?uk=
http://tr100.tor4me.info/ann?uk=
http://tr1000.tor4me.info/ann?uk=

It is the same tracker. You can put any digit instead of the tracker number and it will work.
More examples

https://bt.t-ru.org/ann?pk=
https://bt2.t-ru.org/ann?pk=
https://bt3.t-ru.org/ann?pk=
https://bt4.t-ru.org/ann?pk=
http://bt01.nnm-club.cc:2710/ann?uk=
http://bt02.nnm-club.cc:2710/ann?uk=
http://bt01.nnm-club.info:2710/ann?uk=
http://bt02.nnm-club.info:2710/ann?uk=
http://bt01.ipv6.nnm-club.cc:2710/ann?uk=
http://bt02.ipv6.nnm-club.cc:2710/ann?uk=
http://bt01.ipv6.nnm-club.info:2710/ann?uk=
http://bt02.ipv6.nnm-club.info:2710/ann?uk=
http://[2a01:d0:a580:1::2]:2710/ann?uk=

There are only 4 trackers in the screenshot
2023-10-31_152906

@glassez
Copy link
Member

glassez commented Oct 31, 2023

I wonder what is the point in such subdomains...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants