Use the new ProcessPrivilege concept (see Bug 183806) to enforce our goal of prohibiting Cookie API access inside the WebContent process.
<rdar://problem/38762306>
Created attachment 336344 [details] Patch
Attachment 336344 [details] did not pass style-queue: ERROR: Source/WebKit/WebProcess/WebProcess.cpp:223: Too many spaces inside { }. [whitespace/braces] [5] ERROR: Source/WebCore/platform/network/NetworkStorageSession.h:94: The parameter name "cookieAPI" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 336345 [details] Patch
Created attachment 336350 [details] Patch
Created attachment 336375 [details] Patch
WebContent really only uses NetworkStorageSession objects to know which Session ID to use when talking to the Network process. It would be nice if NetworkStorageSession had two flavors -- one for use in the WebContent process, which didn't have any networking features in it, and another for the Network process, which could own the various platform networking API objects.
Comment on attachment 336375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336375&action=review > Source/WebCore/platform/network/NetworkStorageSession.h:73 > + enum class CookieAPI { Allowed, NotAllowed }; Maybe CookieAPIAccess would be clearer? > Source/WebCore/platform/network/NetworkStorageSession.h:94 > + NetworkStorageSession(PAL::SessionID, RetainPtr<CFURLStorageSessionRef>&&, RetainPtr<CFHTTPCookieStorageRef>&&, CookieAPI); With this API, it seems that we could have in the same process some NetworkStorageSession that have Cookie API access and some NetworkStorageSession that do not have Cookie API access. Is it what we want? If not, I wonder whether, instead of passing a parameter, we could have a process wide singleton telling whether CookieAPI access is allowed. Then we could check it at the right places without passing this value. Something like a static CookieAPI::isAllowed() and a corresponding setter that would also check the assertion. Or directly using the hasPriviledge function. > Source/WebCore/platform/network/NetworkStorageSession.h:95 > + NetworkStorageSession(PAL::SessionID); explicit > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:82 > + m_platformCookieStorage = platformCookieStorage ? WTFMove(platformCookieStorage) : cookieAPI == CookieAPI::Allowed ? cookieStorage() : nullptr; If cookieAPI access is not allowed, should platformCookieStorage be null? Should we assert it?
Created attachment 336407 [details] Patch
Created attachment 336433 [details] Patch
Comment on attachment 336433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336433&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:248 > + WebCore::NetworkStorageSession::permitProcessToUseCookieAPI(true); We could probably use std::call_once here. Is it soon enough though? Can there be a way to use some Cookie API that would need that permission before creating a WebProcessPool?
Created attachment 336437 [details] Patch
Comment on attachment 336437 [details] Patch r=me. It would be nice to check that if we create a website data store, get a cookie store and modify it, we do not hit those assertions. The WebProcessPool relaxing code is probably happening too late in that scenario. An API test would tell us.
Comment on attachment 336437 [details] Patch Attachment 336437 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7081747 Number of test failures exceeded the failure limit.
Created attachment 336449 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 336523 [details] Patch for landing
Comment on attachment 336523 [details] Patch for landing Clearing flags on attachment: 336523 Committed r229978: <https://trac.webkit.org/changeset/229978>
All reviewed patches have been landed. Closing bug.