Bug 159039 - Integrate WebKit's CFURLConnection with App Transport Security
Summary: Integrate WebKit's CFURLConnection with App Transport Security
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-06-22 14:35 PDT by Oliver Hunt
Modified: 2016-06-23 10:21 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.16 KB, patch)
2016-06-22 14:37 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (2.57 KB, patch)
2016-06-22 14:57 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (2.61 KB, patch)
2016-06-22 15:00 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (2.71 KB, patch)
2016-06-22 15:33 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2016-06-22 14:35:55 PDT
Integrate WebKit's CFURLConnection with App Transport Security
Comment 1 Oliver Hunt 2016-06-22 14:37:43 PDT
Created attachment 281870 [details]
Patch
Comment 2 Alex Christensen 2016-06-22 14:45:18 PDT
Comment on attachment 281870 [details]
Patch

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

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:134
> +    extern const CFStringRef _kCFURLConnectionPropertyATSContext;

This should probably go in CFNetworkSPI.h.

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:233
> +    [propertyDictionary setObject:@{@"NSAllowsArbitraryLoadsInWebContent": @""} forKey:(NSString *)_kCFURLConnectionPropertyATSContext];

What does @"" mean here?
Comment 3 Alex Christensen 2016-06-22 14:47:56 PDT
Is there a radar associated with this?
Comment 4 Oliver Hunt 2016-06-22 14:57:13 PDT
Created attachment 281874 [details]
Patch
Comment 5 Oliver Hunt 2016-06-22 14:59:47 PDT
<rdar://problem/26953685>
Comment 6 Oliver Hunt 2016-06-22 15:00:56 PDT
Created attachment 281876 [details]
Patch
Comment 7 Alex Christensen 2016-06-22 15:29:03 PDT
Comment on attachment 281876 [details]
Patch

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

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:231
> +    [propertyDictionary setObject:@{@"NSAllowsArbitraryLoadsInWebContent": @""} forKey:(NSString *)_kCFURLConnectionPropertyATSContext];

This needs to be surrounded by an #if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100)
Or something similar.
Is this even needed on Mac?
Comment 8 Oliver Hunt 2016-06-22 15:33:05 PDT
Created attachment 281879 [details]
Patch
Comment 9 WebKit Commit Bot 2016-06-22 16:23:58 PDT
Comment on attachment 281879 [details]
Patch

Clearing flags on attachment: 281879

Committed r202356: <http://trac.webkit.org/changeset/202356>
Comment 10 WebKit Commit Bot 2016-06-22 16:24:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 David Kilzer (:ddkilzer) 2016-06-23 10:17:33 PDT
Comment on attachment 281879 [details]
Patch

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

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:230
> +#if TARGET_OS_IPHONE || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100)

Kind of weird to use TARGET_OS_IPHONE instead of PLATFORM(IOS) here.

Should we be checking for iOS 10 here as well?  (The property is probably ignored on older builds, so it's probably not necessary to add a version check.
Comment 12 Oliver Hunt 2016-06-23 10:21:01 PDT
(In reply to comment #11)
> Comment on attachment 281879 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281879&action=review
> 
> > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:230
> > +#if TARGET_OS_IPHONE || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101100)
> 
> Kind of weird to use TARGET_OS_IPHONE instead of PLATFORM(IOS) here.
> 
> Should we be checking for iOS 10 here as well?  (The property is probably
> ignored on older builds, so it's probably not necessary to add a version
> check.

I used identical guards to the definition point to avoid any chance of things going wrong :)

And yeah, it's a no-op option elsewhere :)