Service Worker: Redirect loses hash fragment

https://bugs.webkit.org/show_bug.cgi?id=258195 rdar://111208014 Reviewed by Chris Dumez. Make sure to implement the fragment identifier handling defined in https://fetch.spec.whatwg.org/#concept-response-location-url: - If the response has a fragment identifier and the URL computed from the location header does not, add the fragment identifier back. * LayoutTests/http/wpt/service-workers/navigation-redirect-with-hash-worker.js: Added. * LayoutTests/http/wpt/service-workers/navigation-redirect-with-hash.https-expected.txt: Added. * LayoutTests/http/wpt/service-workers/navigation-redirect-with-hash.https.html: Added. * LayoutTests/http/wpt/service-workers/resources/redirect-with-hash.py: Added. (main): * Source/WebCore/platform/network/ResourceRequestBase.cpp: (WebCore::ResourceRequestBase::redirectedRequest const): * Source/WebCore/platform/network/ResourceRequestBase.h: * Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp: (WebKit::ServiceWorkerFetchTask::processRedirectResponse): Canonical link: https://commits.webkit.org/265845@main
WebKit · Jul 7, 2023 · e4b3080 · e4b3080
1 parent e919dcf
commit e4b3080
Show file tree

Hide file tree

Showing 7 changed files with 55 additions and 4 deletions.
diff --git a/LayoutTests/http/wpt/service-workers/navigation-redirect-with-hash-worker.js b/LayoutTests/http/wpt/service-workers/navigation-redirect-with-hash-worker.js
@@ -0,0 +1,3 @@
+addEventListener("fetch", (e) => {
+    e.respondWith(fetch(e.request));
+});
diff --git a/LayoutTests/http/wpt/service-workers/navigation-redirect-with-hash.https-expected.txt b/LayoutTests/http/wpt/service-workers/navigation-redirect-with-hash.https-expected.txt
@@ -0,0 +1,4 @@
+
+PASS setup
+PASS load iframe with hash
+
diff --git a/LayoutTests/http/wpt/service-workers/navigation-redirect-with-hash.https.html b/LayoutTests/http/wpt/service-workers/navigation-redirect-with-hash.https.html
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<title>Navigation redirect with hash</title>
+<meta name=timeout content=long>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/service-workers/service-worker/resources/test-helpers.sub.js"></script>
+<body>
+<script>
+promise_test(async () => {
+    const registration = await navigator.serviceWorker.register("navigation-redirect-with-hash-worker.js", { scope : "resources" });
+    var activeWorker = registration.active;
+    if (!activeWorker) {
+        activeWorker = registration.installing;
+        await new Promise(resolve => {
+            activeWorker.addEventListener('statechange', () => {
+                if (activeWorker.state === "activated")
+                    resolve(registration);
+            });
+        });
+    }
+}, "setup");
+
+promise_test(async () => {
+   const frame = await with_iframe("/WebKit/service-workers/resources/redirect-with-hash.py#test");
+   assert_equals(frame.contentWindow.location.hash, "#test");
+   frame.remove(); 
+}, "load iframe with hash");
+</script>
+</body>
diff --git a/LayoutTests/http/wpt/service-workers/resources/redirect-with-hash.py b/LayoutTests/http/wpt/service-workers/resources/redirect-with-hash.py
@@ -0,0 +1,8 @@
+from wptserve.utils import isomorphic_decode
+
+
+def main(request, response):
+    headers = []
+    headers.append((b"Location", b"/redirected"))
+
+    return 302, headers, b""
diff --git a/Source/WebCore/platform/network/ResourceRequestBase.cpp b/Source/WebCore/platform/network/ResourceRequestBase.cpp
@@ -154,7 +154,7 @@ void ResourceRequestBase::redirectAsGETIfNeeded(const ResourceRequestBase &redir
     }
 }
 
-ResourceRequest ResourceRequestBase::redirectedRequest(const ResourceResponse& redirectResponse, bool shouldClearReferrerOnHTTPSToHTTPRedirect) const
+ResourceRequest ResourceRequestBase::redirectedRequest(const ResourceResponse& redirectResponse, bool shouldClearReferrerOnHTTPSToHTTPRedirect, ShouldSetHash shouldSetHash) const
 {
     ASSERT(redirectResponse.isRedirection());
     // This method is based on https://fetch.spec.whatwg.org/#http-redirect-fetch.
@@ -163,7 +163,12 @@ ResourceRequest ResourceRequestBase::redirectedRequest(const ResourceResponse& r
     auto request = asResourceRequest();
     auto location = redirectResponse.httpHeaderField(HTTPHeaderName::Location);
 
-    request.setURL(location.isEmpty() ? URL { } : URL { redirectResponse.url(), location });
+    // https://fetch.spec.whatwg.org/#concept-response-location-url
+    auto url = location.isEmpty() ? URL { } : URL { redirectResponse.url(), location };
+    if (shouldSetHash == ShouldSetHash::Yes && url.fragmentIdentifier().isEmpty() && !redirectResponse.url().fragmentIdentifier().isEmpty())
+        url.setFragmentIdentifier(redirectResponse.url().fragmentIdentifier());
+
+    request.setURL(WTFMove(url));
 
     request.redirectAsGETIfNeeded(*this, redirectResponse);
 

diff --git a/Source/WebCore/platform/network/ResourceRequestBase.h b/Source/WebCore/platform/network/ResourceRequestBase.h
@@ -128,7 +128,9 @@ class ResourceRequestBase {
     WEBCORE_EXPORT void setURL(const URL& url);
 
     void redirectAsGETIfNeeded(const ResourceRequestBase &, const ResourceResponse&);
-    WEBCORE_EXPORT ResourceRequest redirectedRequest(const ResourceResponse&, bool shouldClearReferrerOnHTTPSToHTTPRedirect) const;
+
+    enum class ShouldSetHash : bool { No, Yes };
+    WEBCORE_EXPORT ResourceRequest redirectedRequest(const ResourceResponse&, bool shouldClearReferrerOnHTTPSToHTTPRedirect, ShouldSetHash = ShouldSetHash::No) const;
 
     WEBCORE_EXPORT void removeCredentials();
 

diff --git a/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp b/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp
@@ -205,7 +205,7 @@ void ServiceWorkerFetchTask::processRedirectResponse(ResourceResponse&& response
 
     if (shouldSetSource == ShouldSetSource::Yes)
         response.setSource(ResourceResponse::Source::ServiceWorker);
-    auto newRequest = m_currentRequest.redirectedRequest(response, m_loader.parameters().shouldClearReferrerOnHTTPSToHTTPRedirect);
+    auto newRequest = m_currentRequest.redirectedRequest(response, m_loader.parameters().shouldClearReferrerOnHTTPSToHTTPRedirect, ResourceRequest::ShouldSetHash::Yes);
 
     m_loader.willSendServiceWorkerRedirectedRequest(ResourceRequest(m_currentRequest), WTFMove(newRequest), WTFMove(response));
 }