Windows - Disable option to add PATH during an AllUsers install

[pseudoyim]

Description

A critical privilege vulnerability was found in non-default installations of Anaconda installers for Windows users (CVE 2022-26526). This also impacts all Windows installers built using the latest release of constructor (v3.3.1).

To mitigate this vulnerability, this PR disables the option to add the installation to the PATH environment variable during an "All Users" installation. Users can still add the installation to their PATH environment variable during a "Just Me" installation. Additionally, when installing with Administrator privileges, non-admin system Users will no longer have "Write" permissions.

Currently, the user sees these Advanced Installation Options after selecting Just Me or All Users:

With the changes in this PR, if the user selects All Users, the option to add to the PATH environment variable will show as disabled:

Checklist - did you ...

• Add a file to the news directory (using the template) for the next release's release notes?
• Add / update outdated documentation?
• Add / update necessary tests?

[pseudoyim]

@jaimergp - is this the only key for which the description needs to be updated (regarding how the AddToPath option is disabled for AllUsers installations)?

[dbast]

wouldn't the CVE be already be fixed in the "all users" cases by making the folder only admin writable but still allowing to have it in PATH?

[jaimergp]

@jaimergp - is this the only key for which the description needs to be updated (regarding how the AddToPath option is disabled for AllUsers installations)?

I was misremembering there ways a key similar to register_python but apparently there's none? So nothing to document there, I guess.

[pseudoyim]

LGTM. Can you provide screenshots for the following items?

• Help message with the new description
• Error message that appears if /AddToPath=1 is passed in an "All Users" installation.

@jaimergp, per your request:

Help message with the new description

Error message that appears if /AddToPath=1 is passed in an "All Users" installation.

It pops up again when the GUI gets to the installation type screen:

[pseudoyim]

@jaimergp @hoechenberger - When I was testing the MessageBox (/AddToPath=1 is disabled and ignored in 'All Users' installations), it kept popping up exactly two times in a row (after hitting the first OK).

I traced the problem to !insertmacro ParseCommandLineArgs being called twice in main.nsi.tmpl (line 323 and line 470). I think it only needs to be called once and this function seemed unnecessary to me, so I removed it in this commit. After I removed it, this solved the problem of the MessageBox popping up twice.

Could you please review this and validate my thinking?

[jaimergp]

Hm, I am not sure... the logic in that part is really opaque. It looks like they are first parsing the CLI arguments, then overriding some defaults, then parsing again.

Both calls were introduced in the same PR (particularly, this commit), but judging by the branch name that PR is part of a merge/reunification. After that, I can't find any history on why you need it twice.

Since we can't prove it was added by accident, and all we can say it looks like deliberate, I'd be inclined to say we shouldn't remove it.

Things we can do to prevent the double warning:

• Move the AllUsers vs AddToPatch check to the OnInit_Release function so it's only done once.
• Flip a global var to "remember" whether the user was warned already.

I think I prefer the 1st one.

This is not a pretty part of the codebase so at the very least we should create an issue to investigate the double argument parsing call.

[pseudoyim]

Thanks for reviewing @jaimergp. Per your recommendation, I restored the OnInit_Release function and moved the AddToPath check under it. I built a test MinicondaX with these changes and the MessageBox shows up only once. Screenshots look the same as what I posted here.

[dbast]

LGTM to me. all review comments are covered now and it locally tests correctly. merging.