Bug 231248 – Add an entitlement check to only allow AdAttributionDaemon to be connected to by the network process

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Add an entitlement check to only allow AdAttributionDaemon to be connected to by the network process

[Alex Christensen]

Created attachment 440257 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 440257 [details]
Patch

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

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/Daemon/PCMDaemonEntryPoint.mm:84
> +            NSLog(@"connection attempted connection without required entitlement");

Does this look right in the log starting with lower case?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:513
> +    xpc_connection_set_event_handler(connection.get(), ^(xpc_object_t event) {
> +        if (event == XPC_ERROR_CONNECTION_INTERRUPTED) {
> +            done = true;
> +    });

Not sure if this tests enough. Isn't the purpose to verify that the connection is never established? This just verifies that it's dropped, perhaps only eventually.

Also, failing as timeout isn't always understood to be a legitimate failure in practice.

[Alex Christensen]

Comment on attachment 440257 [details]
Patch

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

>> Source/WebKit/Shared/EntryPointUtilities/Cocoa/Daemon/PCMDaemonEntryPoint.mm:84
>> +            NSLog(@"connection attempted connection without required entitlement");
> 
> Does this look right in the log starting with lower case?

I'll capitalize.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:483
> +    system([NSString stringWithFormat:@"launchctl unload %@ 2> /dev/null", plistLocation.path].fileSystemRepresentation);

I committed this separately in http://trac.webkit.org/r283574

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:513
>> +    });
> 
> Not sure if this tests enough. Isn't the purpose to verify that the connection is never established? This just verifies that it's dropped, perhaps only eventually.
> 
> Also, failing as timeout isn't always understood to be a legitimate failure in practice.

This is what it looks like at this API level when a connection is never established.  I'll change this to make sure event is always equal to XPC_ERROR_CONNECTION_INTERRUPTED to make sure that nothing else happens.

I agree that it's not great to have the failure mode be a timeout, but I think that's how this test needs to be.

[Alex Christensen]

Created attachment 440285 [details]
Patch

[EWS]

Committed r283586 (242544@main): <https://commits.webkit.org/242544@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/83910831>