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

Use MSI API to allow UAC prompts on MSI silent installs #1398

Merged
merged 17 commits into from Aug 27, 2021

Conversation

florelis
Copy link
Member

@florelis florelis commented Aug 25, 2021

Issue:

When installing an .msi with ShellExecute (which calls msiexec) in --silent mode, if the installation requires a UAC prompt it would fail instead.

Change:

Use the MSI API instead of ShellExecute so we can use the INSTALLUILEVEL_UACONLY flag to allow the UAC prompt to appear on silent installs.
Most of the change is parsing the arguments we pass to msiexec into something we can pass to the MSI API. This doesn't mimic msiexec arg parsing exactly. This currently supports the /q and /l options, and command line properties. The /n option (instance code) is not supported yet; we can look into it in the future if needed.

Validated:

Confirmed that silent install works with some packages that were failing (e.g. Zoom.Zoom, RustLang.rust-msvc)

Fixes #1143

Microsoft Reviewers: Open in CodeFlow

@florelis florelis requested a review from a team as a code owner August 25, 2021 21:51
@ghost ghost added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Aug 25, 2021
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@JohnMcPMS JohnMcPMS left a comment

Choose a reason for hiding this comment

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

I would also suggest a one-off test pass over the entire set of manifests in winget-pkgs. Here is some code that I used to generate the list of switches that are currently in use. It should be no trouble to parse everything that you see (a bit harder to simulate what they would look like at install time, but probably not necessary).

    using namespace std::filesystem;

    std::queue<path> pq;
    pq.push({ R"(C:\git\pkgs\manifests)" });

    std::ofstream outfile{ R"(C:\Temp\manifestgrep.txt)"};

    while (!pq.empty())
    {
        path current = pq.front();
        pq.pop();

        bool processAsManifest = false;
        for (const auto& child : directory_iterator{ current })
        {
            if (!child.is_directory())
            {
                processAsManifest = true;
                break;
            }
            else
            {
                pq.push(child.path());
            }
        }

        if (processAsManifest)
        {
            Manifest::Manifest manifest = Manifest::YamlParser::CreateFromPath(current);

            for (const auto& installer : manifest.Installers)
            {
                if (installer.InstallerType == Manifest::InstallerTypeEnum::Wix || installer.InstallerType == Manifest::InstallerTypeEnum::Msi)
                {
                    if (!installer.Switches.empty())
                    {
                        outfile << manifest.Id << "[" << manifest.Version << "] " << std::endl;
                        for (const auto& s : installer.Switches)
                        {
                            outfile << "  " << s.second << std::endl;
                        }
                    }
                }
            }
        }
    }

src/AppInstallerCLICore/Workflows/InstallFlow.cpp Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/MsiExecArguments.cpp Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/MsiExecArguments.cpp Outdated Show resolved Hide resolved
if (isQuote)
{
// We will ignore space characters enclosed between double quotes
withinQuotes = !withinQuotes;
Copy link
Member

Choose a reason for hiding this comment

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

So the tokens in 1 a" word "b 2 are 1, a" word "b, and 2? It would seem that the quotes sticking to the outer characters is legit from PowerShell (not sure exactly which code is parsing the command line there though).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, those would be the tokens.

A real use case of something like that would be having a property like SOMEPATH="C:\a path\with spaces". In that case, the whole thing would be the single token. It can also happen with options, like /i"C:\my app.msi", although none of the options we parse need this.

But I'm still accepting weird things like this" "as" a "single" "token. I probably should make the token end after the first pair of quotes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to double check what msiexec actually does/accept. The documentation says that they accept things like this: msiexec /i A:\Example.msi PROPERTY="Embedded ""Quotes"" White Space"

// If the value is quoted, removes the quotes and replaces escaped characters.
std::string ParseValue(std::string_view valueToken)
{
if (valueToken.empty() || valueToken[0] != '"')
Copy link
Member

Choose a reason for hiding this comment

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

In my example above, a" word "b on the terminal ends up being a word b in main. Do we need to properly handle internal quotes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe you can wrap it in single quotes to get the double quotes into main. I.e. app.exe 'a" word "b' would receive a" word "b as a single arg.

Copy link
Member

Choose a reason for hiding this comment

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

I am referring to removing the internal quotes. It seems like we do in some cases and not in others. Properties should keep them intact I would imagine since they are passed as a big string. Things like the log file location should have them removed though to create a path.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how much parsing of the args there is before it gets to the exe (i.e. if PS, cmd, ShellExecute, CreateProcess or any other intermediary can remove the quotes) but I'm assuming we are working at the same level that msiexec does its parsing and we have to do the same thing it does. From what I could gather, the only case where msiexec removes quotes is when they surround a value for an option (only log file in our case), and that is external quotes only.

}

// Verify that token is quoted correctly
if (valueToken.size() <= 1 || valueToken.back() != '"')
Copy link
Member

Choose a reason for hiding this comment

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

Not that there are any valid cases for it really, but you can pass an empty token on the command line with "". If the end of stream is also a valid way to end a quoted section as stated above, then " at the end of the stream would be a valid empty value.

src/AppInstallerCommonCore/MsiExecArguments.cpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Aug 26, 2021
@florelis
Copy link
Member Author

I tried parsing all the switches for MSI installers in the winget-pkgs repo. I found 3 that my code could not parse:

  • LuanRoger.WorkOffice[0.3.0]
  • Mozilla.Firefox[82.0.1]
  • 7zip.7zip.Alpha.msi[21.02.00.0]

All of them are actually manifest errors. winget cannot install them if you make it use those switches, neither can msiexec if called directly. I'll update the manifests later today.

@github-actions

This comment has been minimized.

@florelis
Copy link
Member Author

Here are the manifest updates mentioned in my last comment:
microsoft/winget-pkgs#25996
microsoft/winget-pkgs#25997
microsoft/winget-pkgs#25998

@JohnMcPMS
Copy link
Member

I tried parsing all the switches for MSI installers in the winget-pkgs repo. I found 3 that my code could not parse:

  • LuanRoger.WorkOffice[0.3.0]
  • Mozilla.Firefox[82.0.1]
  • 7zip.7zip.Alpha.msi[21.02.00.0]

All of them are actually manifest errors. winget cannot install them if you make it use those switches, neither can msiexec if called directly. I'll update the manifests later today.

Before we make this the default behavior for all MSI installs we need to ensure that it is part of manifest validation. I don't think that is strictly necessary as part of this change, but maybe at least a comment near the experimental feature will help us remember?

Copy link
Member

@JohnMcPMS JohnMcPMS left a comment

Choose a reason for hiding this comment

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

I still think we need to be handling the internal quotes differently, but maybe this is exactly how msiexec is doing it. The internal quotes are very corner case, as just using surrounding quotes should work equally well, but strange decisions are made all the time.

src/AppInstallerCLITests/MsiExecArguments.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLITests/MsiExecArguments.cpp Show resolved Hide resolved
@florelis florelis merged commit 9a15df4 into microsoft:master Aug 27, 2021
@florelis florelis deleted the directmsi branch September 27, 2021 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Silent install of MSI package fails with 1603
2 participants