WebUI: Unable to remove trackers with | in their urls

[yesh0]

qBittorrent & operating system versions

qBittorrent: v4.5.2 x64
Operating system: Arch Linux (6.3.1-zen2-1-zen x86_64)
Qt: 6.4.2
libtorrent-rasterbar: 2.0.8.0

What is the problem?

With Web UI, one cannot possibly remove trackers with | in their urls.

Steps to reproduce

1. Log in to Web UI
2. Add a tracker with | in their url (like https://example.com/a?b=c|d|e) to a seed
3. Now try to remove it

Additional context

The controller handling the removal does not seem to provide a way to escape certain characters:
https://github.com/qbittorrent/qBittorrent/blob/b1492bcd7d339ed1e758402d86005ccd4f11610b/src/webui/api/torrentscontroller.cpp#L810C2-L824

Log(s) & preferences file(s)

No response

[luzpaz]

Any possibility this can make it in to the next release ?

[Piccirello]

| is generally not considered a safe URL character and should be percent encoded. Are there valid trackers that are using a pipe?

Here's a potential patch if we wanted to move forward with this:

diff --git a/src/webui/api/torrentscontroller.cpp b/src/webui/api/torrentscontroller.cpp
index ec94daa26..3596dd952 100644
--- a/src/webui/api/torrentscontroller.cpp
+++ b/src/webui/api/torrentscontroller.cpp
@@ -854,8 +854,20 @@ void TorrentsController::removeTrackersAction()
     if (!torrent)
         throw APIError(APIErrorType::NotFound);
 
+    const bool encoded = parseBool(params()[u"encoded"_s]).value_or(false);
     const QStringList urls = params()[u"urls"_s].split(u'|');
-    torrent->removeTrackers(urls);
+    if (encoded) {
+        QStringList decodedUrls;
+        for (const QString &url : urls)
+        {
+            decodedUrls.append(QUrl::fromPercentEncoding(url.toUtf8()));
+        }
+
+        torrent->removeTrackers(decodedUrls);
+    }
+    else {
+        torrent->removeTrackers(urls);
+    }
 
     if (!torrent->isPaused())
         torrent->forceReannounce();
diff --git a/src/webui/www/private/scripts/prop-trackers.js b/src/webui/www/private/scripts/prop-trackers.js
index 015759a99..ca823560f 100644
--- a/src/webui/www/private/scripts/prop-trackers.js
+++ b/src/webui/www/private/scripts/prop-trackers.js
@@ -220,7 +220,8 @@ window.qBittorrent.PropTrackers = (function() {
             method: 'post',
             data: {
                 hash: current_hash,
-                urls: selectedTrackers.join("|")
+                urls: selectedTrackers.map((t) => encodeURI(t)).join("|"),
+                encoded: true,
             },
             onSuccess: function() {
                 updateData();

[glassez]

Are there valid trackers that are using a pipe?

Unlike #20592, where we are talking about an unintentionally inserted | character, the author of this Issue seems to be referring to existing use case:

2. Add a tracker with | in their url (like https://example.com/a?b=c|d|e) to a seed

IMO, the problem is not that | is passed non-encoded, but that it is used by the endpoint as a value separator.

[Piccirello]

IMO, the problem is not that | is passed non-encoded, but that it is used by the endpoint as a value separator.

Definitely agree, however changing the value separator would be a breaking change. An approach like the patch above would be backwards compatible, if that's important to us. Otherwise changing the value separator is probably easier.

Btw I searched through the Web API docs and the only other endpoint I could find that used | as a separator between URLs is the Install Search Plugin endpoint. So thankfully this issue is fairly localized.

[glassez]

Definitely agree, however changing the value separator would be a breaking change.

Upcoming v5.0 contains breaking changes anyway, so I would fix this issue by replacing the separators.

[Piccirello]

Upcoming v5.0 contains breaking changes anyway, so I would fix this issue by replacing the separators.

Could you point me to some more info on this? Assuming we're using proper semantic versioning for the WebAPI, it's still on v2, meaning we can't break anything. But I'm we're not using semver and/or we're ok with breaking changes then I'd like to fix this.

[glassez]

Upcoming v5.0 contains breaking changes anyway, so I would fix this issue by replacing the separators.

Could you point me to some more info on this? Assuming we're using proper semantic versioning for the WebAPI, it's still on v2, meaning we can't break anything. But I'm we're not using semver and/or we're ok with breaking changes then I'd like to fix this.

In qBittorrent WebAPI version x.y.z the numbers have the following meaning:

1. Increasing x means global redesign of API
2. Increasing y means breaking changes that can break compatibility of existing clients unless they update to support changed API (e.g. changing existing parameters, result formats, removing something etc.)
3. Increasing z means non-breaking changes that aren't supposed to break existing clients (e.g. adding some optional parameters).

[glassez]

Upcoming v5.0 contains breaking changes anyway, so I would fix this issue by replacing the separators.

Could you point me to some more info on this? Assuming we're using proper semantic versioning for the WebAPI, it's still on v2, meaning we can't break anything. But I'm we're not using semver and/or we're ok with breaking changes then I'd like to fix this.

As I stated in #20366 (comment) it is unlikely to be fixed by using another separator character. Doesn't you agree?

[Piccirello]

As I stated in #20366 (comment) it is unlikely to be fixed by using another separator character. Doesn't you agree?

We can enforce the requirement that the individual values are URL encoded. For example, trackers=https://example.com,https://example.org becomes trackers=https%3A%2F%2Fexample.com,https%3A%2F%2Fexample.org. Then we'd be safe to use |, ,, or a variety of other separators.

Otherwise, the only other idea I have is to switch to a different content type, like JSON. But that seems like a bigger architectural change that would warrant changing many APIs instead of just the two affected by this.

[glassez]

We can enforce the requirement that the individual values are URL encoded.

I don't think we should enforce such requirements. (How are you going to do that?)
Wouldn't it be enough to just apply percent-decoding for each list item on the server side? If I'm not mistaken, it would allow the client to send both encoded and non-encoded items, so that the use of encoding would remain the responsibility of the client (and it isn't even "breaking" change of API).

[Piccirello]

I don't think we should enforce such requirements. (How are you going to do that?)

We enforce it via documentation and by percent decoding values server-side. In some cases a client that's not performing percent encoding of values will not work properly, which is expected given this requirement.

Wouldn't it be enough to just apply percent-decoding for each list item on the server side? If I'm not mistaken, it would allow the client to send both encoded and non-encoded items, so that the use of encoding would remain the responsibility of the client (and it isn't even "breaking" change of API).

I suspect this would work in the vast majority of cases, but there may be URLs that intentionally use a percent character where it is not part of an encoded character. From Wikipedia:

URIs that differ only by whether a reserved character is percent-encoded or appears literally are normally considered not equivalent (denoting the same resource) unless it can be determined that the reserved characters in question have no reserved purpose. This determination is dependent upon the rules established for reserved characters by individual URI schemes.

My fear may be overblown and it's possible your approach would work great in practice (which would be ideal), but I'm just not sure.

[glassez]

My fear may be overblown and it's possible your approach would work great in practice (which would be ideal), but I'm just not sure.

The following is quite satisfactory to "my approach", as it does not actually break the compatibility of existing clients:

We enforce it via documentation and by percent decoding values server-side. In some cases a client that's not performing percent encoding of values will not work properly, which is expected given this requirement.

[glassez]

there may be URLs that intentionally use a percent character where it is not part of an encoded character. From Wikipedia:

URIs that differ only by whether a reserved character is percent-encoded or appears literally are normally considered not equivalent (denoting the same resource) unless it can be determined that the reserved characters in question have no reserved purpose. This determination is dependent upon the rules established for reserved characters by individual URI schemes.

I don't believe this could apply to the percent character.
As per https://www.w3.org/Addressing/URL/uri-spec.html:

The percent sign ("%", ASCII 25 hex) is used as the escape character in the encoding scheme and is never allowed for anything else.

[Piccirello]

Good find, I think you are correct. As further evidence, it seems we're already decoding all query parameters without side effects.

Wouldn't it be enough to just apply percent-decoding for each list item on the server side? If I'm not mistaken, it would allow the client to send both encoded and non-encoded items, so that the use of encoding would remain the responsibility of the client (and it isn't even "breaking" change of API).

This now seems like the best path forward. As I mentioned above, we really only need this on torrents/removeTrackers and search/installPlugin.