[webkit-reviews] review denied: [Bug 193571] [GTK][WPE] Add API to add paths to sandbox : [Attachment 359484] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has denied Patrick Griffis
<pgriffis at igalia.com>'s request for review:
Bug 193571: [GTK][WPE] Add API to add paths to sandbox
https://bugs.webkit.org/show_bug.cgi?id=193571

Attachment 359484: Patch

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




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

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

Good!

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1182
> + * Adds a path to be mounted in the sandbox. @path must exist before any web
process
> + * has been created otherwise it will be silently ignored. It is a fatal
error to

Maybe we should attempt a g_mkdir_with_parents(), so we don't have to worry
about this if the directory is writable? Bad idea?

We could add a flags parameter to control the behavior?

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1189
> +void webkit_web_context_add_path_to_sandbox(WebKitWebContext* context, const
char* path, gboolean read_only)

Add an enum. Join the inquisition against boolean parameters!

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1197
> +    context->priv->processPool->appendSandboxPaths(String(path), read_only);

I think you can just pass path and let it be implicitly be converted toString.

> Source/WebKit/UIProcess/WebProcessPool.h:474
> +    void appendSandboxPaths(const String& path, bool readOnly) {
m_extraSandboxPaths.set(path, readOnly); };

bool isn't OK here either; there should be a different new enum (since this is
below the API layer and can't use the new enum you'll add to the API).

There really is an inquisition, you know. Be the change you want to see in
function signatures!

> Source/WebKit/UIProcess/WebProcessPool.h:733
> +    HashMap<String, bool> m_extraSandboxPaths;

Then you can use the new enum here too. The code really will be more readable.

> Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:56
> +    String extraPaths;
> +    for (const auto& entry : m_processPool->sandboxPaths()) {
> +	   if (!extraPaths.isEmpty())
> +	       extraPaths.append('\0');
> +	   extraPaths.append(entry.key);
> +	   extraPaths.append(entry.value ? ":ro" : ":rw");
> +    }
> +
> +    launchOptions.extraInitializationData.add("extra-sandbox-paths",
extraPaths);

Why are you doing this instead of just adding a second HashMap to struct
LaunchOptions? Would be much simpler, right?