Skip to content

Commit

Permalink
RedirectSOAuthorizationSession should support all redirections
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=254263
rdar://106379935

Reviewed by Alex Christensen.

Handle SSO redirections in the loader code path.
This has the benefit of supporting all redirections, including 307 redirections.
This also simplifies the UIProcess code path.

We need to handle redirections in two ways:
- Either the SSO redirection happens while the load has not started.
  We handle it at DocumentLoader level by enabling to use substitute data for redirections.
- Or the SSO redirection happens while the load already started (redirection case).
  In that case, we register the redirection in network process so that NetworkResourceLoader handles it.

We add some tests and need to update some tests as redirections from file URLs to HTTP is not allowed.

This new code path is only enabled for 307 redirections on a POST request for now.
This patch should not change behavior for other interceptions.
In the future, we will probably move all redirections to follow that new code path.

Coverd by existing and added API tests.

* Source/WebCore/loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::handleSubstituteDataLoadNow):
(WebCore::DocumentLoader::redirectReceived):
(WebCore::DocumentLoader::setRedirectionAsSubstituteData):
* Source/WebCore/loader/DocumentLoader.h:
* Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:
(WebKit::NetworkConnectionToWebProcess::useRedirectionForCurrentNavigation):
* Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:
* Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in:
* Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::continueWillSendRequest):
(WebKit::NetworkResourceLoader::useRedirectionForCurrentNavigation):
* Source/WebKit/NetworkProcess/NetworkResourceLoader.h:
* Source/WebKit/UIProcess/Cocoa/SOAuthorization/RedirectSOAuthorizationSession.mm:
(WebKit::RedirectSOAuthorizationSession::completeInternal):
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::useRedirectionForCurrentNavigation):
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::useRedirectionForCurrentNavigation):
* Source/WebKit/WebProcess/WebPage/WebPage.h:
* Source/WebKit/WebProcess/WebPage/WebPage.messages.in:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/SOAuthorizationTests.mm:
(-[TestSOAuthorizationBasicDelegate webView:didFinishNavigation:]):
(-[TestSOAuthorizationDelegate webView:didFinishNavigation:]):
(-[TestSOAuthorizationDelegate webView:decidePolicyForNavigationAction:decisionHandler:]):
(overrideBeginAuthorizationWithURL):
(TestWebKitAPI::TEST):

Canonical link: https://commits.webkit.org/265706@main
  • Loading branch information
youennf committed Jul 3, 2023
1 parent 41c888f commit 996faad
Show file tree
Hide file tree
Showing 14 changed files with 382 additions and 2 deletions.
23 changes: 23 additions & 0 deletions Source/WebCore/loader/DocumentLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,18 @@ void DocumentLoader::handleSubstituteDataLoadNow()
{
Ref<DocumentLoader> protectedThis = Ref { *this };

if (m_substituteData.response().isRedirection()) {
auto newRequest = m_request.redirectedRequest(m_substituteData.response(), true);
auto substituteData = std::exchange(m_substituteData, { });
auto callback = [protectedThis, newRequest] (auto&& request) mutable {
if (request.isNull())
return;
protectedThis->loadMainResource(WTFMove(newRequest));
};
redirectReceived(WTFMove(newRequest), substituteData.response(), WTFMove(callback));
return;
}

ResourceResponse response = m_substituteData.response();
if (response.url().isEmpty())
response = ResourceResponse(m_request.url(), m_substituteData.mimeType(), m_substituteData.content()->size(), m_substituteData.textEncoding());
Expand Down Expand Up @@ -592,6 +604,11 @@ void DocumentLoader::matchRegistration(const URL& url, SWClientConnection::Regis
void DocumentLoader::redirectReceived(CachedResource& resource, ResourceRequest&& request, const ResourceResponse& redirectResponse, CompletionHandler<void(ResourceRequest&&)>&& completionHandler)
{
ASSERT_UNUSED(resource, &resource == m_mainResource);
redirectReceived(WTFMove(request), redirectResponse, WTFMove(completionHandler));
}

void DocumentLoader::redirectReceived(ResourceRequest&& request, const ResourceResponse& redirectResponse, CompletionHandler<void(ResourceRequest&&)>&& completionHandler)
{
#if ENABLE(SERVICE_WORKER)
if (m_serviceWorkerRegistrationData) {
m_serviceWorkerRegistrationData = { };
Expand Down Expand Up @@ -787,6 +804,12 @@ bool DocumentLoader::tryLoadingRequestFromApplicationCache()
return tryLoadingSubstituteData();
}

void DocumentLoader::setRedirectionAsSubstituteData(ResourceResponse&& response)
{
ASSERT(response.isRedirection());
m_substituteData = { FragmentedSharedBuffer::create(), { }, WTFMove(response), SubstituteData::SessionHistoryVisibility::Visible };
}

bool DocumentLoader::tryLoadingSubstituteData()
{
if (!m_substituteData.isValid() || !m_frame->page())
Expand Down
4 changes: 4 additions & 0 deletions Source/WebCore/loader/DocumentLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,8 @@ class DocumentLoader
void setShouldOpenExternalURLsPolicy(ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy) { m_shouldOpenExternalURLsPolicy = shouldOpenExternalURLsPolicy; }
ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicyToPropagate() const;

WEBCORE_EXPORT void setRedirectionAsSubstituteData(ResourceResponse&&);

#if ENABLE(CONTENT_FILTERING)
#if ENABLE(CONTENT_FILTERING_IN_NETWORKING_PROCESS)
void setBlockedPageURL(const URL& blockedPageURL) { m_blockedPageURL = blockedPageURL; }
Expand Down Expand Up @@ -542,6 +544,8 @@ class DocumentLoader
WEBCORE_EXPORT void handleProvisionalLoadFailureFromContentFilter(const URL& blockedPageURL, SubstituteData&) final;
#endif

void redirectReceived(ResourceRequest&&, const ResourceResponse&, CompletionHandler<void(ResourceRequest&&)>&&);

void dataReceived(const SharedBuffer&);

bool maybeLoadEmpty();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1444,6 +1444,12 @@ void NetworkConnectionToWebProcess::addAllowedFirstPartyForCookies(const Registr
connection().send(Messages::NetworkProcessConnection::AddAllowedFirstPartyForCookies(firstPartyForCookies), 0);
}

void NetworkConnectionToWebProcess::useRedirectionForCurrentNavigation(WebCore::ResourceLoaderIdentifier identifier, WebCore::ResourceResponse&& response)
{
if (auto* loader = m_networkResourceLoaders.get(identifier))
loader->useRedirectionForCurrentNavigation(WTFMove(response));
}

} // namespace WebKit

#undef CONNECTION_RELEASE_LOG
Expand Down
2 changes: 2 additions & 0 deletions Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,8 @@ class NetworkConnectionToWebProcess
#endif

void addAllowedFirstPartyForCookies(const RegistrableDomain&);
void useRedirectionForCurrentNavigation(WebCore::ResourceLoaderIdentifier, WebCore::ResourceResponse&&);

private:
NetworkConnectionToWebProcess(NetworkProcess&, WebCore::ProcessIdentifier, PAL::SessionID, NetworkProcessConnectionParameters, IPC::Connection::Identifier);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,4 +120,5 @@ messages -> NetworkConnectionToWebProcess LegacyReceiver {
#endif

AddAllowedFirstPartyForCookies(WebCore::RegistrableDomain firstPartyDomain)
UseRedirectionForCurrentNavigation(WebCore::ResourceLoaderIdentifier resourceLoadIdentifier, WebCore::ResourceResponse response)
}
19 changes: 19 additions & 0 deletions Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1338,6 +1338,15 @@ void NetworkResourceLoader::continueWillSendRequest(ResourceRequest&& newRequest
{
LOADER_RELEASE_LOG("continueWillSendRequest: (isAllowedToAskUserForCredentials=%d)", isAllowedToAskUserForCredentials);

if (m_redirectionForCurrentNavigation) {
LOADER_RELEASE_LOG("continueWillSendRequest: using stored redirect response");
auto redirection = std::exchange(m_redirectionForCurrentNavigation, { });
auto redirectRequest = newRequest.redirectedRequest(*redirection, parameters().shouldClearReferrerOnHTTPSToHTTPRedirect);
m_shouldRestartLoad = true;
willSendRedirectedRequest(WTFMove(newRequest), WTFMove(redirectRequest), WTFMove(*redirection));
return;
}

#if ENABLE(SERVICE_WORKER)
if (shouldTryToMatchRegistrationOnRedirection(parameters().options, !!m_serviceWorkerFetchTask)) {
m_serviceWorkerRegistration = { };
Expand Down Expand Up @@ -2075,6 +2084,16 @@ void NetworkResourceLoader::handleProvisionalLoadFailureFromContentFilter(const
}
#endif // ENABLE(CONTENT_FILTERING_IN_NETWORKING_PROCESS)

void NetworkResourceLoader::useRedirectionForCurrentNavigation(WebCore::ResourceResponse&& response)
{
LOADER_RELEASE_LOG("useRedirectionForCurrentNavigation");

ASSERT(isMainFrameLoad());
ASSERT(response.isRedirection());

m_redirectionForCurrentNavigation = makeUnique<WebCore::ResourceResponse>(WTFMove(response));
}

} // namespace WebKit

#undef LOADER_RELEASE_LOG
Expand Down
4 changes: 4 additions & 0 deletions Source/WebKit/NetworkProcess/NetworkResourceLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ class NetworkResourceLoader final

void willSendServiceWorkerRedirectedRequest(WebCore::ResourceRequest&&, WebCore::ResourceRequest&& redirectRequest, WebCore::ResourceResponse&&);

void useRedirectionForCurrentNavigation(WebCore::ResourceResponse&&);

private:
NetworkResourceLoader(NetworkResourceLoadParameters&&, NetworkConnectionToWebProcess&, CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse, Vector<uint8_t>&&)>&&);

Expand Down Expand Up @@ -328,6 +330,8 @@ class NetworkResourceLoader final

PrivateRelayed m_privateRelayed { PrivateRelayed::No };
MemoryCompactRobinHoodHashMap<String, String> m_reportingEndpoints;

std::unique_ptr<WebCore::ResourceResponse> m_redirectionForCurrentNavigation;
};

} // namespace WebKit
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,15 @@
auto* navigationAction = this->navigationAction();
ASSERT(navigationAction);
auto* page = this->page();
if ((response.httpStatusCode() != 302 && response.httpStatusCode() != 200) || !page) {
// FIXME: Enable the useRedirectionForCurrentNavigation code path for all redirections.
if ((response.httpStatusCode() != 302 && response.httpStatusCode() != 200 && !(response.httpStatusCode() == 307 && navigationAction->request().httpMethod() == "POST"_s)) || !page) {
AUTHORIZATIONSESSION_RELEASE_LOG("completeInternal: httpState=%d page=%d, so falling back to web path.", response.httpStatusCode(), !!page);
fallBackToWebPathInternal();
return;
}
invokeCallback(true);

if (response.httpStatusCode() == 302) {
invokeCallback(true);
#if PLATFORM(IOS) || PLATFORM(VISION)
// MobileSafari has a WBSURLSpoofingMitigator, which will not display the provisional URL for navigations without user gestures.
// For slow loads that are initiated from the MobileSafari Favorites screen, the aforementioned behavior will create a period
Expand All @@ -90,11 +92,18 @@
}
#endif
page->loadRequest(ResourceRequest(response.httpHeaderFields().get(HTTPHeaderName::Location)));
return;
}
if (response.httpStatusCode() == 200) {
invokeCallback(true);
page->setShouldSuppressSOAuthorizationInNextNavigationPolicyDecision();
page->loadData(IPC::DataReference(static_cast<const uint8_t*>(data.bytes), data.length), "text/html"_s, "UTF-8"_s, response.url().string(), nullptr, navigationAction->shouldOpenExternalURLsPolicy());
return;
}

ASSERT(response.httpStatusCode() == 307 && navigationAction->request().httpMethod() == "POST"_s);
page->useRedirectionForCurrentNavigation(response);
invokeCallback(false);
}

void RedirectSOAuthorizationSession::beforeStart()
Expand Down
6 changes: 6 additions & 0 deletions Source/WebKit/UIProcess/WebPageProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12806,6 +12806,12 @@ void WebPageProxy::setSystemPreviewCompletionHandlerForLoadTesting(CompletionHan
}
#endif

void WebPageProxy::useRedirectionForCurrentNavigation(const ResourceResponse& response)
{
ASSERT(response.isRedirection());
send(Messages::WebPage::UseRedirectionForCurrentNavigation(response));
}

} // namespace WebKit

#undef WEBPAGEPROXY_RELEASE_LOG
Expand Down
2 changes: 2 additions & 0 deletions Source/WebKit/UIProcess/WebPageProxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -2236,6 +2236,8 @@ class WebPageProxy final : public API::ObjectImpl<API::Object::Type::Page>, publ
void reportNetworkIssue(const URL&);
#endif

void useRedirectionForCurrentNavigation(const WebCore::ResourceResponse&);

#if ENABLE(ACCESSIBILITY_ANIMATION_CONTROL)
void pauseAllAnimations(CompletionHandler<void()>&&);
void playAllAnimations(CompletionHandler<void()>&&);
Expand Down
28 changes: 28 additions & 0 deletions Source/WebKit/WebProcess/WebPage/WebPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@
#include <WebCore/StaticRange.h>
#include <WebCore/StyleProperties.h>
#include <WebCore/SubframeLoader.h>
#include <WebCore/SubresourceLoader.h>
#include <WebCore/SubstituteData.h>
#include <WebCore/TextIterator.h>
#include <WebCore/TextRecognitionOptions.h>
Expand Down Expand Up @@ -8919,6 +8920,33 @@ const void* WebPage::logIdentifier() const
return reinterpret_cast<const void*>(intHash(m_identifier.toUInt64()));
}

void WebPage::useRedirectionForCurrentNavigation(WebCore::ResourceResponse&& response)
{
auto* localMainFrame = dynamicDowncast<LocalFrame>(m_page->mainFrame());
if (!localMainFrame) {
WEBPAGE_RELEASE_LOG_ERROR(Loading, "WebPage::useRedirectionForCurrentNavigation failed without frame");
return;
}

auto* loader = localMainFrame->loader().policyDocumentLoader();
if (!loader)
loader = localMainFrame->loader().provisionalDocumentLoader();

if (!loader) {
WEBPAGE_RELEASE_LOG_ERROR(Loading, "WebPage::useRedirectionForCurrentNavigation failed without loader");
return;
}

if (auto* resourceLoader = loader->mainResourceLoader()) {
WEBPAGE_RELEASE_LOG(Loading, "WebPage::useRedirectionForCurrentNavigation to network process");
WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::UseRedirectionForCurrentNavigation(resourceLoader->identifier(), response), 0);
return;
}

WEBPAGE_RELEASE_LOG(Loading, "WebPage::useRedirectionForCurrentNavigation as substiute data");
loader->setRedirectionAsSubstituteData(WTFMove(response));
}

} // namespace WebKit

#undef WEBPAGE_RELEASE_LOG
Expand Down
2 changes: 2 additions & 0 deletions Source/WebKit/WebProcess/WebPage/WebPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -2129,6 +2129,8 @@ class WebPage : public API::ObjectImpl<API::Object::Type::BundlePage>, public IP
bool hasPendingEditorStateUpdate() const;
bool shouldAvoidComputingPostLayoutDataForEditorState() const;

void useRedirectionForCurrentNavigation(WebCore::ResourceResponse&&);

WebCore::PageIdentifier m_identifier;

std::unique_ptr<WebCore::Page> m_page;
Expand Down
2 changes: 2 additions & 0 deletions Source/WebKit/WebProcess/WebPage/WebPage.messages.in
Original file line number Diff line number Diff line change
Expand Up @@ -735,5 +735,7 @@ GenerateSyntheticEditingCommand(enum:uint8_t WebKit::SyntheticEditingCommandType
PlayAllAnimations() -> ()
#endif

UseRedirectionForCurrentNavigation(WebCore::ResourceResponse response);

UpdateFrameSize(WebCore::FrameIdentifier frame, WebCore::IntSize newSize)
}
Loading

0 comments on commit 996faad

Please sign in to comment.