Unveil discussion: TO-DO list

Hi Rodrigo, 

Here are my initial thoughts on your review. Some of the stuff I will
need help with, the rest I can probably manage on my own:  

On Sun, 28 Jul 2024 22:45:38 +0200
Rodrigo Arias <[email protected]> wrote:

> I recommend you add a configure switch (in configure.ac) to enable or 
> disable unveil(). By default you can keep it disabled and let the
> user enable it manually, until we have more feedback to make it
> enabled by default. Maybe by defining ENABLE_UNVEIL? You can take the
> --enable-svg and ENABLE_SVG as an example.

Ok, I think I can handle that. 

> You should also make a dillorc configure option to enable/disable the 
> unveil feature in runtime, so it can help debug problems. You can
> make the dillorc option enabled by default, which will only take
> action when unveil support has been compiled in.

For dillo.cc that shouldn't be too hard. I would probably have more
difficulties with with the plugins though.

> The download directory is set in the dillorc configuration file, and
> can be any other place. Is it viable to read the configuration first
> and then unveil the appropriate directory?

Got that partially working, but ran in to some challenges, see my
earlier message.

> You'll want to use the $(sysconfdir) autoconf variable:

I can take a look, I didn't really consider portability at all since
unveil is exclusive to OpenBSD, and will probably remain that way for
some time. 

> As this won't be a simple patch, I suggest you open a PR in GitHub,
> so the CI can compile your patch revisions for multiple platforms and
> pass the tests. Otherwise I would have to spend the time to do it
> myself.

If this patch gets to a point where your concerns here are addressed, I
will consider it. 

> The err() function is non-portable, please use Dillo MSG* macros or 
> perror() + exit(). This should be caught by the CI.
> 
> Also, ensure the indentation is kept at 3 characters (not my
> decision).

Ok, that shouldn't be a problem. 

> You should find wget by locating it in the $PATH, not assuming it
> would be here. Users may place their own wget binary somewhere else
> and this would break the downloads.

Will look at it. This brings up a completely separate question: why is
wget hardcoded and not changeable as the downloader, and also has a
hardcoded useragent which can't be changed by the user. To me, that's
not ideal, and maybe you have some thoughts on it. 

> The .Xauthority file should be read from $AUTHORITY and then from
> there if not set.
 
I will try :)

> Not sure if we want to constraint file:// like this. What if we are 
> using Dillo to read local HTML files in ~/?

Would you prefer to just not do any unveil in file.c then? It's
probably not a huge risk.

> Plugins can also be found on the dpi_dir directory defined by the
> user in the .dillo/dpidrc, so we would need to parse it first.

I will look at it, but this could be more difficult for me. 

> I would also protect dpidrc from writing as well as dillorc.

Agreed. No problem. 

Regards,
Alex
_______________________________________________
Dillo-dev mailing list -- dillo-dev-lx9mn2B4QYRWk0Htik3J/[email protected]
To unsubscribe send an email to dillo-dev-leave-lx9mn2B4QYRWk0Htik3J/[email protected]