<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - [GTK][WPE] Add API to add paths to sandbox"
   href="https://bugs.webkit.org/show_bug.cgi?id=193571#c9">Comment # 9</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - [GTK][WPE] Add API to add paths to sandbox"
   href="https://bugs.webkit.org/show_bug.cgi?id=193571">bug 193571</a>
              from <span class="vcard"><a class="email" href="mailto:pgriffis@igalia.com" title="Patrick Griffis <pgriffis@igalia.com>"> <span class="fn">Patrick Griffis</span></a>
</span></b>
        <pre>(In reply to Michael Catanzaro from <a href="show_bug.cgi?id=193571#c8">comment #8</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=359484&action=diff" name="attach_359484" title="Patch">attachment 359484</a> <a href="attachment.cgi?id=359484&action=edit" title="Patch">[details]</a></span>
> Patch

> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=359484&action=review">https://bugs.webkit.org/attachment.cgi?id=359484&action=review</a>

> 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?</span >
>
<span class="quote">> We could add a flags parameter to control the behavior?</span >

Well its blocking IO...

<span class="quote">> > 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.</span >

Alright, enums.

<span class="quote">> > 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?</span >

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.</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>