-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Cherry-pick 272448.87@safari-7618-branch (b70a943). https://bugs.webk…
…it.org/show_bug.cgi?id=267298 Regression: Third-party service worker processes get killed when trying to do loads https://bugs.webkit.org/show_bug.cgi?id=267298 rdar://120587179 Reviewed by Youenn Fablet. We recently added origin IPC message checks to make sure that WebProcesses can only access cookies from specific origins. Due to a bug in our service worker code, the network process would allow the wrong origin in the case of third-party service workers (it would allow the client origin instead of the top origin). As a result, the network process would kill the third-party service worker process as soon as it would try to attempt a load. This was occurring on https://boingboing.net/2024/01/06/stanley-water-bottle-madness-grips-america.html for example, with the tiktok.com service worker. When the network process needs to launch a service worker process, it sends an EstablishRemoteWorkerContextConnectionToNetworkProcess IPC to the UIProcess, with a given registrable domain. The UIProcess would use this registrable domain to select a suitable WebProcess or launch one. When launching a new process, the UIProcess would send an IPC to the network process telling it this new WebProcess is allowed to access cookies under the given registrable domain. Both from the process selection point of view and from the network process cookie access point of view, we expect this registrable domain to be the top origin's registrable domain. As a result, in the example above, the third-party tiktok.com service worker under boingboing.net, would be expected to use a "boingboing.net" WebProcess and only have access to cookies under the "boingboing.net" first party. However, our service worker logic was using the registrable domain of the service worker script URL instead. As a result, we would select a "tiktok.com" WebProcess for the service worker process, which was wrong from a site isolation perspective (since top-level tiktok.com would share the same process as tiktok.com under boingboing.net). Also, the network process would allow access to top-level tiktok.com cookies if the WebProcess requested them, which was wrong too. Later on, the service worker would try to do a load. The network request would request use "boingboing.net" as firstParty for cookies, which is correct. However, the network process would reject such load, since the process is only allowed to use "tiktok.com" as first party for cookies. It would then kill the service worker process for good measure since it would assume it is compromised. To address the issue, we now properly use the registrable domain of the top level origin when sending the EstablishRemoteWorkerContextConnectionToNetworkProcess IPC for and service worker connection selection in general. I updated the shared worker code as well to maintain consistency. Note that in order to write an API test for this, I had to restore the service worker from disk first. When the service worker is newly registered by JS, we would first tell the network process to allow the wrong client origin. However, a later IPC to tell the network process to also allow the top level origin. As a result, newly registered third-party service workers would not get terminated which is why our tests were passing. Those service worker origins were allowed access to cookies they shouldn't have access to though so there was a security issue still for them. When restoring the service worker from disk though, we'd only send a single IPC to the network process telling it to allow the original of the service worker script URL. This allowed me to write a test and explains the service worker processes terminations on boingboing.net. * Source/WebCore/workers/service/server/SWServer.cpp: (WebCore::SWServer::addRegistrationFromStore): (WebCore::SWServer::scheduleJob): (WebCore::SWServer::tryInstallContextData): (WebCore::SWServer::runServiceWorkerIfNecessary): (WebCore::SWServer::markAllWorkersForRegistrableDomainAsTerminated): (WebCore::SWServer::removeContextConnectionIfPossible): (WebCore::SWServer::fireFunctionalEvent): * Source/WebCore/workers/service/server/SWServerToContextConnection.cpp: (WebCore::SWServerToContextConnection::terminateWhenPossible): * Source/WebCore/workers/service/server/SWServerWorker.cpp: (WebCore::m_lastNavigationWasAppInitiated): (WebCore::SWServerWorker::contextConnection): (WebCore::SWServerWorker::terminationIfPossibleTimerFired): * Source/WebCore/workers/service/server/SWServerWorker.h: (WebCore::SWServerWorker::topRegistrableDomain const): (WebCore::SWServerWorker::registrableDomain const): Deleted. * Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp: (WebKit::WebSWServerConnection::startFetch): * Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm: (-[SWMessageHandlerForRestoreFromDiskTest resetExpectedMessage:]): Canonical link: https://commits.webkit.org/272448.87@safari-7618-branch Canonical link: https://commits.webkit.org/266719.389@webkitglib/2.42
- Loading branch information
Showing
9 changed files
with
188 additions
and
24 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters