Bug 211647 - Fail navigations to non app-bound domains after use of app-bound APIs
Summary: Fail navigations to non app-bound domains after use of app-bound APIs
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-05-08 15:05 PDT by katherine_cheney
Modified: 2020-05-11 16:59 PDT (History)
5 users (show)

See Also:


Attachments
Patch (76.86 KB, patch)
2020-05-08 16:02 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (78.15 KB, patch)
2020-05-11 13:12 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (78.15 KB, patch)
2020-05-11 13:32 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (79.90 KB, patch)
2020-05-11 16:03 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch for landing (79.87 KB, patch)
2020-05-11 16:29 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-05-08 15:05:41 PDT
Navigations to non app-bound domains should not be able to occur after using certain APIs covered by In-App Browser Privacy protections.
Comment 1 katherine_cheney 2020-05-08 15:06:30 PDT
<rdar://problem/62978159>
Comment 2 katherine_cheney 2020-05-08 16:02:01 PDT
Created attachment 398902 [details]
Patch
Comment 3 katherine_cheney 2020-05-11 13:12:14 PDT
Created attachment 399046 [details]
Patch
Comment 4 katherine_cheney 2020-05-11 13:12:37 PDT
(In reply to katherine_cheney from comment #3)
> Created attachment 399046 [details]
> Patch

Hoping this fixes EWS test bots.
Comment 5 katherine_cheney 2020-05-11 13:32:26 PDT
Created attachment 399049 [details]
Patch
Comment 6 Brent Fulgham 2020-05-11 14:23:25 PDT
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?
Comment 7 katherine_cheney 2020-05-11 15:18:30 PDT
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!
Comment 8 Brent Fulgham 2020-05-11 15:23:56 PDT
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.
Comment 9 katherine_cheney 2020-05-11 16:03:23 PDT
Created attachment 399062 [details]
Patch
Comment 10 Brent Fulgham 2020-05-11 16:05:31 PDT
Comment on attachment 399062 [details]
Patch

Looks good! r=me.
Comment 11 Chris Dumez 2020-05-11 16:11:42 PDT
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.
Comment 12 katherine_cheney 2020-05-11 16:29:55 PDT
Created attachment 399068 [details]
Patch for landing
Comment 13 katherine_cheney 2020-05-11 16:31:02 PDT
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.
Comment 14 EWS 2020-05-11 16:59:46 PDT
Committed r261506: <https://trac.webkit.org/changeset/261506>

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