Add check for Content-Type when downloading tracker list #22137

userwiths · 2025-01-10T08:48:05Z

Description

Some users might get confused around platforms like github and their raw links. By mistake such user has entered the HTML page link and each HTML line was taken as a tracker.
In order to fix this issue, we add a check on the Content-Type header of the response. We accept text/plain and show a descriptive error message otherwise.

src/gui/trackersadditiondialog.cpp

glassez · 2025-01-10T10:54:45Z

Some users might get confused around platforms like github and their raw links. By mistake such user has entered the HTML page link and each HTML line was taken as a tracker.
In order to fix this issue, we add a check on the Content-Type header of the response. We accept text/plain and show a descriptive error message otherwise.

The plain text may also contain arbitrary/inappropriate data. Shouldn't the lines themselves be validated instead?

userwiths · 2025-01-10T12:02:43Z

I thought that the text area was meant to be used in order to "preview" the url's a user was about to add.
But I agree that we could limit their chances of entering gibberish accidentally.
Latest commit adds a simple check to see if the text looks like a url.

stalkerok · 2025-01-10T20:49:57Z

~~Doesn't work, incorrect list added without a problem.~~
Working.

But it looks like you need to add checking here as well.

stalkerok · 2025-01-17T15:59:16Z

Tested!

github-actions · 2025-03-29T00:20:07Z

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

userwiths · 2025-03-29T10:02:54Z

@stalkerok @glassez
I see that there is a stale-bot, I'm wondering if there is anything I could do in order to close this PR "naturally" (Merged or Closed due to issues) or keep it open until then, rather than have the bot close it automatically.

luzpaz · 2025-03-30T19:11:21Z

What's left to do here ?

userwiths · 2025-03-31T08:12:36Z

A review maybe.
Don't know if this comment (#22137 (comment)) counts as such or not.

glassez

Perhaps I haven't identified all the problems yet...

glassez · 2025-03-31T08:38:39Z

src/base/net/downloadmanager.h

        QByteArray data;
        Path filePath;
        QString magnetURI;
+        QString contentType;


Suggested change

QByteArray data;

Path filePath;

QString magnetURI;

QString contentType;

QString contentType;

QByteArray data;

Path filePath;

QString magnetURI;

Fields were reordered as requested, contentType appears right before data

glassez · 2025-03-31T08:38:58Z

src/gui/optionsdialog.cpp

@@ -83,6 +83,7 @@
 #include "watchedfolderoptionsdialog.h"
 #include "watchedfoldersmodel.h"
 #include "webui/webui.h"
+#include "base/net/downloadmanager.h"


Wrong order.

It is even redundant since it is included in optionsdialog.h.

glassez · 2025-03-31T08:40:11Z

src/gui/optionsdialog.cpp

@@ -1229,8 +1230,42 @@ void OptionsDialog::saveBittorrentTabOptions() const
    session->setAddTrackersEnabled(m_ui->checkEnableAddTrackers->isChecked());
    session->setAdditionalTrackers(m_ui->textTrackers->toPlainText());

+    auto enabledAddTrackers = m_ui->checkAddTrackersFromURL->isChecked();


Suggested change

auto enabledAddTrackers = m_ui->checkAddTrackersFromURL->isChecked();

const bool isAddTrackersEnabled = m_ui->checkAddTrackersFromURL->isChecked();

glassez · 2025-03-31T08:40:41Z

src/gui/optionsdialog.cpp

@@ -1229,8 +1230,42 @@ void OptionsDialog::saveBittorrentTabOptions() const
    session->setAddTrackersEnabled(m_ui->checkEnableAddTrackers->isChecked());
    session->setAdditionalTrackers(m_ui->textTrackers->toPlainText());

+    auto enabledAddTrackers = m_ui->checkAddTrackersFromURL->isChecked();
+    auto url = m_ui->textTrackersURL->text();


Suggested change

auto url = m_ui->textTrackersURL->text();

const QString url = m_ui->textTrackersURL->text();

glassez · 2025-03-31T08:50:52Z

src/gui/optionsdialog.cpp

@@ -1229,8 +1230,42 @@ void OptionsDialog::saveBittorrentTabOptions() const
    session->setAddTrackersEnabled(m_ui->checkEnableAddTrackers->isChecked());
    session->setAdditionalTrackers(m_ui->textTrackers->toPlainText());

+    auto enabledAddTrackers = m_ui->checkAddTrackersFromURL->isChecked();
+    auto url = m_ui->textTrackersURL->text();
+    if (!url.isEmpty() && enabledAddTrackers)


Isn't it more logical to check whether the feature is enabled first?

Suggested change

if (!url.isEmpty() && enabledAddTrackers)

if (isAddTrackersEnabled && !url.isEmpty())

glassez · 2025-03-31T09:32:49Z

src/gui/trackersadditiondialog.cpp

+{
+    if (endpoint.isEmpty())
+        return 0;
+    QUrl url(endpoint.toString());


Suggested change

QUrl url(endpoint.toString());

const QUrl url {endpoint.toString()};

Changed it, should be ok now.

glassez · 2025-03-31T09:33:47Z

src/gui/trackersadditiondialog.cpp

 void TrackersAdditionDialog::onAccepted() const
 {
    const QList<BitTorrent::TrackerEntryStatus> currentTrackers = m_torrent->trackers();
    const int baseTier = !currentTrackers.isEmpty() ? (currentTrackers.last().tier + 1) : 0;

    QList<BitTorrent::TrackerEntry> entries = BitTorrent::parseTrackerEntries(m_ui->textEditTrackersList->toPlainText());
-    for (BitTorrent::TrackerEntry &entry : entries)
+    for (BitTorrent::TrackerEntry &entry : entries) {


Broken coding style.

Suggested change

for (BitTorrent::TrackerEntry &entry : entries) {

for (BitTorrent::TrackerEntry &entry : entries)

{

glassez · 2025-03-31T09:35:45Z

src/gui/trackersadditiondialog.cpp

 void TrackersAdditionDialog::onAccepted() const
 {
    const QList<BitTorrent::TrackerEntryStatus> currentTrackers = m_torrent->trackers();
    const int baseTier = !currentTrackers.isEmpty() ? (currentTrackers.last().tier + 1) : 0;

    QList<BitTorrent::TrackerEntry> entries = BitTorrent::parseTrackerEntries(m_ui->textEditTrackersList->toPlainText());
-    for (BitTorrent::TrackerEntry &entry : entries)
+    for (BitTorrent::TrackerEntry &entry : entries) {
+        auto isValid = isValidEndpoint(entry.url);


Seems this variable doesn't make much sense.

Suggested change

auto isValid = isValidEndpoint(entry.url);

if (isValidEndpoint(entry.url))

Yeah, you are right, addressed.

glassez · 2025-03-31T09:37:55Z

src/gui/trackersadditiondialog.cpp

+        auto isValid = isValidEndpoint(entry.url);
+        if (!isValid)
+        {
+            QMessageBox::warning(const_cast<TrackersAdditionDialog *>(this), tr("Invalid tracker URL"), tr("The tracker URL \"%1\" is invalid").arg(entry.url));


I would drop const specifier from onAccepted() than use const_cast here.

Sure thing, done.

glassez · 2025-03-31T09:43:01Z

src/gui/trackersadditiondialog.h

@@ -65,6 +65,7 @@ private slots:
 private:
    void saveSettings();
    void loadSettings();
+    int isValidEndpoint(const QStringView &endpoint) const;


This function is independent of TrackersAdditionDialog class, so it can be declared in an anonymous namespace in the trackersadditiondialog.cpp file.

Fair enough, made it an anonymous function.

userwiths · 2025-03-31T13:52:12Z

Ok, all points should be covered now, even those on which I did not comment.

In case there is anything else, feel free to send it my way, im gonna take a look as soon as possible.

PS: thx for the time.

glassez · 2025-04-01T08:19:43Z

src/gui/optionsdialog.cpp

+    if (isAddTrackersEnabled && !url.isEmpty())
+    {
+        Net::DownloadManager::instance()->download(url, Preferences::instance()->useProxyForGeneralPurposes()
+            , this, &OptionsDialog::onAdditionalTrackersDownload);


Suggested change

, this, &OptionsDialog::onAdditionalTrackersDownload);

, this, &OptionsDialog::onAdditionalTrackersDownloadFinished);

glassez · 2025-04-01T10:48:10Z

Well, here's my general review.

I have no particular complaints about the TrackersAdditionDialog.

As for the main option, its processing is done in a dubious way, IMO.
To begin with, it would be more correct to check downloaded trackers list directly in the place where it is processed by the BitTorrent Session, and reject unsuitable data with a message entry in the log.
A preliminary check in the Options dialog may look like a good idea, but it is not implemented correctly. IMO, it is a bad idea to initiate data downloading when applying options. Wouldn't it be better to add a "Test" button to do this explicitly? Otherwise, we have two cases:

The user clicked the "Apply" button. In this case, there is a chance that user will see a corresponding message about an unsuccessful download of trackers.
The user clicked the "Ok" button which closes the dialog immediatelly after applying the options. This is at least a race condition. Most likely, the user will not receive any message at all, since the dialog will be destroyed before that.

Check if the URL from where we are downloading the trackers returns `Content-Type` as `text`. Otherwise consider it might be a non-usable format for this functionality (html,xml,etc.ect.)

…d URL\'s

userwiths · 2025-05-12T11:06:06Z

Hey, I failed to get back to this sooner, but now it should be as you described.

I went with the message box implementation earlier thinking it would be easier for users to understand when/what happens.
I removed the changes done by me to the optionsdialog files.
Now the check is done in sessionimpl and we log a message describing the expected and the received Content-Type in case of mismatch.

If there's anything else I can refine, leave a comment and im gonna take a look at it.

github-actions · 2025-07-12T00:23:01Z

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

userwiths · 2025-07-12T13:45:57Z

@glassez (pinging cause last reviewer/commenter)

I don't mean to bother, but would hate the bot to auto close a possible fix.

xavier2k6 · 2025-07-12T22:30:37Z

Review pending........

glassez · 2025-07-15T12:25:53Z

src/gui/trackersadditiondialog.cpp

@@ -74,14 +74,35 @@ TrackersAdditionDialog::~TrackersAdditionDialog()
    delete m_ui;
 }

-void TrackersAdditionDialog::onAccepted() const
+bool isValidEndpoint(const QStringView endpoint)


This function should be placed inside anonymous namespace above TrackersAdditionDialog definitions.

glassez · 2025-07-15T12:30:56Z

src/gui/trackersadditiondialog.cpp

+        if (!isValidEndpoint(entry.url))
+        {
+            QMessageBox::warning(this, tr("Invalid tracker URL"), tr("The tracker URL \"%1\" is invalid").arg(entry.url));
+            return;


IMO, it looks unfriendly if you reject all the entered trackers because of invalid one. Or am I misunderstood it?

glassez · 2025-07-15T12:35:20Z

src/base/bittorrent/sessionimpl.cpp

@@ -4105,6 +4105,11 @@ void SessionImpl::updateTrackersFromURL()
        {
            if (result.status == Net::DownloadStatus::Success)
            {
+                if (!result.contentType.contains(u"text/plain"_s, Qt::CaseInsensitive))
+                {
+                    LogMsg(tr("Cannot add trackers from URL, expected Content-Type is \'text/plain\' received \"%1\"").arg(result.contentType), Log::WARNING);


Single and double quotes are used inconsistently in this message.

I doubt that single quotes need to be escaped.

The message format seems inappropriate (and even grammatically invalid).

@Chocobo1?

Chocobo1 reviewed Jan 10, 2025

View reviewed changes

src/gui/trackersadditiondialog.cpp Outdated Show resolved Hide resolved

glassez changed the title ~~fix: Add check for Content-Type when downloading tracker list.~~ Add check for Content-Type when downloading tracker list Jan 10, 2025

userwiths requested a review from Chocobo1 January 13, 2025 16:34

github-actions bot added the Stale label Mar 29, 2025

github-actions bot removed the Stale label Mar 30, 2025

xavier2k6 requested a review from a team March 31, 2025 08:22

glassez reviewed Mar 31, 2025

View reviewed changes

userwiths force-pushed the fix/download-tracker-list-content-type-check-13061 branch from c87669f to bc8c6bb Compare March 31, 2025 13:00

userwiths requested a review from glassez March 31, 2025 13:52

glassez reviewed Apr 1, 2025

View reviewed changes

userwiths added 6 commits May 12, 2025 13:07

fix: Add check for Content-Type when downloading tracker list.

67db0c0

Check if the URL from where we are downloading the trackers returns `Content-Type` as `text`. Otherwise consider it might be a non-usable format for this functionality (html,xml,etc.ect.)

Check header after request succeeds, check if added trackers are vali…

db41945

…d URL\'s

Add check to Bittorrent tab in preferences too.

db5fc72

Address PR comments.

c194b45

Remove redundant include.

156f6cf

fix: As advised, move logic away from the settings window into session.

5567d02

userwiths force-pushed the fix/download-tracker-list-content-type-check-13061 branch from 9f9ff75 to 5567d02 Compare May 12, 2025 10:17

return removed empty line

669f8cd

userwiths requested a review from glassez May 12, 2025 11:06

github-actions bot added the Stale label Jul 12, 2025

xavier2k6 requested a review from a team July 12, 2025 18:23

github-actions bot removed the Stale label Jul 13, 2025

glassez reviewed Jul 15, 2025

View reviewed changes

	auto enabledAddTrackers = m_ui->checkAddTrackersFromURL->isChecked();
	const bool isAddTrackersEnabled = m_ui->checkAddTrackersFromURL->isChecked();

	auto url = m_ui->textTrackersURL->text();
	const QString url = m_ui->textTrackersURL->text();

	if (!url.isEmpty() && enabledAddTrackers)
	if (isAddTrackersEnabled && !url.isEmpty())

	QUrl url(endpoint.toString());
	const QUrl url {endpoint.toString()};

	for (BitTorrent::TrackerEntry &entry : entries) {
	for (BitTorrent::TrackerEntry &entry : entries)
	{

	auto isValid = isValidEndpoint(entry.url);
	if (isValidEndpoint(entry.url))

	, this, &OptionsDialog::onAdditionalTrackersDownload);
	, this, &OptionsDialog::onAdditionalTrackersDownloadFinished);

Uh oh!

Add check for Content-Type when downloading tracker list #22137

Are you sure you want to change the base?

Add check for Content-Type when downloading tracker list #22137

Uh oh!

Conversation

userwiths commented Jan 10, 2025 • edited by glassez Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

glassez commented Jan 10, 2025

Uh oh!

userwiths commented Jan 10, 2025

Uh oh!

stalkerok commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stalkerok commented Jan 17, 2025

Uh oh!

github-actions bot commented Mar 29, 2025

Uh oh!

userwiths commented Mar 29, 2025

Uh oh!

luzpaz commented Mar 30, 2025

Uh oh!

userwiths commented Mar 31, 2025

Uh oh!

glassez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

userwiths commented Mar 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glassez commented Apr 1, 2025

Uh oh!

userwiths commented May 12, 2025

Uh oh!

github-actions bot commented Jul 12, 2025

Uh oh!

userwiths commented Jul 12, 2025

Uh oh!

xavier2k6 commented Jul 12, 2025

Uh oh!

Choose a reason for hiding this comment

userwiths commented Jan 10, 2025 •

edited by glassez

Loading

stalkerok commented Jan 10, 2025 •

edited

Loading