Octoprint: Plugin install/update under windows fails because file:// URL is single quoted

Created on 24 Aug 2020 · 23Comments · Source: OctoPrint/OctoPrint

Template

What were you doing?

Attempting to install and / or upgrade plugins from the browser interface.

What did you expect to happen?

I expect the plugins to install.

What happened instead?

The plugins did not install or upgrade. Instead I got an error: ERROR: Invalid requirement

Did the same happen when running OctoPrint in safe mode?

Not tried - I assume that plugin install / upgrade is not possible in Safe Mode which is designed to run without plugins,

Version of OctoPrint

1.4.2

Operating System running OctoPrint

Windows 10 64-bit

Printer model & used firmware incl. version

Not relevant in this instance but it is a Dagoma DiscoEasy 200 with latest Dagoma Marlin firmware.

Browser and version of browser, operating system running browser

Google Chrome / Brave / Microsoft Edge Chromium all on Windows 10 64-bit.

Link to octoprint.log

octoprint.log

Link to contents of terminal tab or serial.log

n/a

Link to contents of Javascript console in the browser

n/a

Link to contents of plugin manager log

plugin_pluginmanager_console.log

Description

I have installed Octoprint under windows 10, but plugin installs / updates fail.

As you can see from the plugin manager log, it is attempting to run PIP to install / upgrade the plugin, but it fails:

2020-08-30 18:12:45,158   c:\octoprint\scripts\python.exe -m pip --disable-pip-version-check install 'file:///c:/users/octopr~1/appdata/local/temp/tmpnfrijr/octoprint-activefiltersextended-master.zip' --no-cache-dir
2020-08-30 18:12:50,092 ! ERROR: Invalid requirement: "'file:///c:/users/octopr~1/appdata/local/temp/tmpnfrijr/octoprint-activefiltersextended-master.zip'"

I tried running this command manually and it failed. When I ran the command manually replacing the single quotes around the file:// with double quotes it worked first time.

I have tracked the source of the single quotes to this line:

https://github.com/OctoPrint/OctoPrint/blob/f79d217ffae4027302ce6351e9fafbc426c3de52/src/octoprint/plugins/pluginmanager/__init__.py#L511

The issue here is that, as far as I can work out, the quoting rules for Linux and Windows are completely different. sarge.shell_quote is needed in Bash and other Linux shells to stop interpretation of a string (see https://www.gnu.org/software/bash/manual/html_node/Quoting.html#Quoting). On Windows you need to wrap a string in double quotes if it includes spaces.

approved bug done

Source

Sophist-UK

All 23 comments

Hi @Sophist-UK,

It looks like there is some information missing from your bug report that will be needed in order to solve the problem. Read the Contribution Guidelines which will provide you with a template to fill out here so that your bug report is ready to be investigated (I promise I'll go away then too!).

If you did not intend to report a bug but wanted to request a feature or brain storm about some kind of development, please take special note of the title format to use as described in the Contribution Guidelines.

Please do not abuse the bug tracker as a support forum - that can be found at discourse.octoprint.org. Go there for any kind of issues with network connectivity, webcam functionality, printer detection or any other kind of such support requests or general questions.

Also make sure you are at the right place - this is the bug tracker of the official version of OctoPrint, not the Raspberry Pi image OctoPi nor any unbundled third party OctoPrint plugins or unofficial versions. Make sure too that you have read through the Frequently Asked Questions and searched the existing tickets for your problem - try multiple search terms please.

I'm marking this one now as needing some more information. Please understand that if you do not provide that information within the next two weeks (until 2020-09-07 22:50 UTC) I'll close this ticket so it doesn't clutter the bug tracker. This is nothing personal, so please just be considerate and help the maintainers solve this problem quickly by following the guidelines linked above. Remember, the less time the devs have to spend running after information on tickets, the more time they have to actually solve problems and add awesome new features. Thank you!

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being, so don't expect any replies from me :) Your ticket is read by humans too, I'm just not one of them.

GitIssueBot on 24 Aug 2020

@Sophist-UK Worked for me on windows, just a second ago. Logs are desperately needed here, (as well as the rest of the information in the bug report template, it's really not optional)

Maybe some more details of the plugin/file you tried to install would be useful too.

cp2004 on 24 Aug 2020

This is on the official Octoprint recently installed.

Plugins would not installed under Windows.
This was true for two plugins both of which had zip files. I found and checked that the zip files were readable and not corrupted.
The plugin error log didn't say much except give the command that was run and that it failed.
I opened a command prompt window and copied and pasted the command, and received the same error.
I replaced the single quotes around the file:// string with double quotes and it ran perfectly first time.
I looked at the source code and found where the single quotes came from.

I fail to see what additional information will do to help diagnose the problem further. sarge.shell_quote is specifically for Linux shells and is not intended to be for Windows AFAICS.

However, I will gather logs etc. from the Octoprint machine tomorrow and provide a correctly formatted issue description - though had this been provided as a template as Github allows, I would have done so in the first place.

Sophist-UK on 24 Aug 2020

Hmm .., I created the issue by clicking on the line in the file and selecting create issue. It appears that when you do that you don't get the Bug template provided that IS defined after all. [Sigh]

Sophist-UK on 24 Aug 2020

Was just wondering how you missed that.
For logs, plugin_pluginmanager_console.log is needed here (as well as octoprint.log) & the rest of the template where applicable.

The command is not quoted in the log of my plugin installs, so I really want to see which bit is causing the problem there.

cp2004 on 24 Aug 2020

I didn't miss the template. Github didn't provide it when I created the issue from the line in the source file. I will provide logs tomorrow.

Sophist-UK on 24 Aug 2020

👍1

@Sophist-UK Have you got those logs 🙂?

I wasn't able to reproduce this in any way (file paths inc. with spaces etc.) so they're required for this to be analysed.

Thanks.

cp2004 on 27 Aug 2020

I haven't forgotten - but I have some other high priority things I need to focus on first.

Sophist-UK on 27 Aug 2020

@cp2004 Issue updated with logs and using the bug template.

Sophist-UK on 30 Aug 2020

👍1

Fixed for 1.5.x

Not tried - I assume that plugin install / upgrade is not possible in Safe Mode which is designed to run without plugins,

Just for the record: it is possible. Third party plugins will install, plugins will update, they just won't load/get initialized and hooked into the system. And bundled plugins aren't affected by safe mode to begin with.

foosel on 3 Sep 2020

The fix in https://github.com/OctoPrint/OctoPrint/commit/e72aab34488337ed9e4486269a08d5b88e392e4c should work on Windows as it removes the single quotes without adding double quotes.

This will work if there are not spaces in path - but if there are spaces in path it wouldn't work even if file:// was wrapped in double quotes because according to RFC8089 file:// URLs should be percent-encoded to avoid spaces and other special characters and I cannot see anything to apply percent-encoding in the code.

It is unclear to me whether Linux shells would be vulnerable to substitution now that sarge.shell_quote is removed or if percent-encoding is added. Whilst this seems unlikely, it seems to me that it might be possible that a specially crafted OctoPrint plugin name would create a vulnerability.

Sophist-UK on 3 Sep 2020

The thing is, the only thing that will get passed to this function since 1.4.1 is a temporary file name generated on the system. No user input is involved here, ~~so there should be no crafting be possible~~. // edit never mind, I forgot the file name from the content disposition header will get used

But you are right, better err on the side of caution and be as aggressive in escaping as possible, also regarding whitespace (which usually shouldn't show up in the file name now that it gets generated anyhow, but better safe than sorry).

I've pushed 6132f481ce630755c3b96aca03619303abe4e895. It returns the code to always use the generated file url for installing and makes sure to percent quote that. On windows that still needs a bit of special treatment since the drive letter needs to split off first (it must not be encoded), but in my local tests it now worked both with a regular temp file as well as an injected path with whitespace.

Side note: I also tested with enclosing the path (not the file url) in windows with double quotes, but that led to pip complaining as well. Quoting of any kind of that argument is apparently not well received under Windows.

foosel on 4 Sep 2020

I do not think this is good coding.

It should be percent-encoded in Linux as well - RFC8089 applies to both Windows and Linux.
I have no idea what the lower casing is for.

In my opinion the code should be:

        try:
            # Py3
            from urllib.parse import quote as url_quote
        except ImportError:
            # Py2
            from urllib import quote as url_quote

        path_url = "file:///" + url_quote(os.path.abspath(path).replace(os.sep, "/"))

        already_installed_check = lambda line: path_url in line.lower() # lower case in case of windows

        if os.sep == "/":
            path_url = shell_quote(path_url)

        self._logger.info("Installing plugin from {}".format(source))
        pip_args = ["--disable-pip-version-check", "install", path_url, "--no-cache-dir"]

Sophist-UK on 4 Sep 2020

It should be percent-encoded in Linux as well - RFC8089 applies to both Windows and Linux.

It is.

        path = os.path.abspath(path)
        if os.sep != "/":
            # windows gets special handling
            drive, loc = os.path.splitdrive(path)
            path_url = "file:///" + drive.lower() + url_quote(loc.replace(os.sep, "/").lower())
            shell_quote = lambda x: x # do not shell quote under windows, non posix shell
        else:
            path_url = "file://" + url_quote(path) ### ⬅ THERE
            shell_quote = sarge.shell_quote

I have no idea what the lower casing is for.

Reinstallation detection, needed to detect pip complaining about an already installed package in order to force a reinstall. Needs to be lower case since otherwise issues can arise with windows' case insensitivity in path names. See the comment.

Your code won't work as it will also percent encode the drive letter under windows, breaking install. Otherwise it does exactly the same, just harder to maintain as it covers up the OS differences, and possibly it also breaks reinstall detection by rewriting path_url that's needed for that.

foosel on 4 Sep 2020

        try:
            # Py3
            from urllib.parse import quote as url_quote
        except ImportError:
            # Py2
            from urllib import quote as url_quote

        drive, loc = os.path.splitdrive(os.path.abspath(path))
        path_url = "file:///" + drive + url_quote(loc.replace(os.sep, "/"))

        if os.sep != "/":
            # windows case insensitive file system
            install_path = path_url = path_url.lower()
        else:
            # linux shell safe quote
            install_path = sarge.shell_quote(path_url)

        already_installed_check = lambda line: path_url in line.lower() # lower case in case of windows

        self._logger.info("Installing plugin from {}".format(source))
        pip_args = ["--disable-pip-version-check", "install", install_path, "--no-cache-dir"]

drive is empty if Linux file system.

Sophist-UK on 4 Sep 2020

There's no reason to split the path on non-windows, so why move it into the general workflow? And why make the code less readable by trying to do as much as possible on every single line? I don't see a reason to change the code when it already does what it needs to do and is something I'll have to maintain for the years to come, so if you are just proposing changes here for the sake of disagreeing I think there's no reason to continue this discussion. Otherwise please tell me what you think is wrong with what is now committed.

foosel on 4 Sep 2020

No - I am not simply suggesting changes just in order to disagree. My personal opinion is that my code is more elegant and easier to maintain because it is simpler because more code works for both Windows and Linux and only the bits that need to be different are split. Your code e.g. has a lambda function for shell_quote which is less obvious and has duplicates e.g. the "file://" string.

But in the end this is a question of style and I am not going to create friction with you if you don't like the style I am proposing.

Sophist-UK on 4 Sep 2020

Your code e.g. has a lambda function for shell_quote which is less obvious

Yes, because I cannot overwrite path_url with the shell quoted version under Linux, that will - as already mentioned - break reinstall detection. I also cannot do a full url_quote on the whole path as that will break the drive letter under windows. Your code as is will not work and even break something that wasn't broken before. This isn't a question of elegance here but rather correctness. Fixing your's so it is correct makes it pretty much mine, just with some less lines at the cost of general readability. Strike that, I was having an earlier version in mind.

In any case, I find your's less readable than mine, they are functionally identical (edit: though I just realised that path_url = "file:///" + drive + url_quote(loc.replace(os.sep, "/")) will actually cause path_url to start with file://// as loc will always start with a /, so now the file url is broken under linux), I have to maintain it, so I won't change it again unless there's something wrong in it.

foosel on 4 Sep 2020

I think that is a typo - it previously had file:// for both cases, now it has file:/// for windows and file:// for Linux which doesn't seem right to me.

Sophist-UK on 4 Sep 2020