diff --git a/src/core/NetworkRequest.h b/src/core/NetworkRequest.h index f5ce3302a..8817dae19 100644 --- a/src/core/NetworkRequest.h +++ b/src/core/NetworkRequest.h @@ -53,7 +53,8 @@ class NetworkRequest : public QObject QUrl m_requested_url; public: - // TODO Disallow insecure connections by default? + static constexpr auto UNLIMITED_REDIRECTS = std::numeric_limits::max(); + explicit NetworkRequest(QUrl targetURL, bool allowInsecure, unsigned int maxRedirects, std::chrono::milliseconds timeoutDuration, QList> headers, @@ -96,6 +97,8 @@ private slots: signals: void success(QByteArray); void failure(); +private: + Q_DISABLE_COPY(NetworkRequest) }; class NetworkRequestBuilder { diff --git a/tests/TestNetworkRequest.cpp b/tests/TestNetworkRequest.cpp index 7063772cb..4cdae043e 100644 --- a/tests/TestNetworkRequest.cpp +++ b/tests/TestNetworkRequest.cpp @@ -11,11 +11,7 @@ using ContentTypeParameters_t = QHash; Q_DECLARE_METATYPE(ContentTypeParameters_t); Q_DECLARE_METATYPE(std::chrono::milliseconds); -namespace { - void processRequests() { - QTest::qWait(300); - } -} +static constexpr auto TIMEOUT_GRACE_MS = 25; void TestNetworkRequest::testNetworkRequest() { @@ -61,7 +57,7 @@ void TestNetworkRequest::testNetworkRequest() connect(&request, &NetworkRequest::failure, [&didError]() { didError = true; }); request.fetch(); - processRequests(); + QTest::qWait(300); // Ensures that predicates match - i.e., the header was set correctly QCOMPARE(manager.matchedRequests().length(), 1); @@ -156,7 +152,8 @@ void TestNetworkRequest::testNetworkRequestTimeout() connect(&request, &NetworkRequest::failure, [&didError]() { didError = true; }); request.fetch(); - processRequests(); + // Wait until the timeout should have (or not) occured + QTest::qWait(std::chrono::duration_cast(timeout).count() + TIMEOUT_GRACE_MS); QTEST_ASSERT(didError || didSucceed); @@ -229,7 +226,7 @@ void TestNetworkRequest::testNetworkRequestRedirects() connect(&request, &NetworkRequest::failure, [&didError]() { didError = true; }); request.fetch(); - processRequests(); + QTest::qWait(300); QTEST_ASSERT(didError || didSucceed); // Ensures that predicates match - i.e., the header was set correctly @@ -252,4 +249,65 @@ void TestNetworkRequest::testNetworkRequestRedirects_data() QTest::newRow("more redirects than allowed (1, 0)") << 1 << 0; QTest::newRow("more redirects than allowed (2, 1)") << 2 << 1; QTest::newRow("more redirects than allowed (3, 2)") << 3 << 2; -} \ No newline at end of file +} + +void TestNetworkRequest::testNetworkRequestTimeoutWithRedirects() +{ + // Test that the timeout parameter is respected even when redirects are involved: + // - Set up a request that will redirect 2 times + // - Each request should have a delay of 250ms + // - The timeout should be 400ms + // -> The request should fail + const int numRedirects = 3; + const auto delayPerRequest = std::chrono::milliseconds{250}; + const auto timeout = std::chrono::milliseconds{400}; + const auto requestedURL = QUrl("https://example.com"); + + // Create mock reply + // Create and configure the mocked network access manager + MockNetworkAccess::Manager manager; + + QStringList requestedUrls; + + auto* reply = &manager.whenGet(requestedURL).reply(); + + std::vector> timers; + auto nextDelay = delayPerRequest; + for(int i = 0; i < numRedirects; ++i) { + auto redirectTarget = QUrl("https://example.com/redirect" + QString::number(i)); + + auto timer(std::make_unique()); + timer->setSingleShot(true); + timer->start(delayPerRequest); + nextDelay += delayPerRequest; + + reply->withRedirect(redirectTarget).withFinishDelayUntil(timer.get(), &QTimer::timeout); + reply = &manager.whenGet(redirectTarget).reply(); + + timers.push_back(std::move(timer)); + } + reply->withBody(QString{"test-content"}.toUtf8()); + + // Create request + NetworkRequest request = NetworkRequestBuilder(requestedURL).setManager(&manager) + .setTimeout(timeout) + .setMaxRedirects(NetworkRequest::UNLIMITED_REDIRECTS).build(); + + bool didSucceed = false, didError = false; + // Check request + QSignalSpy spy(&request, &NetworkRequest::success); + connect(&request, &NetworkRequest::success, [&didSucceed](const QByteArray&) { + didSucceed = true; + }); + + QSignalSpy errorSpy(&request, &NetworkRequest::failure); + connect(&request, &NetworkRequest::failure, [&didError]() { didError = true; }); + + request.fetch(); + // Wait until the timeout should have occured + QTest::qWait(std::chrono::duration_cast(timeout).count() + TIMEOUT_GRACE_MS); + + QTEST_ASSERT(didError || didSucceed); + QCOMPARE(didSucceed, false); + QCOMPARE(didError, true); +} diff --git a/tests/TestNetworkRequest.h b/tests/TestNetworkRequest.h index 140c6a0f9..cae69415c 100644 --- a/tests/TestNetworkRequest.h +++ b/tests/TestNetworkRequest.h @@ -17,6 +17,8 @@ private slots: void testNetworkRequestRedirects(); void testNetworkRequestRedirects_data(); + + void testNetworkRequestTimeoutWithRedirects(); }; #endif // KEEPASSXC_TESTNETWORKREQUEST_HPP