[Webkit-unassigned] [Bug 193571] [GTK][WPE] Add API to add paths to sandbox

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=193571

--- Comment #9 from Patrick Griffis <pgriffis at igalia.com> ---
(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 359484 [details]
> 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?

Well its blocking IO...

> > 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.

Alright, enums.

> > 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?

Because Youenn was very strongly against adding things to LaunchOptions last review since an extra data field already existed... I just avoided touching it from the start but I agree just adding it there would be much cleaner.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190121/e5e24d7e/attachment.html>