Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Martin Edström]

I've just been struggling with my custom setting for
`org-latex-to-html-convert-command` outputting many math snippets
wrong. The fault was mine: I didn't correctly shell-quote the input.
I propose to add a warning in the docstring, because many people will
trip the same problem.

The thing is that double-quotes don't work in shell commands.  I had
\"%i\", but it should've been '%i':

(setopt org-latex-to-html-convert-command "node
/home/kept/pub/texToMathML.js \"%i\"")

Math snippets that start with $, like $y = 200$ of course get
butchered into just " = 200$" because of course the first $y gets
interpreted as a shell environment variable.

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

Martin Edström <meedstrom91@gmail.com> writes:

> I've just been struggling with my custom setting for
> `org-latex-to-html-convert-command` outputting many math snippets
> wrong. The fault was mine: I didn't correctly shell-quote the input.
> I propose to add a warning in the docstring, because many people will
> trip the same problem.

> The thing is that double-quotes don't work in shell commands.  I had
> \"%i\", but it should've been '%i':

Would you be interested to submit a patch?
See https://orgmode.org/worg/org-contribute.html#first-patch

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Martin Edström]

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]

I will try to do a patch, thanks for the link. Stay tuned.

On Sun, Feb 18, 2024 at 15:06 Ihor Radchenko <yantar92@posteo.net> wrote:

> Martin Edström <meedstrom91@gmail.com> writes:
>
> > I've just been struggling with my custom setting for
> > `org-latex-to-html-convert-command` outputting many math snippets
> > wrong. The fault was mine: I didn't correctly shell-quote the input.
> > I propose to add a warning in the docstring, because many people will
> > trip the same problem.
>
> > The thing is that double-quotes don't work in shell commands.  I had
> > \"%i\", but it should've been '%i':
>
> Would you be interested to submit a patch?
> See https://orgmode.org/worg/org-contribute.html#first-patch
>
> --
> Ihor Radchenko // yantar92,
> Org mode contributor,
> Learn more about Org mode at <https://orgmode.org/>.
> Support Org development at <https://liberapay.com/org-mode>,
> or support my work at <https://liberapay.com/yantar92>
>

[-- Attachment #2: Type: text/html, Size: 1816 bytes --]

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Martin Edström]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

Here you go!

Tests passed (14 SKIPPED), compiled fine.  I've made no prior
contributions and this changes 5 lines. I'm ok if you want to rephrase
it in any way.

Martin

On Sun, 18 Feb 2024 at 19:56, Martin Edström <meedstrom91@gmail.com> wrote:
>
> I will try to do a patch, thanks for the link. Stay tuned.
>
> On Sun, Feb 18, 2024 at 15:06 Ihor Radchenko <yantar92@posteo.net> wrote:
>>
>> Martin Edström <meedstrom91@gmail.com> writes:
>>
>> > I've just been struggling with my custom setting for
>> > `org-latex-to-html-convert-command` outputting many math snippets
>> > wrong. The fault was mine: I didn't correctly shell-quote the input.
>> > I propose to add a warning in the docstring, because many people will
>> > trip the same problem.
>>
>> > The thing is that double-quotes don't work in shell commands.  I had
>> > \"%i\", but it should've been '%i':
>>
>> Would you be interested to submit a patch?
>> See https://orgmode.org/worg/org-contribute.html#first-patch
>>
>> --
>> Ihor Radchenko // yantar92,
>> Org mode contributor,
>> Learn more about Org mode at <https://orgmode.org/>.
>> Support Org development at <https://liberapay.com/org-mode>,
>> or support my work at <https://liberapay.com/yantar92>

[-- Attachment #2: 0001-lisp-org.el-Enhance-a-docstring.patch --]
[-- Type: text/x-patch, Size: 1332 bytes --]

From d3b1b0a3cc4deac7ac47f446fb0bf27f61169ac4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Edstr=C3=B6m?= <meedstrom91@gmail.com>
Date: Sun, 18 Feb 2024 20:29:48 +0100
Subject: [PATCH] lisp/org.el: Enhance a docstring

* org.el (org-latex-to-html-convert-command): Add a note in the
docstring about proper shell-quoting.

It can trip you up because wrongly quoted input still works with some
math snippets, so the command may work during testing but not later
when you have different math snippets in play.

TINYCHANGE
---
 lisp/org.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index 947037559..6b2ebf9ac 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -3275,7 +3275,11 @@ Replace format-specifiers in the command as noted below and use
 %i:     The LaTeX fragment to be converted.
 
 For example, this could be used with LaTeXML as
-\"latexmlc \\='literal:%i\\=' --profile=math --preload=siunitx.sty 2>/dev/null\"."
+\"latexmlc \\='literal:%i\\=' --profile=math --preload=siunitx.sty 2>/dev/null\".
+
+Since this is a shell-command, remember to use single-quotes
+around \\='%i\\=', not double-quotes!  Else a math fragment such
+as \"$y = 200$\" gets butchered into only \" = 200\"."
   :group 'org-latex
   :package-version '(Org . "9.4")
   :type '(choice
-- 
2.40.1

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

Martin Edström <meedstrom91@gmail.com> writes:

> Tests passed (14 SKIPPED), compiled fine.  I've made no prior
> contributions and this changes 5 lines. I'm ok if you want to rephrase
> it in any way.

Applied onto main, with amendments.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=a8443f2c7
Thanks for your contribution!

You are now listed as one of the Org mode contributors:
https://git.sr.ht/~bzg/worg/commit/53d1e6e3

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Max Nikulin]

On 19/02/2024 02:36, Martin Edström wrote:
> +Since this is a shell-command, remember to use single-quotes
> +around \\='%i\\=', not double-quotes!  Else a math fragment such
> +as \"$y = 200$\" gets butchered into only \" = 200\"."

I am afraid, the code, not the docstring must be fixed. I have not tried 
it, but I expect an issue with

     Test \(f' = df/dx\)

So `shell-quote-argument' is necessary and quotes around %i must be 
stripped similar to %s in mailcap entries in `org-open-file'.

Notice that dollar-math $x = y$ is not recommended and considered as 
obsolete syntax. Use \(x = y\) or \[x = y\] (the latter for display math).

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Martin Edström]

[-- Attachment #1: Type: text/plain, Size: 1619 bytes --]

Ah yes. My custom Node script (which I hope it's OK to paste below) strips
the beginnings and ends of the input. I suppose that it could be done
already on the Elisp side, which would take care of interpreting the
$-signs as envvars too.

const katex = require('katex')
let input = process.argv[2].trim()
let disp = true
if (input.slice(0, 2) === '\\[' || input.slice(0, 2) === '$$') {
  input = input.slice(2, -2)
}
else if (input.slice(0, 2) === '\\(') {
  input = input.slice(2, -2)
  disp = false
}
else if (input.slice(0, 1) === '$') {
  input = input.slice(1, -1)
  disp = false
}
else {
  console.error("Did you quote the input correctly?")
  process.exit(1)
}
console.log(katex.renderToString(
  input, {
    displayMode: disp,
    output: 'mathml',
    trust: true,
    strict: false,
    throwOnError: false,
  }
))

On Wed, Feb 21, 2024 at 15:38 Max Nikulin <manikulin@gmail.com> wrote:

> On 19/02/2024 02:36, Martin Edström wrote:
> > +Since this is a shell-command, remember to use single-quotes
> > +around \\='%i\\=', not double-quotes!  Else a math fragment such
> > +as \"$y = 200$\" gets butchered into only \" = 200\"."
>
> I am afraid, the code, not the docstring must be fixed. I have not tried
> it, but I expect an issue with
>
>      Test \(f' = df/dx\)
>
> So `shell-quote-argument' is necessary and quotes around %i must be
> stripped similar to %s in mailcap entries in `org-open-file'.
>
> Notice that dollar-math $x = y$ is not recommended and considered as
> obsolete syntax. Use \(x = y\) or \[x = y\] (the latter for display math).
>

[-- Attachment #2: Type: text/html, Size: 2213 bytes --]

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Martin Edström]

Actually, I agree about your test case, that looks like it'd cause a problem.

So we patch the function to use `shell-quote-argument'?

On Wed, 21 Feb 2024 at 15:38, Max Nikulin <manikulin@gmail.com> wrote:
>
> On 19/02/2024 02:36, Martin Edström wrote:
> > +Since this is a shell-command, remember to use single-quotes
> > +around \\='%i\\=', not double-quotes!  Else a math fragment such
> > +as \"$y = 200$\" gets butchered into only \" = 200\"."
>
> I am afraid, the code, not the docstring must be fixed. I have not tried
> it, but I expect an issue with
>
>      Test \(f' = df/dx\)
>
> So `shell-quote-argument' is necessary and quotes around %i must be
> stripped similar to %s in mailcap entries in `org-open-file'.
>
> Notice that dollar-math $x = y$ is not recommended and considered as
> obsolete syntax. Use \(x = y\) or \[x = y\] (the latter for display math).

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Martin Edström]

And ignore my suggestion about stripping input on the Elisp side.
Didn't think that through.

On Wed, 21 Feb 2024 at 16:04, Martin Edström <meedstrom91@gmail.com> wrote:
>
> Actually, I agree about your test case, that looks like it'd cause a problem.
>
> So we patch the function to use `shell-quote-argument'?
>
> On Wed, 21 Feb 2024 at 15:38, Max Nikulin <manikulin@gmail.com> wrote:
> >
> > On 19/02/2024 02:36, Martin Edström wrote:
> > > +Since this is a shell-command, remember to use single-quotes
> > > +around \\='%i\\=', not double-quotes!  Else a math fragment such
> > > +as \"$y = 200$\" gets butchered into only \" = 200\"."
> >
> > I am afraid, the code, not the docstring must be fixed. I have not tried
> > it, but I expect an issue with
> >
> >      Test \(f' = df/dx\)
> >
> > So `shell-quote-argument' is necessary and quotes around %i must be
> > stripped similar to %s in mailcap entries in `org-open-file'.
> >
> > Notice that dollar-math $x = y$ is not recommended and considered as
> > obsolete syntax. Use \(x = y\) or \[x = y\] (the latter for display math).

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> On 19/02/2024 02:36, Martin Edström wrote:
>> +Since this is a shell-command, remember to use single-quotes
>> +around \\='%i\\=', not double-quotes!  Else a math fragment such
>> +as \"$y = 200$\" gets butchered into only \" = 200\"."
>
> I am afraid, the code, not the docstring must be fixed. I have not tried 
> it, but I expect an issue with
>
>      Test \(f' = df/dx\)
>
> So `shell-quote-argument' is necessary and quotes around %i must be 
> stripped similar to %s in mailcap entries in `org-open-file'.

That would be backwards-incompatible.
What about introducing %e replacement that will be shell-escaped?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Max Nikulin]

On 23/02/2024 19:46, Ihor Radchenko wrote:
> Max Nikulin <manikulin@gmail.com> writes:
> 
>> On 19/02/2024 02:36, Martin Edström wrote:
>>> +Since this is a shell-command, remember to use single-quotes
>>> +around \\='%i\\=', not double-quotes!  Else a math fragment such
>>> +as \"$y = 200$\" gets butchered into only \" = 200\"."
>>
>> I am afraid, the code, not the docstring must be fixed. I have not tried
>> it, but I expect an issue with
>>
>>       Test \(f' = df/dx\)
>>
>> So `shell-quote-argument' is necessary and quotes around %i must be
>> stripped similar to %s in mailcap entries in `org-open-file'.
> 
> That would be backwards-incompatible.
> What about introducing %e replacement that will be shell-escaped?

Ihor, it is just a bug and its manifestation depends on content of .org 
files more than on user configuration. Do you really want to allow part 
of equations be executed as shell commands for the sake of miracle 
backward compatibility?

To minimize user annoyance my suggestion is to strip quotes in words like
- '%i'
- "%i"
- 'something%i'
- "something%i"
- something='%i'
- something="%i"
before calling `format-spec' with `shell-quote-argument' result.

Please, revert the commit that added a misleading recommendation.

By the way, single quotes have no special meaning in cmd.exe on windows.

Example of silent error resulting in incorrect equation:

(let ((org-latex-to-html-convert-command
        "printf '%%s' '%i'"))
   (org-format-latex-as-html "$f'' = df/dx$"))
"$f = df/dx$"

Random parts of math becomes part of shell command:

(let ((org-latex-to-html-convert-command
        "printf '%%s' '%i'"))
   (org-format-latex-as-html "$f' = df/dx$"))
"/bin/bash: -c: line 1: unexpected EOF while looking for matching `''
"

Something weird may be executed in the case of sufficiently complex 
equations.

It should be more reliable to pass fragment to command stdin. It can be 
done if %i is missed in `org-latex-to-html-convert-command'.

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]

Max Nikulin <manikulin@gmail.com> writes:

> (let ((org-latex-to-html-convert-command
>         "printf '%%s' '%i'"))
>    (org-format-latex-as-html "$f' = df/dx$"))
> "/bin/bash: -c: line 1: unexpected EOF while looking for matching `''
> "
>
> Something weird may be executed in the case of sufficiently complex 
> equations.

> It should be more reliable to pass fragment to command stdin. It can be 
> done if %i is missed in `org-latex-to-html-convert-command'.

I agree that it will be more reliable to shell-escape argument.
However, I am concerned that escaping may break certain uses like

somecommand << EOF
%i
EOF

In the above scenario, escaping will break things.

That's why I prefer to add a new replacement, not change the meaning of
%i. We might even remove %i from the docstring, keeping support in the
code for backwards-compatibility.

Also, I just looked closer into the example with latexml we provide in
the docstring and played around with it.

I noticed that

with

(defun org-format-latex-as-html (latex-fragment)
  "Convert LATEX-FRAGMENT to HTML.
This uses  `org-latex-to-html-convert-command', which see."
  (let ((cmd (format-spec org-latex-to-html-convert-command
			  `((?i . ,latex-fragment)
                            (?I . ,(shell-quote-argument latex-fragment))))))
    (message "Running %s" cmd)
    (shell-command-to-string cmd)))

(with-temp-file "/tmp/test2.html"
(let ((org-latex-to-html-convert-command
	      "latexmlc literal:%I --profile=math --preload=siunitx.sty 2>/dev/null"))
	 (insert (org-format-latex-as-html "$f' = df/dx$"))))

test2.html is rendered *incorrectly* as in the attached screenshot.

In contrast, manually providing output file as

latexmlc literal:\$f\'\ =\ df/dx\$ --profile=math --preload=siunitx.sty --output /tmp/test3.html

yields correct rendering.


[-- Attachment #2: incorrect-rendering.png --]
[-- Type: image/png, Size: 11391 bytes --]

[-- Attachment #3: correct-rendering.png --]
[-- Type: image/png, Size: 6596 bytes --]

[-- Attachment #4: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Max Nikulin]

On 26/02/2024 17:48, Ihor Radchenko wrote:
> Max Nikulin writes:
>> Something weird may be executed in the case of sufficiently complex
>> equations.
> 
>> It should be more reliable to pass fragment to command stdin. It can be
>> done if %i is missed in `org-latex-to-html-convert-command'.
> 
> I agree that it will be more reliable to shell-escape argument.
> However, I am concerned that escaping may break certain uses like
> 
> somecommand << EOF
> %i
> EOF
> 
> In the above scenario, escaping will break things.

It is unsafe to use such command. Variable expansion, etc. is performed 
inside here document blocks. Try

cat << EOF
\[f(i), \text{where $i \ne 10$}\]
EOF

That is why I proposed to use stdin in the case of missed %i.

`org-latex-to-html-convert-command' should be set to something like
"latexmlc --profile=math --preload=siunitx.sty - 2>/dev/null"
this case.

> That's why I prefer to add a new replacement, not change the meaning of
> %i. We might even remove %i from the docstring, keeping support in the
> code for backwards-compatibility.

What you calls backward compatibility is actually a means to get strange 
results in the case of complex math. It is better to force users to 
update configuration (I hope, it actually will not be necessary) and to 
ensure safe command without pitfalls related to missed parts of equations.

> (with-temp-file "/tmp/test2.html"
> (let ((org-latex-to-html-convert-command
> 	      "latexmlc literal:%I --profile=math --preload=siunitx.sty 2>/dev/null"))
> 	 (insert (org-format-latex-as-html "$f' = df/dx$"))))
> 
> test2.html is rendered *incorrectly* as in the attached screenshot.

Looks like missed <meta charset="UTF-8"> inside <head>

> In contrast, manually providing output file as
> 
> latexmlc literal:\$f\'\ =\ df/dx\$ --profile=math --preload=siunitx.sty --output /tmp/test3.html
> 
> yields correct rendering.

Perhaps this time the browser just guessed file encoding. Anyway 
rendering is incorrect. Gecko puts derivative into the correct place. I 
have no idea if it is a fault of latexml generating incorrect MathML or 
a browser which is likely a KHTML descendant.

It seems, latexml is terribly broken in Debian. With 
--preload=siunitx.sty it hangs during processing of expl3-code.tex, 
without this option it removes all files in /tmp.

I am still strongly against code that may cause execution of equations 
as shell commands and may silently lose parts of equations.

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Max Nikulin]

On 25/02/2024 17:41, Max Nikulin wrote:
>> Max Nikulin writes:

>>> So `shell-quote-argument' is necessary and quotes around %i must be
>>> stripped similar to %s in mailcap entries in `org-open-file'.
...
> Please, revert the commit that added a misleading recommendation.
...
> It should be more reliable to pass fragment to command stdin. It can be 
> done if %i is missed in `org-latex-to-html-convert-command'.

I have realized that there is `org-latex-to-mathml-convert-command' 
introduced a decade earlier and affected by the same issue with possible 
leak of formula to shell command. Even if there are reasons against 
obsoleting `org-latex-to-html-convert-command' in favor of 
`org-latex-to-mathml-convert-command', both user options should be 
handled by the same function.

I am unsure if it is an intended feature that when an org file is opened 
from a remote location like /ssh:... then 
`org-latex-to-html-convert-command' is executed on the remote host. It 
makes implementation of stdin more tricky. Ideally, it should be 
configurable where the command is executed: where emacs is running, 
where the document resides, or even with specific `default-directory'.

Double quotes are recommended around %i for ODT export
(info "(org) LaTeX math snippets")
https://orgmode.org/manual/LaTeX-math-snippets.html
and it should be fixed as well.

It seems --preload=siunitx.sty should be recommended any more for latexml:
https://github.com/brucemiller/LaTeXML/issues/2268
Problem width loading expl3-code.tex

Perhaps at least some cases may be handled by pandoc
https://list.orgmode.org/CAEPTPEzvx5ZhY5qrCJnFtAC_NpPC9d1a-Q=yE+XNTrPXiMpTag@mail.gmail.com/
David Lukeš. Using pandoc to convert LaTeX math to MathML
Tue, 1 Mar 2022 15:59:36 +0100

Unfortunately I am not familiar with MathML enough to evaluate that 
there are no caveats with pandoc.

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

Max Nikulin <manikulin@gmail.com> writes:

>>> It should be more reliable to pass fragment to command stdin. It can be
>>> done if %i is missed in `org-latex-to-html-convert-command'.
>> 
>> I agree that it will be more reliable to shell-escape argument.
>> However, I am concerned that escaping may break certain uses like
>> 
>> somecommand << EOF
>> %i
>> EOF
>> 
>> In the above scenario, escaping will break things.
>
> It is unsafe to use such command. Variable expansion, etc. is performed 
> inside here document blocks. Try
>
> cat << EOF
> \[f(i), \text{where $i \ne 10$}\]
> EOF

I did non know this. Thanks for the info.

> That is why I proposed to use stdin in the case of missed %i.
>
> `org-latex-to-html-convert-command' should be set to something like
> "latexmlc --profile=math --preload=siunitx.sty - 2>/dev/null"
> this case.

I decided not to introduce stdin. User can always use echo %i | ... instead.

>> That's why I prefer to add a new replacement, not change the meaning of
>> %i. We might even remove %i from the docstring, keeping support in the
>> code for backwards-compatibility.
>
> What you calls backward compatibility is actually a means to get strange 
> results in the case of complex math. It is better to force users to 
> update configuration (I hope, it actually will not be necessary) and to 
> ensure safe command without pitfalls related to missed parts of equations.

Agree.
This breaking change cannot be avoided, unfortunately.
Even stripping quotes is unreliable when we use the example from
docstring: 'literal:%i'. So, we have to bite the bullet.

>> test2.html is rendered *incorrectly* as in the attached screenshot.
>
> Looks like missed <meta charset="UTF-8"> inside <head>
> ...

Exporting Org document using

(setq org-html-with-latex 'html)
(setq org-latex-to-html-convert-command "latexmlc 'literal:%i' --profile=math --preload=siunitx.sty 2>/dev/null")

renders just fine, so these caveats appear to be terminal-specific. Not
our problem.

Attaching tentative patch that fixes the problem.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-latex-to-mathml-html-convert-command-Prevent-she.patch --]
[-- Type: text/x-patch, Size: 4506 bytes --]

From 34e5e14260cf895b32f13ed8f4c2e50684f91baf Mon Sep 17 00:00:00 2001
Message-ID: <34e5e14260cf895b32f13ed8f4c2e50684f91baf.1709896570.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Fri, 8 Mar 2024 14:05:12 +0300
Subject: [PATCH] org-latex-to-mathml/html-convert-command: Prevent shell
 expansion

* lisp/org.el (org-create-math-formula):
(org-format-latex-as-html): Shell-quote LaTeX fragment text when
replacing %i placeholder.  This prevents shell expansion of
$... and similar constructs inside the code.
(org-latex-to-mathml-convert-command):
(org-latex-to-html-convert-command): Update the docstring.
* etc/ORG-NEWS (~org-latex-to-mathml-convert-command~ and
~org-latex-to-html-convert-command~ shell-escape LaTeX code): Announce
the breaking change.

Reported-by: Max Nikulin <manikulin@gmail.com>
Link: https://orgmode.org/list/735645dd-1ddf-4579-a6dd-2700f3e83c94@gmail.com
---
 etc/ORG-NEWS | 10 ++++++++++
 lisp/org.el  | 17 ++++++-----------
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index abe62daaf..9f628bc10 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -13,6 +13,16 @@ Please send Org bug reports to mailto:emacs-orgmode@gnu.org.
 
 * Version 9.7 (not released yet)
 ** Important announcements and breaking changes
+*** ~org-latex-to-mathml-convert-command~ and ~org-latex-to-html-convert-command~ shell-escape LaTeX code
+
+Previously, ~org-latex-to-mathml-convert-command~ and
+~org-latex-to-html-convert-command~ replaced %i placeholders with raw
+LaTeX fragment text, potentially triggered shell-expansion.
+
+Now, the %i placeholders are shell-escaped to prevent shell expansion - this will prevent.
+
+The existing customizations that assume no shell-escaping must be updated.
+
 *** When ~org-link-file-path-type~ is a function, its argument is now a filename as it is read by ~org-insert-link~; not an absolute path
 
 Previously, when ~org-link-file-path-type~ is set to a function, the
diff --git a/lisp/org.el b/lisp/org.el
index 33d90506b..a00d50c51 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -3246,7 +3246,7 @@ (defcustom org-latex-to-mathml-convert-command nil
 %j:     Executable file in fully expanded form as specified by
         `org-latex-to-mathml-jar-file'.
 %I:     Input LaTeX file in fully expanded form.
-%i:     The latex fragment to be converted.
+%i:     Shell-escaped LaTeX fragment to be converted.
 %o:     Output MathML file.
 
 This command is used by `org-create-math-formula'.
@@ -3255,7 +3255,7 @@ (defcustom org-latex-to-mathml-convert-command nil
 \"java -jar %j -unicode -force -df %o %I\".
 
 When using LaTeXML set this option to
-\"latexmlmath \"%i\" --presentationmathml=%o\"."
+\"latexmlmath %i --presentationmathml=%o\"."
   :group 'org-latex
   :version "24.1"
   :type '(choice
@@ -3268,15 +3268,10 @@ (defcustom org-latex-to-html-convert-command nil
 directly replace the LaTeX fragment in the resulting HTML.
 Replace format-specifiers in the command as noted below and use
 `shell-command' to convert LaTeX to HTML.
-%i:     The LaTeX fragment to be converted.
+%i:     The LaTeX fragment to be converted (shell-escaped).
 
 For example, this could be used with LaTeXML as
-\"latexmlc \\='literal:%i\\=' --profile=math --preload=siunitx.sty 2>/dev/null\".
-
-The LaTeX fragment is replaced as is, without escaping special shell
-syntax.  It may be necessary to use single-quotes around \\='%i\\=', not
-double-quotes.  Else a math fragment such as \"$y = 200$\" may be
-expanded to \" = 200\"."
+\"latexmlc literal:%i --profile=math --preload=siunitx.sty 2>/dev/null\"."
   :group 'org-latex
   :package-version '(Org . "9.4")
   :type '(choice
@@ -16210,7 +16205,7 @@ (defun org-create-math-formula (latex-frag &optional mathml-file)
 			      (expand-file-name
 			       org-latex-to-mathml-jar-file))))
 		 (?I . ,(shell-quote-argument tmp-in-file))
-		 (?i . ,latex-frag)
+		 (?i . ,(shell-quote-argument latex-frag))
 		 (?o . ,(shell-quote-argument tmp-out-file)))))
 	 mathml shell-command-output)
     (when (called-interactively-p 'any)
@@ -16277,7 +16272,7 @@ (defun org-format-latex-as-html (latex-fragment)
   "Convert LATEX-FRAGMENT to HTML.
 This uses  `org-latex-to-html-convert-command', which see."
   (let ((cmd (format-spec org-latex-to-html-convert-command
-			  `((?i . ,latex-fragment)))))
+			  `((?i . ,(shell-quote-argument latex-fragment))))))
     (message "Running %s" cmd)
     (shell-command-to-string cmd)))
 
-- 
2.43.0


[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Max Nikulin]

On 08/03/2024 18:16, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
> I decided not to introduce stdin. User can always use echo %i | ... instead.

printf "%%s" %i

should be safer. However in this particular case, input that may be 
recognized like echo options ("-n") should be wrapped with LaTeX delimiters.

> Even stripping quotes is unreliable when we use the example from
> docstring: 'literal:%i'.

My idea is to recognize this case. If stripping is not performed then it 
is necessary to detect if user command is safe. Otherwise apostrophe in 
a formula (even after escaping) may cause leaking math to shell. I have 
not figured out if it is possible to bypass double quotes, but extra 
slashes may distort math expression.

It is trivial to cause shell failure when single quotes are used around 
%i. I am in doubts concerning double quotes. Perhaps stripping them is 
more reliable.

> Attaching tentative patch that fixes the problem.

I think it is in the right direction.
- Manual needs update as well.
- I would explicitly stress that quotes causes undefined or even 
dangerous behavior. See e.g. the last paragraph
https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s07.html
- I expected it as bugfix.

I have tried to add some unit tests, but I faced an issue with 
`org-create-math-formula'. It creates temporary files in 
`default-directory' and does not remove them on failure. Moreover, it 
does not work in a container where git is not installed:

Debugger entered--Lisp error: (file-missing "Searching for program" "No 
such file or directory" "git")

that is called from `find-file-hook'.

(ert-deftest test-org/create-math-formula ()
   "Test shell special characters escaping in `org-create-math-formula'."
   (let ((org-latex-to-mathml-convert-command
          "printf \"<math 
xmlns=\\\"http://www.w3.org/1998/Math/MathML\\\"><I%%sI></math>\" %i >%o"))
     ;; No backslashes added by `shell-quote-argumet'
     ;; are leaked to command arguments. dash(1) "Double Quotes":
     ;;
     ;;     The backslash inside double quotes is historically weird,
     ;;     and serves to quote only the following characters:
     ;;         $ ` " \ <newline>.
     ;;     Otherwise it remains literal.
     (should
      (equal "<I(|)`[[\\]]{}#$'!I>"
              (org-create-math-formula "(|)`[[\\]]{}#$'!")))
     ;; Multiple words
     (should
      (equal "<Iwords ; |I>"
              (org-create-math-formula "words ; |")))
     ;; Bypass single quote
     (should
      (equal "<Iapostrophe' ; |I>"
             (org-create-math-formula "apostrophe' ; |")))
     ;; Bypass double quote
     (should
      (equal "<Iquote\" ; |I>"
             (org-create-math-formula "quote\" ; |")))))

[PATCH] Unit tests for function calling MathML converters (Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command)

[Max Nikulin]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On 09/03/2024 22:23, Max Nikulin wrote:
> On 08/03/2024 18:16, Ihor Radchenko wrote:
> 
>> Attaching tentative patch that fixes the problem.

I propose to add unit tests, see first attachment.

> I have tried to add some unit tests, but I faced an issue with 
> `org-create-math-formula'. It creates temporary files in 
> `default-directory' and does not remove them on failure. Moreover, it 
> does not work in a container where git is not installed:
> 
> Debugger entered--Lisp error: (file-missing "Searching for program" "No 
> such file or directory" "git")
> 
> that is called from `find-file-hook'.

Is second attachment appropriate to fix the issue or it has some 
undesired effects.

[-- Attachment #2: 0002-test-org.el-LaTeX-to-MathML-tests-of-shell-escaping.patch --]
[-- Type: text/x-patch, Size: 4128 bytes --]

From 73541dbeafff47f03d4aa2f47da70ba71d5b8253 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Sun, 10 Mar 2024 11:16:46 +0700
Subject: [PATCH 2/3] test-org.el: LaTeX to MathML tests of shell escaping

* testing/lisp/test-org.el (test-org/format-latex-as-html)
(test-org/create-math-formula): New tests for escaping of shell specials
in commands executed by `org-format-latex-as-html'
and `org-create-math-formula'.

These tests do not require applications for conversion of LaTeX
snippets and use simple shell commands instead.
---
 testing/lisp/test-org.el | 75 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index cecf9b412..652fc0676 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -9590,6 +9590,81 @@ (ert-deftest test-org/org--open-file-format-command ()
                                (string-match-p "\\`Invalid format.*%2" err-text))
                     err)))))
 
+\f
+;;; LaTeX-related functions.
+
+(ert-deftest test-org/format-latex-as-html ()
+  "Test shell special characters escaping in `org-format-latex-as-html'."
+  (let ((org-latex-to-html-convert-command
+         "printf \"<I%%sI>\" %i"))
+    ;; No backslashes added by `shell-quote-argumet'
+    ;; are leaked to command arguments.  See e.g. dash(1) "Double Quotes":
+    ;;
+    ;;     The backslash inside double quotes is historically weird,
+    ;;     and serves to quote only the following characters:
+    ;;         $ ` " \ <newline>.
+    ;;     Otherwise it remains literal.
+    ;;
+    ;; Actually extra backslashes may appear if a user adds
+    ;; double quotes around "%i", however it is not subject
+    ;; of this test.
+    (should
+     (equal "<I(|)`[[\\]]{}#$'!I>"
+            (org-format-latex-as-html "(|)`[[\\]]{}#$'!")))
+    ;; The following tests generate shell errors if %i
+    ;; substitution is not passed throuhg `shell-quote-argument'.
+    ;; Multiple words.
+    (should
+     (equal "<Iwords ; |I>"
+                   (org-format-latex-as-html "words ; |")))
+    ;; Bypass single quote.
+    ;; The same snippet causes shell error if '%i' is wrapped
+    ;; in single quotes in user configuration.
+    (should
+     (equal "<Iapostrophe' ; |I>"
+            (org-format-latex-as-html "apostrophe' ; |")))
+    ;; Bypass double quote.
+    ;; Double quotes around "%i" in user configuration leads
+    ;; to extra backslashes (see above), however likely
+    ;; such user error can not cause execution of the argument
+    ;; as raw shell commands.
+    (should
+     (equal "<Iquote\" ; |I>"
+            (org-format-latex-as-html "quote\" ; |")))))
+
+(defun test-org/extract-mathml-math (xml)
+  "Extract body from result of `org-create-math-formula'."
+  (and (string-match "<math[^>]*>\\(\\(?:.\\|\n\\)*\\)</math>" xml)
+       (match-string 1 xml)))
+
+(ert-deftest test-org/create-math-formula ()
+  "Test shell special characters escaping in `org-create-math-formula'."
+  ;; The function requires <math>...</math> elements.
+  (let ((org-latex-to-mathml-convert-command
+         "printf \"<math xmlns=\\\"http://www.w3.org/1998/Math/MathML\\\"><I%%sI></math>\" %i >%o"))
+    ;; See comments in `test-org/format-latex-as-html'.
+    ;;
+    ;; No backslashes added by `shell-quote-argumet'
+    ;; are leaked to command arguments.
+    (should (equal "<I(|)`[[\\]]{}#$'!I>"
+            (test-org/extract-mathml-math
+             (org-create-math-formula "(|)`[[\\]]{}#$'!"))))
+    ;; Multiple words.
+    (should
+     (equal "<Iwords ; |I>"
+            (test-org/extract-mathml-math
+             (org-create-math-formula "words ; |"))))
+    ;; Bypass single quote.
+    (should
+     (equal "<Iapostrophe' ; |I>"
+            (test-org/extract-mathml-math
+             (org-create-math-formula "apostrophe' ; |"))))
+    ;; Bypass double quote.
+    (should
+     (equal "<Iquote\" ; |I>"
+            (test-org/extract-mathml-math
+             (org-create-math-formula "quote\" ; |"))))))
+
 (provide 'test-org)
 
 ;;; test-org.el ends here
-- 
2.39.2


[-- Attachment #3: 0003-org.el-Avoid-find-file-no-select-during-MathML-gener.patch --]
[-- Type: text/x-patch, Size: 1668 bytes --]

From cb5d20b54349dabea25241072feca4822ae0e77d Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Sun, 10 Mar 2024 11:22:15 +0700
Subject: [PATCH 3/3] org.el: Avoid `find-file-no-select' during MathML
 generation

* lisp/org.el (org-create-math-formula): Bypass `find-file-hook' during
reading MathML files.

At least in Emacs-28 calling `find-file-noselect' for a file in a
directory under git control when git is not installed causes failures:

    Lisp error:
    (file-missing "Searching for program" "No such file or directory" "git")

`org-create-math-formula' uses document `default-directory' for
temporary files for conversion process input and output.
I have faced an issue with Org mode unit tests running in a minimal
container with no git installed.
---
 lisp/org.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index a00d50c51..caf1bfa67 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16215,14 +16215,15 @@ (defun org-create-math-formula (latex-frag &optional mathml-file)
     (setq shell-command-output (shell-command-to-string cmd))
     (setq mathml
 	  (when (file-readable-p tmp-out-file)
-	    (with-current-buffer (find-file-noselect tmp-out-file t)
+	    (with-temp-buffer
+              (insert-file-contents tmp-out-file)
 	      (goto-char (point-min))
 	      (when (re-search-forward
 		     (format "<math[^>]*?%s[^>]*?>\\(.\\|\n\\)*</math>"
 			     (regexp-quote
 			      "xmlns=\"http://www.w3.org/1998/Math/MathML\""))
 		     nil t)
-		(prog1 (match-string 0) (kill-buffer))))))
+		(match-string 0)))))
     (cond
      (mathml
       (setq mathml
-- 
2.39.2

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> Even stripping quotes is unreliable when we use the example from
>> docstring: 'literal:%i'.
>
> My idea is to recognize this case. If stripping is not performed then it 
> is necessary to detect if user command is safe. Otherwise apostrophe in 
> a formula (even after escaping) may cause leaking math to shell. I have 
> not figured out if it is possible to bypass double quotes, but extra 
> slashes may distort math expression.
>
> It is trivial to cause shell failure when single quotes are used around 
> %i. I am in doubts concerning double quotes. Perhaps stripping them is 
> more reliable.

May you list the cases to you propose to recognize?

>> Attaching tentative patch that fixes the problem.
>
> I think it is in the right direction.
> - Manual needs update as well.

Yes,

  #+begin_src emacs-lisp
  (setq org-latex-to-mathml-convert-command
        "latexmlmath \"%i\" --presentationmathml=%o")
  #+end_src

example in "LaTeX math snippets" section should be updated. (note to self)

> - I would explicitly stress that quotes causes undefined or even 
> dangerous behavior. See e.g. the last paragraph
> https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s07.html

In ORG-NEWS?

> - I expected it as bugfix.

It is a breaking change.
Also, only users who customized the variable may be prone to unexpected
shell expansion. So, I do not see it as a critical bug.
Hence, not for bugfix.

> I have tried to add some unit tests, but I faced an issue with 
> `org-create-math-formula'. It creates temporary files in 
> `default-directory' and does not remove them on failure. Moreover, it 
> does not work in a container where git is not installed:
> ...
> Debugger entered--Lisp error: (file-missing "Searching for program" "No 
> such file or directory" "git")
>
> that is called from `find-file-hook'.

with emacs -Q?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Max Nikulin]

On 12/03/2024 20:03, Ihor Radchenko wrote:
> Max Nikulin writes:
>> It is trivial to cause shell failure when single quotes are used around
>> %i. I am in doubts concerning double quotes. Perhaps stripping them is
>> more reliable.
> 
> May you list the cases to you propose to recognize?

Sun, 25 Feb 2024 17:41:43 +0700
https://list.orgmode.org/6e49c590-ad27-4fb0-b1f2-6a89c60a0b58@gmail.com

- '%i' and "%i" in any position including e.g. --option='%i' and 
protocol:"%i"
- 'something%i' and "something%i" surrounded by spaces or at the end of 
command but with no spaces in "something". It should be applied to %%%i, 
but not to %%i.

>> - I would explicitly stress that quotes causes undefined or even
>> dangerous behavior. See e.g. the last paragraph
>> https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s07.html
> 
> In ORG-NEWS?

Since quotes were recommended in the manual and the docscring, it is 
better to say there that they lead to undefined behavior. ORG-NEWS is to 
make the change visible for those who have read docs earlier.

>> - I expected it as bugfix.
> 
> It is a breaking change.
> Also, only users who customized the variable may be prone to unexpected
> shell expansion. So, I do not see it as a critical bug.
> Hence, not for bugfix.

I am still in doubts. I have no idea how much users need ODT export with 
math and rely on the backend shipped with Org. All of them have to 
customize the user option and those who added %i with quotes have risk 
to get incorrect output. If quotes around %i are stripped then the 
change is not breaking one for most of them.

>> Moreover, it
>> does not work in a container where git is not installed:
>> ...
>> Debugger entered--Lisp error: (file-missing "Searching for program" "No
>> such file or directory" "git")
> 
> with emacs -Q?

emacs -Q --batch --eval '(find-file-noselect "not-found.txt" t)'
Starting new Ispell process ispell with default dictionary... \
Error enabling Flyspell mode:
(Searching for program No such file or directory ispell)
Error: (file-missing "Searching for program" "No such file or directory" 
"git")

Emacs-28.2

ispell error is due to my .dir-locals-2.el
  (text-mode . ((mode . flyspell)))
bug to get the git error it is enough to create .git subdirectory.

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> On 12/03/2024 20:03, Ihor Radchenko wrote:
>> Max Nikulin writes:
>>> It is trivial to cause shell failure when single quotes are used around
>>> %i. I am in doubts concerning double quotes. Perhaps stripping them is
>>> more reliable.
>> 
>> May you list the cases to you propose to recognize?
>
> Sun, 25 Feb 2024 17:41:43 +0700
> https://list.orgmode.org/6e49c590-ad27-4fb0-b1f2-6a89c60a0b58@gmail.com
>
> - '%i' and "%i" in any position including e.g. --option='%i' and 
> protocol:"%i"
> - 'something%i' and "something%i" surrounded by spaces or at the end of 
> command but with no spaces in "something".

I am not confident that it will be safe. For example, consider something
awkward like foo\"%ibar\". I imagine that other edge cases are possible,
especially in exotic shells.

> ...  It should be applied to %%%i, 
> but not to %%i.

I am not sure what you mean here.

>>> - I expected it as bugfix.
>> 
>> It is a breaking change.
>> Also, only users who customized the variable may be prone to unexpected
>> shell expansion. So, I do not see it as a critical bug.
>> Hence, not for bugfix.
>
> I am still in doubts. I have no idea how much users need ODT export with 
> math and rely on the backend shipped with Org. All of them have to 
> customize the user option and those who added %i with quotes have risk 
> to get incorrect output. If quotes around %i are stripped then the 
> change is not breaking one for most of them.

It does not matter that most users will not be affected. Some users
being affected is enough to not commit this to bugfix. Our policy is not
to commit unsafe changes that may break existing configurations to
bugfix branch. Except critical fixes. See
https://orgmode.org/worg/org-maintenance.html#release-types

>>> Moreover, it
>>> does not work in a container where git is not installed:
>>> ...
>>> Debugger entered--Lisp error: (file-missing "Searching for program" "No
>>> such file or directory" "git")
>> 
>> with emacs -Q?
>
> emacs -Q --batch --eval '(find-file-noselect "not-found.txt" t)'
> Starting new Ispell process ispell with default dictionary... \
> Error enabling Flyspell mode:
> (Searching for program No such file or directory ispell)
> Error: (file-missing "Searching for program" "No such file or directory" 
> "git")
>
> Emacs-28.2
>
> ispell error is due to my .dir-locals-2.el
>   (text-mode . ((mode . flyspell)))
> bug to get the git error it is enough to create .git subdirectory.

This looks like Emacs bug. Likely in `vc-refresh-state'.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Max Nikulin]

On 15/03/2024 20:49, Ihor Radchenko wrote:
> Max Nikulin writes:
>> On 12/03/2024 20:03, Ihor Radchenko wrote:
>> - '%i' and "%i" in any position including e.g. --option='%i' and
>> protocol:"%i"
>> - 'something%i' and "something%i" surrounded by spaces or at the end of
>> command but with no spaces in "something".
> 
> I am not confident that it will be safe. For example, consider something
> awkward like foo\"%ibar\". I imagine that other edge cases are possible,
> especially in exotic shells.

I think quotes should not be stripped in such peculiar cases. The 
variants I suggested do not match it. Is it realistic?

>> ...  It should be applied to %%%i,
>> but not to %%i.
> 
> I am not sure what you mean here.

"%%" is a way to specify literal "%" in `format-spec'. So '%%i' means in 
%i shell command and unquoting should not be applied to it.

>>>> - I expected it as bugfix.
> 
> It does not matter that most users will not be affected. Some users
> being affected is enough to not commit this to bugfix. Our policy is not
> to commit unsafe changes that may break existing configurations to
> bugfix branch. Except critical fixes.

Reasons why I consider this issue a severe enough:
- Something weird may be executed as shell commands
- Incorrect formulas in exported documents are more than just 
disappointment. An example of complain related to another bug:
Re: Inequalities in math blocks. Wed, 06 Oct 2021 09:39:23 +0200. 
https://list.orgmode.org/m2bl42bo0k.fsf@me.com

 From my point of view, it is better to explain users that they are 
disturbed to be on the safe side. It is not choice between good and bad 
variants. Any decision is bad.

>> emacs -Q --batch --eval '(find-file-noselect "not-found.txt" t)'
>> Error: (file-missing "Searching for program" "No such file or directory"
>> "git")
> 
> This looks like Emacs bug. Likely in `vc-refresh-state'.

It as an Emacs bug that missing git executable leads to a fatal error.

It is a bug in Org that some hooks are called when just file content is 
necessary.

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>>> On 12/03/2024 20:03, Ihor Radchenko wrote:
>>> - '%i' and "%i" in any position including e.g. --option='%i' and
>>> protocol:"%i"
>>> - 'something%i' and "something%i" surrounded by spaces or at the end of
>>> command but with no spaces in "something".
>> 
>> I am not confident that it will be safe. For example, consider something
>> awkward like foo\"%ibar\". I imagine that other edge cases are possible,
>> especially in exotic shells.
>
> I think quotes should not be stripped in such peculiar cases. The 
> variants I suggested do not match it. Is it realistic?

I am afraid that there are other peculiar cases. I do not know how to
determine which case is peculiar and when it is safe to strip the quotes
in the code. I feel that if we do try to strip only "safe" cases, we
will introduce subtle bugs and then introduce even more breaking changes
by fixing those bugs.

It is more robust to not strip the quotes at all and go ahead with
breaking change.

>>>>> - I expected it as bugfix.
>> 
>> It does not matter that most users will not be affected. Some users
>> being affected is enough to not commit this to bugfix. Our policy is not
>> to commit unsafe changes that may break existing configurations to
>> bugfix branch. Except critical fixes.
>
> Reasons why I consider this issue a severe enough:
> - Something weird may be executed as shell commands
> - Incorrect formulas in exported documents are more than just 
> disappointment. An example of complain related to another bug:
> Re: Inequalities in math blocks. Wed, 06 Oct 2021 09:39:23 +0200. 
> https://list.orgmode.org/m2bl42bo0k.fsf@me.com

I do not see these reasons as _critical_. In my mind, critical reasons
would be (1) Org mode completely broken for many users (it is not); (2)
Security vulnerability.

This particular case seems to be subjective, so it is a judgment call.
If you insist that the fix should land on bugfix, we can add Bastien to
the discussion to get a third opinion.

>>> emacs -Q --batch --eval '(find-file-noselect "not-found.txt" t)'
>>> Error: (file-missing "Searching for program" "No such file or directory"
>>> "git")
>> 
>> This looks like Emacs bug. Likely in `vc-refresh-state'.
>
> It as an Emacs bug that missing git executable leads to a fatal error.
>
> It is a bug in Org that some hooks are called when just file content is 
> necessary.

I would not necessarily call it a bug, but I do not see downsides of
using `insert-file-contents' instead of `find-file-noselect' and not
running `find-file-hook' in this particular case (other cases in Org
tree appears to be fine from a quick glance).

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

>> It is a bug in Org that some hooks are called when just file content is 
>> necessary.
>
> I would not necessarily call it a bug, but I do not see downsides of
> using `insert-file-contents' instead of `find-file-noselect' and not
> running `find-file-hook' in this particular case (other cases in Org
> tree appears to be fine from a quick glance).

https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=b338a9069

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Max Nikulin]

On 19/03/2024 21:49, Ihor Radchenko wrote:
>>
>> but I do not see downsides of
>> using `insert-file-contents' instead of `find-file-noselect' and not
>> running `find-file-hook' in this particular case (other cases in Org
>> tree appears to be fine from a quick glance).
> 
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=b338a9069

`with-temp-buffer' makes `kill-buffer' unnecessary.

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=b338a9069
>
> `with-temp-buffer' makes `kill-buffer' unnecessary.

Oops.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4f548f948

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

fixup! and git

[Max Nikulin]

On 19/03/2024 23:27, Ihor Radchenko wrote:
> Oops.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4f548f948
> 
> fixup! org-create-math-formula: Do not run file-related hooks when checking output

"fixup!" is a keyword for "git rebase --interactive --autosquash" and 
usually added for minor correction of commits that have not been pushed 
yet to a shared repository. It is a bit confusing to see it in a public 
repository in a branch that is not a draft.

Re: fixup! and git

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=4f548f948
>> 
>> fixup! org-create-math-formula: Do not run file-related hooks when checking output
>
> "fixup!" is a keyword for "git rebase --interactive --autosquash" and 
> usually added for minor correction of commits that have not been pushed 
> yet to a shared repository. It is a bit confusing to see it in a public 
> repository in a branch that is not a draft.

I personally find it more clear compared for something like "Fix <commit
hash>" or "Fix previous commit".

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

[-- Attachment #1: Type: text/plain, Size: 453 bytes --]

Max Nikulin <manikulin@gmail.com> writes:

>> Attaching tentative patch that fixes the problem.
>
> I think it is in the right direction.
> - Manual needs update as well.
> - I would explicitly stress that quotes causes undefined or even 
> dangerous behavior. See e.g. the last paragraph
> https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s07.html

I have incorporated the above suggestions into the attached version of
the patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-latex-to-mathml-html-convert-command-Prevent-she.patch --]
[-- Type: text/x-patch, Size: 5262 bytes --]

From 5dbe4457d0d938e8830888bc3ac58d6a43136558 Mon Sep 17 00:00:00 2001
Message-ID: <5dbe4457d0d938e8830888bc3ac58d6a43136558.1711873441.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Fri, 8 Mar 2024 14:05:12 +0300
Subject: [PATCH] org-latex-to-mathml/html-convert-command: Prevent shell
 expansion

* lisp/org.el (org-create-math-formula):
(org-format-latex-as-html): Shell-quote LaTeX fragment text when
replacing %i placeholder.  This prevents shell expansion of
$... and similar constructs inside the code.
(org-latex-to-mathml-convert-command):
(org-latex-to-html-convert-command): Update the docstring.
* etc/ORG-NEWS (~org-latex-to-mathml-convert-command~ and
~org-latex-to-html-convert-command~ shell-escape LaTeX code): Announce
the breaking change.
* doc/org-manual.org (LaTeX math snippets): Update example.

Reported-by: Max Nikulin <manikulin@gmail.com>
Link: https://orgmode.org/list/735645dd-1ddf-4579-a6dd-2700f3e83c94@gmail.com
---
 doc/org-manual.org |  2 +-
 etc/ORG-NEWS       | 10 ++++++++++
 lisp/org.el        | 21 ++++++++++-----------
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/doc/org-manual.org b/doc/org-manual.org
index c4f62644f..acc4512a5 100644
--- a/doc/org-manual.org
+++ b/doc/org-manual.org
@@ -15176,7 +15176,7 @@ **** LaTeX math snippets
 
   #+begin_src emacs-lisp
   (setq org-latex-to-mathml-convert-command
-        "latexmlmath \"%i\" --presentationmathml=%o")
+        "latexmlmath %i --presentationmathml=%o")
   #+end_src
 
   To quickly verify the reliability of the LaTeX-to-MathML
diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index ee2cdfd16..739c3a43b 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -13,6 +13,16 @@ Please send Org bug reports to mailto:emacs-orgmode@gnu.org.
 
 * Version 9.7 (not released yet)
 ** Important announcements and breaking changes
+*** ~org-latex-to-mathml-convert-command~ and ~org-latex-to-html-convert-command~ shell-escape LaTeX code
+
+Previously, ~org-latex-to-mathml-convert-command~ and
+~org-latex-to-html-convert-command~ replaced %i placeholders with raw
+LaTeX fragment text, potentially triggering shell-expansion.
+
+Now, the %i placeholders are shell-escaped to prevent shell expansion.
+
+The existing customizations that assume no shell-escaping must be updated.
+
 *** Built-in HTML, LaTeX, Man, Markdown, ODT, and Texinfo exporters preserve the link protocol during export
 
 Previously, some link types where not exported as =protocol:uri= but
diff --git a/lisp/org.el b/lisp/org.el
index f3fae134d..f56767a1a 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -3266,7 +3266,9 @@ (defcustom org-latex-to-mathml-convert-command nil
 %j:     Executable file in fully expanded form as specified by
         `org-latex-to-mathml-jar-file'.
 %I:     Input LaTeX file in fully expanded form.
-%i:     The latex fragment to be converted.
+%i:     Shell-escaped LaTeX fragment to be converted.
+        It must not be used inside a quoted argument, the result of %i
+        expansion inside a quoted argument is undefined.
 %o:     Output MathML file.
 
 This command is used by `org-create-math-formula'.
@@ -3275,7 +3277,7 @@ (defcustom org-latex-to-mathml-convert-command nil
 \"java -jar %j -unicode -force -df %o %I\".
 
 When using LaTeXML set this option to
-\"latexmlmath \"%i\" --presentationmathml=%o\"."
+\"latexmlmath %i --presentationmathml=%o\"."
   :group 'org-latex
   :version "24.1"
   :type '(choice
@@ -3288,15 +3290,12 @@ (defcustom org-latex-to-html-convert-command nil
 directly replace the LaTeX fragment in the resulting HTML.
 Replace format-specifiers in the command as noted below and use
 `shell-command' to convert LaTeX to HTML.
-%i:     The LaTeX fragment to be converted.
+%i:     The LaTeX fragment to be converted (shell-escaped).
+        It must not be used inside a quoted argument, the result of %i
+        expansion inside a quoted argument is undefined.
 
 For example, this could be used with LaTeXML as
-\"latexmlc \\='literal:%i\\=' --profile=math --preload=siunitx.sty 2>/dev/null\".
-
-The LaTeX fragment is replaced as is, without escaping special shell
-syntax.  It may be necessary to use single-quotes around \\='%i\\=', not
-double-quotes.  Else a math fragment such as \"$y = 200$\" may be
-expanded to \" = 200\"."
+\"latexmlc literal:%i --profile=math --preload=siunitx.sty 2>/dev/null\"."
   :group 'org-latex
   :package-version '(Org . "9.4")
   :type '(choice
@@ -16332,7 +16331,7 @@ (defun org-create-math-formula (latex-frag &optional mathml-file)
 			      (expand-file-name
 			       org-latex-to-mathml-jar-file))))
 		 (?I . ,(shell-quote-argument tmp-in-file))
-		 (?i . ,latex-frag)
+		 (?i . ,(shell-quote-argument latex-frag))
 		 (?o . ,(shell-quote-argument tmp-out-file)))))
 	 mathml shell-command-output)
     (when (called-interactively-p 'any)
@@ -16400,7 +16399,7 @@ (defun org-format-latex-as-html (latex-fragment)
   "Convert LATEX-FRAGMENT to HTML.
 This uses  `org-latex-to-html-convert-command', which see."
   (let ((cmd (format-spec org-latex-to-html-convert-command
-			  `((?i . ,latex-fragment)))))
+			  `((?i . ,(shell-quote-argument latex-fragment))))))
     (message "Running %s" cmd)
     (shell-command-to-string cmd)))
 
-- 
2.44.0


[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Unit tests for function calling MathML converters (Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command)

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> Is second attachment appropriate to fix the issue or it has some 
> undesired effects.

Thanks!

> +(ert-deftest test-org/format-latex-as-html ()
> +  "Test shell special characters escaping in `org-format-latex-as-html'."
> +  (let ((org-latex-to-html-convert-command
> +         "printf \"<I%%sI>\" %i"))

I have a concern about this test - it will not work on windows or in
non-standard system shells. We should probably disable the test unless
"printf" can be evaluated in the current system shell.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Max Nikulin]

On 31/03/2024 15:25, Ihor Radchenko wrote:
> Max Nikulin writes:
>>
>> I think it is in the right direction.
>> - Manual needs update as well.
>> - I would explicitly stress that quotes causes undefined or even
>> dangerous behavior. See e.g. the last paragraph
>> https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s07.html
> 
> I have incorporated the above suggestions into the attached version of
> the patch.

Thanks, I have not tried the updated patch in action, but it looks like 
what I expect.

> +++ b/etc/ORG-NEWS
> @@ -13,6 +13,16 @@ Please send Org bug reports to mailto:emacs-orgmode@gnu.org.
>  
>  * Version 9.7 (not released yet)
>  ** Important announcements and breaking changes
> +*** ~org-latex-to-mathml-convert-command~ and ~org-latex-to-html-convert-command~ shell-escape LaTeX code
> +
> +Previously, ~org-latex-to-mathml-convert-command~ and
> +~org-latex-to-html-convert-command~ replaced %i placeholders with raw
> +LaTeX fragment text, potentially triggering shell-expansion.
> +
> +Now, the %i placeholders are shell-escaped to prevent shell expansion.
> +
> +The existing customizations that assume no shell-escaping must be updated.
> +

I would consider explicit mention of stripping quotes

+Previously, =%i= placeholders in the ~org-latex-to-mathml-convert-command~
and ~org-latex-to-html-convert-command~ user options were replaced
with raw LaTeX fragment text, potentially triggering shell-expansion
and incorrect result.

Now, the =%i= placeholders are shell-escaped to prevent shell expansion.

If you have single or double quotes around =%i= then update
customizations and remove quotes.

Re: [PATCH] Unit tests for function calling MathML converters (Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command)

[Max Nikulin]

On 31/03/2024 15:27, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> +(ert-deftest test-org/format-latex-as-html ()
>> +  "Test shell special characters escaping in `org-format-latex-as-html'."
>> +  (let ((org-latex-to-html-convert-command
>> +         "printf \"<I%%sI>\" %i"))
> 
> I have a concern about this test - it will not work on windows or in

I do not mind to add

    (skip-unless (not (memq system-type '(ms-dos windows-nt))))

> non-standard system shells. We should probably disable the test unless
> "printf" can be evaluated in the current system shell.

POSIX printf is more portable than "echo". Anyway Makefile expects a 
POSIX compatible shell. So I believe, it is developer responsibility to 
run build and test with a sane shell even if they prefer something 
unusual as the interactive shell.

Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> I have incorporated the above suggestions into the attached version of
>> the patch.
>
> Thanks, I have not tried the updated patch in action, but it looks like 
> what I expect.

> I would consider explicit mention of stripping quotes
> ...

I incorporated your suggestions and applied the updated version of the
patch.
Fixed, on main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=a698d073a

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Unit tests for function calling MathML converters (Re: Warn about shell-expansion in the docstring of org-latex-to-html-convert-command)

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> I have a concern about this test - it will not work on windows or in
>
> I do not mind to add
>
>     (skip-unless (not (memq system-type '(ms-dos windows-nt))))
>
>> non-standard system shells. We should probably disable the test unless
>> "printf" can be evaluated in the current system shell.
>
> POSIX printf is more portable than "echo". Anyway Makefile expects a 
> POSIX compatible shell. So I believe, it is developer responsibility to 
> run build and test with a sane shell even if they prefer something 
> unusual as the interactive shell.

Agree.
Applied, onto main, after adding skip-unless.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=a3bcb5536

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>