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 <rodarima@gmail.com> 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