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

win_package - Test to solve quotes issues #58089

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@ShachafGoldstein
Copy link
Contributor

commented Jun 19, 2019

SUMMARY

Fixes: #58044
Fixes: #40973

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

win_package.ps1

ADDITIONAL INFORMATION
@ansibot

This comment has been minimized.

@nitzmahone

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

Not sure if you were intending to try and get this merged, but I don't think we'd actually want to do it this way- lots of ways "just remove all the quotes" can fail. If anything, we probably need to expose some of the cmdline parsing stuff to take that apart properly, or at least just remove quotes only from the ends of the strings...

@ansibot ansibot removed the needs_triage label Jun 19, 2019

@ansibot ansibot added the stale_ci label Jun 27, 2019

@ShachafGoldstein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

Not sure if you were intending to try and get this merged, but I don't think we'd actually want to do it this way- lots of ways "just remove all the quotes" can fail. If anything, we probably need to expose some of the cmdline parsing stuff to take that apart properly, or at least just remove quotes only from the ends of the strings...

correct but from all the examples I saw, the only place vendors put the " is around the EXE full name and all the parameters are without "", I can make it strip just the EXE quotes with some string manipulation and indexes if you prefer

@ShachafGoldstein ShachafGoldstein changed the title [WIP] win_package - Test to solve quotes issues win_package - Test to solve quotes issues Jun 28, 2019

@ansibot ansibot added core_review and removed WIP stale_ci labels Jun 28, 2019

@nitzmahone

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Just poking around in the code, I actually think this might be a bug related to the Argv-ToString call on quoted non-list args... There are already two layers of command-line parsing going on by actual command-line parsers (Argv-ToString and the one inside Run-Command), so adding another layer is not the right thing to do (especially since this PR in its current form just strips all quotes, which will break anything of any complexity involving other quoted args or escaped quotes).

The more proper solution here is probably to not coerce $uninstall_arguments` to a list in the non-msiexec case, and skip the Argv-ToString call if it's already a string. That way, we can remain oblivious to any quoting in use and leave that to a proper command-line parser.

There may also be a related bug if the Msiexec string is quoted, or if MsiExec were an absolute path (right now we're just checking if it starts with 'MsiExec'), but I'm not sure if some installers set that manually or not (IIRC Windows Installer always puts those in unquoted and not path-qualified).

@ShachafGoldstein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

I fixed it to only strip the EXE quotes in the last commit but I also saw all the parsing and this was the best immediate way to handle the issues coming.

Maybe we could rewrite this part of the module or the relevant functions to be smarter in the parsing but I'm not sure how long that will take

@jborean93

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2019

So I was looking into this a bit more and it looks like @nitzmahone is right and we don't want to use Argv-ToString at all when reading the UninstallString key. I don't think it's a bug in Argv-ToString per say as it's just escaping an already escaped command line but there's no sane way to detect this. So we should just not escape values from the UninstallString property.

I was looking at some examples on my test host and I found the value in this string is typically;

  • Just the path to the uninstall executable that may or may not be quoted
  • A path to the uninstall executable with extra arguments added to the end that is quoted if needed
  • A string that starts with MsiExec.exe but we already handle this scenario

Considering this value is used by the Control Panel interface to uninstall a program manually I would be happy to just use these values as they are. The only exception to this rule would be the first scenario where the value is just the path to an executable and is not quoted. We would have to do something like the following to make sure it is quoted.

if (Test-Path -LiteralPath $UninstallString -ItemType Leaf) {
    $UninstallString = "`"$UninstallString`""
}

This checks if the raw value is just the path to a file and we manually quote it if that's true. It shouldn't be true for an already quoted path as " is not a valid char for a filepath.

We would still need to escape arguments passed in from Ansible if it's a list but the value returned from here is just appended to the UninstallString value.

On a side note, when looking at the packages I had installed I found a few of them had a registry property QuietUninstallString. This had the actual arguments required to silently uninstall a package which would be quote useful for win_package. It would be a good idea to check for that value and use that instead if arguments has not already been provided by the user. This allows us to maintain backwards compatibility when someone has manually specified the uninstall arguments vs someone who just specified the package_id. We should persue this but I would recommend doing it in another PR.

@ansibot ansibot added the stale_ci label Jul 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.