Skip to content

Commit

Permalink
Add support to sameSitePolicy handling.
Browse files Browse the repository at this point in the history
sameSitePolicy property was added to NSHTTPCookie with iOS 13, consider
it when switching between canonical cookies and system cookies.

Initially use CookieOption with access semantic Strict, but eventually
download_session_cookie should update the semantic based on the origin
of the request.

This CL accommodates the changes that happened recently on the canonical cookie .
So in iOS 12 we don't miss cross site non-secure cookies. and on the same time starting
from iOS 13 check on same-site policy value and follow chromium rules in sending cookies
across requests.

Bug: 1018272, 1006785
Change-Id: I77213f528a9f533998e783db897a80a990bea87d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1890507
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Commit-Queue: Mohammad Refaat <mrefaat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713216}
  • Loading branch information
mrefaat88 authored and Commit Bot committed Nov 6, 2019
1 parent 8b8b60f commit b5b5342
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 20 deletions.
49 changes: 45 additions & 4 deletions ios/net/cookies/system_cookie_util.mm
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@
// Undocumented property of NSHTTPCookie.
NSString* const kNSHTTPCookieHttpOnly = @"HttpOnly";

// Possible value for SameSite policy. WebKit doesn't use the value "none" or
// any other value, it uses the empty value to represent none, and any value
// that is not "strict" or "lax" will be considered as none.
NSString* const kNSHTTPCookieSameSiteNone = @"none";

// Key in NSUserDefaults telling wether a low cookie count must be reported as
// an error.
NSString* const kCheckCookieLossKey = @"CookieUtilIOSCheckCookieLoss";
Expand Down Expand Up @@ -70,16 +75,30 @@ void ReportUMACookieLoss(CookieLossType loss, CookieEvent event) {
net::CanonicalCookie CanonicalCookieFromSystemCookie(
NSHTTPCookie* cookie,
const base::Time& ceation_time) {
net::CookieSameSite same_site = net::CookieSameSite::NO_RESTRICTION;
if (@available(iOS 13, *)) {
same_site = net::CookieSameSite::UNSPECIFIED;
if ([cookie.sameSitePolicy isEqual:NSHTTPCookieSameSiteLax])
same_site = net::CookieSameSite::LAX_MODE;

if ([cookie.sameSitePolicy isEqual:NSHTTPCookieSameSiteStrict])
same_site = net::CookieSameSite::STRICT_MODE;

if ([[cookie.sameSitePolicy lowercaseString]
isEqual:kNSHTTPCookieSameSiteNone])
same_site = net::CookieSameSite::NO_RESTRICTION;
}

return net::CanonicalCookie(
base::SysNSStringToUTF8([cookie name]),
base::SysNSStringToUTF8([cookie value]),
base::SysNSStringToUTF8([cookie domain]),
base::SysNSStringToUTF8([cookie path]), ceation_time,
base::Time::FromDoubleT([[cookie expiresDate] timeIntervalSince1970]),
base::Time(), [cookie isSecure], [cookie isHTTPOnly],
// TODO(mkwst): When iOS begins to support 'SameSite' and 'Priority'
// attributes, pass them through here.
net::CookieSameSite::NO_RESTRICTION, net::COOKIE_PRIORITY_DEFAULT);
base::Time(), [cookie isSecure], [cookie isHTTPOnly], same_site,
// When iOS begins to support 'Priority' attribute, pass it
// through here.
net::COOKIE_PRIORITY_DEFAULT);
}

void ReportGetCookiesForURLResult(SystemCookieStoreType store_type,
Expand Down Expand Up @@ -140,6 +159,28 @@ void ReportGetCookiesForURLCall(SystemCookieStoreType store_type) {
[NSDate dateWithTimeIntervalSince1970:cookie.ExpiryDate().ToDoubleT()];
[properties setObject:expiry forKey:NSHTTPCookieExpires];
}

if (@available(iOS 13, *)) {
// In iOS 13 sameSite property in NSHTTPCookie is used to specify the
// samesite policy.
NSString* same_site = @"";
switch (cookie.SameSite()) {
case net::CookieSameSite::LAX_MODE:
same_site = NSHTTPCookieSameSiteLax;
break;
case net::CookieSameSite::STRICT_MODE:
same_site = NSHTTPCookieSameSiteStrict;
break;
case net::CookieSameSite::NO_RESTRICTION:
same_site = kNSHTTPCookieSameSiteNone;
break;
case net::CookieSameSite::UNSPECIFIED:
// All other values of same site policy will be treated as no value .
break;
}
properties[NSHTTPCookieSameSitePolicy] = same_site;
}

if (cookie.IsSecure())
[properties setObject:@"Y" forKey:NSHTTPCookieSecure];
if (cookie.IsHttpOnly())
Expand Down
40 changes: 30 additions & 10 deletions ios/net/cookies/system_cookie_util_unittest.mm
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,18 @@
"IOS.Cookies.GetCookiesForURLCallResult";

void CheckSystemCookie(const base::Time& expires, bool secure, bool httponly) {
net::CookieSameSite same_site = net::CookieSameSite::NO_RESTRICTION;
if (@available(iOS 13, *)) {
// SamesitePolicy property of NSHTTPCookieStore is available on iOS 13+.
same_site = net::CookieSameSite::LAX_MODE;
}
// Generate a canonical cookie.
net::CanonicalCookie canonical_cookie(
kCookieName, kCookieValue, kCookieDomain, kCookiePath,
base::Time(), // creation
expires,
base::Time(), // last_access
secure, httponly, net::CookieSameSite::NO_RESTRICTION,
net::COOKIE_PRIORITY_DEFAULT);
secure, httponly, same_site, net::COOKIE_PRIORITY_DEFAULT);
// Convert it to system cookie.
NSHTTPCookie* system_cookie =
SystemCookieFromCanonicalCookie(canonical_cookie);
Expand All @@ -53,6 +57,10 @@ void CheckSystemCookie(const base::Time& expires, bool secure, bool httponly) {
EXPECT_EQ(secure, [system_cookie isSecure]);
EXPECT_EQ(httponly, [system_cookie isHTTPOnly]);
EXPECT_EQ(expires.is_null(), [system_cookie isSessionOnly]);

if (@available(iOS 13, *)) {
EXPECT_NSEQ(NSHTTPCookieSameSiteLax, [system_cookie sameSitePolicy]);
}
// Allow 1 second difference as iOS rounds expiry time to the nearest second.
base::Time system_cookie_expire_date = base::Time::FromDoubleT(
[[system_cookie expiresDate] timeIntervalSince1970]);
Expand All @@ -79,14 +87,23 @@ void VerifyGetCookiesResultHistogram(
base::Time expire_date = creation_time + base::TimeDelta::FromHours(2);
NSDate* system_expire_date =
[NSDate dateWithTimeIntervalSince1970:expire_date.ToDoubleT()];
NSHTTPCookie* system_cookie = [[NSHTTPCookie alloc] initWithProperties:@{
NSHTTPCookieDomain : @"foo",
NSHTTPCookieName : @"a",
NSHTTPCookiePath : @"/",
NSHTTPCookieValue : @"b",
NSHTTPCookieExpires : system_expire_date,
@"HttpOnly" : @YES,
}];
NSMutableDictionary* properties =
[NSMutableDictionary dictionaryWithDictionary:@{
NSHTTPCookieDomain : @"foo",
NSHTTPCookieName : @"a",
NSHTTPCookiePath : @"/",
NSHTTPCookieValue : @"b",
NSHTTPCookieExpires : system_expire_date,
@"HttpOnly" : @YES,
}];
if (@available(iOS 13, *)) {
// sameSitePolicy is only available on iOS 13+.
properties[NSHTTPCookieSameSitePolicy] = NSHTTPCookieSameSiteStrict;
}

NSHTTPCookie* system_cookie =
[[NSHTTPCookie alloc] initWithProperties:properties];

ASSERT_TRUE(system_cookie);
net::CanonicalCookie chrome_cookie =
CanonicalCookieFromSystemCookie(system_cookie, creation_time);
Expand All @@ -105,6 +122,9 @@ void VerifyGetCookiesResultHistogram(
EXPECT_FALSE(chrome_cookie.IsSecure());
EXPECT_TRUE(chrome_cookie.IsHttpOnly());
EXPECT_EQ(net::COOKIE_PRIORITY_DEFAULT, chrome_cookie.Priority());
if (@available(iOS 13, *)) {
EXPECT_EQ(net::CookieSameSite::STRICT_MODE, chrome_cookie.SameSite());
}

// Test session and secure cookie.
system_cookie = [[NSHTTPCookie alloc] initWithProperties:@{
Expand Down
17 changes: 14 additions & 3 deletions ios/web/download/download_session_cookie_storage.mm
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,23 @@ - (void)deleteCookie:(NSHTTPCookie*)cookie {
- (nullable NSArray<NSHTTPCookie*>*)cookiesForURL:(NSURL*)URL {
NSMutableArray<NSHTTPCookie*>* result = [NSMutableArray array];
GURL gURL = net::GURLWithNSURL(URL);
net::CookieOptions options;
options.set_include_httponly();
// TODO(crbug.com/1018272): Compute the cookie access semantic, and update
// |options| with it.
net::CookieOptions options = net::CookieOptions::MakeAllInclusive();
net::CookieAccessSemantics cookieAccessSemantics =
net::CookieAccessSemantics::LEGACY;
if (@available(iOS 13, *)) {
// Using |UNKNOWN| semantics to allow the experiment to switch between non
// legacy (where cookies that don't have a specific same-site access policy
// and not secure will not be included), and legacy mode.
cookieAccessSemantics = net::CookieAccessSemantics::UNKNOWN;
}
for (NSHTTPCookie* cookie in self.cookies) {
net::CanonicalCookie canonical_cookie =
net::CanonicalCookieFromSystemCookie(cookie, base::Time());
if (canonical_cookie.IncludeForRequestURL(gURL, options).IsInclude())
if (canonical_cookie
.IncludeForRequestURL(gURL, options, cookieAccessSemantics)
.IsInclude())
[result addObject:cookie];
}
return [result copy];
Expand Down
17 changes: 14 additions & 3 deletions ios/web/net/cookies/wk_http_system_cookie_store.mm
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,20 @@ bool ShouldIncludeForRequestUrl(NSHTTPCookie* cookie, const GURL& url) {
// to support cookieOptions this function can be modified to support that.
net::CanonicalCookie canonical_cookie =
net::CanonicalCookieFromSystemCookie(cookie, base::Time());
net::CookieOptions options;
options.set_include_httponly();
return canonical_cookie.IncludeForRequestURL(url, options).IsInclude();
// Cookies handled by this method are app specific cookies, so it's safe to
// use strict same site context.
net::CookieOptions options = net::CookieOptions::MakeAllInclusive();
net::CookieAccessSemantics cookie_access_semantics =
net::CookieAccessSemantics::LEGACY;
if (@available(iOS 13, *)) {
// Using |UNKNOWN| semantics to allow the experiment to switch between non
// legacy (where cookies that don't have a specific same-site access policy
// and not secure will not be included), and legacy mode.
cookie_access_semantics = net::CookieAccessSemantics::UNKNOWN;
}
return canonical_cookie
.IncludeForRequestURL(url, options, cookie_access_semantics)
.IsInclude();
}

} // namespace
Expand Down

0 comments on commit b5b5342

Please sign in to comment.