Update bl-recent-files-pipemenu

[johnraff]

Double-quoting file paths opens a possible vulnerability: words in filenames following a dollar sign will be treated as variables and expanded when opening from the menu.

Test: create a file named $HOME, open it in a text editor so it is added to recently-used.xbel, then open it from the recent files menu. The editor will open a new file named /home/[username]

EDIT: The issue does not appear when using the generated xml menu directly with openbox, but only via jgmenu's jgmenu_run ob module. Presumably the shell is invoked by jgmenu to interpret file paths. I think it's still a potential vulnerability that we might as well avoid, however, since the fix looks simple.

The fix is to quote filepaths with single quotes, but please check my proposed quote() function.

[johnraff]

@2ion sorry, forgot to tag you in the last message.

[johanmalm]

Looks okay.

I always struggle to follow the logic around complex shell quoting, but think I get it.

Yes, popen() is used which uses shell. https://github.com/johanmalm/jgmenu/blob/58fde284040dbbae334b4502e4f33084c0a192fc/src/jgmenu.c#L1399

[ghost]

Not sure if this is better or worse since %q in Lua's string.format has nothing to do with POSIX shell quoting anyway except accidentally covering the same quoting rules minus stuff like $. What we would need is something like Python3's shlex.quote instead. I would rewrite this entire menu today in Python anyway analog to the sshconfig menu.

I can't think of a case that would break right now so I guess it's ok to merge.

[johnraff]

%q in Lua's string.format has nothing to do with POSIX shell quoting anyway except accidentally covering the same quoting rules minus stuff like $

Yes, I should have looked at some more references! ( http://lua-users.org/wiki/StringLibraryTutorial %q puts quotes around a string argument's value ) More searching showed that the purpose of %q is to protect the string within lua, not in the shell. It does more than just add double quotes. eg https://www.gammon.com.au/scripts/doc.php?lua=string.format

%q - formats a string in such a way Lua can read it back in (eg. as Lua source). Basically this means: It puts a backslash in front of the double quote character (hex 22), backslash itself (hex 5C), and newline (hex 0A). The nul character (hex 00) becomes \000 Carriage return (hex 0D) becomes \r The string itself is surrounded by double quotes.

One way forward could be to go on using the lua quote function, but adding a backslash to escape dollars \$. That works, but leaves some other special characters that the shell might try to interpret: ` ! and the backslash itself . Single quotes are more robust for the shell but I have no idea what effect strange characters in filepaths might have on lua if not quoted.

Further complication will arise if I continue with a further suggestion, which would involve applying url.unescape to filepaths because not all apps can handle the file:// protocol...

[johnraff]

Let's put this PR on hold for a bit.

I'll fork a test branch with this and my other pending suggestions all together so you can see how they interact, and whether it would be safe to apply or not - possibly with some adjustments.

Those suggestions: 1) Single-quote filepaths (this PR). 2) Accomodate compound commands - ie [executable + option(s)]. 3) Pass unix filepaths, not uri-encoded file:// syntax. 4) Drop non-existent (deleted or moved) files from the menu.

2] & 3] were found to be necessary in order to use the file-comparison entries that meld adds to recently-used.xbel. I think it would be a useful enhancement, even if meld's failure to honour the file:// syntax should be deprecated. (Another gnome app, file-roller, fails to add the %u to the exec attribute btw.)

Fairly simple code already exists for 1) ~ 4). (I'll post the branch soon.)

I'm still thinking about this though:

5) Use the modified attribute to order the menu correctly (order in recently-used.xbel is not reliable).

Even more tenuous:

6) If a \<title> tag exists, use it as the menu label instead of the filename.

@2ion if you have any comment on any of this, especially 5) & 6) ...

[johnraff]

New branch uploaded, pull request #62

[johnraff]

Closing because #62 incorporates the suggested changes here.