Bug 205407 - Create a mechanism for 'safe by default' web views
Summary: Create a mechanism for 'safe by default' web views
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-18 12:22 PST by Brent Fulgham
Modified: 2020-01-07 17:44 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.52 KB, patch)
2019-12-18 15:06 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (1.59 KB, patch)
2019-12-19 13:32 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (5.00 KB, patch)
2020-01-02 18:12 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2020-01-06 09:37 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (13.87 KB, patch)
2020-01-07 14:26 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (13.86 KB, patch)
2020-01-07 15:34 PST, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2019-12-18 12:22:51 PST
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.
Comment 1 Radar WebKit Bug Importer 2019-12-18 12:23:13 PST
<rdar://problem/58053071>
Comment 2 Kate Cheney 2019-12-18 15:06:17 PST
Created attachment 386012 [details]
Patch
Comment 3 Kate Cheney 2019-12-19 13:32:31 PST
Created attachment 386139 [details]
Patch
Comment 4 Andy Estes 2019-12-20 09:29:49 PST
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.
Comment 5 Kate Cheney 2020-01-02 18:12:06 PST
Created attachment 386651 [details]
Patch
Comment 6 Sam Weinig 2020-01-03 11:51:17 PST
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.
Comment 7 Andy Estes 2020-01-03 14:52:25 PST
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.
Comment 8 Sam Weinig 2020-01-03 16:37:25 PST
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.
Comment 9 Kate Cheney 2020-01-03 17:13:27 PST
(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.
Comment 10 Kate Cheney 2020-01-06 09:20:31 PST
(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.
Comment 11 Kate Cheney 2020-01-06 09:37:52 PST
Created attachment 386855 [details]
Patch
Comment 12 Sam Weinig 2020-01-06 15:36:41 PST
(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?
Comment 13 Kate Cheney 2020-01-06 17:18:37 PST
(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
Comment 14 Alex Christensen 2020-01-06 17:22:49 PST
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?
Comment 15 Andy Estes 2020-01-07 09:48:25 PST
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.
Comment 16 Andy Estes 2020-01-07 09:49:36 PST
(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.
Comment 17 Kate Cheney 2020-01-07 14:26:16 PST
Created attachment 387033 [details]
Patch
Comment 18 Alex Christensen 2020-01-07 14:49:50 PST
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.
Comment 19 Kate Cheney 2020-01-07 15:34:29 PST
Created attachment 387044 [details]
Patch
Comment 20 WebKit Commit Bot 2020-01-07 17:44:13 PST
Comment on attachment 387044 [details]
Patch

Clearing flags on attachment: 387044

Committed r254180: <https://trac.webkit.org/changeset/254180>
Comment 21 WebKit Commit Bot 2020-01-07 17:44:15 PST
All reviewed patches have been landed.  Closing bug.