Commit 7e02852d authored by Harald Sitter's avatar Harald Sitter 🏳️‍🌈
Browse files

move bugz api from qurlquery to a custom container

it's a bit meh but we want to get aggressive percent encoding, qurlquery
doesn't give us that.

so what we had before was that one would feed QUQs into the API, then
the Connection would decode the QUQ to more comprehensively encode it
before constructing the final request QUrl that gets passed to KIO.
this had a number of entirely unwanted side effects the QUQ internally
partially recodes from user input and we'd have to select a decoding
choice when getting it out of QUQ. depending on the input that went
gloriously wrong

e.g. password with the verbatim part %FF would decode to unicode

similarly we can't simply use a more limited decoding set because it
couldn't possibly small enough

e.g. a password with the verbatim part %3C would decode to < even when
using DecodeReserved

to solve this properly we now have two distinct states for the queries
represented by the actual objects we deal with.

our Query object is literal input and never transcodes

just before passing to KIO our Query is translated to a QUrlQuery and as
part of that comprehensively encoded through toPercentEncoding. this
should then ensure that what KIO deals with is indeed encoded in a
similar fashion to how a browser would do it, and how bugz expects it.

to ensure this works correctly the connection test has been extended
accordingly

BUG: 435442
FIXED-IN: 5.21.5


(cherry picked from commit d657b0e6)
parent 83b5de15
......@@ -3,6 +3,7 @@ set(lib_SRCS
bugzilla.cpp
connection.cpp
exceptions.cpp
query.cpp
clients/attachmentclient.cpp
clients/bugclient.cpp
......
......@@ -22,7 +22,7 @@ public:
Q_UNREACHABLE();
}
virtual APIJob *get(const QString &path, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *get(const QString &path, const Query &query = Query()) const override
{
Q_UNUSED(path)
Q_UNUSED(query)
......@@ -30,7 +30,7 @@ public:
return nullptr;
}
virtual APIJob *post(const QString &path, const QByteArray &data, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *post(const QString &path, const QByteArray &data, const Query &query = Query()) const override
{
Q_UNUSED(query);
if (path == "/bug/1/attachment") {
......@@ -43,7 +43,7 @@ public:
return nullptr;
}
virtual APIJob *put(const QString &path, const QByteArray &, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *put(const QString &path, const QByteArray &, const Query &query = Query()) const override
{
Q_UNUSED(path)
Q_UNUSED(query)
......
......@@ -22,7 +22,7 @@ public:
Q_UNREACHABLE();
}
virtual APIJob *get(const QString &path, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *get(const QString &path, const Query &query = Query()) const override
{
if (path == "/field/bug/rep_platform" && query.toString() == "") {
return new JobDouble{QFINDTESTDATA("data/field.rep_platform.json")};
......@@ -31,13 +31,13 @@ public:
return nullptr;
}
virtual APIJob *post(const QString &path, const QByteArray &, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *post(const QString &path, const QByteArray &, const Query &query = Query()) const override
{
Q_ASSERT_X(false, "post", qUtf8Printable(QStringLiteral("unmapped: %1; %2").arg(path, query.toString())));
return nullptr;
}
virtual APIJob *put(const QString &path, const QByteArray &, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *put(const QString &path, const QByteArray &, const Query &query = Query()) const override
{
Q_ASSERT_X(false, "put", qUtf8Printable(QStringLiteral("unmapped: %1; %2").arg(path, query.toString())));
return nullptr;
......
......@@ -49,7 +49,7 @@ public:
Q_UNREACHABLE();
}
virtual APIJob *get(const QString &path, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *get(const QString &path, const Query &query = Query()) const override
{
if (path == "/bug" && query.toString() == "product=dragonplayer") {
return new JobDouble{QFINDTESTDATA("data/bugs.dragonplayer.json")};
......@@ -61,7 +61,7 @@ public:
return nullptr;
}
virtual APIJob *post(const QString &path, const QByteArray &data, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *post(const QString &path, const QByteArray &data, const Query &query = Query()) const override
{
qDebug() << path << query.toString();
if (path == "/bug" && query.isEmpty()) {
......@@ -79,7 +79,7 @@ public:
return nullptr;
}
virtual APIJob *put(const QString &path, const QByteArray &data, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *put(const QString &path, const QByteArray &data, const Query &query = Query()) const override
{
if (path == "/bug/54321" && query.isEmpty()) {
QJsonParseError e;
......
......@@ -22,7 +22,7 @@ public:
Q_UNREACHABLE();
}
virtual APIJob *get(const QString &path, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *get(const QString &path, const Query &query = Query()) const override
{
if (path == "/version") {
return new JobDouble{QFINDTESTDATA("data/bugzilla.version.json")};
......@@ -33,13 +33,13 @@ public:
return nullptr;
}
virtual APIJob *post(const QString &path, const QByteArray &, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *post(const QString &path, const QByteArray &, const Query &query = Query()) const override
{
Q_ASSERT_X(false, "post", qUtf8Printable(QStringLiteral("unmapped: %1; %2").arg(path, query.toString())));
return nullptr;
}
virtual APIJob *put(const QString &path, const QByteArray &, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *put(const QString &path, const QByteArray &, const Query &query = Query()) const override
{
Q_ASSERT_X(false, "put", qUtf8Printable(QStringLiteral("unmapped: %1; %2").arg(path, query.toString())));
return nullptr;
......
......@@ -22,7 +22,7 @@ public:
Q_UNREACHABLE();
}
virtual APIJob *get(const QString &path, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *get(const QString &path, const Query &query = Query()) const override
{
if (path == "/bug/407363/comment" && query.toString().isEmpty()) {
return new JobDouble{QFINDTESTDATA("data/comments.json")};
......@@ -34,7 +34,7 @@ public:
return nullptr;
}
virtual APIJob *post(const QString &path, const QByteArray &, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *post(const QString &path, const QByteArray &, const Query &query = Query()) const override
{
Q_UNUSED(path);
Q_UNUSED(query);
......@@ -42,7 +42,7 @@ public:
return nullptr;
}
virtual APIJob *put(const QString &path, const QByteArray &, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *put(const QString &path, const QByteArray &, const Query &query = Query()) const override
{
Q_UNUSED(path);
Q_UNUSED(query);
......
/*
SPDX-FileCopyrightText: 2019 Harald Sitter <sitter@kde.org>
SPDX-FileCopyrightText: 2019-2021 Harald Sitter <sitter@kde.org>
SPDX-License-Identifier: LGPL-2.1-only OR LGPL-3.0-only OR LicenseRef-KDE-Accepted-LGPL
*/
......@@ -70,7 +70,13 @@ private Q_SLOTS:
// needs it encoded which is a bit weird because it doesn't actually
// require full-form encoding either (i.e. space becomes plus and
// plus becomes encoded).
if (httpBlob.startsWith("GET /hi?informal=yes%2Bcertainly")) {
//
// This further broke because we force-recoded the query items but then that caused over-decoding (%FF)
// because QUrlQuery internally stores the DecodeReserved variant and we blindly FullDecode leading to the
// verbatim percent value getting decoded at the same time we can't DecodeReserved because that would
// still decode verbatim reserved sequences form the input password (e.g. the password containing %3C aka <).
// https://bugs.kde.org/show_bug.cgi?id=435442
if (httpBlob.startsWith("GET /hi?informal=yes%2Bcertainly&password=%253C___m%26T9zSZ%3E0%2Cq%25FFDN")) {
QFile file(QFINDTESTDATA("data/hi.http"));
file.open(QFile::ReadOnly | QFile::Text);
socket->write(file.readAll());
......@@ -86,8 +92,9 @@ private Q_SLOTS:
QUrl root("http://localhost");
root.setPort(t.serverPort());
HTTPConnection c(root);
QUrlQuery query;
Query query;
query.addQueryItem("informal", "yes+certainly");
query.addQueryItem("password", "%3C___m&T9zSZ>0,q%FFDN");
auto job = c.get("/hi", query);
job->exec();
QCOMPARE(job->data(), "Hello!\n");
......
......@@ -23,7 +23,7 @@ public:
Q_UNREACHABLE();
}
virtual APIJob *get(const QString &path, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *get(const QString &path, const Query &query = Query()) const override
{
Q_UNUSED(path);
Q_UNUSED(query);
......@@ -34,14 +34,14 @@ public:
return nullptr;
}
virtual APIJob *post(const QString &path, const QByteArray &, const QUrlQuery &query = QUrlQuery()) const override
virtual APIJob *post(const QString &path, const QByteArray &, const Query &query = Query()) const override
{
qDebug() << path << query.toString();
Q_UNREACHABLE();
return nullptr;
}
virtual APIJob *put(const QString &, const QByteArray &, const QUrlQuery & = QUrlQuery()) const override
virtual APIJob *put(const QString &, const QByteArray &, const Query & = Query()) const override
{
Q_UNREACHABLE();
}
......
......@@ -31,7 +31,7 @@ LoginDetails login(KJob *kjob)
APIJob *login(const QString &username, const QString &password, const Connection &connection)
{
QUrlQuery query;
Query query;
query.addQueryItem(QStringLiteral("login"), username);
query.addQueryItem(QStringLiteral("password"), password);
query.addQueryItem(QStringLiteral("restrict_login"), QStringLiteral("true"));
......
......@@ -8,9 +8,9 @@
namespace Bugzilla
{
QUrlQuery BugSearch::toQuery() const
Query BugSearch::toQuery() const
{
QUrlQuery query;
Query query;
QSet<QString> seen;
for (const QString &product : products) {
......
......@@ -24,7 +24,7 @@ class BugSearch : public QueryCommand
BUGZILLA_MEMBER_PROPERTY(QStringList, order);
public:
virtual QUrlQuery toQuery() const override;
virtual Query toQuery() const override;
};
} // namespace Bugzilla
......
......@@ -10,13 +10,13 @@
namespace Bugzilla
{
QUrlQuery QueryCommand::toQuery() const
Query QueryCommand::toQuery() const
{
QUrlQuery query;
Query query;
return expandQuery(query, QSet<QString>());
}
QUrlQuery QueryCommand::expandQuery(QUrlQuery &query, const QSet<QString> &seen) const
Query QueryCommand::expandQuery(Query &query, const QSet<QString> &seen) const
{
const auto propertyCount = metaObject()->propertyCount();
for (int i = 0; i < propertyCount; ++i) {
......
......@@ -10,9 +10,9 @@
#include <QObject>
#include <QSet>
#include <QString>
#include <QUrlQuery>
#include "commandbase.h"
#include "query.h"
namespace Bugzilla
{
......@@ -21,8 +21,8 @@ class QueryCommand : public QObject
public:
using QObject::QObject;
virtual QUrlQuery toQuery() const;
QUrlQuery expandQuery(QUrlQuery &query, const QSet<QString> &seen) const;
virtual Query toQuery() const;
Query expandQuery(Query &query, const QSet<QString> &seen) const;
};
} // namespace Bugzilla
......
/*
SPDX-FileCopyrightText: 2019 Harald Sitter <sitter@kde.org>
SPDX-FileCopyrightText: 2019-2021 Harald Sitter <sitter@kde.org>
SPDX-License-Identifier: LGPL-2.1-only OR LGPL-3.0-only OR LicenseRef-KDE-Accepted-LGPL
*/
......@@ -7,6 +7,7 @@
#include "connection.h"
#include <KIOCore/KIO/TransferJob>
#include <QUrlQuery>
#include "bugzilla_debug.h"
......@@ -54,21 +55,21 @@ void HTTPConnection::setToken(const QString &authToken)
m_token = authToken;
}
APIJob *HTTPConnection::get(const QString &path, const QUrlQuery &query) const
APIJob *HTTPConnection::get(const QString &path, const Query &query) const
{
qCDebug(BUGZILLA_LOG) << path << query.toString();
auto job = new TransferAPIJob(KIO::get(url(path, query), KIO::Reload, KIO::HideProgressInfo));
return job;
}
APIJob *HTTPConnection::post(const QString &path, const QByteArray &data, const QUrlQuery &query) const
APIJob *HTTPConnection::post(const QString &path, const QByteArray &data, const Query &query) const
{
qCDebug(BUGZILLA_LOG) << path << query.toString();
auto job = new TransferAPIJob(KIO::http_post(url(path, query), data, KIO::HideProgressInfo));
return job;
}
APIJob *HTTPConnection::put(const QString &path, const QByteArray &data, const QUrlQuery &query) const
APIJob *HTTPConnection::put(const QString &path, const QByteArray &data, const Query &query) const
{
qCDebug(BUGZILLA_LOG) << path << query.toString();
auto job = new TransferAPIJob(KIO::put(url(path, query), KIO::HideProgressInfo));
......@@ -81,7 +82,7 @@ QUrl HTTPConnection::root() const
return m_root;
}
QUrl HTTPConnection::url(const QString &appendix, QUrlQuery query) const
QUrl HTTPConnection::url(const QString &appendix, Query query) const
{
QUrl url(m_root);
url.setPath(m_root.path() + appendix);
......@@ -91,13 +92,12 @@ QUrl HTTPConnection::url(const QString &appendix, QUrlQuery query) const
}
// https://bugs.kde.org/show_bug.cgi?id=413920
// Force encoding. QUrlQuery by default wouldn't encode '+' and bugzilla doesn't like that...
// Force encoding. Query by default wouldn't encode '+' and bugzilla doesn't like that...
// For any query argument. Tested with username, password, and products (for bug search)
// on bugzilla 5.0.6. As a result let's force full encoding on every argument.
QUrlQuery escapedQuery(query); // copy delimiter properties and the like
escapedQuery.clear(); // but then throw away the values
for (const auto &pair : query.queryItems(QUrl::FullyDecoded)) {
escapedQuery.addQueryItem(pair.first, QString::fromUtf8(QUrl::toPercentEncoding(pair.second)));
QUrlQuery escapedQuery;
for (auto it = query.cbegin(); it != query.cend(); ++it) {
escapedQuery.addQueryItem(it.key(), QString::fromUtf8(it.value().toUtf8().toPercentEncoding()));
}
url.setQuery(escapedQuery);
......
......@@ -9,10 +9,10 @@
#include <QObject>
#include <QUrl>
#include <QUrlQuery>
#include "apijob.h"
#include "exceptions.h"
#include "query.h"
namespace Bugzilla
{
......@@ -30,9 +30,9 @@ public:
virtual void setToken(const QString &authToken) = 0;
virtual APIJob *get(const QString &path, const QUrlQuery &query = QUrlQuery()) const = 0;
virtual APIJob *post(const QString &path, const QByteArray &data, const QUrlQuery &query = QUrlQuery()) const = 0;
virtual APIJob *put(const QString &path, const QByteArray &data, const QUrlQuery &query = QUrlQuery()) const = 0;
virtual APIJob *get(const QString &path, const Query &query = Query()) const = 0;
virtual APIJob *post(const QString &path, const QByteArray &data, const Query &query = Query()) const = 0;
virtual APIJob *put(const QString &path, const QByteArray &data, const Query &query = Query()) const = 0;
};
/**
......@@ -41,20 +41,22 @@ public:
class HTTPConnection : public Connection
{
Q_OBJECT
friend class ConnectionTest;
public:
explicit HTTPConnection(const QUrl &root = QUrl(QStringLiteral("http://bugstest.kde.org/rest")), QObject *parent = nullptr);
~HTTPConnection();
virtual void setToken(const QString &authToken) override;
virtual APIJob *get(const QString &path, const QUrlQuery &query = QUrlQuery()) const override;
virtual APIJob *post(const QString &path, const QByteArray &data, const QUrlQuery &query = QUrlQuery()) const override;
virtual APIJob *put(const QString &path, const QByteArray &data, const QUrlQuery &query = QUrlQuery()) const override;
virtual APIJob *get(const QString &path, const Query &query = Query()) const override;
virtual APIJob *post(const QString &path, const QByteArray &data, const Query &query = Query()) const override;
virtual APIJob *put(const QString &path, const QByteArray &data, const Query &query = Query()) const override;
QUrl root() const;
private:
QUrl url(const QString &appendix, QUrlQuery query) const;
QUrl url(const QString &appendix, Query query) const;
QUrl m_root;
QString m_token;
......
/*
SPDX-License-Identifier: LGPL-2.1-only OR LGPL-3.0-only OR LicenseRef-KDE-Accepted-LGPL
SPDX-FileCopyrightText: 2021 Harald Sitter <sitter@kde.org>
*/
#include "query.h"
namespace Bugzilla
{
bool Query::hasQueryItem(const QString &key)
{
return contains(key);
}
void Query::addQueryItem(const QString &key, const QString &value)
{
insert(key, value);
}
// Don't use this to do anything other than streaming to qDebug.
// The output is not encoded and thus not necessarily a valid URL query.
QString Query::toString() const
{
QString output;
for (auto it = cbegin(); it != cend(); ++it) {
if (!output.isEmpty()) {
output += QStringLiteral("&");
}
output += it.key() + QStringLiteral("=") + it.value();
}
return output;
}
}
/*
SPDX-License-Identifier: LGPL-2.1-only OR LGPL-3.0-only OR LicenseRef-KDE-Accepted-LGPL
SPDX-FileCopyrightText: 2021 Harald Sitter <sitter@kde.org>
*/
#pragma once
#include <QMap>
#include <QString>
namespace Bugzilla
{
// Query container. Do not use QUrlQuery. Since bugzilla wants more encoding
// than QUrlQuery would provide by default we always store the input values
// in this Query container. Only once we are actually ready to construct
// the final request is this container converted to a QUrlQuery.
//
// NB: This class intentionally has no QUrlQuery converter function because
// any behavior we need must be implemented in this container, not run
// through QUrlQuery to prevent encoding confusion.
//
// QMap is used as base because order makes test assertions easier to check.
class Query : public QMap<QString, QString>
{
public:
using QMap<QString, QString>::QMap;
// Compat rigging so this feels like QUrlQuery and reduces porting
// noise for bugfix backport.
bool hasQueryItem(const QString &key);
void addQueryItem(const QString &key, const QString &value);
// Don't use this to do anything other than streaming to qDebug.
// The output is not encoded and thus not necessarily a valid URL query.
QString toString() const;
};
}
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment