RESOLVED FIXED 177394
Figure out how to disable all cookies in the stateless network storage session
https://bugs.webkit.org/show_bug.cgi?id=177394
Summary Figure out how to disable all cookies in the stateless network storage session
John Wilander
Reported 2017-09-22 15:35:30 PDT
https://bugs.webkit.org/show_bug.cgi?id=177393 rolled-back the cookie jar change for stateless network sessions that was introduced in https://bugs.webkit.org/show_bug.cgi?id=176386. We should figure out what caused the regressions and again disabled the cookie jar on the stateless session.
Attachments
Patch (53.70 KB, patch)
2018-01-12 18:03 PST, John Wilander
no flags
Patch (56.01 KB, patch)
2018-01-16 13:24 PST, John Wilander
no flags
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.25 MB, application/zip)
2018-01-16 14:33 PST, EWS Watchlist
no flags
Patch (57.57 KB, patch)
2018-01-16 15:10 PST, John Wilander
no flags
Patch (59.23 KB, patch)
2018-01-16 17:38 PST, John Wilander
no flags
Archive of layout-test-results from ews102 for mac-sierra (2.24 MB, application/zip)
2018-01-16 18:41 PST, EWS Watchlist
no flags
Patch for landing (59.17 KB, patch)
2018-01-17 10:40 PST, John Wilander
no flags
Patch for landing (59.47 KB, patch)
2018-01-17 17:17 PST, John Wilander
no flags
Brent Fulgham
Comment 1 2017-09-23 17:02:54 PDT
I think this was a problem because we tied the decision to use "cookieless" loading to the decision that we did not want to support certificate access. Looking at the code, it was pretty common to not want to allow credential storage while still needing to support cookie access. Alex: It seems like we might want to revisit the decision to only have "stateless" and "stateful" sessions. We might want to support "no certificate with cookies" as a distinct mode.
Brent Fulgham
Comment 2 2017-09-23 18:04:32 PDT
We might be hitting this during Redirects: DocumentThreadableLoader::redirectReceived { ... m_options.allowCredentials = DoNotAllowStoredCredentials; ... }
Radar WebKit Bug Importer
Comment 3 2017-09-23 18:20:03 PDT
Brent Fulgham
Comment 4 2017-09-23 19:45:14 PDT
The earlier rollout was <http://trac.webkit.org/changeset/222414>.
Brent Fulgham
Comment 5 2017-09-23 19:50:41 PDT
It looks like Web Compat requires that we have a “Cookies + Credentials”, “Cookies + No Credentials”, and “Stateless” network sessions.
John Wilander
Comment 6 2018-01-12 18:03:24 PST
Alex Christensen
Comment 7 2018-01-12 18:10:50 PST
Comment on attachment 331262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331262&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h:88 > + NSHTTPCookieStorage* m_emptyDenyAllCookieStorage; Let's use one if it's truly stateless. static RetainPtr<NSHTTPCookieStorage> g_emptyCookieStorage; or something like that.
Alex Christensen
Comment 8 2018-01-13 12:51:10 PST
It looks like we'll need to add NSHTTPCookieStorage _initWithIdentifier:private: to CFNetworkSPI.h, too.
John Wilander
Comment 9 2018-01-16 13:24:56 PST
EWS Watchlist
Comment 10 2018-01-16 14:33:29 PST
Comment on attachment 331423 [details] Patch Attachment 331423 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6098027 New failing tests: http/tests/resourceLoadStatistics/non-prevalent-resources-can-access-cookies-in-a-third-party-context.html
EWS Watchlist
Comment 11 2018-01-16 14:33:30 PST
Created attachment 331433 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
John Wilander
Comment 12 2018-01-16 14:50:19 PST
The one new test case that failed wasn't skipped for non-ITP WK2. Will fix.
John Wilander
Comment 13 2018-01-16 15:10:15 PST
Alex Christensen
Comment 14 2018-01-16 16:19:29 PST
Comment on attachment 331436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331436&action=review > Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:182 > +- (id) _initWithIdentifier:(NSString*) identifier private:(bool) isPrivate; No space before identifier. Space after NSString > Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:188 > +- (void) _setExplicitCookieStorage:(CFHTTPCookieStorageRef) storage; I think this doesn't need to be here because we only call the SPI with performSelector. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h:83 > + NSHTTPCookieStorage* emptyDenyAllCookieStorage(); This should be static because it uses no member variables. static NSHTTPCookieStorage *statelessCookieStorage() > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:107 > + static NSHTTPCookieStorage* emptyCookieStorage; static NeverDestroyed<RetainPtr<NSHTTPCookieStorage>> Then we can adopt the alloc'd object and pretend we manage it. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:308 > + [m_task.get() performSelector:NSSelectorFromString(@"_setExplicitCookieStorage:") withObject:(NSObject*)m_session->networkStorageSession().nsCookieStorage()._cookieStorage]; > + m_hasBeenSetToUseEmptyDenyAllCookieStorage = false; I'd make a private member function: setCookiesBlocked(bool)
John Wilander
Comment 15 2018-01-16 17:38:43 PST
John Wilander
Comment 16 2018-01-16 17:40:07 PST
Thanks, Alex! Your comments are addressed. I also made the partitioning code into a dedicated function and got rid of the WebCore::StoredCredentialsPolicy::DoNotUse setting when creating the task.
EWS Watchlist
Comment 17 2018-01-16 17:41:40 PST
Attachment 331443 [details] did not pass style-queue: ERROR: Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:131: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
John Wilander
Comment 18 2018-01-16 17:48:01 PST
The style error is for a comment in code I moved. The comment needs multi-space aligning since it has a truth table.
EWS Watchlist
Comment 19 2018-01-16 18:41:33 PST
Comment on attachment 331443 [details] Patch Attachment 331443 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6100499 New failing tests: imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-bitrate.html
EWS Watchlist
Comment 20 2018-01-16 18:41:34 PST
Created attachment 331447 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
John Wilander
Comment 21 2018-01-16 19:02:40 PST
The test failure does not seem related to me.
Alex Christensen
Comment 22 2018-01-17 10:00:42 PST
Comment on attachment 331443 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331443&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:105 > +RetainPtr<NSHTTPCookieStorage>& NetworkDataTaskCocoa::statelessCookieStorage() This is fine. Returning an NSHTTPCookieStorage * would look a little cleaner. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:112 > + return statelessCookieStorage.get(); I think it would be nice to assert that NSHTTPCookieStorage.cookies.count is 0 here. That way we could catch if anything unexpected happens. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:123 > + if (shouldBlock) > + [m_task.get() performSelector:NSSelectorFromString(@"_setExplicitCookieStorage:") withObject:(NSObject*)statelessCookieStorage().get()._cookieStorage]; > + else > + [m_task.get() performSelector:NSSelectorFromString(@"_setExplicitCookieStorage:") withObject:(NSObject*)m_session->networkStorageSession().nsCookieStorage()._cookieStorage]; .get() should be unnecessary. We could also make this simpler: NSHTTPCookieStorage *storage = shouldBlock ? statelessCookieStorage().get(): m_session->networkStorageSession().nsCookieStorage(); [m_task performSelector:NSSelectorFromString(@"_setExplicitCookieStorage:") withObject:(NSObject*)storage._cookieStorage];
John Wilander
Comment 23 2018-01-17 10:40:12 PST
Created attachment 331511 [details] Patch for landing
John Wilander
Comment 24 2018-01-17 10:41:16 PST
Thanks, Alex! I did all the additional changes you suggested.
WebKit Commit Bot
Comment 25 2018-01-17 11:16:28 PST
Comment on attachment 331511 [details] Patch for landing Clearing flags on attachment: 331511 Committed r227076: <https://trac.webkit.org/changeset/227076>
WebKit Commit Bot
Comment 26 2018-01-17 11:16:29 PST
All reviewed patches have been landed. Closing bug.
Matt Lewis
Comment 27 2018-01-17 15:50:49 PST
Reverted r227076 for reason: This breaks internal builds Committed r227093: <https://trac.webkit.org/changeset/227093>
Alex Christensen
Comment 28 2018-01-17 16:44:17 PST
Comment on attachment 331511 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=331511&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:109 > + statelessCookieStorage.get() = adoptNS([[NSHTTPCookieStorage alloc] _initWithIdentifier:nil private:YES]); It looks like the internal Sierra header incorrectly marks the NSString of _initWithIdentifier:private: as non-nullable. Let's do this: #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wnonnull" #endif statelessCookieStorage.get() = adoptNS([[NSHTTPCookieStorage alloc] _initWithIdentifier:nil private:YES]); #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) #pragma clang diagnostic pop #endif
John Wilander
Comment 29 2018-01-17 17:17:01 PST
Created attachment 331565 [details] Patch for landing
WebKit Commit Bot
Comment 30 2018-01-17 17:58:10 PST
Comment on attachment 331565 [details] Patch for landing Clearing flags on attachment: 331565 Committed r227103: <https://trac.webkit.org/changeset/227103>
WebKit Commit Bot
Comment 31 2018-01-17 17:58:12 PST
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 32 2019-01-14 17:29:21 PST
Why are these comments still in NetworkSessionCocoa::NetworkSessionCocoa()? // FIXME: https://bugs.webkit.org/show_bug.cgi?id=177394 // configuration.HTTPCookieStorage = nil; // configuration.HTTPCookieAcceptPolicy = NSHTTPCookieAcceptPolicyNever;
Note You need to log in before you can comment on or make changes to this bug.