Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Commit

Permalink
Workaround for caching with iOS 8 and NSURLSession.
Browse files Browse the repository at this point in the history
Observationally, we've seen the NSURL loading system fail to insert
items into the NSURLCache when using NSURLSession and iOS 7/8.
NSURLConnection works fine on all these, and NSURLSession works fine on
iOS 9. Best guess is there's a bug in the NSURLProtocol implementation
that fails to insert into the cache.

So, here we are creating a workaround for that specific case. CocoaSPDY
will buffer the data internally, up to a certain size limit based on the
cache size, and will manually insert into the cache when the response is
done. This could potentially be expanded in the future for unclaimed,
finished push responses, but for now those are excluded.

This change also adds a few additional caching-related unit tests.
  • Loading branch information
kgoodier committed Dec 11, 2015
1 parent 11d02ec commit 2a2c2f8
Show file tree
Hide file tree
Showing 5 changed files with 270 additions and 14 deletions.
23 changes: 23 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,29 @@ Most existing SPDY implementations use a TLS extension called Next Protocol Impl
In order to aid with protocol inference, this SPDY implementation includes a non-standard settings id at index 0: `SETTINGS_MINOR_VERSION`. This is necessary to differentiate between SPDY/3 and SPDY/3.1 connections that were not negotiated with NPN, since only the major version is included in the frame header. Because not all servers may support this particular setting, sending it can be disabled at runtime through protocol configuration.

## Implementation Notes
### Caching with NSURLCache
CocoaSPDY partially supports HTTP caching using NSURLCaches, though there are a number of caveats. The default policy for `NSURLConnection` is to disable caching, in order to maintain backward-compatibility and mimize risk. CocoaSPDY does not perform HEAD validation requests, so any cached response that will require revalidation with the server will result in a full request and response.

**NSURLConnection**
* Ensure the global `[NSURLCache sharedCached]` is set to the desired `NSURLCache`
* Set the `NSMutableURLRequest.cachePolicy` to the desired policy (`NSURLRequestUseProtocolCachePolicy` will result in no caching)

**NSURLSession**
* Set the `NSMutableURLRequest.SPDYURLSession` to your NSURLSession instance
* Set the `NSURLSessionConfiguration.URLCache` to the desired `NSURLCache`
* Set the `NSURLSessionConfiguration.cachePolicy` to the desired policy (can be left as `NSURLRequestUseProtocolCachePolicy`)

**Request/response requirements**
* Request must not include an `Authorization` header.
* Request must be a GET.
* Response must include a `Date` header.
* Response must include a `Cache-Control` header with a "max-age" parameter set to a non-zero value. And when retrieving the cached response, this must not result in a stale item.
* The `Cache-Control` response header must not have "no-cache", "no-store", or "must-revalidate" in it.
* To prevent caching of a response, set `Cache-Control: no-store` in the request.
* For all the requirements, see implementation details at https://github.com/twitter/CocoaSPDY/blob/develop/SPDY/SPDYCacheStoragePolicy.m

Essentially, you must opt-in with `NSURLConnection` by setting a cache policy or with `NSURLSession` by configuring a cache. The response must be cacheable and have a max-age defined, and the request must be a GET request. The cached response must be usable without revalidation.

### CRIME attack
The [CRIME attack](http://en.wikipedia.org/wiki/CRIME) is a plaintext injection technique that exploits the fact that information can be inferred from compressed content length to potentially reveal the contents of an encrypted stream. This is a serious issue for browsers, which are subject to hijacks that may allow an attacker to issue an arbitrary number of requests with known plaintext header content and observe the resulting effect on compression.

Expand Down
104 changes: 97 additions & 7 deletions SPDY/SPDYStream.m
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ @implementation SPDYStream

NSURLRequest *_pushRequest; // stored because we need a strong reference, _request is weak.
NSHTTPURLResponse *_response;

NSURLCacheStoragePolicy _cachePolicy;
NSMutableData *_cacheDataBuffer; // only used for manual caching.
NSInteger _cacheMaxItemSize;
}

- (instancetype)initWithProtocol:(SPDYProtocol *)protocol
Expand Down Expand Up @@ -332,6 +336,9 @@ - (void)_close
[self markUnblocked]; // just in case. safe if already unblocked.
_metadata.blockedMs = _blockedElapsed * 1000;

// Manually make the willCacheResponse callback when needed
[self _storeCacheResponse];

[_client URLProtocolDidFinishLoading:_protocol];

if (_delegate && [_delegate respondsToSelector:@selector(streamClosed:)]) {
Expand Down Expand Up @@ -585,11 +592,24 @@ - (void)didReceiveResponse
return;
}

if (_client) {
NSURLCacheStoragePolicy cachePolicy = SPDYCacheStoragePolicy(_request, _response);
[_client URLProtocol:_protocol
didReceiveResponse:_response
cacheStoragePolicy:cachePolicy];
_cachePolicy = SPDYCacheStoragePolicy(_request, _response);
[_client URLProtocol:_protocol
didReceiveResponse:_response
cacheStoragePolicy:_cachePolicy];

// Prepare for manual caching, if needed

This comment has been minimized.

Copy link
@NSProgrammer

NSProgrammer Dec 11, 2015

Collaborator

getting the NSURLSession's cache every time when this could be an NSURLSession or NSURLConnection request is confusing me. Though it will be more verbose, it will also appear more explicit if we break it up a little:

if (NSURLCacheStorageNotAllowed != _cachePolicy && // cacheable?
    [self _shouldUseManualCaching]) { // is this an NSURLSession request that needs a workaround?
    NSURLCache *cache = _protocol.associatedSession.configuration.URLCache;
    if (cache != nil && _local) { // we have a cache and it's not a push request?
        ...
    }
}

This comment has been minimized.

Copy link
@kgoodier

kgoodier Dec 11, 2015

Author Contributor

Since it will just be nil for NSURLConnection, this isn't particularly expensive. But I think your suggestion is clearer visually and a little easier to reason about, so I'll change it.

NSURLCache *cache = _protocol.associatedSession.configuration.URLCache;
if (_cachePolicy != NSURLCacheStorageNotAllowed && // cacheable?
[self _shouldUseManualCaching] && // hack needed and NSURLSession used?
cache != nil && // cache configured (NSURLSession-specific)?
_local) { // not a push request?

// The NSURLCache has a heuristic to limit the max size of items based on the capacity of the
// cache. This is our attempt to mimic that behavior and prevent unlimited buffering of large
// responses. These numbers were found by manually experimenting and are only approximate.
// See SPDYNSURLCachingTest testResponseNearItemCacheSize_DoesUseCache.
_cacheDataBuffer = [[NSMutableData alloc] init];
_cacheMaxItemSize = MAX(cache.memoryCapacity * 0.05, cache.diskCapacity * 0.01);

This comment has been minimized.

Copy link
@NSProgrammer

NSProgrammer Dec 11, 2015

Collaborator

max size is max of 5% of memory and 1% of disk? Apple specified the following in NSURLSessionDataDelegate:

The response size is small enough to reasonably fit within the cache.
(For example, if you provide a disk cache, the response must be no larger
than about 5% of the disk cache size.)

It's clearly not specific, so I'll leave it to you

This comment has been minimized.

Copy link
@kgoodier

kgoodier Dec 11, 2015

Author Contributor

I ran some manual tests and found roughly 1% to be the boundary. Maybe with a larger cache the limit percentage goes up? I don't know how or even if we can come up with an exact match. I was erring towards smaller, but since Apple states roughly 5%, I'm ok going with that. It's arbitrary.

}
}

Expand Down Expand Up @@ -689,7 +709,7 @@ - (void)didLoadData:(NSData *)data
NSUInteger inflatedLength = DECOMPRESSED_CHUNK_LENGTH - _zlibStream.avail_out;
inflatedData.length = inflatedLength;
if (inflatedLength > 0) {
[_client URLProtocol:_protocol didLoadData:inflatedData];
[self _didLoadDataChunk:inflatedData];
}

// This can happen if the decompressed data is size N * DECOMPRESSED_CHUNK_LENGTH,
Expand All @@ -711,7 +731,77 @@ - (void)didLoadData:(NSData *)data
}
} else {
NSData *dataCopy = [[NSData alloc] initWithBytes:data.bytes length:dataLength];
[_client URLProtocol:_protocol didLoadData:dataCopy];
[self _didLoadDataChunk:dataCopy];
}
}

- (void)_didLoadDataChunk:(NSData *)data
{
[_client URLProtocol:_protocol didLoadData:data];

if (_cacheDataBuffer) {
NSUInteger bufferSize = _cacheDataBuffer.length + data.length;
if (bufferSize < _cacheMaxItemSize) {

This comment has been minimized.

Copy link
@NSProgrammer

NSProgrammer Dec 11, 2015

Collaborator

you have < for less than exclusive but I would expect this to be <= for less than inclusive. It's just 1 byte, so just a nitpick.

This comment has been minimized.

Copy link
@kgoodier

kgoodier Dec 11, 2015

Author Contributor

:)

[_cacheDataBuffer appendData:data];
} else {
// Throw away anything already buffered, it's going to be too big
_cacheDataBuffer = nil;
}
}
}

// NSURLSession on iOS 8 and iOS 7 does not cache items. Seems to be a bug with its interation
// with NSURLProtocol. This flag represents whether our workaround, to insert into the cache
// ourselves, is turned on.
- (bool)_shouldUseManualCaching

This comment has been minimized.

Copy link
@NSProgrammer

NSProgrammer Dec 11, 2015

Collaborator

a few things...

  1. you can populate your osVersion with a dispatch once so you don't have to keep computing it
  2. I think CocoaSPDY is supposed to be iOS & Mac OS X, so you need to support checking for <= iOS 8 and Mac OS X <= 10.10

This comment has been minimized.

Copy link
@kgoodier

kgoodier Dec 11, 2015

Author Contributor

true and true.

{
NSInteger osVersion = 0;
NSProcessInfo *processInfo = [NSProcessInfo processInfo];
if ([processInfo respondsToSelector:@selector(operatingSystemVersion)]) {
osVersion = [processInfo operatingSystemVersion].majorVersion;
}

// iOS 8 and earlier and using NSURLSession
return (osVersion <= 8 && _protocol.associatedSession != nil && _protocol.associatedSessionTask != nil);
}

- (void)_storeCacheResponse
{
if (_cacheDataBuffer == nil) {
return;
}

NSCachedURLResponse *cachedResponse;
cachedResponse = [[NSCachedURLResponse alloc] initWithResponse:_response
data:_cacheDataBuffer
userInfo:nil
storagePolicy:_cachePolicy];
NSURLCache *cache = _protocol.associatedSession.configuration.URLCache;

This comment has been minimized.

Copy link
@NSProgrammer

NSProgrammer Dec 11, 2015

Collaborator

you should assert the cache isn't nil

This comment has been minimized.

Copy link
@kgoodier

kgoodier Dec 12, 2015

Author Contributor

done

NSURLSessionDataTask *dataTask = (NSURLSessionDataTask *)_protocol.associatedSessionTask;

This comment has been minimized.

Copy link
@NSProgrammer

NSProgrammer Dec 11, 2015

Collaborator

assert the task isn't nil

This comment has been minimized.

Copy link
@kgoodier

kgoodier Dec 12, 2015

Author Contributor

done


// Make "official" willCacheResponse callback to app, bypassing the NSURL loading system.
id<NSURLSessionDataDelegate> delegate = (id)_protocol.associatedSession.delegate;
if ([delegate respondsToSelector:@selector(URLSession:dataTask:willCacheResponse:completionHandler:)]) {
NSOperationQueue *queue = _protocol.associatedSession.delegateQueue;

This comment has been minimized.

Copy link
@NSProgrammer

NSProgrammer Dec 11, 2015

Collaborator

you refer to the session a bunch, let's pull it into a variable up top and reuse it

This comment has been minimized.

Copy link
@kgoodier

kgoodier Dec 12, 2015

Author Contributor

done

[(queue) ?: [NSOperationQueue mainQueue] addOperationWithBlock:^{

This comment has been minimized.

Copy link
@NSProgrammer

NSProgrammer Dec 11, 2015

Collaborator

I see an edge case here. Since this work to call the delegate is async, it is feasible (and actually reasonably possible) for the completion callbacks to happen first before the "willCache" callback happens. If this is the case, it is very feasible for a delegate to have all it's work related to that task end up being "torn down" (on complete, discard all variables that were maintaining state). Since that is the case, it is also feasible that there is dependent state for how the "willCache" callback will behave but since that will have been cleared/invalidated/reset the behavior in willCache is now undefined.

I know it's added work, but the cache work MUST be completed before the completion of the request (to the protocol client) can be triggered.

This comment has been minimized.

Copy link
@kgoodier

kgoodier Dec 12, 2015

Author Contributor

I dug deeper into my assumptions about the order of operations within an NSOperationQueue, and while they are generally right, Apple says it can't be guaranteed, so you're right, we need to be more explicit about this. Will fix.

From NSOperationQueue docs:

An operation queue executes its queued operation objects based on their priority and readiness. If all of the queued operation objects have the same priority and are ready to execute when they are put in the queue—that is, their isReady method returns YES—they are executed in the order in which they were submitted to the queue. However, you should never rely on queue semantics to ensure a specific execution order of operation objects. Changes in the readiness of an operation can change the resulting execution order. If you need operations to execute in a specific order, use operation-level dependencies as defined by the NSOperation class.

[delegate URLSession:_protocol.associatedSession dataTask:dataTask willCacheResponse:cachedResponse completionHandler:^(NSCachedURLResponse * cachedResponse) {
// This block may execute asynchronously at any time. No need to come back to the SPDY/NSURL thread
if (cachedResponse) {
if ([cache respondsToSelector:@selector(storeCachedResponse:forDataTask:)]) {
[cache storeCachedResponse:cachedResponse forDataTask:dataTask];
} else {
[cache storeCachedResponse:cachedResponse forRequest:_request];
}
}
}];
}];
} else {
// willCacheResponse delegate not implemented. Default behavior is to cache.
if ([cache respondsToSelector:@selector(storeCachedResponse:forDataTask:)]) {
[cache storeCachedResponse:cachedResponse forDataTask:dataTask];
} else {
[cache storeCachedResponse:cachedResponse forRequest:_request];
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions SPDYUnitTests/SPDYIntegrationTestHelper.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,13 @@
- (BOOL)didLoadFromNetwork;
- (BOOL)didLoadFromCache;
- (BOOL)didGetResponse;
- (BOOL)didLoadData;
- (BOOL)didGetError;
- (BOOL)didCacheResponse;

- (void)reset;
- (void)loadRequest:(NSURLRequest *)request;
- (void)provideResponseWithStatus:(NSUInteger)status cacheControl:(NSString *)cacheControl date:(NSDate *)date dataChunks:(NSArray *)dataChunks;
- (void)provideResponseWithStatus:(NSUInteger)status cacheControl:(NSString *)cacheControl date:(NSDate *)date;
- (void)provideBasicUncacheableResponse;
- (void)provideBasicCacheableResponse;
Expand Down
26 changes: 20 additions & 6 deletions SPDYUnitTests/SPDYIntegrationTestHelper.m
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,24 @@ - (instancetype)init

- (BOOL)didLoadFromNetwork
{
return self.didGetResponse && _stream != nil;
return self.didGetResponse && self.didLoadData && _stream != nil;
}

- (BOOL)didLoadFromCache
{
return self.didGetResponse && _stream == nil;
return self.didGetResponse && self.didLoadData && _stream == nil;
}

- (BOOL)didGetResponse
{
return _response != nil;
}

- (BOOL)didLoadData
{
return _data.length > 0;
}

- (BOOL)didGetError
{
return _connectionError != nil;
Expand Down Expand Up @@ -100,12 +105,10 @@ - (NSString *)dateHeaderValue:(NSDate *)date
return string;
}

- (void)provideResponseWithStatus:(NSUInteger)status cacheControl:(NSString *)cacheControl date:(NSDate *)date
- (void)provideResponseWithStatus:(NSUInteger)status cacheControl:(NSString *)cacheControl date:(NSDate *)date dataChunks:(NSArray *)dataChunks
{
[SPDYMockSessionManager shared].streamQueuedBlock = ^(SPDYStream *stream) {
_stream = stream;
uint8_t dataBytes[] = {1};
NSData *data = [NSData dataWithBytes:dataBytes length:1];
NSMutableDictionary *headers = [NSMutableDictionary dictionaryWithDictionary:@{
@":status": [@(status) stringValue],
@":version": @"1.1",
Expand All @@ -119,10 +122,19 @@ - (void)provideResponseWithStatus:(NSUInteger)status cacheControl:(NSString *)ca

[stream mergeHeaders:headers];
[stream didReceiveResponse];
[stream didLoadData:data];
for (NSData *data in dataChunks) {
[stream didLoadData:data];
}
};
}

- (void)provideResponseWithStatus:(NSUInteger)status cacheControl:(NSString *)cacheControl date:(NSDate *)date
{
uint8_t dataBytes[] = {1};
NSArray *dataChunks = @[ [NSData dataWithBytes:dataBytes length:1] ];
[self provideResponseWithStatus:status cacheControl:cacheControl date:date dataChunks:dataChunks];
}

- (void)provideBasicUncacheableResponse
{
[self provideResponseWithStatus:200 cacheControl:@"no-store, no-cache, must-revalidate" date:[NSDate date]];
Expand Down Expand Up @@ -157,6 +169,7 @@ - (NSString *)description
NSDictionary *params = @{
@"didLoadFromNetwork": @([self didLoadFromNetwork]),
@"didGetResponse": @([self didGetResponse]),
@"didLoadData": @([self didLoadData]),
@"didGetError": @([self didGetError]),
@"didCacheResponse": @([self didCacheResponse]),
@"stream": _stream ?: @"<nil>",
Expand Down Expand Up @@ -237,6 +250,7 @@ - (instancetype)init
}
return self;
}

- (void)loadRequest:(NSMutableURLRequest *)request
{
[super loadRequest:request];
Expand Down
Loading

0 comments on commit 2a2c2f8

Please sign in to comment.