[webkit-reviews] review denied: [Bug 185761] [GTK][WPE] Add mediaDevices.enumerateDevices support : [Attachment 340841] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has denied Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 185761: [GTK][WPE] Add mediaDevices.enumerateDevices support
https://bugs.webkit.org/show_bug.cgi?id=185761

Attachment 340841: Patch

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




--- Comment #6 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 340841
  --> https://bugs.webkit.org/attachment.cgi?id=340841
Patch

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

Wow, huge patch detected.

You drew my attention because it adds new GTK/WPE API. ;) I have enough
questions and concerns about the new API that I'm going to use r-, even though
it's a fairly small portion of the patch.

> Source/WebKit/ChangeLog:21
> +	   webkit_permission_request_resolve_check API, adding the hash salt
> +	   and the decision about allowing access to the devices. Main point
> +	   of this API is to make sure that the user can control the webpages
> +	   and that they don't get smart trying to fingerprint the system.
There is a

What exactly is the salt used for? What is being salted in the first place? How
is the browser supposed to know what it's supposed to salt?

>> Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:141
>> +	if (!gst_element_link_pads(m_tee.get(), "src_%u", queue, "sink"))
>> +	    ASSERT_NOT_REACHED();
>> +
>> +	if (!gst_element_link(queue, newSink))
>> +	    ASSERT_NOT_REACHED();
> 
> This is enabled only for debug builds I think. You'd need an early return for
release builds.

A minor point, but I would say the code is good and an early return should not
be added, because the code should assume the assertion is never reached. That's
the point of the asserts, after all. Unless GCC is giving -Wreturn-type or
-Wswitch warnings, which shouldn't occur in this case. You wouldn't add an
early return in GStreamerCapturer::play or GStreamerCapturer::stop, would you?
Of course not! This isn't any different.

> Source/WebKit/UIProcess/API/glib/WebKitPermissionRequest.cpp:92
> + * We have to call this function to specify if a request is allowed
> + * using the @allowed parameter. Also we add the random data that
> + * would be used as a hash salt in case the user did not allow the
> + * origin for this kind of requests, using the
> + * @salt parameter.
> + */

Since: 2.22

> Source/WebKit/UIProcess/API/glib/WebKitPermissionRequest.cpp:93
> +void webkit_permission_request_resolve_check(WebKitPermissionRequest
*request, const gchar* mediaIdentifierHashSalt, gboolean allowed)

The mediaIdentifierHashSalt parameter is specific to
WebKitUserMediaPermissionRequest. Surely it shouldn't be in any
WebKitPermissionRequest functions? Why does it need to be part of the
interface, instead of specific to WebKitUserMediaPermissionRequest?

This causes more problems below, that would probably be best avoided here.

> Source/WebKit/UIProcess/API/glib/WebKitUserMediaPermissionRequest.cpp:57
>  struct _WebKitUserMediaPermissionRequestPrivate {
>      RefPtr<UserMediaPermissionRequestProxy> request;
> +    RefPtr<UserMediaPermissionCheckProxy> checkRequest;

I think it would be a lot simpler to add a new
WebKitUserMediaPermissionCheckRequest subclass instead, that could subclass
WebKitUserMediaPermissionRequest, and then embedders would know not to actually
prompt the user. But maybe Carlos Garcia will have a better idea.

>>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:449
>>> +	 }
>> 
>> What's the desired amount of random data you're after? Effectively you're
getting 64 * 128 / 16 = 512 bits, but hashSaltSize doesn't help clarify that.
>> 
>> Assuming 512 bits, allow me to be nit-picky and recommend using std:array:
>> ```
>>     static constexpr size_t hashSaltSize = 512;
>>     std::array<uint64_t, hashSaltSize / sizeof(uint64_t)> randomData;
>>    
cryptographicallyRandomValues(reinterpret_cast<void*>(randomData.data()),
randomData.size() * sizeof(uint64_t));
>> 
>>     StringBuilder builder;
>>     builder.reserveCapacity(randomData.size() * sizeof(uint64_t));
>>     for (auto val : randomData)
>>	   appendUnsigned64AsHex(val, builder);
>>     priv->mediaIdentifierHashSalt = builder.toString();
>> ```
> 
> Regarding the size, this is basically a default method we expect embedders to
override and properly create a storage of hash salts per origin, which should
be handled like cookies. My understanding is that the salt should be as big as
to avoid to have many collisions per origin. But in this case we are storing
one per webview because this is just the default testing method, it is just a
matter of not having a fixed one and being able to even fingerprint this
testing situation. I guess 128 is more than enough for this, that is why my
goal was 128.
> 
> I think your calculation is not correct, basically the method chooses random
hexadecimal values from a fixed array of size 16, that means 4 bits to choose a
character per uint64_t number, so 16 hexadecimal characters per uint64_t, that
is why we need 128 / 16 unint64_t elements to get 128 hexadecimal values.

WebKitWebView is surely not an appropriate place for this code? This all comes
down to the weird salt parameter in webkit_permission_request_resolve_check().

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1532
> +	* This signal is emitted when WebKit wants to check the
> +	* permissions already given to an origin by a user, such as
> +	* allowing the browser to use the microphone or the
> +	* camera. Usually the embedder does not ask the user to resolve
> +	* the request but uses what the user answered previously, denying
> +	* by default if the user did not give permissions specifically to
> +	* this origin.

OK, I see what the motivation for adding this was, but I think it's a
misunderstanding of how WebKitPermissionRequest works. The browser doesn't
*have* to display a permission request to the user. E.g. for Epiphany we always
check whether the user has already made a decision, and just return that if so.
The difference is you want the browser to *never* display the permission
request. That's simple enough, since browsers need to manually implement
support for each new type of permission request: you can just document that
your new permission request type is not intended to be displayed to the user.
Right? Then we would not need WebKitWebView::permission-check at all?

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1550
> +	*

Since: 2.22

> Source/WebKit/UIProcess/API/gtk/WebKitPermissionRequest.h:47
> +    void (* resolve_check)  (WebKitPermissionRequest *request,
> +				const gchar* salt,
> +				gboolean allowed);

You can't add new vfuncs without removing padding, and for some reason there's
no padding. So it requires a soname bump (for both GTK and WPE). Wow. If we've
ever had to do this before, it was before I started contributing to WebKit.
We'll need to discuss with Carlos Garcia, and take the opportunity to add a
bunch of new padding to WebKitWebView as well.

But I'm confused whether the salt is really an appropriate parameter to have
here. Like I said above, I think it really belongs in
WebKitUserMediaPermissionRequest, so we can probably avoid this.

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:235
> +    gboolean   (* permission_check)		  (WebKitWebView	      
*web_view,
> +						   WebKitPermissionRequest    
*permission_request);

As was mentioned on Matrix, you'll need to remove padding here too. Same for
the WPE header.