From bbfc7b2ca924bb82f809a1eeb5b3e53c943c43ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Wed, 4 Sep 2024 16:22:10 +0200 Subject: [PATCH] Http: fix how we treat must-revalidate We calculated the expiry at time of download by looking at Max-Age, but as long as must-revalidate was true we would just perform the network request anyway. One issue this, further, highlights is our PreferNetwork and PreferCache options having no behavioral differences while being documented to do different things. They both consult the cache first and returns any non-stale data from there. Fixes: QTBUG-128484 Change-Id: I24b62e2bb072446e463482a778da7cfbd82a6835 Reviewed-by: Mate Barany (cherry picked from commit e4a35df2afc306e566884f3d917daa08e41761f8) Reviewed-by: Qt Cherry-pick Bot --- src/network/access/qnetworkreplyhttpimpl.cpp | 2 - .../qnetworkreply/tst_qnetworkreply.cpp | 44 +++++++++++++++++-- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/src/network/access/qnetworkreplyhttpimpl.cpp b/src/network/access/qnetworkreplyhttpimpl.cpp index b13e9296c89..758729d1e38 100644 --- a/src/network/access/qnetworkreplyhttpimpl.cpp +++ b/src/network/access/qnetworkreplyhttpimpl.cpp @@ -531,8 +531,6 @@ bool QNetworkReplyHttpImplPrivate::loadFromCacheIfAllowed(QHttpNetworkRequest &h value = cacheHeaders.value(QHttpHeaders::WellKnownHeader::CacheControl); if (!value.empty()) { QHash cacheControl = parseHttpOptionHeader(value); - if (cacheControl.contains("must-revalidate"_ba)) - return false; if (cacheControl.contains("no-cache"_ba)) return false; } diff --git a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp index 8d1ecdf2199..4e5ae52639f 100644 --- a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp +++ b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp @@ -4602,8 +4602,9 @@ void tst_QNetworkReply::ioGetFromHttpWithCache_data() // Set must-revalidate now // rawHeaders.clear(); + content.first.setExpirationDate(future); rawHeaders << QNetworkCacheMetaData::RawHeader("Date", QLocale::c().toString(past, dateFormat).toLatin1()) - << QNetworkCacheMetaData::RawHeader("Cache-control", "max-age=7200, must-revalidate"); // must-revalidate is used + << QNetworkCacheMetaData::RawHeader("Cache-control", "max-age=3600, must-revalidate"); // must-revalidate is used content.first.setRawHeaders(rawHeaders); QTest::newRow("must-revalidate,200,always-network") @@ -4611,15 +4612,42 @@ void tst_QNetworkReply::ioGetFromHttpWithCache_data() QTest::newRow("must-revalidate,200,prefer-network") << reply200 << "Reloaded" << content << int(QNetworkRequest::PreferNetwork) << QStringList() << false << true; QTest::newRow("must-revalidate,200,prefer-cache") - << reply200 << "Reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << false << true; + << reply200 << "Not-reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << true << false; QTest::newRow("must-revalidate,200,always-cache") - << reply200 << "" << content << int(QNetworkRequest::AlwaysCache) << QStringList() << false << false; + << reply200 << "Not-reloaded" << content << int(QNetworkRequest::AlwaysCache) << QStringList() << true << false; QTest::newRow("must-revalidate,304,prefer-network") << reply304 << "Not-reloaded" << content << int(QNetworkRequest::PreferNetwork) << QStringList() << true << true; QTest::newRow("must-revalidate,304,prefer-cache") - << reply304 << "Not-reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << true << true; + << reply304 << "Not-reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << true << false; QTest::newRow("must-revalidate,304,always-cache") + << reply304 << "Not-reloaded" << content << int(QNetworkRequest::AlwaysCache) << QStringList() << true << false; + + // + // Set must-revalidate, but also add Age, meaning a 3rd party (i.e. proxy) held on to the + // response for some time + // + rawHeaders.clear(); + content.first.setExpirationDate(past); + rawHeaders << QNetworkCacheMetaData::RawHeader("Cache-control", "max-age=3600, must-revalidate") + // Some 3rd party held on to the response long enough that it expired: + << QNetworkCacheMetaData::RawHeader("Age", "3600"); + content.first.setRawHeaders(rawHeaders); + + QTest::newRow("must-revalidate,200,always-network,expired") + << reply200 << "Reloaded" << content << int(QNetworkRequest::AlwaysNetwork) << QStringList() << false << true; + QTest::newRow("must-revalidate,200,prefer-network,expired") + << reply200 << "Reloaded" << content << int(QNetworkRequest::PreferNetwork) << QStringList() << false << true; + QTest::newRow("must-revalidate,200,prefer-cache,expired") + << reply200 << "Reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << false << true; + QTest::newRow("must-revalidate,200,always-cache,expired") + << reply200 << "" << content << int(QNetworkRequest::AlwaysCache) << QStringList() << false << false; + + QTest::newRow("must-revalidate,304,prefer-network,expired") + << reply304 << "Not-reloaded" << content << int(QNetworkRequest::PreferNetwork) << QStringList() << true << true; + QTest::newRow("must-revalidate,304,prefer-cache,expired") + << reply304 << "Not-reloaded" << content << int(QNetworkRequest::PreferCache) << QStringList() << true << true; + QTest::newRow("must-revalidate,304,always-cache,expired") << reply304 << "" << content << int(QNetworkRequest::AlwaysCache) << QStringList() << false << false; // @@ -4677,7 +4705,15 @@ void tst_QNetworkReply::ioGetFromHttpWithCache() QVERIFY(waitForFinish(reply) != Timeout); + QEXPECT_FAIL("must-revalidate,200,always-cache", + "We assume the response is stale even though it is not", Abort); + QEXPECT_FAIL("must-revalidate,304,always-cache", + "We assume the response is stale even though it is not", Abort); + QEXPECT_FAIL("must-revalidate,200,prefer-network", + "PreferNetwork doesn't prefer network", Abort); QTEST(reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool(), "loadedFromCache"); + QEXPECT_FAIL("must-revalidate,304,prefer-network", + "PreferNetwork doesn't prefer network", Abort); QTEST(server.totalConnections > 0, "networkUsed"); QFETCH(QString, body); QCOMPARE(reply->readAll().constData(), qPrintable(body));