Better parsing of Cache-Control header.

Feedback from code review:
- parse quoted strings
- use strptime_l
- zero tm struct for strptime_l
- allow requests with Authorization to be cached

SPDY/SPDYCacheStoragePolicy.m

@@ -12,6 +12,8 @@
 //
 
 #import "SPDYCacheStoragePolicy.h"
+#include <time.h>
+#include <xlocale.h>
 
 typedef struct _HTTPTimeFormatInfo {
     const char *readFormat;
@@ -31,12 +33,12 @@
 {
     NSDate *date = nil;
     if (string) {
-        struct tm parsedTime;
         const char *utf8String = [string UTF8String];
 
         for (int format = 0; (size_t)format < (sizeof(kTimeFormatInfos) / sizeof(kTimeFormatInfos[0])); format++) {
             HTTPTimeFormatInfo info = kTimeFormatInfos[format];
-            if (info.readFormat != NULL && strptime(utf8String, info.readFormat, &parsedTime)) {
+            struct tm parsedTime = { 0 };
+            if (info.readFormat != NULL && strptime_l(utf8String, info.readFormat, &parsedTime, NULL)) {
                 NSTimeInterval ti = (info.usesHasTimezoneInfo ? mktime(&parsedTime) : timegm(&parsedTime));
                 date = [NSDate dateWithTimeIntervalSince1970:ti];
                 if (date) {
@@ -49,18 +51,109 @@
     return date;
 }
 
+NSString *GetKey(const char **ppStr) {
+    const char *p = *ppStr;
+
+    // Advance to next delimiter
+    while (*p != '\0' && *p != '=' && *p != ',') {
+        p++;
+    }
+
+    // No progress? Error.
+    if (p == *ppStr) {
+        return nil;
+    }
+
+    NSString *str = [[NSString alloc] initWithBytes:*ppStr length:(p - *ppStr) encoding:NSUTF8StringEncoding];
+    *ppStr = p;
+    return [str stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];
+}
+
+NSString *GetValue(const char **ppStr) {
+    // **ppStr, at input, should be null for EOS, "," for single token, or "=" for dual token.
+    const char *p = *ppStr;
+
+    // Single token with no value, EOS
+    if (*p == '\0') {
+        return @"";
+    }
+
+    // Single token with no value, more after
+    if (*p == ',') {
+        (*ppStr)++;
+        return @"";
+    }
+
+    // Must be token with value, error if not
+    if (*p != '=') {
+        return nil;
+    }
+
+    // skip '='
+    p++;
+    (*ppStr)++;
+
+    // Value is either a quoted string or a token
+    NSString *str;
+    if (*p == '"') {
+        p++; // skip opening quote
+
+        // Advance to delimiter
+        while (*p != '\0' && *p != '"') {
+            p++;
+        }
+
+        // EOS before closing quote? Error.
+        if (*p == '\0') {
+            return nil;
+        }
+
+        p++; // skip closing quote
+
+        // Don't trim whitespace from within quoted string
+        str = [[NSString alloc] initWithBytes:(*ppStr + 1) length:(p - *ppStr - 2) encoding:NSUTF8StringEncoding];
+    } else {
+        // Advance to delimiter
+        while (*p != '\0' && *p != ',') {
+            p++;
+        }
+
+        // No progress? Error.
+        if (p == *ppStr) {
+            return nil;
+        }
+
+        str = [[[NSString alloc] initWithBytes:*ppStr length:(p - *ppStr) encoding:NSUTF8StringEncoding] stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];
+    }
+
+    // Skip trailing whitespace and trailing token delimiter
+    while (*p != '\0' && (*p == ' ' || *p == ',')) {
+        p++;
+    }
+
+    *ppStr = p;
+    return str;
+}
+
 NSDictionary *HTTPCacheControlParameters(NSString *cacheControl)
 {
     if (cacheControl.length == 0) {
         return nil;
     }
 
-    NSArray *components = [cacheControl componentsSeparatedByString:@","];
-    NSMutableDictionary *parameters = [NSMutableDictionary dictionaryWithCapacity:components.count];
-    for (NSString *component in components) {
-        NSArray *pair = [component componentsSeparatedByString:@"="];
-        NSString *key = [pair[0] stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];
-        NSString *value = pair.count == 2 ? [pair[1] stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]] : @"";
+    const char *pStr = [cacheControl cStringUsingEncoding:NSUTF8StringEncoding];
+
+    NSMutableDictionary *parameters = [[NSMutableDictionary alloc] init];
+
+    while (YES) {
+        NSString *key = GetKey(&pStr);
+        if (key.length == 0) {
+            break;
+        }
+        NSString *value = GetValue(&pStr);
+        if (value == nil) {
+            break;
+        }
         parameters[key] = value;
     }
     return parameters;
@@ -125,14 +218,13 @@ extern NSURLCacheStoragePolicy SPDYCacheStoragePolicy(NSURLRequest *request, NSH
     }
 
     // If we still think it might be cacheable, look at the "Cache-Control" header in 
-    // the request. Also rule out requests with Authorization in them.
+    // the request.
 
     if (cacheable) {
         NSString *requestHeader;
 
         requestHeader = [[request valueForHTTPHeaderField:@"cache-control"] lowercaseString];
-        if ((requestHeader != nil && [requestHeader rangeOfString:@"no-store"].location != NSNotFound) ||
-            [request valueForHTTPHeaderField:@"authorization"].length > 0) {
+        if (requestHeader != nil && [requestHeader rangeOfString:@"no-store"].location != NSNotFound) {
             cacheable = NO;
         }
     }
@@ -207,14 +299,18 @@ extern SPDYCachedResponseState SPDYCacheLoadingPolicy(NSURLRequest *request, NSC
         }
     }
 
-    // Note: there's a lot more validation we should do, to be a well-behaving user agent.
+    // Note: there's a lot more validation we should do, to be a well-behaving user agent. RFC7234
+    // should be consulted.
     // We don't support Pragma header.
     // We don't support Expires header.
     // We don't support Vary header.
     // We don't support ETag response header or If-None-Match request header.
     // We don't support Last-Modified response header or If-Modified-Since request header.
     // We don't look at more of the Cache-Control parameters, including ones that specify a field name.
+    // We need to generate the Age header in the cached response.
+    // We need to invalidate the cached item if PUT,POST,DELETE request gets a successful response.
+    // - including the item in Location header.
     // ...
 
     return SPDYCachedResponseStateValid;
-}
+}

SPDYUnitTests/SPDYMockSessionManager.m

@@ -48,14 +48,6 @@ + (SPDYMockSessionManager *)shared
     return instance;
 }
 
-- (instancetype)init
-{
-    self = [super init];
-    if (self != nil) {
-    }
-    return self;
-}
-
 #pragma mark SPDYSessionManager methods
 
 - (SPDYPushStreamManager *)pushStreamManager

SPDYUnitTests/SPDYNSURLCachingTest.m

@@ -98,7 +98,10 @@ - (void)testCacheableResponse_DoesInsertAndLoadFromCache
         XCTAssertTrue(testHelper.didLoadFromNetwork, @"%@", testHelper);
         XCTAssertTrue(testHelper.didCacheResponse, @"%@", testHelper);
 
-        // Now make request again. Should pull from cache.
+        // Now make request again. Should pull from cache. Create a new request object just in case.
+        request = [[NSMutableURLRequest alloc] initWithURL:[NSURL URLWithString:@"http://example.com/test/path"]];
+        request.cachePolicy = cachePolicy;
+
         [testHelper reset];
         [testHelper loadRequest:request];
 
@@ -424,7 +427,7 @@ - (void)testWithExpiredItem_DoesNotUseCache
     }
 }
 
-- (void)testRequestWithAuthorization_DoesNotUseCache
+- (void)testRequestWithAuthorization_DoesUseCache
 {
     for (NSArray *testParams in [self parameterizedTestInputs]) {
         GET_TEST_PARAMS;
@@ -439,7 +442,40 @@ - (void)testRequestWithAuthorization_DoesNotUseCache
         [testHelper loadRequest:request];
 
         XCTAssertTrue(testHelper.didLoadFromNetwork, @"%@", testHelper);
-        XCTAssertFalse(testHelper.didCacheResponse, @"%@", testHelper);
+        XCTAssertTrue(testHelper.didCacheResponse, @"%@", testHelper);
+
+        // Now make request again. Should pull from cache.
+        [testHelper reset];
+        [testHelper loadRequest:request];
+
+        XCTAssertEqual(testHelper.didLoadFromCache, shouldPullFromCache, @"%@", testHelper);
+    }
+}
+
+- (void)disabled_testRequestWithAuthorization_DoesNotUseCache_WhenHeaderChanges
+{
+    for (NSArray *testParams in [self parameterizedTestInputs]) {
+        GET_TEST_PARAMS;
+
+        NSLog(@"- using %@, policy %tu, shouldPullFromCache %tu", [testHelper class], cachePolicy, NO);
+        [self resetSharedCache];
+
+        NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL:[NSURL URLWithString:@"http://example.com/test/path"]];
+        request.cachePolicy = cachePolicy;
+        [request addValue:@"foo" forHTTPHeaderField:@"Authorization"];
+
+        [testHelper provideBasicCacheableResponse];
+        [testHelper loadRequest:request];
+
+        XCTAssertTrue(testHelper.didLoadFromNetwork, @"%@", testHelper);
+        XCTAssertTrue(testHelper.didCacheResponse, @"%@", testHelper);
+
+        // Now make request again. Should not pull from cache.
+        [testHelper reset];
+        [request setValue:@"bar" forHTTPHeaderField:@"Authorization"];
+        [testHelper loadRequest:request];
+
+        XCTAssertEqual(testHelper.didLoadFromCache, NO, @"%@", testHelper);
     }
 }
 

SPDYUnitTests/SPDYURLCacheTest.m

@@ -12,6 +12,9 @@
 #import <XCTest/XCTest.h>
 #import "SPDYCacheStoragePolicy.h"
 
+// Access to private function
+NSDictionary *HTTPCacheControlParameters(NSString *cacheControl);
+
 @interface SPDYURLCacheTest : XCTestCase
 @end
 
@@ -98,4 +101,75 @@ - (void)testCacheAllowedFor200WithNoStoreResponseHeader
     XCTAssertEqual(policy, NSURLCacheStorageNotAllowed);
 }
 
+#pragma mark HTTP Cache-Control parsing tests
+
+- (void)testOneTokenWithoutValue
+{
+    NSDictionary *params = HTTPCacheControlParameters(@"no-cache");
+    XCTAssertEqual(params.count, 1ul);
+    XCTAssertEqualObjects(params[@"no-cache"], @"");
+}
+
+- (void)testTwoTokensWithoutValues
+{
+    NSDictionary *params = HTTPCacheControlParameters(@"no-cache,no-store");
+    XCTAssertEqual(params.count, 2ul);
+    XCTAssertEqualObjects(params[@"no-cache"], @"");
+    XCTAssertEqualObjects(params[@"no-store"], @"");
+}
+
+- (void)testTwoTokensWithoutValuesWithSpaces
+{
+    NSDictionary *params = HTTPCacheControlParameters(@" no-cache, no-store ");
+    XCTAssertEqual(params.count, 2ul);
+    XCTAssertEqualObjects(params[@"no-cache"], @"");
+    XCTAssertEqualObjects(params[@"no-store"], @"");
+}
+
+- (void)testOneTokenWithValue
+{
+    NSDictionary *params = HTTPCacheControlParameters(@"max-age=5");
+    XCTAssertEqual(params.count, 1ul);
+    XCTAssertEqualObjects(params[@"max-age"], @"5");
+}
+
+- (void)testTwoTokensWithValues
+{
+    NSDictionary *params = HTTPCacheControlParameters(@"max-age=5,s-maxage=6");
+    XCTAssertEqual(params.count, 2ul);
+    XCTAssertEqualObjects(params[@"max-age"], @"5");
+    XCTAssertEqualObjects(params[@"s-maxage"], @"6");
+}
+
+- (void)testTwoTokensWithValuesWithSpaces
+{
+    NSDictionary *params = HTTPCacheControlParameters(@" max-age = 5, s-maxage= 6 ");
+    XCTAssertEqual(params.count, 2ul);
+    XCTAssertEqualObjects(params[@"max-age"], @"5");
+    XCTAssertEqualObjects(params[@"s-maxage"], @"6");
+}
+
+- (void)testOneTokenWithQuotedValue
+{
+    NSDictionary *params = HTTPCacheControlParameters(@"vary=\"foo\"");
+    XCTAssertEqual(params.count, 1ul);
+    XCTAssertEqualObjects(params[@"vary"], @"foo");
+}
+
+- (void)testTwoTokensWithQuotedValues
+{
+    NSDictionary *params = HTTPCacheControlParameters(@"extension=\"foo=bar\",vary=\"foo\"");
+    XCTAssertEqual(params.count, 2ul);
+    XCTAssertEqualObjects(params[@"extension"], @"foo=bar");
+    XCTAssertEqualObjects(params[@"vary"], @"foo");
+}
+
+- (void)testTwoTokensWithQuotedValuesWithSpaces
+{
+    NSDictionary *params = HTTPCacheControlParameters(@" extension=\" foo = bar, baz \" , vary=\"foo\" ");
+    XCTAssertEqual(params.count, 2ul);
+    XCTAssertEqualObjects(params[@"extension"], @" foo = bar, baz ");
+    XCTAssertEqualObjects(params[@"vary"], @"foo");
+}
+
 @end