New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't use a shell to call inkscape #1512
Conversation
Python generally recommends against using `shell=True` when calling subprocesses (https://docs.python.org/3/library/subprocess.html#security-considerations). This also causes issues with shell metacharacters (see #1469). I'm also not entirely sure that the shell command *is* fully trustable - I don't know where {to_filename} and {from_filename} are from. Rest of nbconvert also prefers using lists instead of strings to call commands. svg2pdf also uses a command instead of a string now. We leave the old string implementation alone for backwards compatibility, although I'd really prefer to remove it. We don't need the quotes set up in #1469, since using a list automatically deals with that.
nbconvert/preprocessors/svg2pdf.py
Outdated
# For backwards compatibility with specifying strings | ||
# Okay-ish, since the string is trusted | ||
full_cmd = self.command.format(*template_vars) | ||
subprocess.call(full_cmd, shell=isinstance(str)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must be isinstance(full_cmd, str)
nbconvert/preprocessors/svg2pdf.py
Outdated
@@ -62,26 +62,37 @@ def _inkscape_version_default(self): | |||
raise RuntimeError("Unable to find inkscape executable --version") | |||
return output.decode('utf-8').split(' ')[1] | |||
|
|||
command = Unicode( | |||
help="""The command to use for converting SVG to PDF | |||
# FIXME: Deprecate passing a list here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be
# FIXME: Deprecate passing a list here | |
# FIXME: Deprecate passing a string here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested changed merged with main. Let's get this in and try to double check the alpha is working for folks on Windows more rigerously after that
Python generally recommends against using
shell=True
when calling
subprocesses (https://docs.python.org/3/library/subprocess.html#security-considerations).
This also causes issues with shell metacharacters (see
#1469). I'm also
not entirely sure that the shell command is fully trustable
from. Rest of nbconvert also prefers using lists instead
of strings to call commands.
svg2pdf also uses a command instead of a string now. We leave
the old string implementation alone for backwards compatibility,
although I'd really prefer to remove it.
We don't need the quotes set up in
#1469,
since using a list automatically deals with that.