fix(core): Fix "Invoke-ExternalCommand" quoting rules (#5945)

niheaven · web-flow · commit 1752050614f2 · 2024-05-11T14:49:49.000+08:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,7 @@
 - **autoupdate:** Copy `PSCustomObject`-type properties within `autoupdate` to prevent reference changes ([#5934](https://github.com/ScoopInstaller/Scoop/issues/5934))
 - **system:** Fix argument passing to `Split-PathLikeEnvVar()` in deprecated `strip_path()` ([#5937](https://github.com/ScoopInstaller/Scoop/issues/5937))
 - **scoop-cache:** Fix regression in 36026f18 ([#5944](https://github.com/ScoopInstaller/Scoop/issues/5944))
+- **core:** Fix "Invoke-ExternalCommand" quoting rules ([#5945](https://github.com/ScoopInstaller/Scoop/issues/5945))
 
 ## [v0.4.1](https://github.com/ScoopInstaller/Scoop/compare/v0.4.0...v0.4.1) - 2024-04-25
 
diff --git a/lib/core.ps1 b/lib/core.ps1
@@ -771,31 +771,35 @@ function Invoke-ExternalCommand {
         $Process.StartInfo.WindowStyle = [System.Diagnostics.ProcessWindowStyle]::Hidden
     }
     if ($ArgumentList.Length -gt 0) {
-        $ArgumentList = $ArgumentList | ForEach-Object { [regex]::Split($_.Replace('"', ''), '(?<=(?<![:\w])[/-]\w+) | (?=[/-])') }
-        # Use legacy argument escaping for commands having non-standard behavior
-        # with regard to argument passing. `msiexec` requires some args like
-        # `TARGETDIR="C:\Program Files"`, which is non-standard, therefore we
-        # treat it as a legacy command.
+        # Remove existing double quotes and split arguments
+        # '(?<=(?<![:\w])[/-]\w+) ' matches a space after a command line switch starting with a slash ('/') or a hyphen ('-')
+        # The inner item '(?<![:\w])[/-]' matches a slash ('/') or a hyphen ('-') not preceded by a colon (':') or a word character ('\w')
+        # so that it must be a command line switch, otherwise, it would be a path (e.g. 'C:/Program Files') or other word (e.g. 'some-arg')
+        # ' (?=[/-])' matches a space followed by a slash ('/') or a hyphen ('-'), i.e. the space before a command line switch
+        $ArgumentList = $ArgumentList.ForEach({ $_ -replace '"' -split '(?<=(?<![:\w])[/-]\w+) | (?=[/-])' })
+        # Use legacy argument escaping for commands having non-standard behavior with regard to argument passing.
+        # `msiexec` requires some args like `TARGETDIR="C:\Program Files"`, which is non-standard, therefore we treat it as a legacy command.
+        # NSIS installer's '/D' param may not work with the ArgumentList property, so we need to escape arguments manually.
         # ref-1: https://learn.microsoft.com/en-us/powershell/scripting/learn/experimental-features?view=powershell-7.4#psnativecommandargumentpassing
-        $LegacyCommand = $FilePath -match '^((cmd|cscript|find|sqlcmd|wscript|msiexec)(\.exe)?|.*\.(bat|cmd|js|vbs|wsf))$'
+        # ref-2: https://nsis.sourceforge.io/Docs/Chapter3.html
+        $LegacyCommand = $FilePath -match '^((cmd|cscript|find|sqlcmd|wscript|msiexec)(\.exe)?|.*\.(bat|cmd|js|vbs|wsf))$' -or
+            ($ArgumentList -match '^/S$|^/D=[A-Z]:[\\/].*$').Length -eq 2
         $SupportArgumentList = $Process.StartInfo.PSObject.Properties.Name -contains 'ArgumentList'
         if ((-not $LegacyCommand) -and $SupportArgumentList) {
             # ArgumentList is supported in PowerShell 6.1 and later (built on .NET Core 2.1+)
             # ref-1: https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.processstartinfo.argumentlist?view=net-6.0
             # ref-2: https://docs.microsoft.com/en-us/powershell/scripting/whats-new/differences-from-windows-powershell?view=powershell-7.2#net-framework-vs-net-core
-            $ArgumentList | ForEach-Object { $Process.StartInfo.ArgumentList.Add($_) }
+            $ArgumentList.ForEach({ $Process.StartInfo.ArgumentList.Add($_) })
         } else {
-            # escape arguments manually in lower versions, refer to https://docs.microsoft.com/en-us/previous-versions/17w5ykft(v=vs.85)
-            $escapedArgs = $ArgumentList | ForEach-Object {
-                # escape N consecutive backslash(es), which are followed by a double quote or at the end of the string, to 2N consecutive ones
-                $s = $_ -replace '(\\+)(""|$)', '$1$1$2'
-                # quote the path if it contains spaces and is not NSIS's '/D' argument
-                # ref: https://nsis.sourceforge.io/Docs/Chapter3.html
-                if ($s -match ' ' -and $s -notmatch '/D=[A-Z]:[\\/].*') {
-                    $s -replace '([A-Z]:[\\/].*)', '"$1"'
-                } else {
-                    $s
-                }
+            # Escape arguments manually in lower versions
+            $escapedArgs = switch -regex ($ArgumentList) {
+                # Quote paths starting with a drive letter
+                '(?<!/D=)[A-Z]:[\\/].*' { $_ -replace '([A-Z]:[\\/].*)', '"$1"'; continue }
+                # Do not quote paths if it is NSIS's '/D' argument
+                '/D=[A-Z]:[\\/].*' { $_; continue }
+                # Quote args with spaces
+                ' ' { "`"$_`""; continue }
+                default { $_; continue }
             }
             $Process.StartInfo.Arguments = $escapedArgs -join ' '
         }
diff --git a/lib/decompress.ps1 b/lib/decompress.ps1
@@ -156,7 +156,7 @@ function Expand-MsiArchive {
         $ArgList = @('x', $Path, "$DestinationPath\")
     } else {
         $MsiPath = 'msiexec.exe'
-        $ArgList = @('/a', "`"$Path`"", '/qn', "TARGETDIR=`"$DestinationPath\SourceDir`"")
+        $ArgList = @('/a', $Path, '/qn', "TARGETDIR=$DestinationPath\SourceDir")
     }
     $LogPath = "$(Split-Path $Path)\msi.log"
     if ($Switches) {
diff --git a/test/Scoop-Decompress.Tests.ps1 b/test/Scoop-Decompress.Tests.ps1
@@ -14,7 +14,7 @@ Describe 'Decompression function' -Tag 'Scoop', 'Windows', 'Decompress' {
 
         function test_extract($extract_fn, $from, $removal) {
             $to = (strip_ext $from) -replace '\.tar$', ''
-            & $extract_fn ($from -replace '/', '\') ($to -replace '/', '\') -Removal:$removal
+            & $extract_fn ($from -replace '/', '\') ($to -replace '/', '\') -Removal:$removal -ExtractDir $args[0]
             return $to
         }
 
@@ -25,7 +25,7 @@ Describe 'Decompression function' -Tag 'Scoop', 'Windows', 'Decompress' {
         }
         It 'Test cases should exist and hash should match' {
             $testcases | Should -Exist
-            (Get-FileHash -Path $testcases -Algorithm SHA256).Hash.ToLower() | Should -Be '791bfce192917a2ff225dcdd87d23ae5f720b20178d85e68e4b1b56139cf8e6a'
+            (Get-FileHash -Path $testcases -Algorithm SHA256).Hash.ToLower() | Should -Be '23a23a63e89ff95f5ef27f0cacf08055c2779cf41932266d8f509c2e200b8b63'
         }
         It 'Test cases should be extracted correctly' {
             { Microsoft.PowerShell.Archive\Expand-Archive -Path $testcases -DestinationPath $working_dir } | Should -Not -Throw
@@ -50,12 +50,31 @@ Describe 'Decompression function' -Tag 'Scoop', 'Windows', 'Decompress' {
             $test6_1 = "$working_dir\7ZipTest6.part01.rar"
             $test6_2 = "$working_dir\7ZipTest6.part02.rar"
             $test6_3 = "$working_dir\7ZipTest6.part03.rar"
+            $test7 = "$working_dir\NSISTest.exe"
+        }
+
+        AfterEach {
+            Remove-Item -Path $to -Recurse -Force
         }
 
         It 'extract normal compressed file' {
             $to = test_extract 'Expand-7zipArchive' $test1
             $to | Should -Exist
             "$to\empty" | Should -Exist
+            (Get-ChildItem $to).Count | Should -Be 3
+        }
+
+        It 'extract "extract_dir" correctly' {
+            $to = test_extract 'Expand-7zipArchive' $test1 $false 'tmp'
+            $to | Should -Exist
+            "$to\empty" | Should -Exist
+            (Get-ChildItem $to).Count | Should -Be 1
+        }
+
+        It 'extract "extract_dir" with spaces correctly' {
+            $to = test_extract 'Expand-7zipArchive' $test1 $false 'tmp 2'
+            $to | Should -Exist
+            "$to\empty" | Should -Exist
             (Get-ChildItem $to).Count | Should -Be 1
         }
 
@@ -94,21 +113,39 @@ Describe 'Decompression function' -Tag 'Scoop', 'Windows', 'Decompress' {
             (Get-ChildItem $to).Count | Should -Be 1
         }
 
+        It 'extract NSIS installer' {
+            $to = test_extract 'Expand-7zipArchive' $test7
+            $to | Should -Exist
+            "$to\empty" | Should -Exist
+            (Get-ChildItem $to).Count | Should -Be 1
+        }
+
+        It 'self-extract NSIS installer' {
+            $to = "$working_dir\NSIS Test"
+            $null = Invoke-ExternalCommand -FilePath $test7 -ArgumentList @('/S', '/NCRC', "/D=$to")
+            $to | Should -Exist
+            "$to\empty" | Should -Exist
+            (Get-ChildItem $to).Count | Should -Be 1
+        }
+
         It 'works with "-Removal" switch ($removal param)' {
             $test1 | Should -Exist
-            test_extract 'Expand-7zipArchive' $test1 $true
+            $to = test_extract 'Expand-7zipArchive' $test1 $true
+            $to | Should -Exist
             $test1 | Should -Not -Exist
             $test5_1 | Should -Exist
             $test5_2 | Should -Exist
             $test5_3 | Should -Exist
-            test_extract 'Expand-7zipArchive' $test5_1 $true
+            $to = test_extract 'Expand-7zipArchive' $test5_1 $true
+            $to | Should -Exist
             $test5_1 | Should -Not -Exist
             $test5_2 | Should -Not -Exist
             $test5_3 | Should -Not -Exist
             $test6_1 | Should -Exist
             $test6_2 | Should -Exist
             $test6_3 | Should -Exist
-            test_extract 'Expand-7zipArchive' $test6_1 $true
+            $to = test_extract 'Expand-7zipArchive' $test6_1 $true
+            $to | Should -Exist
             $test6_1 | Should -Not -Exist
             $test6_2 | Should -Not -Exist
             $test6_3 | Should -Not -Exist
diff --git a/test/Scoop-TestLib.ps1 b/test/Scoop-TestLib.ps1
@@ -1,13 +1,13 @@
 # copies fixtures to a working directory
 function setup_working($name) {
-    $fixtures = "$PSScriptRoot/fixtures/$name"
+    $fixtures = "$PSScriptRoot\fixtures\$name"
     if (!(Test-Path $fixtures)) {
         Write-Host "couldn't find fixtures for $name at $fixtures" -f red
         exit 1
     }
 
     # reset working dir
-    $working_dir = "$([IO.Path]::GetTempPath())ScoopTestFixtures/$name"
+    $working_dir = "$([IO.Path]::GetTempPath())ScoopTestFixtures\$name"
 
     if (Test-Path $working_dir) {
         Remove-Item -Recurse -Force $working_dir
diff --git a/test/fixtures/decompress/TestCases.zip b/test/fixtures/decompress/TestCases.zip

Original file line number	Diff line number	Diff line change
`@@ -156,7 +156,7 @@ function Expand-MsiArchive {`
`156`	`156`	`$ArgList = @('x', $Path, "$DestinationPath\")`
`157`	`157`	`} else {`
`158`	`158`	`$MsiPath = 'msiexec.exe'`
`159`		- $ArgList = @('/a', "`"$Path`"", '/qn', "TARGETDIR=`"$DestinationPath\SourceDir`"")
	`159`	`+ $ArgList = @('/a', $Path, '/qn', "TARGETDIR=$DestinationPath\SourceDir")`
`160`	`160`	`}`
`161`	`161`	`$LogPath = "$(Split-Path $Path)\msi.log"`
`162`	`162`	`if ($Switches) {`