Bug 182865 – Add an entitlement check for service worker on iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Ryosuke Niwa]

We need an entitlement check for service worker on iOS

<rdar://problem/37505903>

[Ryosuke Niwa]

Created attachment 334011 [details]
Adds a check

[Ryosuke Niwa]

Comment on attachment 334011 [details]
Adds a check

Actually, no. I have to add this to WebKitTestRunner as well.

Comment on attachment 334011 [details]
Adds a check

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:613
> +#if PLATFORM(IOS) && ENABLE(SERVICE_WORKER)
> +    if (!WebKit::processHasEntitlement(@"com.apple.developer.WebKit.ServiceWorkers"))
> +        pageConfiguration->preferenceValues().set(WebKit::WebPreferencesKey::serviceWorkersEnabledKey(), WebKit::WebPreferencesStore::Value(false));
> +#endif

This is not a meaningful way to restrict capabilities based on entitlements. To be effective, the entitlement check needs to happen in a different process (typically, the process that provides the capability).

[Ryosuke Niwa]

(In reply to mitz from comment #3)
> Comment on attachment 334011 [details]
> Adds a check
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334011&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:613
> > +#if PLATFORM(IOS) && ENABLE(SERVICE_WORKER)
> > +    if (!WebKit::processHasEntitlement(@"com.apple.developer.WebKit.ServiceWorkers"))
> > +        pageConfiguration->preferenceValues().set(WebKit::WebPreferencesKey::serviceWorkersEnabledKey(), WebKit::WebPreferencesStore::Value(false));
> > +#endif
> 
> This is not a meaningful way to restrict capabilities based on entitlements.
> To be effective, the entitlement check needs to happen in a different
> process (typically, the process that provides the capability).

Hm... I guess we need to check this again in WebContent process?

[EWS Watchlist]

Comment on attachment 334011 [details]
Adds a check

Attachment 334011 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6531610

New failing tests:
http/tests/security/http-0.9/xhr-blocked.html

[EWS Watchlist]

Created attachment 334018 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 334011 [details]
Adds a check

Attachment 334011 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6531520

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 334019 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Ryosuke Niwa]

Created attachment 334020 [details]
WIP

[Ryosuke Niwa]

Created attachment 334021 [details]
WIP

[EWS Watchlist]

Comment on attachment 334021 [details]
WIP

Attachment 334021 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6532977

New failing tests:
http/tests/security/http-0.9/xhr-blocked.html

[EWS Watchlist]

Created attachment 334025 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Ryosuke Niwa]

Comment on attachment 334025 [details]
Archive of layout-test-results from ews100 for mac-sierra

This test failure is not related to the entitlement check.

[Ryosuke Niwa]

Created attachment 334076 [details]
Adds an entitlement check

[Ryosuke Niwa]

Created attachment 334085 [details]
Fixed non-Cocoa builds

[Ryosuke Niwa]

Comment on attachment 334085 [details]
Fixed non-Cocoa builds

Clearing flags on attachment: 334085

Committed r228589: <https://trac.webkit.org/changeset/228589>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

Comment on attachment 334085 [details]
Fixed non-Cocoa builds

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

> Source/WebKit/Shared/mac/SandboxUtilities.mm:109
> +bool connectedProcessHasEntitlement(xpc_connection_t connection, NSString *entitlement)
> +{
> +    audit_token_t token;
> +    xpc_connection_get_audit_token(connection, &token);
> +    auto task = adoptCF(SecTaskCreateWithAuditToken(NULL, token));
> +
> +    auto value = adoptCF(SecTaskCopyValueForEntitlement(task.get(), (__bridge CFStringRef)entitlement, nullptr));
> +    if (!value)
> +        return false;
> +
> +    if (CFGetTypeID(value.get()) != CFBooleanGetTypeID())
> +        return false;
> +
> +    return CFBooleanGetValue(static_cast<CFBooleanRef>(value.get()));
> +}

In XPCServiceInitializerDelegate::hasEntitlement we use xpc_connection_copy_entitlement_value, which appears to be much more succinct than this. Is there a reason to prefer this version here?

[Ryosuke Niwa]

(In reply to mitz from comment #18)
> Comment on attachment 334085 [details]
> Fixed non-Cocoa builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334085&action=review
>
> In XPCServiceInitializerDelegate::hasEntitlement we use
> xpc_connection_copy_entitlement_value, which appears to be much more
> succinct than this. Is there a reason to prefer this version here?

Oh, I didn't know this function. We can fix it use this function instead.

[Ryosuke Niwa]

Reopening to attach new patch.

[Ryosuke Niwa]

Created attachment 334097 [details]
Addresses Dan's comment

[Ryosuke Niwa]

Committed r228933: <https://trac.webkit.org/changeset/228933>