Bug 118003 – [Mac] Document URL is not updated by HSTS

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Alexey Proskuryakov]

When opening an HTTP link to an HSTS enabled site, WebKit does not update document URL, so window.location remains an http one.

<rdar://problem/14241270>

[Alexey Proskuryakov]

Created attachment 205430 [details]
proposed fix

[Brady Eidson]

Comment on attachment 205430 [details]
proposed fix

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

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsDelegate.mm:80
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090
> +    // <rdar://problem/14241270>: Synthesize a response for HSTS.
> +    if (!redirectResponse && ![[[newRequest URL] scheme] isEqualToString:[[[connection currentRequest] URL] scheme]]) {
> +        NSDictionary *synthesizedResponseHeaderFields = @{ @"Location": [[newRequest URL] absoluteString], @"Cache-Control": @"no-store" };
> +        redirectResponse = [[[NSHTTPURLResponse alloc] initWithURL:[[connection currentRequest] URL] statusCode:302 HTTPVersion:(NSString *)kCFHTTPVersion1_1 headerFields:synthesizedResponseHeaderFields] autorelease];
> +    }
> +#endif

This exact same code is copied below in WebCoreResourceHandelAsOperationQueueDelegate.mm.  Are you sure we can't find a suitable place to share it?

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:121
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090
> +    if (!redirectResponse && ![[[newRequest URL] scheme] isEqualToString:[[[connection currentRequest] URL] scheme]]) {
> +        NSDictionary *synthesizedResponseHeaderFields = @{ @"Location": [[newRequest URL] absoluteString], @"Cache-Control": @"no-store" };
> +        redirectResponse = [[[NSHTTPURLResponse alloc] initWithURL:[[connection currentRequest] URL] statusCode:302 HTTPVersion:(NSString *)kCFHTTPVersion1_1 headerFields:synthesizedResponseHeaderFields] autorelease];
> +    }
> +#endif

Except in this copy there's no comment pointing to the radar.  I like the comment, and think it should be wherever this code is.  And, I think that this code should be in precisely one place... :)

[Brady Eidson]

Created attachment 205431 [details]
Patch v2 - Bigger patch, more shared code.

[WebKit Commit Bot]

Attachment 205431 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/mac/MacNetworkingExtras.h', u'Source/WebCore/platform/network/mac/MacNetworkingExtras.mm', u'Source/WebCore/platform/network/mac/ResourceHandleMac.mm', u'Source/WebCore/platform/network/mac/ResourceResponseMac.mm', u'Source/WebCore/platform/network/mac/WebCoreResourceHandleAsDelegate.mm', u'Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm', u'Source/WebCore/platform/network/mac/WebCoreURLResponse.h', u'Source/WebCore/platform/network/mac/WebCoreURLResponse.mm']" exit_code: 1
Source/WebCore/platform/network/mac/MacNetworkingExtras.h:32:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to comment #4)
> Attachment 205431 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/mac/MacNetworkingExtras.h', u'Source/WebCore/platform/network/mac/MacNetworkingExtras.mm', u'Source/WebCore/platform/network/mac/ResourceHandleMac.mm', u'Source/WebCore/platform/network/mac/ResourceResponseMac.mm', u'Source/WebCore/platform/network/mac/WebCoreResourceHandleAsDelegate.mm', u'Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm', u'Source/WebCore/platform/network/mac/WebCoreURLResponse.h', u'Source/WebCore/platform/network/mac/WebCoreURLResponse.mm']" exit_code: 1
> Source/WebCore/platform/network/mac/MacNetworkingExtras.h:32:  Extra space before ( in function call  [whitespace/parens] [4]
> Total errors found: 1 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Fixed locally.

[Brady Eidson]

(In reply to comment #5)
> (In reply to comment #4)
> > Attachment 205431 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/mac/MacNetworkingExtras.h', u'Source/WebCore/platform/network/mac/MacNetworkingExtras.mm', u'Source/WebCore/platform/network/mac/ResourceHandleMac.mm', u'Source/WebCore/platform/network/mac/ResourceResponseMac.mm', u'Source/WebCore/platform/network/mac/WebCoreResourceHandleAsDelegate.mm', u'Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm', u'Source/WebCore/platform/network/mac/WebCoreURLResponse.h', u'Source/WebCore/platform/network/mac/WebCoreURLResponse.mm']" exit_code: 1
> > Source/WebCore/platform/network/mac/MacNetworkingExtras.h:32:  Extra space before ( in function call  [whitespace/parens] [4]
> > Total errors found: 1 in 8 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> Fixed locally.

Errrrr..... nevermind - bogus check-webkit-style not understand ObjC categories vs. function calls.

[Alexey Proskuryakov]

Comment on attachment 205431 [details]
Patch v2 - Bigger patch, more shared code.

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

> Source/WebCore/platform/network/mac/MacNetworkingExtras.h:39
> +@class NSURLConnection;
> +@class NSURLRequest;
> +@class NSURLResponse;

This header doesn't look like it's Objective-C only (there is #ifdef __OBJC__).

> Source/WebCore/platform/network/mac/MacNetworkingExtras.h:42
> +void synthesizeRedirectResponseIfNecessary(NSURLConnection *, NSURLRequest *newRequest, NSURLResponse **redirectResponse);

Can the synthesized response be returned normally?

> Source/WebCore/platform/network/mac/MacNetworkingExtras.mm:326
> +#ifdef __OBJC__

Inside a .mm file?!

> Source/WebCore/platform/network/mac/MacNetworkingExtras.mm:332
> +    ASSERT_UNUSED(redirectResponse, redirectResponse);
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 1090
> +    UNUSED_PARAM(connection);
> +    UNUSED_PARAM(newRequest);

This seems inconsistent, ASSERT_UNUSED is also not always unused.

[Brady Eidson]

(In reply to comment #7)
> (From update of attachment 205431 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205431&action=review
> 
> > Source/WebCore/platform/network/mac/MacNetworkingExtras.h:39
> > +@class NSURLConnection;
> > +@class NSURLRequest;
> > +@class NSURLResponse;
> 
> This header doesn't look like it's Objective-C only (there is #ifdef __OBJC__).

And these @class forward decls are in the #ifdef __OBJC__ block for that reason!

> > Source/WebCore/platform/network/mac/MacNetworkingExtras.h:42
> > +void synthesizeRedirectResponseIfNecessary(NSURLConnection *, NSURLRequest *newRequest, NSURLResponse **redirectResponse);
> 
> Can the synthesized response be returned normally?

Probably.
> 
> > Source/WebCore/platform/network/mac/MacNetworkingExtras.mm:326
> > +#ifdef __OBJC__
> 
> Inside a .mm file?!

heh.

> 
> > Source/WebCore/platform/network/mac/MacNetworkingExtras.mm:332
> > +    ASSERT_UNUSED(redirectResponse, redirectResponse);
> > +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 1090
> > +    UNUSED_PARAM(connection);
> > +    UNUSED_PARAM(newRequest);
> 
> This seems inconsistent, ASSERT_UNUSED is also not always unused.

The assert makes sense 100% of the time with the current method signature, but taking your earlier feedback this will change.

[Brady Eidson]

Created attachment 205432 [details]
Patch v3 - Cleaner

[WebKit Commit Bot]

Attachment 205432 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/network/mac/MacNetworkingExtras.h', u'Source/WebCore/platform/network/mac/MacNetworkingExtras.mm', u'Source/WebCore/platform/network/mac/ResourceHandleMac.mm', u'Source/WebCore/platform/network/mac/ResourceResponseMac.mm', u'Source/WebCore/platform/network/mac/WebCoreResourceHandleAsDelegate.mm', u'Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm', u'Source/WebCore/platform/network/mac/WebCoreURLResponse.h', u'Source/WebCore/platform/network/mac/WebCoreURLResponse.mm']" exit_code: 1
Source/WebCore/platform/network/mac/MacNetworkingExtras.h:32:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 205432 [details]
Patch v3 - Cleaner

Attachment 205432 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/946210

[Alexey Proskuryakov]

Comment on attachment 205432 [details]
Patch v3 - Cleaner

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

r=me. Please fix the build before landing.

> Source/WebCore/ChangeLog:15
> +        * platform/network/mac/MacNetworkingExtras.h: Renamed from Source/WebCore/platform/network/mac/WebCoreURLResponse.h.

This file name doesn't make me happy, it's so non-descriptive.

> Source/WebCore/platform/network/mac/MacNetworkingExtras.mm:328
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 1090

We usually put "new" code first.

> Source/WebCore/platform/network/mac/MacNetworkingExtras.mm:334
> +    if (!redirectResponse && ![[[newRequest URL] scheme] isEqualToString:[[[connection currentRequest] URL] scheme]]) {

I think that this would be easier to read with an early return:

if (redirectResponse)
    return redirectResponse;

if (long condition)
   ...

[Brady Eidson]

(In reply to comment #12)
> > Source/WebCore/ChangeLog:15
> > +        * platform/network/mac/MacNetworkingExtras.h: Renamed from Source/WebCore/platform/network/mac/WebCoreURLResponse.h.
> 
> This file name doesn't make me happy, it's so non-descriptive.

The more I think about it, the addition of "synthesizeRedirectResponseIfNecessary" fits in with the previous theme of the header.

Maybe it doesn't need a name change at all.

[Brady Eidson]

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

[Alexey Proskuryakov]

The code you add is about NSURLConnection, so having it in WebCoreURLResponse.h is not right.

Consider a different API where the fact that HSTS mutated the request is explicitly communicated, and we don't have to guess like this. The code would be all different, despite NSURLResponse remaining unchanged.

[Brady Eidson]

(In reply to comment #15)
> The code you add is about NSURLConnection, so having it in WebCoreURLResponse.h is not right.

The code I added refers to NSURLConnection, yes.  It also refers to NSURLResponse.

But it's core purpose is about fixing a response object to meet WebCore's expectations inside delegate callbacks.

I see no difference between this and the purpose of adjustMIMETypeIfNecessary.

While I wholeheartedly agree that "WebCoreURLResponse" is not a great name for this header, I would suggest that it was a poor name before this patch, and it is no worse a name after this patch.

> Consider a different API where the fact that HSTS mutated the request is explicitly communicated, and we don't have to guess like this. The code would be all different, despite NSURLResponse remaining unchanged.

I don't think this file is explicitly about NSURLResponse.

[Brady Eidson]

I'm definitely open to a better name than WebCoreURLResponse, and would gladly do the rename in a followup.

I just think that this patch is inline with the previous purpose of the header, and makes the poor name no worse.