Bug 205407 – Create a mechanism for 'safe by default' web views

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

Many WebKit applications use it as a convenient way of displaying local resources, such as in help systems, editing programs, splash screens, and so forth.

These use cases do not need the full machinery required for a full-fledged web browser, and could have a considerably tighter sandbox and more limited API access by default when used in one of these modes.

We should allow for a set of common WebView use cases, and help developers avoid introducing security risks to their applications by accidentally enabling powerful features.

This patch will create some common use categories that we can use as a basis for a hierarchy of features.

[Radar WebKit Bug Importer]

<rdar://problem/58053071>

[Kate Cheney]

Created attachment 386012 [details]
Patch

[Kate Cheney]

Created attachment 386139 [details]
Patch

[Andy Estes]

Comment on attachment 386139 [details]
Patch

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

I'd like to see this patch make use of this new enum in some way and then have an API test.

Can you also add a _WKWebViewCategory-typed property to WKWebViewConfiguration, decide what the default value will be for clients that do not explicitly set a value, and then write an API test?

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:43
> +    _WKWebViewCategoryWebBrowser,
> +    _WKWebViewCategoryInAppBrowser,
> +    _WKWebViewCategoryHybridApp,

I'd sort these.

[Kate Cheney]

Created attachment 386651 [details]
Patch

[Sam Weinig]

Comment on attachment 386651 [details]
Patch

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

This seems like a weird concept. Traditionally, we have said that all WKWebView's should be "Safe by Default".

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:40
> +typedef NS_ENUM(NSUInteger, _WKWebViewCategory) {

Category feels like a very broad concept for what you are defining here. I think a more specific name would help in understanding what this if for.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:46
> +#define DEFAULT_WEBVIEW_CATEGORY _WKWebViewCategory::_WKWebViewCategoryHybridApp

I don't believe the :: syntax is valid in normal objective-c (e.g. not objective-c++). We also don't traditionally use macros like this in API headers. If you need to expose a way of figuring out the default, it should probably be a class method, so it can be changed over time.

[Andy Estes]

Comment on attachment 386651 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:46
>> +#define DEFAULT_WEBVIEW_CATEGORY _WKWebViewCategory::_WKWebViewCategoryHybridApp
> 
> I don't believe the :: syntax is valid in normal objective-c (e.g. not objective-c++). We also don't traditionally use macros like this in API headers. If you need to expose a way of figuring out the default, it should probably be a class method, so it can be changed over time.

I don't think the default does need to be exposed, so I'd recommend defining the default near where it's used in WKWebViewConfiguration.mm.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:125
> +@property (nonatomic, readonly) _WKWebViewCategory _webViewCategory WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Doesn't this property need to be read-write so that clients can change it from the default value?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/Configuration.mm:65
> +    EXPECT_EQ([configuration _webViewCategory], _WKWebViewCategory::_WKWebViewCategoryHybridApp);

You should just say _WKWebViewCategoryHybridApp here.

[Sam Weinig]

I am curious if you considered any alternatives to this explicit categorization concept. 

For instance, if the goal is to make it easier for developers to get default values that match their use case more closely, then one approach would be to expose a set convenience class methods on WKWebViewConfiguration that expose configurations that come close to what they want. e.g. using your naming:

+ (WKWebViewConfiguration *)defaultHybridAppConfiguration;
+ (WKWebViewConfiguration *)defaultWebBrowserConfiguration;
+ (WKWebViewConfiguration *)defaultInAppBrowserConfiguration;


Any tightening of sandboxes or changes to exposed APIs that needs to happen for one of these could then be exposed as individual configuration options that could be tweaked from any configuration, even a fully custom one.

[Kate Cheney]

(In reply to Andy Estes from comment #7)
> Comment on attachment 386651 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386651&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:125
> > +@property (nonatomic, readonly) _WKWebViewCategory _webViewCategory WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Doesn't this property need to be read-write so that clients can change it
> from the default value?
> 

Yes, and it should have a setter too with API tests. I will change this.

[Kate Cheney]

(In reply to Sam Weinig from comment #8)
> I am curious if you considered any alternatives to this explicit
> categorization concept. 
> 
> For instance, if the goal is to make it easier for developers to get default
> values that match their use case more closely, then one approach would be to
> expose a set convenience class methods on WKWebViewConfiguration that expose
> configurations that come close to what they want. e.g. using your naming:
> 
> + (WKWebViewConfiguration *)defaultHybridAppConfiguration;
> + (WKWebViewConfiguration *)defaultWebBrowserConfiguration;
> + (WKWebViewConfiguration *)defaultInAppBrowserConfiguration;
> 
> 
> Any tightening of sandboxes or changes to exposed APIs that needs to happen
> for one of these could then be exposed as individual configuration options
> that could be tweaked from any configuration, even a fully custom one.

I see what you're suggesting. I think explicit categorization makes more sense here though, because I don't think many (if any) of the safety checks will involve default values from WKWebViewConfiguration. If that changes as we develop this more, we can circle back to the idea of methods.

[Kate Cheney]

Created attachment 386855 [details]
Patch

[Sam Weinig]

(In reply to katherine_cheney from comment #10)
> (In reply to Sam Weinig from comment #8)
> > I am curious if you considered any alternatives to this explicit
> > categorization concept. 
> > 
> > For instance, if the goal is to make it easier for developers to get default
> > values that match their use case more closely, then one approach would be to
> > expose a set convenience class methods on WKWebViewConfiguration that expose
> > configurations that come close to what they want. e.g. using your naming:
> > 
> > + (WKWebViewConfiguration *)defaultHybridAppConfiguration;
> > + (WKWebViewConfiguration *)defaultWebBrowserConfiguration;
> > + (WKWebViewConfiguration *)defaultInAppBrowserConfiguration;
> > 
> > 
> > Any tightening of sandboxes or changes to exposed APIs that needs to happen
> > for one of these could then be exposed as individual configuration options
> > that could be tweaked from any configuration, even a fully custom one.
> 
> I see what you're suggesting. I think explicit categorization makes more
> sense here though, because I don't think many (if any) of the safety checks
> will involve default values from WKWebViewConfiguration. If that changes as
> we develop this more, we can circle back to the idea of methods.

Can you outline the specific changes you have in mind?

[Kate Cheney]

(In reply to Sam Weinig from comment #12)
> (In reply to katherine_cheney from comment #10)
> > (In reply to Sam Weinig from comment #8)
> > > I am curious if you considered any alternatives to this explicit
> > > categorization concept. 
> > > 
> > > For instance, if the goal is to make it easier for developers to get default
> > > values that match their use case more closely, then one approach would be to
> > > expose a set convenience class methods on WKWebViewConfiguration that expose
> > > configurations that come close to what they want. e.g. using your naming:
> > > 
> > > + (WKWebViewConfiguration *)defaultHybridAppConfiguration;
> > > + (WKWebViewConfiguration *)defaultWebBrowserConfiguration;
> > > + (WKWebViewConfiguration *)defaultInAppBrowserConfiguration;
> > > 
> > > 
> > > Any tightening of sandboxes or changes to exposed APIs that needs to happen
> > > for one of these could then be exposed as individual configuration options
> > > that could be tweaked from any configuration, even a fully custom one.
> > 
> > I see what you're suggesting. I think explicit categorization makes more
> > sense here though, because I don't think many (if any) of the safety checks
> > will involve default values from WKWebViewConfiguration. If that changes as
> > we develop this more, we can circle back to the idea of methods.
> 
> Can you outline the specific changes you have in mind?

Please take a look at the radar

[Alex Christensen]

Comment on attachment 386855 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:182
> +    _WKWebViewCategory _webViewCategory;

Please put anything new in API::PageConfiguration
Could you also add a comment saying such?

[Andy Estes]

Comment on attachment 386855 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:56
> +#define DEFAULT_WEBVIEW_CATEGORY _WKWebViewCategoryHybridApp

I don't think you need this #define.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:274
> +    _webViewCategory = DEFAULT_WEBVIEW_CATEGORY;

I'd use _WKWebViewCategoryHybridApp directly instead of DEFAULT_WEBVIEW_CATEGORY.

[Andy Estes]

(In reply to Andy Estes from comment #15)
> Comment on attachment 386855 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386855&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:56
> > +#define DEFAULT_WEBVIEW_CATEGORY _WKWebViewCategoryHybridApp
> 
> I don't think you need this #define.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:274
> > +    _webViewCategory = DEFAULT_WEBVIEW_CATEGORY;
> 
> I'd use _WKWebViewCategoryHybridApp directly instead of
> DEFAULT_WEBVIEW_CATEGORY.

And I'd also move _webViewCategory to API::PageConfiguration per Alex's suggestion.

[Kate Cheney]

Created attachment 387033 [details]
Patch

[Alex Christensen]

Comment on attachment 387033 [details]
Patch

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

r=me with a few nits

> Source/WebKit/Shared/WebViewCategory.h:30
> +enum WebViewCategory {

This should be enum class WebViewCategory : uint8_t

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1149
> +static WebKit::WebViewCategory convertToAPIWebViewCategory(_WKWebViewCategory category)

toWebKitWebViewCategory

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1163
> +static _WKWebViewCategory convertFromAPIWebViewCategory(WebKit::WebViewCategory category)

toWKWebViewCategory

> Tools/TestWebKitAPI/Tests/WebKitCocoa/Configuration.mm:65
> +    EXPECT_EQ([configuration _webViewCategory], _WKWebViewCategoryHybridApp);

These can be merged to one test.

[Kate Cheney]

Created attachment 387044 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387044 [details]
Patch

Clearing flags on attachment: 387044

Committed r254180: <https://trac.webkit.org/changeset/254180>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.