WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/34613960
>
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
Created
attachment 331262
[details]
Patch
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
Created
attachment 331423
[details]
Patch
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
Created
attachment 331436
[details]
Patch
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
Created
attachment 331443
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug