[webkit-reviews] review granted: [Bug 226772] [Cocoa] Harden WebAuthn process by restricting to browser-entitled processes : [Attachment 431223] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 226772: [Cocoa] Harden WebAuthn process by restricting to browser-entitled
processes
https://bugs.webkit.org/show_bug.cgi?id=226772

Attachment 431223: Patch

https://bugs.webkit.org/attachment.cgi?id=431223&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 431223
  --> https://bugs.webkit.org/attachment.cgi?id=431223
Patch

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

> Source/WTF/wtf/cocoa/Entitlements.h:39
> +WTF_EXPORT_PRIVATE String processEntitlementValue(audit_token_t, const char*
entitlement);

Given this is used only in Cocoa-specific code, should it return
RetainPtr<CFStringRef> instead?

Or since it’s only used for equality checks, maybe it should be:

    WTF_EXPORT_PRIVATE bool hasEntitlementValue(audit_token_t, const char*
entitlement, const char* value);

Then the client could just write:

    if (!WTF:: hasEntitlementValue(auditToken.value(),
"com.apple.pac.shared_region_id", "WebContent") {

> Source/WTF/wtf/cocoa/Entitlements.mm:72
> +    if (!value)
> +	   return { };
> +
> +    if (CFGetTypeID(value.get()) != CFStringGetTypeID())

Better as a single if, I think.

Might even want to put a helper somewhere because it’s *so* common for us to
want to check both nullity and if a CFTypeRef is a CFStringRef at the same
time.

Someone motivated might even find a way to make it work with the syntax
is<CFStringRef> and downcast<CFStringRef> the way we do with checked casts in
our DOM classes, which would be neat! Obviously not for this patch. I’d just do
it here in this file probably for now and refactor later.