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.

Sign up for GitHub

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

Fix Visicut calling inkscape in an AppImage. #721

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e435f2e
Fix Visicut calling inkscape in an AppImage.
jnweiger Apr 8, 2024
ab42570
Update tools/inkscape_extension/visicut_export.py
jnweiger Apr 8, 2024
64f5e4f
Update tools/inkscape_extension/visicut_export.py
jnweiger Apr 8, 2024
5bfbeef
rewritten as suggested
jnweiger Apr 8, 2024
4a551e2
Ported the appimage patch to pathlib. Much nicer code and windows com…
jnweiger Apr 9, 2024
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
12 changes: 6 additions & 6 deletions tools/inkscape_extension/visicut_export.py
View file
Open in desktop
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 @@ -334,13 +335,12 @@ def get_original_filename(filename):
# 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.)
dir = os.path.dirname(INKSCAPEBIN)
while dir != '/':
apprun_path = os.path.join(dir, "AppRun")
if os.path.exists(apprun_path):
INKSCAPEBIN = apprun_path
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
dir = os.path.dirname(dir)

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

Expand Down
Loading