Skip to content
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

Fix Visicut calling inkscape in an AppImage. #721

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions tools/inkscape_extension/visicut_export.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import random
import string
import socket
from pathlib import Path

try:
from os import fsencode
Expand Down Expand Up @@ -140,13 +141,17 @@ def is_exe(fpath):

def inkscape_version():
"""Return Inkscape version number as float, e.g. version "0.92.4" --> return: float 0.92"""
version = subprocess.check_output([INKSCAPEBIN, "--version"], stderr=DEVNULL).decode('ASCII', 'ignore')
assert version.startswith("Inkscape ")
match = re.match("Inkscape ([0-9]+\.[0-9]+).*", version)
assert match is not None
version_raw = subprocess.check_output([INKSCAPEBIN, "--version"], stderr=DEVNULL).decode('ASCII', 'ignore')
## When inkscape lives in an appimage, AppRun may pollute stdout with extra information.
# Go through all the lines, and find the one that starts with Inkscape
lines = version_raw.splitlines()
version = [line for line in lines if line.startswith("Inkscape ")]
assert len(version) == 1, "inkscape --version did not return a version number: " + version_raw
match = re.match("Inkscape ([0-9]+\.[0-9]+).*", version[0])
assert match is not None, "failed to parse version number from " + version[0]
version_float = float(match.group(1))
return version_float



# Strip SVG to only contain selected elements, convert objects to paths, unlink clones
Expand All @@ -157,7 +162,7 @@ def inkscape_version():
# The idea is similar to http://bazaar.launchpad.net/~nikitakit/inkscape/svg2sif/view/head:/share/extensions/synfig_prepare.py#L181 , but more primitive - there is no need for more complicated preprocessing here
def stripSVG_inkscape(src, dest, elements):
version = inkscape_version()

# create temporary file for opening with inkscape.
# delete this file later so that it will disappear from the "recently opened" list.
tmpfile = tempfile.NamedTemporaryFile(delete=False, prefix='temp-visicut-', suffix='.svg')
Expand Down Expand Up @@ -199,8 +204,8 @@ def stripSVG_inkscape(src, dest, elements):
verbs += ["UnhideAllInAllLayers", "EditInvertInAllLayers", "EditDelete", "EditSelectAllInAllLayers", "EditUnlinkClone", "ObjectToPath", "FileSave"]
# --verb=action1;action2;...
command += ["--verb=" + ";".join(verbs)]


DEBUG = False
if DEBUG:
# Inkscape sometimes silently ignores wrong verbs, so we need to double-check that everything's right
Expand Down Expand Up @@ -248,7 +253,7 @@ def stripSVG_inkscape(src, dest, elements):
actions += ["export-area-page"]

command = [INKSCAPEBIN, tmpfile, "--export-overwrite", "--actions=" + ";".join(actions)]

try:
#sys.stderr.write(" ".join(command))
# run inkscape, buffer output
Expand Down Expand Up @@ -326,6 +331,16 @@ def get_original_filename(filename):
VISICUTBIN = which("VisiCut.Linux", [VISICUTDIR, "/usr/share/visicut"])
INKSCAPEBIN = which("inkscape", [INKSCAPEDIR])

## Test if this inkscape is in an appimage.
# We detect this by checking for an AppRun file, in one of the parent folders of our INKSCAPEBIN.
# If so, replace INKSCAPEBIN with AppRun, as this is the only safe way to call inkscape.
# (a direct call mixes libraries from the host system with the appimage, may or may not work.)
for parent in Path(INKSCAPEBIN).parents:
apprun = parent / "AppRun"
if apprun.is_file() and os.access(apprun, os.X_OK):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this os.access check, it's very compact. pathlib should have a shortcut for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe only checking is_file() alone is even more compact? An inkscape AppImage that has an 'AppRun' file which has no execute bits, or is mounted without exec, or suffering from a security policy, or whatever ... is out of scope here anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just failing silently is not the best idea. Your algorithm can still cross mountpoints (that's what the is_mount check was there for before) and I can imagine a bunch of false positive detections of AppRuns in that case. For instance, in some occasions, AppDirs can be integrated in each other (lazy approach but it works, I use that myself occasionally). In such a case, the execute bit on some AppRun in the chain might not be set. Also, AppImages can be extracted and copied to devices that do not have the executable bit set, etc.

The check is fine and it's good to have it. In fact, security wise, we should add the mountpoint check and abort once it's crossed. To do so, this should be appended to the for's body:

    if parent.is_mount():
        break

Security (i.e., preventing critical issues) and safety (i.e., keeping users from experiencing weird behavior) always add a bit of verbosity to the code, but in the end, it's always worth it. We'd have some good reasons to throw exceptions here even. But that'd bloat this PR. I think in its current form, it's a reasonably solid implementation and I don't see any attack vector (without thinking about it for more than 5 minutes).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found an INKSCAPE_COMMAND environment variable that seems to point to the AppRun file. Can you check if we can use this instead? First guess:

INKSCAPEBIN = os.environ.get("INKSCAPE_COMMAND") or which("inkscape", [INKSCAPEDIR])

This would avoid the potential problems.

INKSCAPEBIN = apprun
break

tmpdir = tempfile.mkdtemp(prefix='temp-visicut-')
dest_filename = os.path.join(tmpdir, get_original_filename(filename))

Expand Down Expand Up @@ -384,3 +399,4 @@ def get_original_filename(filename):
sys.exit(1)

# TODO (complicated, probably WONTFIX): cleanup temporary directories -- this is really difficult because we need to make sure that visicut no longer needs the file, even for reloading!
# - Maybe add the PID od the running visicut, then we can detect orphaned temp direcories.
Loading