Bug 210769 - AppBound domain behavior should abide by the limitsNavigationsToAppBoundDomains argument in WKWebView configuration
Summary: AppBound domain behavior should abide by the limitsNavigationsToAppBoundDomai...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: katherine_cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-20 14:59 PDT by katherine_cheney
Modified: 2020-04-21 09:22 PDT (History)
3 users (show)

See Also:


Attachments
Patch (24.91 KB, patch)
2020-04-20 15:41 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch for landing (24.84 KB, patch)
2020-04-20 18:37 PDT, katherine_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 katherine_cheney 2020-04-20 14:59:14 PDT
The limitsNavigationsToAppBoundDomains flag in the WKWebView configuration should opt a WebView into AppBound domain behavior and limit navigations to the domains in the Info.plist
Comment 1 katherine_cheney 2020-04-20 14:59:44 PDT
<rdar://problem/62065241>
Comment 2 katherine_cheney 2020-04-20 15:41:59 PDT
Created attachment 397024 [details]
Patch
Comment 3 katherine_cheney 2020-04-20 16:33:26 PDT
I also think we can eventually clean this up and merge m_isNavigatingToAppBoundDomain and m_hasNavigatedAwayFromAppBoundDomain into one Optional parameter that defaults to WTF::nullopt. But this would be a large refactoring and is probably better for a future patch.
Comment 4 Brent Fulgham 2020-04-20 17:37:27 PDT
Comment on attachment 397024 [details]
Patch

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

Looks good. r=me, but please consider my suggestion for renaming.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3130
> +bool WebPageProxy::setIsNavigatingToAppBoundDomain(bool isMainFrame, const URL& requestURL, Optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain)

I think this would be clearer with a name change: Maybe something like:

bool WebPageProxy::setIsNavigatingToAppBoundDomainAndCheckIfPermitted(...)

> Source/WebKit/UIProcess/WebPageProxy.cpp:3144
> +            if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes) {

I would suggest reversing this check, and doing an early return:

if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::No)
    return false;

Also: If we make the decision to return early here, should we be setting "m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No"?

> Source/WebKit/UIProcess/WebPageProxy.cpp:3150
>          }

Maybe this could then be an 'else' block, and both fall through to the 'true' at the bottom.
Comment 5 Brent Fulgham 2020-04-20 17:38:02 PDT
(In reply to katherine_cheney from comment #3)
> I also think we can eventually clean this up and merge
> m_isNavigatingToAppBoundDomain and m_hasNavigatedAwayFromAppBoundDomain into
> one Optional parameter that defaults to WTF::nullopt. But this would be a
> large refactoring and is probably better for a future patch.

Yes -- this sounds like a good next step. Might simplify the logic and make it easier to understand. :-)
Comment 6 katherine_cheney 2020-04-20 18:22:41 PDT
Comment on attachment 397024 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3130
>> +bool WebPageProxy::setIsNavigatingToAppBoundDomain(bool isMainFrame, const URL& requestURL, Optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain)
> 
> I think this would be clearer with a name change: Maybe something like:
> 
> bool WebPageProxy::setIsNavigatingToAppBoundDomainAndCheckIfPermitted(...)

Good idea, I'll do this

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3144
>> +            if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes) {
> 
> I would suggest reversing this check, and doing an early return:
> 
> if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::No)
>     return false;
> 
> Also: If we make the decision to return early here, should we be setting "m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No"?

Early return sounds good.

m_hasNavigatedAwayFromAppBoundDomain defaults to No, and won't ever be changed to Yes for a webView with the m_limitsNavigationsToAppBoundDomains flag, so it should always be the case that m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No, and we shouldn't need to set it.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3150
>>          }
> 
> Maybe this could then be an 'else' block, and both fall through to the 'true' at the bottom.

Will do!
Comment 7 katherine_cheney 2020-04-20 18:22:46 PDT
Comment on attachment 397024 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3130
>> +bool WebPageProxy::setIsNavigatingToAppBoundDomain(bool isMainFrame, const URL& requestURL, Optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain)
> 
> I think this would be clearer with a name change: Maybe something like:
> 
> bool WebPageProxy::setIsNavigatingToAppBoundDomainAndCheckIfPermitted(...)

Good idea, I'll do this

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3144
>> +            if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes) {
> 
> I would suggest reversing this check, and doing an early return:
> 
> if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::No)
>     return false;
> 
> Also: If we make the decision to return early here, should we be setting "m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No"?

Early return sounds good.

m_hasNavigatedAwayFromAppBoundDomain defaults to No, and won't ever be changed to Yes for a webView with the m_limitsNavigationsToAppBoundDomains flag, so it should always be the case that m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No, and we shouldn't need to set it.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3150
>>          }
> 
> Maybe this could then be an 'else' block, and both fall through to the 'true' at the bottom.

Will do!
Comment 8 katherine_cheney 2020-04-20 18:37:33 PDT
Created attachment 397045 [details]
Patch for landing
Comment 9 EWS 2020-04-20 19:36:32 PDT
Committed r260408: <https://trac.webkit.org/changeset/260408>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397045 [details].
Comment 10 Brent Fulgham 2020-04-21 09:22:46 PDT
Comment on attachment 397024 [details]
Patch

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

>>>> Source/WebKit/UIProcess/WebPageProxy.cpp:3144
>>>> +            if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes) {
>>> 
>>> I would suggest reversing this check, and doing an early return:
>>> 
>>> if (*isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::No)
>>>     return false;
>>> 
>>> Also: If we make the decision to return early here, should we be setting "m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No"?
>> 
>> Early return sounds good.
>> 
>> m_hasNavigatedAwayFromAppBoundDomain defaults to No, and won't ever be changed to Yes for a webView with the m_limitsNavigationsToAppBoundDomains flag, so it should always be the case that m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No, and we shouldn't need to set it.
> 
> Early return sounds good.
> 
> m_hasNavigatedAwayFromAppBoundDomain defaults to No, and won't ever be changed to Yes for a webView with the m_limitsNavigationsToAppBoundDomains flag, so it should always be the case that m_hasNavigatedAwayFromAppBoundDomain = NavigatedAwayFromAppBoundDomain::No, and we shouldn't need to set it.

Ah -- okay. I didn't understand that.