Bug 227589 – WKWebView javascript injection doesn't work if app includes WKAppBoundDomains

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[jcesarmobile]

Sample app:
https://github.com/jcesarmobile/appboundsbug

Steps to reproduce:
1. Clone the app
2. Run the app on iOS 14 or newer (also affects iOS 15 beta 2)

Expected:
The background should be red

Actual behavior:
The background is white

JavaScript injection can be enabled back by adding:
webViewConfiguration.limitsNavigationsToAppBoundDomains = true

and adding localhost to the WKAppBoundDomains

<key>WKAppBoundDomains</key>
<array>
    <string>localhost</string>
</array>


But according to this article https://webkit.org/blog/10882/app-bound-domains/, you should not need to add localhost as it's loading local assets.


Also the article says that to enable app bound domains you can just add the key
<key>WKAppBoundDomains</key>
but that doesn't let the app to compile, you need to add an empty array.


TLDR: Adding WKAppBoundDomains key in the info.plist should not disable JavaScript injection in apps that load local assets, it should only disable it for the external domains that you add in the WKAppBoundDomains list if limitsNavigationsToAppBoundDomains is not set to true. But for local assets it should always work no matter what the value is and users should not need to add localhost to the list.


The sample app is a simple example, but adding WKAppBoundDomains key breaks all Ionic Capacitor plugins as they rely on the JavaScript injection

[Kate Cheney]

Hi, thanks for filing this.

(In reply to jcesarmobile from comment #0)
> Sample app:
> https://github.com/jcesarmobile/appboundsbug
> 
> Steps to reproduce:
> 1. Clone the app
> 2. Run the app on iOS 14 or newer (also affects iOS 15 beta 2)
> 
> Expected:
> The background should be red
> 
> Actual behavior:
> The background is white
> 
> JavaScript injection can be enabled back by adding:
> webViewConfiguration.limitsNavigationsToAppBoundDomains = true
> 

This is expected. In the blog post we mention: "Once the WKAppBoundDomains key is added to the Info.plist, all WKWebView instances in the application default to a mode where JavaScript injection, custom style sheets, cookie manipulation, and message handler use is denied. To gain back access to these APIs, a WKWebView can set the limitsNavigationsToAppBoundDomains flag in their WKWebView configuration".

> and adding localhost to the WKAppBoundDomains
> 
> <key>WKAppBoundDomains</key>
> <array>
>     <string>localhost</string>
> </array>
> 

This should not be needed. Just to clarify, if you remove this value from the Info.plist but keep limitsNavigationsToAppBoundDomains = YES in the configuration, JS injection does not work?

> 
> But according to this article
> https://webkit.org/blog/10882/app-bound-domains/, you should not need to add
> localhost as it's loading local assets.
> 
> 
> Also the article says that to enable app bound domains you can just add the
> key
> <key>WKAppBoundDomains</key>
> but that doesn't let the app to compile, you need to add an empty array.
> 

This may be an error in the docs. Thanks for pointing this out.

> 
> TLDR: Adding WKAppBoundDomains key in the info.plist should not disable
> JavaScript injection in apps that load local assets, it should only disable
> it for the external domains that you add in the WKAppBoundDomains list if
> limitsNavigationsToAppBoundDomains is not set to true. But for local assets
> it should always work no matter what the value is and users should not need
> to add localhost to the list.
> 
> 
> The sample app is a simple example, but adding WKAppBoundDomains key breaks
> all Ionic Capacitor plugins as they rely on the JavaScript injection

[jcesarmobile]

> This should not be needed. Just to clarify, if you remove this value from the Info.plist but keep limitsNavigationsToAppBoundDomains = YES in the configuration, JS injection does not work?

yeah, if I remove "localhost", then the JavaScript injection doesn't work, even with limitsNavigationsToAppBoundDomains = true

[Kate Cheney]

(In reply to jcesarmobile from comment #2)
> > This should not be needed. Just to clarify, if you remove this value from the Info.plist but keep limitsNavigationsToAppBoundDomains = YES in the configuration, JS injection does not work?
> 
> yeah, if I remove "localhost", then the JavaScript injection doesn't work,
> even with limitsNavigationsToAppBoundDomains = true

Thanks for the clarification, I am looking into this.

[Radar WebKit Bug Importer]

<rdar://problem/80327452>

[Kate Cheney]

Created attachment 433463 [details]
Patch

[Brent Fulgham]

Comment on attachment 433463 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServer.cpp:343
> +static unsigned registrationCount(HashMap<ServiceWorkerRegistrationKey, WeakPtr<SWServerRegistration>> registrationMap)

If we hit this on a hot code path, we might want to cache the answer and perhaps update when a new SW is registered or removed.

I'd suggest doing this work in the SWServer::addRegistration and SWServer::removeRegistration. Then you could use the variable in validateRegistrationDomain.

It looks like you might need to update the count in removeFromScopeToRegistrationMap, too, which is weird. I wonder why there is a separate method for that?

[Kate Cheney]

Created attachment 433687 [details]
Patch

[Kate Cheney]

(In reply to Brent Fulgham from comment #6)
> Comment on attachment 433463 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433463&action=review
> 
> > Source/WebCore/workers/service/server/SWServer.cpp:343
> > +static unsigned registrationCount(HashMap<ServiceWorkerRegistrationKey, WeakPtr<SWServerRegistration>> registrationMap)
> 
> If we hit this on a hot code path, we might want to cache the answer and
> perhaps update when a new SW is registered or removed.
> 
> I'd suggest doing this work in the SWServer::addRegistration and
> SWServer::removeRegistration. Then you could use the variable in
> validateRegistrationDomain.
> 
> It looks like you might need to update the count in
> removeFromScopeToRegistrationMap, too, which is weird. I wonder why there is
> a separate method for that?

Added this in the latest patch. Going to wait for EWS before landing. Thanks for the review!

[Kate Cheney]

Created attachment 433696 [details]
Patch

[EWS]

Committed r280016 (239758@main): <https://commits.webkit.org/239758@main>

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