URI Fragments Cached After 301 Redirect

https://bugs.webkit.org/show_bug.cgi?id=255924 rdar://problem/108535944 Reviewed by Youenn Fablet. We end up reusing the fragments when performing a redirect and as such end up serving old parameters. For now we will just disable caching redirects with fragments. This is a temporary fix; a more proper solution of handling fragment caching in the future is required. * LayoutTests/http/tests/links/redirect-301-clear-memory-cache-expected.txt: Added. * LayoutTests/http/tests/links/redirect-301-clear-memory-cache.html: Added. * LayoutTests/http/tests/links/redirect-301-no-inital-fragments-expected.txt: Added. * LayoutTests/http/tests/links/redirect-301-no-inital-fragments.html: Added. * LayoutTests/http/tests/links/redirect-301-with-fragments-expected.txt: Added. * LayoutTests/http/tests/links/redirect-301-with-fragments.html: Added. * LayoutTests/http/tests/links/resources/redirect-helper.pl: Added. * LayoutTests/http/tests/links/resources/redirect-target.html: Added. * Source/WebCore/loader/cache/CachedResource.cpp: (WebCore::CachedResource::redirectReceived): * Source/WebCore/loader/cache/CachedResourceLoader.cpp: (WebCore::CachedResourceLoader::determineRevalidationPolicy const): * Source/WebCore/loader/cache/CachedResourceRequest.h: (WebCore::CachedResourceRequest::hasFragmentIdentifier const): * Source/WebKit/NetworkProcess/cache/NetworkCache.cpp: (WebKit::NetworkCache::makeUseDecision): (WebKit::NetworkCache::makeStoreDecision): * Source/WebKit/NetworkProcess/cache/NetworkCache.h: Canonical link: https://commits.webkit.org/265906@main
WebKit · Jul 10, 2023 · 2054445 · 2054445
1 parent 28cdfd0
commit 2054445
Show file tree

Hide file tree

Showing 13 changed files with 190 additions and 2 deletions.
diff --git a/LayoutTests/http/tests/links/redirect-301-clear-memory-cache-expected.txt b/LayoutTests/http/tests/links/redirect-301-clear-memory-cache-expected.txt
@@ -0,0 +1,2 @@
+
+PASS: document.getElementById("first_load").contentWindow.location.hash should be #key=foo and is. PASS: document.getElementById("second_load").contentWindow.location.hash should be #key=bar and is.
diff --git a/LayoutTests/http/tests/links/redirect-301-clear-memory-cache.html b/LayoutTests/http/tests/links/redirect-301-clear-memory-cache.html
@@ -0,0 +1,44 @@
+<html>
+<head>
+<title>301 Redirect</title>
+
+<iframe id="first_load" width="200" height="200" src="resources/redirect-helper.pl?301#key=foo"></iframe>
+<iframe id="second_load" width="200" height="200"></iframe>
+<div id="console"></div>
+
+<script>
+    function shouldBe(a, b)
+    {
+        var log = (s) => { document.getElementById("console").appendChild(document.createTextNode(s + "\n")); }
+
+        var evalA = eval(a);
+        if (evalA == b) 
+            log("PASS: " + a + " should be " + b + " and is.\n", "green");
+        else {
+            log("FAIL: " + a + " should be " + b + " but instead is " + evalA + ".", "red");
+        }
+    }
+
+    if (window.testRunner) {
+        testRunner.waitUntilDone();
+    }
+
+    document.getElementById('first_load').onload = function() {
+        shouldBe('document.getElementById("first_load").contentWindow.location.hash', "#key=foo");
+
+        // Clear memory cache
+        internals.clearMemoryCache();
+
+        document.getElementById('second_load').src = "resources/redirect-helper.pl?301#key=bar"
+
+        document.getElementById('second_load').onload = function() {
+            shouldBe('document.getElementById("second_load").contentWindow.location.hash', "#key=bar");
+
+            if (window.testRunner) {
+              testRunner.dumpAsText();
+              testRunner.notifyDone();
+            }
+        }
+    }
+</script>
+</html>
diff --git a/LayoutTests/http/tests/links/redirect-301-no-inital-fragments-expected.txt b/LayoutTests/http/tests/links/redirect-301-no-inital-fragments-expected.txt
@@ -0,0 +1,2 @@
+
+PASS: document.getElementById("first_load").contentWindow.location.hash should be and is. PASS: document.getElementById("second_load").contentWindow.location.hash should be #key=bar and is.
diff --git a/LayoutTests/http/tests/links/redirect-301-no-inital-fragments.html b/LayoutTests/http/tests/links/redirect-301-no-inital-fragments.html
@@ -0,0 +1,41 @@
+<html>
+<head>
+<title>301 Redirect</title>
+
+<iframe id="first_load" width="200" height="200" src="resources/redirect-helper.pl?301"></iframe>
+<iframe id="second_load" width="200" height="200"></iframe>
+<div id="console"></div>
+
+<script>
+    function shouldBe(a, b)
+    {
+        var log = (s) => { document.getElementById("console").appendChild(document.createTextNode(s + "\n")); }
+
+        var evalA = eval(a);
+        if (evalA == b) 
+            log("PASS: " + a + " should be " + b + " and is.\n", "green");
+        else {
+            log("FAIL: " + a + " should be " + b + " but instead is " + evalA + ".", "red");
+        }
+    }
+
+    if (window.testRunner) {
+        testRunner.waitUntilDone();
+    }
+
+    document.getElementById('first_load').onload = function() {
+        shouldBe('document.getElementById("first_load").contentWindow.location.hash', "");
+
+        document.getElementById('second_load').src = "resources/redirect-helper.pl?301#key=bar"
+
+        document.getElementById('second_load').onload = function() {
+            shouldBe('document.getElementById("second_load").contentWindow.location.hash', "#key=bar");
+
+            if (window.testRunner) {
+              testRunner.dumpAsText();
+              testRunner.notifyDone();
+            }
+        }
+    }
+</script>
+</html>
diff --git a/LayoutTests/http/tests/links/redirect-301-with-fragments-expected.txt b/LayoutTests/http/tests/links/redirect-301-with-fragments-expected.txt
@@ -0,0 +1,2 @@
+
+PASS: document.getElementById("first_load").contentWindow.location.hash should be #key=foo and is. PASS: document.getElementById("second_load").contentWindow.location.hash should be #key=bar and is.
diff --git a/LayoutTests/http/tests/links/redirect-301-with-fragments.html b/LayoutTests/http/tests/links/redirect-301-with-fragments.html
@@ -0,0 +1,41 @@
+<html>
+<head>
+<title>301 Redirect</title>
+
+<iframe id="first_load" width="200" height="200" src="resources/redirect-helper.pl?301#key=foo"></iframe>
+<iframe id="second_load" width="200" height="200"></iframe>
+<div id="console"></div>
+
+<script>
+    function shouldBe(a, b)
+    {
+        var log = (s) => { document.getElementById("console").appendChild(document.createTextNode(s + "\n")); }
+
+        var evalA = eval(a);
+        if (evalA == b) 
+            log("PASS: " + a + " should be " + b + " and is.\n", "green");
+        else {
+            log("FAIL: " + a + " should be " + b + " but instead is " + evalA + ".", "red");
+        }
+    }
+
+    if (window.testRunner) {
+        testRunner.waitUntilDone();
+    }
+
+    document.getElementById('first_load').onload = function() {
+        shouldBe('document.getElementById("first_load").contentWindow.location.hash', "#key=foo");
+
+        document.getElementById('second_load').src = "resources/redirect-helper.pl?301#key=bar"
+
+        document.getElementById('second_load').onload = function() {
+            shouldBe('document.getElementById("second_load").contentWindow.location.hash', "#key=bar");
+
+            if (window.testRunner) {
+              testRunner.dumpAsText();
+              testRunner.notifyDone();
+            }
+        }
+    }
+</script>
+</html>
diff --git a/LayoutTests/http/tests/links/resources/redirect-helper.pl b/LayoutTests/http/tests/links/resources/redirect-helper.pl
@@ -0,0 +1,25 @@
+#!/usr/bin/perl
+# Script to generate a 30x HTTP redirect (determined by the query parameter)
+
+$REDIRECT_CODE = $ENV{'QUERY_STRING'} || '301';
+
+$STATUS_TEXTS = {
+  '301' => 'Moved Permanently',
+  '302' => 'Moved Temporarily',
+  '303' => 'See Other',
+  '307' => 'Moved Temporarily'
+};
+
+print "Status: $REDIRECT_CODE $STATUS_TEXTS{$REDIRECT_CODE}\r\n";
+print "Location: redirect-target.html\r\n";
+print "Content-type: text/html\r\n";
+print "\r\n";
+
+print <<HERE_DOC_END
+<html>
+<head>
+<title>$REDIRECT_CODE Redirect</title>
+
+<body>This page is a $REDIRECT_CODE redirect.</body>
+</html>
+HERE_DOC_END
diff --git a/LayoutTests/http/tests/links/resources/redirect-target.html b/LayoutTests/http/tests/links/resources/redirect-target.html
@@ -0,0 +1,7 @@
+<html>
+<head>
+<title>Redirect Target</title>
+<body>
+<p>This page is the target of a redirect.</p>
+</body>
+</html>
diff --git a/Source/WebCore/loader/cache/CachedResource.cpp b/Source/WebCore/loader/cache/CachedResource.cpp
@@ -453,6 +453,12 @@ void CachedResource::redirectReceived(ResourceRequest&& request, const ResourceR
 {
     CACHEDRESOURCE_RELEASE_LOG("redirectReceived:");
 
+    // Remove redirect urls from the memory cache if they contain a fragment.
+    // If we cache localhost/#key=foo we will return the same parameters key=foo
+    // on the next visit to the domain even if the parameters have been updated.
+    if (request.url().hasFragmentIdentifier())
+        MemoryCache::singleton().remove(*this);
+
     m_requestedFromNetworkingLayer = true;
     if (!response.isNull())
         updateRedirectChainStatus(m_redirectChainCacheStatus, response);

diff --git a/Source/WebCore/loader/cache/CachedResourceLoader.cpp b/Source/WebCore/loader/cache/CachedResourceLoader.cpp
@@ -1494,6 +1494,11 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
         return Reload;
     }
 
+    // FIXME: We should be able to USE the data here, but cannot currently due to a bug
+    // concerning redirect and URL fragments. https://bugs.webkit.org/show_bug.cgi?id=258934
+    if (cachedResourceRequest.hasFragmentIdentifier() && existingResource->hasRedirections())
+        return Load;
+
     return Use;
 }
 

diff --git a/Source/WebCore/loader/cache/CachedResourceRequest.h b/Source/WebCore/loader/cache/CachedResourceRequest.h
@@ -106,6 +106,7 @@ class CachedResourceRequest {
     const SecurityOrigin* origin() const { return m_origin.get(); }
     SecurityOrigin* origin() { return m_origin.get(); }
 
+    bool hasFragmentIdentifier() const { return !m_fragmentIdentifier.isEmpty(); }
     String&& releaseFragmentIdentifier() { return WTFMove(m_fragmentIdentifier); }
     void clearFragmentIdentifier() { m_fragmentIdentifier = { }; }
 

diff --git a/Source/WebKit/NetworkProcess/cache/NetworkCache.cpp b/Source/WebKit/NetworkProcess/cache/NetworkCache.cpp
@@ -233,6 +233,11 @@ static UseDecision makeUseDecision(NetworkProcess& networkProcess, PAL::SessionI
     if (cachePolicyAllowsExpired(request.cachePolicy()))
         return UseDecision::Use;
 
+    // We could have cached a redirect without a fragment and now may have
+    // a fragment in the URL.
+    if (request.url().hasFragmentIdentifier() && entry.redirectRequest())
+        return UseDecision::NoDueToRequestContainingFragments;
+
     auto decision = responseNeedsRevalidation(*networkProcess.networkSession(sessionID), entry.response(), request, entry.timeStamp());
     if (decision != UseDecision::Validate)
         return decision;
@@ -284,6 +289,11 @@ static StoreDecision makeStoreDecision(const WebCore::ResourceRequest& originalR
             return StoreDecision::NoDueToHTTPStatusCode;
     }
 
+    // FIXME: We are not correctly computing the redirected request URL in case original request
+    // has a fragment identifier and response location URL does not have one. Let's not store it for now.
+    if ((response.isRedirection() || response.isRedirected()) && originalRequest.url().hasFragmentIdentifier())
+        return StoreDecision::NoDueToRequestContainingFragments;
+
     bool isMainResource = originalRequest.requester() == WebCore::ResourceRequestRequester::Main;
     bool storeUnconditionallyForHistoryNavigation = isMainResource || originalRequest.priority() == WebCore::ResourceLoadPriority::VeryHigh;
     if (!storeUnconditionallyForHistoryNavigation) {

diff --git a/Source/WebKit/NetworkProcess/cache/NetworkCache.h b/Source/WebKit/NetworkProcess/cache/NetworkCache.h
@@ -115,7 +115,7 @@ enum class RetrieveDecision {
     NoDueToHTTPMethod,
     NoDueToConditionalRequest,
     NoDueToReloadIgnoringCache,
-    NoDueToStreamingMedia,
+    NoDueToStreamingMedia
 };
 
 enum class StoreDecision {
@@ -127,6 +127,7 @@ enum class StoreDecision {
     NoDueToNoStoreRequest,
     NoDueToUnlikelyToReuse,
     NoDueToStreamingMedia,
+    NoDueToRequestContainingFragments
 };
 
 enum class UseDecision {
@@ -136,7 +137,8 @@ enum class UseDecision {
     NoDueToVaryingHeaderMismatch,
     NoDueToMissingValidatorFields,
     NoDueToDecodeFailure,
-    NoDueToExpiredRedirect
+    NoDueToExpiredRedirect,
+    NoDueToRequestContainingFragments
 };
 
 enum class CacheOption : uint8_t {