Bug 157354 – Handle _schemeUpgraded delegate callbacks in NSURLSessionDataDelegate

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Handle _schemeUpgraded delegate callbacks in NSURLSessionDataDelegate

[Alex Christensen]

Created attachment 278124 [details]
Patch

[Darin Adler]

Comment on attachment 278124 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278124&action=review

Can we make a regression test for this? If not, why not? Usually change log needs to explain this.

I would find the change log comment easier to read if HTTP, HTTPS, and URL were capitalized.

> Source/WebKit2/ChangeLog:10
> +        Added based on synthesizeRedirectResponseIfNecessary in WebCoreURLResponse.mm.

And we need to keep two copies of the code? No way to share code? Or should the two pieces of code point to each other with comments at least?

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:83
> +    if ([[[newRequest URL] scheme] isEqualToString:[[[task currentRequest] URL] scheme]])
> +        return nil;

Does this need to be an ASCII case-insensitive comparison? Will we do the wrong thing if someone uses HTTP instead of http? Test coverage?

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:86
> +    // This is critical for HSTS (<rdar://problem/14241270>).

We should keep Radar URLs in WebKit comments (as opposed to change logs) to a minimum. If this is really valuable then I think it’s OK. I think a better explanation would be preferable to the Radar URL.

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:88
> +    NSDictionary *synthesizedResponseHeaderFields = @{ @"Location": [[newRequest URL] absoluteString], @"Cache-Control": @"no-store" };
> +    return [[[NSHTTPURLResponse alloc] initWithURL:[[task currentRequest] URL] statusCode:302 HTTPVersion:(NSString *)kCFHTTPVersion1_1 headerFields:synthesizedResponseHeaderFields] autorelease];

What’s truly interesting about this code is what it does include and what it does not. I‘d love to see a comment explaining why these two header fields are generated but no others, why HTTP 1.1, and why 302 rather than some other redirect status code.

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:149
> +    } else
> +        completionHandler(nil);

Would be much more elegant with early return:

    auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier, storedCredentials);
    if (!networkDataTask) {
        completionHandler(nil); 
        return;
    }

    ... code that don’t have to be nested inside an if ....

[Alex Christensen]

(In reply to comment #2)
> Comment on attachment 278124 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278124&action=review
> 
> Can we make a regression test for this? If not, why not? Usually change log
> needs to explain this.
I'm going to do more manual testing and then try to write a regression test before landing.
> And we need to keep two copies of the code? No way to share code? Or should
> the two pieces of code point to each other with comments at least?
This one has a NSURLSessionTask parameter and synthesizeRedirectResponseIfNecessary  takes an NSURLConnection.  They should both take just NSURLRequests.
> 
> > Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:83
> > +    if ([[[newRequest URL] scheme] isEqualToString:[[[task currentRequest] URL] scheme]])
> > +        return nil;
> 
> Does this need to be an ASCII case-insensitive comparison? Will we do the
> wrong thing if someone uses HTTP instead of http? Test coverage?
This is probably an existing problem.
> > Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:88
> > +    NSDictionary *synthesizedResponseHeaderFields = @{ @"Location": [[newRequest URL] absoluteString], @"Cache-Control": @"no-store" };
> > +    return [[[NSHTTPURLResponse alloc] initWithURL:[[task currentRequest] URL] statusCode:302 HTTPVersion:(NSString *)kCFHTTPVersion1_1 headerFields:synthesizedResponseHeaderFields] autorelease];
> 
> What’s truly interesting about this code is what it does include and what it
> does not. I‘d love to see a comment explaining why these two header fields
> are generated but no others, why HTTP 1.1, and why 302 rather than some
> other redirect status code.
I'm not sure.  I've seen HSTS redirects in the wild using several different status codes.  This one seems to work, but I'm not sure why it was chosen instead of the others.

[Alex Christensen]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 278124 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=278124&action=review
> > 
> > Can we make a regression test for this? If not, why not? Usually change log
> > needs to explain this.
> I'm going to do more manual testing and then try to write a regression test
> before landing.
We cannot use the existing testing infrastructure to test this because we are using a self-signed certificate, and that is incompatible with HSTS.  See https://tools.ietf.org/html/rfc6797#section-11.3
This goes on the list of things to test with real SSL certificates.

[Alex Christensen]

(In reply to comment #2)
> Would be much more elegant with early return:
> 
>     auto* networkDataTask =
> _session->dataTaskForIdentifier(task.taskIdentifier, storedCredentials);
>     if (!networkDataTask) {
>         completionHandler(nil); 
>         return;
>     }
> 
>     ... code that don’t have to be nested inside an if ....
I agree, but this doesn't match the rest of the code in this file.  I can change it all together in a followup patch.

[Alex Christensen]

http://trac.webkit.org/changeset/200655