Bug 177394 - Figure out how to disable all cookies in the stateless network storage session
Summary: Figure out how to disable all cookies in the stateless network storage session
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks: 175232
  Show dependency treegraph
 
Reported: 2017-09-22 15:35 PDT by John Wilander
Modified: 2019-01-14 17:29 PST (History)
8 users (show)

See Also:


Attachments
Patch (53.70 KB, patch)
2018-01-12 18:03 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (56.01 KB, patch)
2018-01-16 13:24 PST, John Wilander
no flags Details | Formatted Diff | Diff
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 Details
Patch (57.57 KB, patch)
2018-01-16 15:10 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (59.23 KB, patch)
2018-01-16 17:38 PST, John Wilander
no flags Details | Formatted Diff | Diff
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 Details
Patch for landing (59.17 KB, patch)
2018-01-17 10:40 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (59.47 KB, patch)
2018-01-17 17:17 PST, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 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.
Comment 1 Brent Fulgham 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.
Comment 2 Brent Fulgham 2017-09-23 18:04:32 PDT
We might be hitting this during Redirects:

DocumentThreadableLoader::redirectReceived
{
...
    m_options.allowCredentials = DoNotAllowStoredCredentials;
...
}
Comment 3 Radar WebKit Bug Importer 2017-09-23 18:20:03 PDT
<rdar://problem/34613960>
Comment 4 Brent Fulgham 2017-09-23 19:45:14 PDT
The earlier rollout was <http://trac.webkit.org/changeset/222414>.
Comment 5 Brent Fulgham 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.
Comment 6 John Wilander 2018-01-12 18:03:24 PST
Created attachment 331262 [details]
Patch
Comment 7 Alex Christensen 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.
Comment 8 Alex Christensen 2018-01-13 12:51:10 PST
It looks like we'll need to add NSHTTPCookieStorage _initWithIdentifier:private: to CFNetworkSPI.h, too.
Comment 9 John Wilander 2018-01-16 13:24:56 PST
Created attachment 331423 [details]
Patch
Comment 10 EWS Watchlist 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
Comment 11 EWS Watchlist 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
Comment 12 John Wilander 2018-01-16 14:50:19 PST
The one new test case that failed wasn't skipped for non-ITP WK2. Will fix.
Comment 13 John Wilander 2018-01-16 15:10:15 PST
Created attachment 331436 [details]
Patch
Comment 14 Alex Christensen 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)
Comment 15 John Wilander 2018-01-16 17:38:43 PST
Created attachment 331443 [details]
Patch
Comment 16 John Wilander 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.
Comment 17 EWS Watchlist 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.
Comment 18 John Wilander 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.
Comment 19 EWS Watchlist 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
Comment 20 EWS Watchlist 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
Comment 21 John Wilander 2018-01-16 19:02:40 PST
The test failure does not seem related to me.
Comment 22 Alex Christensen 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];
Comment 23 John Wilander 2018-01-17 10:40:12 PST
Created attachment 331511 [details]
Patch for landing
Comment 24 John Wilander 2018-01-17 10:41:16 PST
Thanks, Alex! I did all the additional changes you suggested.
Comment 25 WebKit Commit Bot 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>
Comment 26 WebKit Commit Bot 2018-01-17 11:16:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Matt Lewis 2018-01-17 15:50:49 PST
Reverted r227076 for reason:

This breaks internal builds

Committed r227093: <https://trac.webkit.org/changeset/227093>
Comment 28 Alex Christensen 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
Comment 29 John Wilander 2018-01-17 17:17:01 PST
Created attachment 331565 [details]
Patch for landing
Comment 30 WebKit Commit Bot 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>
Comment 31 WebKit Commit Bot 2018-01-17 17:58:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 32 Simon Fraser (smfr) 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;