From 96f31d7d8ed1c9ab905c24ac039079aea622f4dc Mon Sep 17 00:00:00 2001
From: Alexey Rusakov <Kitsune-Ral@users.sf.net>
Date: Mon, 4 Oct 2021 18:39:21 +0200
Subject: BaseJob: percent-encode variable path parts

This is meant to spare clients from having to percent-encode
room aliases, v3 event ids etc. that happen to hit the endpoint path.
It is unfair to expect clients to do that since they are not supposed
to care about the shape of CS API, which parameter should be encoded
in which way. The trick (together with the slightly updated GTAD
configuration) is to percent-encode parts that happen to be QStrings
and not `const char[]`'s while passing all constant parts as plain
C character literals. This also allows to make it more certain that
the path is correctly encoded by passing and storing QByteArray's
wherever the path is already encoded, and only use QStrings (next to
const char arrays) before that.

Since the change alters the API contract (even if that contract was
crappy), some crude detection of percent-encoded stuff on input is
inserted; if input is already percent-encoded, a warning is put to
the logs, alerting developers about the change.
---
 lib/jobs/basejob.cpp | 51 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)

(limited to 'lib/jobs/basejob.cpp')

diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp
index 85066024..73762e4f 100644
--- a/lib/jobs/basejob.cpp
+++ b/lib/jobs/basejob.cpp
@@ -71,7 +71,7 @@ public:
 
     // Using an idiom from clang-tidy:
     // http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html
-    Private(HttpVerb v, QString endpoint, const QUrlQuery& q,
+    Private(HttpVerb v, QByteArray endpoint, const QUrlQuery& q,
             RequestData&& data, bool nt)
         : verb(v)
         , apiEndpoint(std::move(endpoint))
@@ -106,7 +106,7 @@ public:
 
     // Contents for the network request
     HttpVerb verb;
-    QString apiEndpoint;
+    QByteArray apiEndpoint;
     QHash<QByteArray, QByteArray> requestHeaders;
     QUrlQuery requestQuery;
     RequestData requestData;
@@ -166,14 +166,36 @@ public:
     }
 };
 
-BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint,
+inline bool isHex(QChar c)
+{
+    return c.isDigit() || (c >= u'A' && c <= u'F') || (c >= u'a' && c <= u'f');
+}
+
+QByteArray BaseJob::encodeIfParam(const QString& paramPart)
+{
+    const auto percentIndex = paramPart.indexOf('%');
+    if (percentIndex != -1 && paramPart.size() > percentIndex + 2
+        && isHex(paramPart[percentIndex + 1])
+        && isHex(paramPart[percentIndex + 2])) {
+        qCWarning(JOBS)
+            << "Developers, upfront percent-encoding of job parameters is "
+               "deprecated since libQuotient 0.7; the string involved is"
+            << paramPart;
+        return QUrl(paramPart, QUrl::TolerantMode).toEncoded();
+    }
+    return QUrl::toPercentEncoding(paramPart);
+}
+
+BaseJob::BaseJob(HttpVerb verb, const QString& name, QByteArray endpoint,
                  bool needsToken)
-    : BaseJob(verb, name, endpoint, QUrlQuery {}, RequestData {}, needsToken)
+    : BaseJob(verb, name, std::move(endpoint), QUrlQuery {}, RequestData {},
+              needsToken)
 {}
 
-BaseJob::BaseJob(HttpVerb verb, const QString& name, const QString& endpoint,
+BaseJob::BaseJob(HttpVerb verb, const QString& name, QByteArray endpoint,
                  const QUrlQuery& query, RequestData&& data, bool needsToken)
-    : d(new Private(verb, endpoint, query, std::move(data), needsToken))
+    : d(new Private(verb, std::move(endpoint), query, std::move(data),
+                    needsToken))
 {
     setObjectName(name);
     connect(&d->timer, &QTimer::timeout, this, &BaseJob::timeout);
@@ -194,13 +216,6 @@ QUrl BaseJob::requestUrl() const { return d->reply ? d->reply->url() : QUrl(); }
 
 bool BaseJob::isBackground() const { return d->inBackground; }
 
-//const QString& BaseJob::apiEndpoint() const { return d->apiUrl.path(); }
-
-//void BaseJob::setApiEndpoint(const QString& apiEndpoint)
-//{
-//    d->apiEndpoint = apiEndpoint;
-//}
-
 const BaseJob::headers_t& BaseJob::requestHeaders() const
 {
     return d->requestHeaders;
@@ -259,13 +274,17 @@ const QNetworkReply* BaseJob::reply() const { return d->reply.data(); }
 
 QNetworkReply* BaseJob::reply() { return d->reply.data(); }
 
-QUrl BaseJob::makeRequestUrl(QUrl baseUrl, const QString& path,
+QUrl BaseJob::makeRequestUrl(QUrl baseUrl, const QByteArray& encodedPath,
                              const QUrlQuery& query)
 {
     // Make sure the added path is relative even if it's not (the official
     // API definitions have the leading slash though it's not really correct).
-    baseUrl = baseUrl.resolved(
-        QUrl(path.mid(path.startsWith('/')), QUrl::TolerantMode));
+    const auto pathUrl =
+        QUrl::fromEncoded(encodedPath.mid(encodedPath.startsWith('/')),
+                          QUrl::StrictMode);
+    Q_ASSERT_X(pathUrl.isValid(), __FUNCTION__,
+               qPrintable(pathUrl.errorString()));
+    baseUrl = baseUrl.resolved(pathUrl);
     baseUrl.setQuery(query);
     return baseUrl;
 }
-- 
cgit v1.2.3