Install-ChocolateyInstallPackage.ps1 adds trailing space to silent args #2345

majkinetor · 2021-09-04T10:51:27Z

In powershell function Install-ChocolateyInstallPackage trailing space is added to silentArgs:

choco/src/chocolatey.resources/helpers/functions/Install-ChocolateyInstallPackage.ps1

Line 359 in f68a242

    
           $env:ChocolateyExitCode = Start-ChocolateyProcessAsAdmin "$silentArgs $additionalInstallArgs" $fileFullPath -validExitCodes $validExitCodes -workingDirectory $workingDirectory

The problematic part is statement parameter of function Start-ChocolateyProcessAsAdmin which gets into it as "$silentArgs $additionalInstallArgs".

With Total Commander for example, passing silentArgs as '/AHDU c:\totalcmd' gets executed as '/AHDU c:\totalcmd ' (notice the trailing space). Then TC tries to create folder 'c:\totalcmd ' with space at the end and it fails.

To prove it is the problem and after looking into source code I used workaround solution for that package:

https://github.com/chocolatey-community/chocolatey-coreteampackages/blob/0a7332f2ad972850d197d5f7b706eb904c45b314/automatic/totalcommander/tools/chocolateyInstall.ps1#L8-L9

With that code package installation works which proves that statement parameter handling is problematic and can not be set in this case. This solution is however flagged by gallery validator.

To fix it, something like this will work:

$statements = $silentArgs 
if ($additionalInstallArgs) { $statements += " $additionalInstallArgs" } 
$env:ChocolateyExitCode = Start-ChocolateyProcessAsAdmin $statements $fileFullPath -validExitCodes $validExitCodes -workingDirectory $workingDirectory

The text was updated successfully, but these errors were encountered:

gep13 · 2021-09-04T11:29:37Z

@majkinetor I am not sure if I follow the problem that you are seeing here.

Can you please go back and update this issue using the issue template for reporting a bug? This includes a steps to reproduce section, which will help in understanding the problem that you are seeing.

Thanks

majkinetor · 2021-09-04T16:02:58Z

[moved]

majkinetor · 2021-09-04T16:05:41Z

[moved]

gep13 · 2021-09-06T18:17:13Z

@majkinetor thank you for the additional information, that helps to understand the problem.

Are you able to update the original issue with all that information so that it is in one place?

Thanks

majkinetor · 2021-09-06T18:18:28Z

Sure. Should I PR this change, since its obviously buggy for some class of tools ?

gep13 · 2021-09-06T18:23:21Z

Yip, happy to accept a PR which addresses this problem.

If we can put some tests around this as well, that would be great, but I don't think that would be possible.

majkinetor · 2021-09-06T18:27:41Z

Would testing the change by patching my local system be good enough ? I would change the Install-ChocolateyInstallPackage.ps1 and force install TC package with silentArgs set in the Windows Sandbox and if it doesn't fail, I will call it a day. I can provide videos or logs of pre/post installations.

Changed the initial description.

gep13 · 2021-09-06T18:56:00Z

In terms of "developer" testing, what you have suggested would be great, yes.

What I was thinking about is how to introduce automated tests, to ensure that this isn't regressed in future releases.

We don't have any infrastructure in this repository to add those style of tests, but we do have some internally, so we may have to look to getting a test(s) added there to cover this bug.

majkinetor · 2021-09-06T18:59:03Z

In the meantime, since package is highly used, should I trick the validator or can you (or somebody else) "promote" the TC package ?

https://community.chocolatey.org/packages/TotalCommander/10.0.0.20210903

gep13 · 2021-09-06T19:01:03Z

It is possible to provide an exemption (at the package version level) from package-validation, so if you reply to the moderation thread with a request for exemption, pointing at this issue, one of the moderators should be able to pick it up.

majkinetor · 2021-09-06T19:01:38Z

I will do that. Thanks.

majkinetor · 2021-09-06T19:02:41Z

Actually, I can't do that, since its chocolatey-community user behind the scenes.

TheCakeIsNaOH · 2021-10-06T16:11:04Z

I've added an exemption for the current version (10.0.0.20210903).

gobalius · 2022-12-17T15:16:00Z

@majkinetor did you ever create a PR of your proposed changes to choco/src/chocolatey.resources/helpers/functions/Install-ChocolateyInstallPackage.ps1?

majkinetor · 2022-12-17T15:38:35Z

@gobalius, No.

gobalius · 2022-12-18T18:15:49Z

@majkinetor would you consider creating the PR? I think it's a low risk (unaffected packages will still work) and high gain (packages like totalcommander will not need the workaround and will pass the automatic check) situation.

This prevents the addition of a trailing space after silent arguments in Install-ChocolateyInstallPackage when there are no additional arguments.

Fix a bug where concatenating silentArgs and additionalInstallArgs adds a trailing space at the end of silentArgs even when additionalInstallArgs is empty. Such trailing space can change the meaning of the last positional argument (that may contain spaces). Final arguments are now created as space joined string arrays filtered down to only non-empty items. Fix is applied to all installer types. Fixes: chocolatey#2345

blami · 2023-04-13T03:59:25Z

Hi, I am interested in fixing this (already raised a PR which I believe addresses the issue) as it blocks me from using Chocolatey internally with our installers (that behave similarly to Total Commander).

I looked at previous two PRs and they seem inactive and I feel that solution one of them proposes (simply merging arguments without space in between) would break some other installers while my solution (not adding any spaces before empty strings) should work in those cases (same as draft above).

blami · 2023-05-25T15:01:58Z

@gep13 I'm sorry to tag, not really sure what is process here to get the PR reviewed and eventually merged.

This prevents the addition of a trailing space after silent arguments in Install-ChocolateyInstallPackage when there are no additional arguments.

blami · 2023-07-09T13:48:08Z

@majkinetor I am sorry to tag, but is there anything I can do to have this PR reviewed and eventually merged? Is it just there's not enough reviewer bandwidth or is anything else missing on my side?

majkinetor · 2023-07-09T13:50:22Z

@blami , sorry, I am just a regular guy, not a choco employee. I don't think the outcome deserves the effort that is requested here.

blami · 2023-08-13T20:47:11Z

@gep13 is there anything from my side I can do to have this fix merged?

pauby · 2023-08-14T09:35:18Z

@blami We'll review the PR when we're ready to pull it in. The issue ha snot been assigned to a milestone as yet so it's not something we are currently working on.

blami · 2023-08-14T20:08:30Z

@pauby Thanks for clarification, since there were earlier replies from assignee, particularly this one:

Yip, happy to accept a PR which addresses this problem.

I thought this issue is already on radar. I understand the situation now and will wait until this makes it to some milestone.

This prevents the addition of a trailing space after silent arguments in Install-ChocolateyInstallPackage when there are no additional arguments.

Fix a bug where concatenating silentArgs and additionalInstallArgs adds a trailing space at the end of silentArgs even when additionalInstallArgs is empty. Such trailing space can change the meaning of the last positional argument (that may contain spaces). Final arguments are now created as space joined string arrays filtered down to only non-empty items. Fix is applied to all installer types. Fixes: chocolatey#2345

This adds a set of tests to validate that the spaces that may be passed in by the package are honoured, while not introducing erroneous spaces.

Fix a bug where concatenating silentArgs and additionalInstallArgs adds a trailing space at the end of silentArgs even when additionalInstallArgs is empty. Such trailing space can change the meaning of the last positional argument (that may contain spaces). Final arguments are now created as space joined string arrays filtered down to only non-empty items. Fix is applied to all installer types. It is expected not to manipulate the arguments themselves in case there are any spaces that the package maintainer has included.

This adds a set of tests to validate that the spaces that may be passed in by the package are honoured, while not introducing erroneous spaces.

Fix a bug where concatenating silentArgs and additionalInstallArgs adds a trailing space at the end of silentArgs even when additionalInstallArgs is empty. Such trailing space can change the meaning of the last positional argument (that may contain spaces). Final arguments are now created as space joined string arrays filtered down to only non-empty items. Fix is applied to all installer types. It is expected not to manipulate the arguments themselves in case there are any spaces that the package maintainer has included.

This adds a set of tests to validate that the spaces that may be passed in by the package are honoured, while not introducing erroneous spaces.

Fix a bug where concatenating silentArgs and additionalInstallArgs adds a trailing space at the end of silentArgs even when additionalInstallArgs is empty. Such trailing space can change the meaning of the last positional argument (that may contain spaces). Final arguments are now created as space joined string arrays filtered down to only non-empty items. Fix is applied to all installer types. It is expected not to manipulate the arguments themselves in case there are any spaces that the package maintainer has included.

This adds a set of tests to validate that the spaces that may be passed in by the package are honoured, while not introducing erroneous spaces.

(#2345) Avoid any trailing spaces after silentArgs

majkinetor mentioned this issue Sep 4, 2021

(TotalCommander) v10 is not published chocolatey-community/chocolatey-packages#1708

Closed

pauby added 0 - Backlog Bug labels Sep 7, 2021

pauby assigned gep13 Sep 7, 2021

TheCakeIsNaOH mentioned this issue Dec 24, 2022

(#2345) Do not add trailing space to arguments #2950

Closed

10 tasks

RedBaron2 mentioned this issue Dec 28, 2022

(#2345) This prevents unintended space(s) being passed #2951

Closed

10 tasks

blami mentioned this issue Apr 12, 2023

(#2345) Avoid any trailing spaces after silentArgs #3113

Merged

10 tasks

gep13 added this to the 2.3.0 milestone Dec 14, 2023

gep13 added 3 - Review and removed 0 - Backlog labels Apr 30, 2024

corbob added a commit to blami/choco that referenced this issue May 6, 2024

(chocolatey#2345) Add Pester tests to validate fix

eede2d4

This adds a set of tests to validate that the spaces that may be passed in by the package are honoured, while not introducing erroneous spaces.

corbob added a commit to blami/choco that referenced this issue May 6, 2024

(chocolatey#2345) Add Pester tests to validate fix

27b8874

This adds a set of tests to validate that the spaces that may be passed in by the package are honoured, while not introducing erroneous spaces.

corbob added a commit to blami/choco that referenced this issue May 7, 2024

(chocolatey#2345) Add Pester tests to validate fix

ccf9f93

This adds a set of tests to validate that the spaces that may be passed in by the package are honoured, while not introducing erroneous spaces.

corbob added a commit to blami/choco that referenced this issue May 7, 2024

(chocolatey#2345) Add Pester tests to validate fix

a1b036a

This adds a set of tests to validate that the spaces that may be passed in by the package are honoured, while not introducing erroneous spaces.

corbob added a commit to blami/choco that referenced this issue May 21, 2024

(chocolatey#2345) Add Pester tests to validate fix

9b24fc4

This adds a set of tests to validate that the spaces that may be passed in by the package are honoured, while not introducing erroneous spaces.

corbob added a commit to blami/choco that referenced this issue May 22, 2024

(chocolatey#2345) Add Pester tests to validate fix

7c51b2d

This adds a set of tests to validate that the spaces that may be passed in by the package are honoured, while not introducing erroneous spaces.

gep13 closed this as completed in #3113 May 23, 2024

gep13 added a commit that referenced this issue May 23, 2024

Merge pull request #3113 from blami/fix-2345

872fe03

(#2345) Avoid any trailing spaces after silentArgs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install-ChocolateyInstallPackage.ps1 adds trailing space to silent args #2345

Install-ChocolateyInstallPackage.ps1 adds trailing space to silent args #2345

majkinetor commented Sep 4, 2021 •

edited

gep13 commented Sep 4, 2021

majkinetor commented Sep 4, 2021 •

edited

majkinetor commented Sep 4, 2021 •

edited

gep13 commented Sep 6, 2021

majkinetor commented Sep 6, 2021 •

edited

gep13 commented Sep 6, 2021

majkinetor commented Sep 6, 2021 •

edited

gep13 commented Sep 6, 2021

majkinetor commented Sep 6, 2021

gep13 commented Sep 6, 2021

majkinetor commented Sep 6, 2021

majkinetor commented Sep 6, 2021

TheCakeIsNaOH commented Oct 6, 2021 •

edited

gobalius commented Dec 17, 2022 •

edited

majkinetor commented Dec 17, 2022

gobalius commented Dec 18, 2022

blami commented Apr 13, 2023 •

edited

blami commented May 25, 2023

blami commented Jul 9, 2023

majkinetor commented Jul 9, 2023

blami commented Aug 13, 2023

pauby commented Aug 14, 2023

blami commented Aug 14, 2023

Install-ChocolateyInstallPackage.ps1 adds trailing space to silent args #2345

Install-ChocolateyInstallPackage.ps1 adds trailing space to silent args #2345

Comments

majkinetor commented Sep 4, 2021 • edited

gep13 commented Sep 4, 2021

majkinetor commented Sep 4, 2021 • edited

majkinetor commented Sep 4, 2021 • edited

gep13 commented Sep 6, 2021

majkinetor commented Sep 6, 2021 • edited

gep13 commented Sep 6, 2021

majkinetor commented Sep 6, 2021 • edited

gep13 commented Sep 6, 2021

majkinetor commented Sep 6, 2021

gep13 commented Sep 6, 2021

majkinetor commented Sep 6, 2021

majkinetor commented Sep 6, 2021

TheCakeIsNaOH commented Oct 6, 2021 • edited

gobalius commented Dec 17, 2022 • edited

majkinetor commented Dec 17, 2022

gobalius commented Dec 18, 2022

blami commented Apr 13, 2023 • edited

blami commented May 25, 2023

blami commented Jul 9, 2023

majkinetor commented Jul 9, 2023

blami commented Aug 13, 2023

pauby commented Aug 14, 2023

blami commented Aug 14, 2023

majkinetor commented Sep 4, 2021 •

edited

majkinetor commented Sep 4, 2021 •

edited

majkinetor commented Sep 4, 2021 •

edited

majkinetor commented Sep 6, 2021 •

edited

majkinetor commented Sep 6, 2021 •

edited

TheCakeIsNaOH commented Oct 6, 2021 •

edited

gobalius commented Dec 17, 2022 •

edited

blami commented Apr 13, 2023 •

edited