Re: [PATCH] ob-core: add org-confirm-babel-evaluate-cell custom variable

[Tom Gillespie <tgbugs@gmail.com>]

Hi Bastien,
   In short, we need two variables due to the case where
a user wants to set nil for all vars and a function for code.
More replies in line. Best!
Tom

> I see nothing in etc/ORG-NEWS that describes this change: am I missing
> something?

Looks like any mention of the change is missing to me as well.

> >> Whether it changed or not, what is the problem in 9.6?
> >
> > The problem is that Org now displays more queries.
> >
> >> How does the patch solves this problem?
> >
> > It allows disabling these new queries about lisp evaluation outside
> > code blocks without disabling code block eval confirmation completely.

> Is there a real use-case for this?

Yes. To give one example. I have many files that need to run hundreds
of cells at tangling/export time. I have reviewed all the code in the cells
and know patterns that are safe and wont' burn me. I do not want to
allow execution of code blocks blindly during tangling and confirm eval
is a secondary line of defense against running those blocks during
export. I want to allow the cells to run uninhibited as they did before
this change, so that I don't have to fight with org during the tangling
process.

As it is right now the change has completely broken various CI
pipelines for me, and if these changes do not get in to emacs 29
in time I will be unable to use emacs 29 for CI until they are in the
base image without having to resort to insane hacks and massive
additional configuration changes to work around the issues discussed
in the loading multiple org versions thread.

> It looks like the patch fixes the "too many queries" problem by
> providing a setup that is similar to what was the previous default
> (i.e. only asking about code block evaluation when
> org-confirm-babel-evaluate-cell is set to nil.)

This interpretation is not quite right. org-confirm-babel-evaluate-cell has
no interaction with evaluating code blocks, only with evaluating cells.
When org-confirm-babel-evaluate-cell is nil or evaluates to nil then the
org-babel-confirm-evaluate code runs on a fake block that is created
out of the cell. (This is where there is still a bug that I mentioned up
thread.)

> But are we sure that users need to confirm header args evaluation
> separately from code blocks evaluation?

I am sure that in my workflow I want them split, especially for
global nil/t behavior. I need to think a bit more about your
suggestion to add more values to org-confirm-babel-evaluate
to see whether it might work and what the relative complexity
would be.

It seems to me that duplicating the variable is certainly easier
to maintain, if not to explain to those who do not use babel
regularly. I would be interested to hear from other babel users
their thoughts on the design, because it seems obvious to me,
but again I wrote the code so am the last person whose view
on the clarity of the should be considered. It seems that it
was not clear to Max initially.

> IOW I have difficulties understanding when these would be relevant:

> (setq org-confirm-babel-evaluate-cell nil)

Anywhere that a parenthesized elisp expression occurs
that is not in #+begin_src block.

> (setq org-confirm-babel-evaluate t)

Only inside #+begin_src blocks.

> I think that's the best route: providing, optionnally, more, while not
> annoying users who don't want more.

I think this is probably the right default as well.

> > Yes. Ideally, we need to improve the code evaluation query. It should
> > allow confirming evaluation in bulk and add some code blocks/files to
> > whitelist. Similar to `org--confirm-resource-safe'.
>
> I see, indeed.

Improving general code auditing prior to evaluation of blocks would
be greatly appreciated. The fighting between the minibuffer and
the primary buffer for priority and the fact that you often cannot
see that code that will be run is a significant issue. However, this is
orthogonal to the issue at hand.

> > A concern have been expressed that more queries may annoy users and
> > drive them towards setting `org-confirm-babel-evaluate' to nil globally.
> > Upon doing this, the future more flexible security queries may be not
> > used by such users.
>
> The future more flexible security queries will be made visibile enough
> so that users currently setting `org-confirm-babel-evaluate' to nil
> will *want* to have a look at it.

Yep. We need to get the fundamentals in place to enable those
workflows. I'm too paranoid to simply set that variable to nil, even
when I'm only running code that I wrote, but I suppose that is not
the case for everyone.

> I find "cell" confusing here (as Max said earlier in the thread).  It
> either refers to a table cell or, in Elisp jargon, a "cons cell" (or a
> "function cell").

I also dislike the name cell, but i'm not sure what to call it instead.
I mentioned "parenthized elisp expression" above, since strictly
speaking they aren't closures, they are any valid elisp sexpression.

In the context of org-outside-emacs I imagine a day when those
top level expressions might also be written in some other language
and there would be some way to indicate at a file or section level
or perhaps even single block expression level what language a given
cell should be interpreted as.

In the context of naming this means that "parentized expression"
might not be general enough. Maybe embedded-code or
embedded-expression? org-confirm-babel-evaluate-embedded-expressions?

> What about modifying `org-confirm-babel-evaluate' to allow these values:
>
> - t      : confirm header vars *and* code blocks
> - 'code  : confirm code blocks
> - 'vars  : confirm vars
> - nil    : don't confirm
>
> and set the default value to 'code, while allowing concerned users to
> set it to `t' -- until we have a better system for evaluation query.
>
> WDYT?

In short this is not a viable solution because there is no way
to compose nil for embedded expressions with a function
for blocks. We really do not want to change the function
signature for the function to also have to accept whether
it is a block or an embedded expression. That will break
code for everyone.

The solution I have proposed keeps blocks and embedded
expressions orthogonal precisely to avoid these kinds of
major disruptions.

Better for users to  be confused about how to use a variable
but be able to learn about it or ask about it than for all users
of a feature to have to suffer major breakages to their existing
workflows and have to rewrite existing code to work around
something that supposedly increased security.

Having two separate variables is the only way to retain
backward compatibility and give granular control over
evaluation of code blocks and embedded expressions.