Fail navigations to non app-bound domains after use of app-bound APIs

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

Navigations to non app-bound domains should not be able to occur after using certain APIs covered by In-App Browser Privacy protections.

[Kate Cheney]

<rdar://problem/62978159>

[Kate Cheney]

Created attachment 398902 [details]
Patch

[Kate Cheney]

Created attachment 399046 [details]
Patch

[Kate Cheney]

(In reply to katherine_cheney from comment #3)
> Created attachment 399046 [details]
> Patch

Hoping this fixes EWS test bots.

[Kate Cheney]

Created attachment 399049 [details]
Patch

[Brent Fulgham]

Comment on attachment 399049 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:3132
> +                return false;

Do we need to set any values here, rather than just returning false? E.g., should we make sure the WebViewCategory be set to 'AppBound' here?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7106
> +        m_navigationHasOccured = true;

Since we always set this if it's false, and we always set it to true (no matter what), it's probably better to just set it to true here (without the test first)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7116
> +        send(Messages::WebPageProxy::SetHasExecutedAppBoundBehaviorBeforeNavigation());

This method name sounds like we are asking the page if we should enable In-App Browser Privacy. But we also always send a message to the UIProcess indicating we did something app-bound. I worry that could be confusing to someone reading the code who doesn't expect this side-effect.

Should the message send be done as a separate step?

What does it mean if the answer to 'should allow' is NO. Don't we avoid doing the app-bound thing? So does it make sense to tell the UIProcess that we did some kind of app-bound behavior?

[Kate Cheney]

Comment on attachment 399049 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3132
>> +                return false;
> 
> Do we need to set any values here, rather than just returning false? E.g., should we make sure the WebViewCategory be set to 'AppBound' here?

Since reaching this code means m_limitsNavigationsToAppBoundDomains was not set, I don't think we should set this to be app-bound. The flag should still probably have to be set to trigger app-bound privileges.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7106
>> +        m_navigationHasOccured = true;
> 
> Since we always set this if it's false, and we always set it to true (no matter what), it's probably better to just set it to true here (without the test first)

Yes, will change!

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7116
>> +        send(Messages::WebPageProxy::SetHasExecutedAppBoundBehaviorBeforeNavigation());
> 
> This method name sounds like we are asking the page if we should enable In-App Browser Privacy. But we also always send a message to the UIProcess indicating we did something app-bound. I worry that could be confusing to someone reading the code who doesn't expect this side-effect.
> 
> Should the message send be done as a separate step?
> 
> What does it mean if the answer to 'should allow' is NO. Don't we avoid doing the app-bound thing? So does it make sense to tell the UIProcess that we did some kind of app-bound behavior?

I see what you are saying. Do you mean a separate function that is called at each site in addition to shouldEnableInAppBrowserPrivacyProtections(), or do you mean a separate function within this function?

Yes, you're right. If shouldEnable is true, we definitely shouldn't send the message because it means the behavior was not allowed. Good catch!

[Brent Fulgham]

Comment on attachment 399049 [details]
Patch

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

>>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:7116
>>> +        send(Messages::WebPageProxy::SetHasExecutedAppBoundBehaviorBeforeNavigation());
>> 
>> This method name sounds like we are asking the page if we should enable In-App Browser Privacy. But we also always send a message to the UIProcess indicating we did something app-bound. I worry that could be confusing to someone reading the code who doesn't expect this side-effect.
>> 
>> Should the message send be done as a separate step?
>> 
>> What does it mean if the answer to 'should allow' is NO. Don't we avoid doing the app-bound thing? So does it make sense to tell the UIProcess that we did some kind of app-bound behavior?
> 
> I see what you are saying. Do you mean a separate function that is called at each site in addition to shouldEnableInAppBrowserPrivacyProtections(), or do you mean a separate function within this function?
> 
> Yes, you're right. If shouldEnable is true, we definitely shouldn't send the message because it means the behavior was not allowed. Good catch!

I think it would be clearer if we only sent the message when we were actually doing something app-bound. So I propose we have a second method that just does the message send (if needed), and call it when we use one of the app-bound powers.

[Kate Cheney]

Created attachment 399062 [details]
Patch

[Brent Fulgham]

Comment on attachment 399062 [details]
Patch

Looks good! r=me.

[Chris Dumez]

Comment on attachment 399062 [details]
Patch

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

> Source/WebCore/loader/FrameLoaderClient.h:382
> +    virtual bool shouldEnableInAppBrowserPrivacyProtections() { return false; }

can this be const?

> Source/WebKit/Shared/LoadParameters.h:73
> +    Optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain { WTF::nullopt };

{ WTF::nullopt } is unnecessary.

> Source/WebKit/UIProcess/WebPageProxy.h:2815
> +    Optional<NavigatingToAppBoundDomain> m_isNavigatingToAppBoundDomain { WTF::nullopt };

{ WTF::nullopt } is unnecessary.

> Source/WebKit/WebProcess/WebPage/WebPage.h:2106
> +    Optional<NavigatingToAppBoundDomain> m_isNavigatingToAppBoundDomain { WTF::nullopt };

{ WTF::nullopt } is unnecessary.

[Kate Cheney]

Created attachment 399068 [details]
Patch for landing

[Kate Cheney]

Thanks Chris and Brent!

(In reply to Chris Dumez from comment #11)
> Comment on attachment 399062 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399062&action=review
> 
> > Source/WebCore/loader/FrameLoaderClient.h:382
> > +    virtual bool shouldEnableInAppBrowserPrivacyProtections() { return false; }
> 
> can this be const?
> 

Yes, patch for landing has this change.

> > Source/WebKit/Shared/LoadParameters.h:73
> > +    Optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain { WTF::nullopt };
> 
> { WTF::nullopt } is unnecessary.
> 
> > Source/WebKit/UIProcess/WebPageProxy.h:2815
> > +    Optional<NavigatingToAppBoundDomain> m_isNavigatingToAppBoundDomain { WTF::nullopt };
> 
> { WTF::nullopt } is unnecessary.
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.h:2106
> > +    Optional<NavigatingToAppBoundDomain> m_isNavigatingToAppBoundDomain { WTF::nullopt };
> 
> { WTF::nullopt } is unnecessary.

Removed.

[EWS]

Committed r261506: <https://trac.webkit.org/changeset/261506>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399068 [details].