Bug 182865 - Add an entitlement check for service worker on iOS
Summary: Add an entitlement check for service worker on iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-15 23:30 PST by Ryosuke Niwa
Modified: 2018-02-22 14:05 PST (History)
9 users (show)

See Also:


Attachments
Adds a check (2.03 KB, patch)
2018-02-15 23:32 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.29 MB, application/zip)
2018-02-16 00:33 PST, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.92 MB, application/zip)
2018-02-16 00:42 PST, Build Bot
no flags Details
WIP (6.83 KB, patch)
2018-02-16 01:02 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
WIP (6.85 KB, patch)
2018-02-16 01:04 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.22 MB, application/zip)
2018-02-16 02:04 PST, Build Bot
no flags Details
Adds an entitlement check (16.33 KB, patch)
2018-02-16 15:16 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed non-Cocoa builds (16.09 KB, patch)
2018-02-16 15:51 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Addresses Dan's comment (4.81 KB, patch)
2018-02-16 20:41 PST, Ryosuke Niwa
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-02-15 23:30:12 PST
We need an entitlement check for service worker on iOS

<rdar://problem/37505903>
Comment 1 Ryosuke Niwa 2018-02-15 23:32:03 PST
Created attachment 334011 [details]
Adds a check
Comment 2 Ryosuke Niwa 2018-02-15 23:38:14 PST
Comment on attachment 334011 [details]
Adds a check

Actually, no. I have to add this to WebKitTestRunner as well.
Comment 3 mitz 2018-02-15 23:42:36 PST
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).
Comment 4 Ryosuke Niwa 2018-02-16 00:04:02 PST
(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?
Comment 5 Build Bot 2018-02-16 00:33:01 PST
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
Comment 6 Build Bot 2018-02-16 00:33:02 PST
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
Comment 7 Build Bot 2018-02-16 00:42:11 PST
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.
Comment 8 Build Bot 2018-02-16 00:42:13 PST
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
Comment 9 Ryosuke Niwa 2018-02-16 01:02:45 PST
Created attachment 334020 [details]
WIP
Comment 10 Ryosuke Niwa 2018-02-16 01:04:31 PST
Created attachment 334021 [details]
WIP
Comment 11 Build Bot 2018-02-16 02:04:10 PST
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
Comment 12 Build Bot 2018-02-16 02:04:12 PST
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
Comment 13 Ryosuke Niwa 2018-02-16 11:28:42 PST
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.
Comment 14 Ryosuke Niwa 2018-02-16 15:16:18 PST
Created attachment 334076 [details]
Adds an entitlement check
Comment 15 Ryosuke Niwa 2018-02-16 15:51:50 PST
Created attachment 334085 [details]
Fixed non-Cocoa builds
Comment 16 Ryosuke Niwa 2018-02-16 16:46:54 PST
Comment on attachment 334085 [details]
Fixed non-Cocoa builds

Clearing flags on attachment: 334085

Committed r228589: <https://trac.webkit.org/changeset/228589>
Comment 17 Ryosuke Niwa 2018-02-16 16:46:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 mitz 2018-02-16 19:47:58 PST
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?
Comment 19 Ryosuke Niwa 2018-02-16 20:17:01 PST
(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.
Comment 20 Ryosuke Niwa 2018-02-16 20:41:50 PST
Reopening to attach new patch.
Comment 21 Ryosuke Niwa 2018-02-16 20:41:51 PST
Created attachment 334097 [details]
Addresses Dan's comment
Comment 22 Ryosuke Niwa 2018-02-22 14:05:08 PST
Committed r228933: <https://trac.webkit.org/changeset/228933>