Prevent security exploit in SqlAzureDacpacDeploymentV1 and SqlDacpacDeploymentOnMachineGroupV0 where Invoke-Expression is used

Tasks/SqlAzureDacpacDeploymentV1/DeploySqlAzure.ps1

@@ -34,6 +34,7 @@ $ErrorActionPreference = 'Stop'
 # Initialize Rest API Helpers.
 Import-Module $PSScriptRoot\ps_modules\VstsAzureHelpers_
 Import-Module $PSScriptRoot\ps_modules\VstsAzureRestHelpers_
+Import-Module $PSScriptRoot\ps_modules\Sanitizer
 
 # Import the loc strings.
 Import-VstsLocStrings -LiteralPath $PSScriptRoot/Task.json
@@ -66,6 +67,19 @@ try {
 
     Import-Sqlps
 
+
+    if (-not [string]::IsNullOrWhiteSpace($sqlcmdAdditionalArguments)) {
+        $sqlcmdAdditionalArguments = Get-SanitizedSqlArguments `
+            -InputArgs $sqlcmdAdditionalArguments `
+            -TaskName "SqlAzureDacpacDeploymentV1"
+    }
+
+    if (-not [string]::IsNullOrWhiteSpace($sqlcmdInlineAdditionalArguments)) {
+        $sqlcmdInlineAdditionalArguments = Get-SanitizedSqlArguments `
+            -InputArgs $sqlcmdInlineAdditionalArguments `
+            -TaskName "SqlAzureDacpacDeploymentV1"
+    }
+
     # Detect authentication type for YAML flow
     if (-not $authenticationType) {
         $authenticationType = Detect-AuthenticationType -serverName $serverName -databaseName $databaseName -sqlUsername $sqlUsername -sqlPassword $sqlPassword -aadSqlUsername $aadSqlUserName -aadSqlPassword $aadSqlPassword -connectionString $connectionString
@@ -216,10 +230,10 @@ catch [Exception] {
     }
 
     if ($deploymentAction -eq "DriftReport" -and $LASTEXITCODE -eq 1) {
-        $errorMessage += Get-VstsLocString -Key "SAD_DriftReportWarning"
+        $errorMessage += " " + (Get-VstsLocString -Key "SAD_DriftReportWarning")
     }
 
-    $errorMessage += Get-VstsLocString -Key "SAD_TroubleshootingLink"
+    $errorMessage += " " + (Get-VstsLocString -Key "SAD_TroubleshootingLink")
 
     throw $errorMessage
 }

Tasks/SqlAzureDacpacDeploymentV1/Tests/L0SecurityFunctions.ps1

@@ -0,0 +1,129 @@
+# Unit tests for SQL argument sanitization security functions
+[CmdletBinding()]
+param()
+
+# Import test helpers
+. $PSScriptRoot\MockVariable.ps1
+
+# Import the functions under test
+. $PSScriptRoot\..\Utility.ps1
+
+Describe "Execute-Command Security Tests" {
+    Context "AST Parser Validation" {
+        It "Should use token .Value instead of .Text" {
+            # This test verifies the fix for Ivan's concern about quote handling
+            # We check that the code uses $_.Value which removes quotes,
+            # rather than $_.Text which keeps them (causing double-quoting)
+
+            $utilityContent = Get-Content "$PSScriptRoot\..\Utility.ps1" -Raw
+            $utilityContent -match 'ForEach-Object\s*\{\s*\$_\.Value\s*\}' | Should Be $true
+            $utilityContent -match 'ForEach-Object\s*\{\s*\$_\.Text\s*\}' | Should Be $false
+        }
+
+        It "Should validate parse errors before execution" {
+            $utilityContent = Get-Content "$PSScriptRoot\..\Utility.ps1" -Raw
+            # Should check parseErrors and throw if any exist
+            $utilityContent -match 'if\s*\(\s*\$parseErrors' | Should Be $true
+            $utilityContent -match 'throw.*Invalid sqlpackage argument' | Should Be $true
+        }
+
+        It "Should use array-based argument passing, not Invoke-Expression" {
+            $utilityContent = Get-Content "$PSScriptRoot\..\Utility.ps1" -Raw
+            # Should NOT use Invoke-Expression in Execute-Command
+            $executeCommandSection = $utilityContent -split 'function Execute-Command'
+            if ($executeCommandSection.Count -gt 1) {
+                $functionBody = $executeCommandSection[1] -split 'function ', 2 | Select-Object -First 1
+                $functionBody -match 'Invoke-Expression' | Should Be $false
+            }
+        }
+    }
+}
+
+Describe "Should-UseSanitizedArguments Dual-Gating Tests" {
+    Context "Backward Compatibility" {
+        It "Should fail-open when VstsTaskSdk unavailable" {
+            # When Get-VstsPipelineFeature cmdlet doesn't exist,
+            # should return false (backward compatibility)
+            $utilityContent = Get-Content "$PSScriptRoot\..\Utility.ps1" -Raw
+            $utilityContent -match 'Get-Command.*Get-VstsPipelineFeature.*SilentlyContinue' | Should Be $true
+        }
+    }
+}
+
+Describe "Get-SanitizedSqlArguments Security Tests" {
+    Context "Fail-Closed Behavior" {
+        It "Should validate sanitizer returns an array" {
+            $utilityContent = Get-Content "$PSScriptRoot\..\Utility.ps1" -Raw
+            # Should check that result is an array type
+            $utilityContent -match '\$sanitizedArray -isnot \[Array\]' | Should Be $true
+            $utilityContent -match 'throw.*unexpected type' | Should Be $true
+        }
+
+        It "Should detect empty array (all input blocked)" {
+            $utilityContent = Get-Content "$PSScriptRoot\..\Utility.ps1" -Raw
+            # Should check for empty array
+            $utilityContent -match '\$sanitizedArray\.Count -eq 0' | Should Be $true
+            $utilityContent -match 'throw.*empty array.*blocked' | Should Be $true
+        }
+
+        It "Should throw on sanitizer failure" {
+            $utilityContent = Get-Content "$PSScriptRoot\..\Utility.ps1" -Raw
+            # Should have try-catch that throws on error
+            $getSanitizedSection = $utilityContent -split 'function Get-SanitizedSqlArguments'
+            if ($getSanitizedSection.Count -gt 1) {
+                $functionBody = $getSanitizedSection[1] -split 'function ', 2 | Select-Object -First 1
+                $functionBody -match 'catch\s*\{' | Should Be $true
+                $functionBody -match 'SECURITY ERROR.*Failed to sanitize' | Should Be $true
+                $functionBody -match 'throw\s*\$errorMessage' | Should Be $true
+            }
+        }
+    }
+
+    Context "Telemetry" {
+        It "Should emit telemetry when sanitization modifies input" {
+            $utilityContent = Get-Content "$PSScriptRoot\..\Utility.ps1" -Raw
+            $utilityContent -match 'if\s*\(\s*\$sanitizedString -ne \$InputArgs\s*\)' | Should Be $true
+            $utilityContent -match 'telemetry\.publish.*SqlArgumentSanitization' | Should Be $true
+        }
+    }
+}
+
+Describe "Publish-FeatureFlagCheckTelemetry Tests" {
+    Context "Telemetry Format" {
+        It "Should emit telemetry with correct area and feature" {
+            $utilityContent = Get-Content "$PSScriptRoot\..\Utility.ps1" -Raw
+            $utilityContent -match 'telemetry\.publish area=TaskHub;feature=SqlArgumentSanitizationCheck' | Should Be $true
+        }
+
+        It "Should include checkType in telemetry data" {
+            $utilityContent = Get-Content "$PSScriptRoot\..\Utility.ps1" -Raw
+            # Function should accept CheckType parameter and include in data
+            $telemetrySection = $utilityContent -split 'function Publish-FeatureFlagCheckTelemetry'
+            if ($telemetrySection.Count -gt 1) {
+                $functionBody = $telemetrySection[1] -split 'function ', 2 | Select-Object -First 1
+                $functionBody -match 'checkType\s*=' | Should Be $true
+            }
+        }
+    }
+}
+
+Describe "Comment Restoration Tests" {
+    Context "Original Comments" {
+        It "Should have 'Initialize Rest API Helpers' comment in DeploySqlAzure.ps1" {
+            $deployContent = Get-Content "$PSScriptRoot\..\DeploySqlAzure.ps1" -Raw
+            $deployContent -match '# Initialize Rest API Helpers' | Should Be $true
+        }
+
+        It "Should have 'Import the loc strings' comment in DeploySqlAzure.ps1" {
+            $deployContent = Get-Content "$PSScriptRoot\..\DeploySqlAzure.ps1" -Raw
+            $deployContent -match '# Import the loc strings' | Should Be $true
+        }
+
+        It "Should have function comment in Utility.ps1" {
+            $utilityContent = Get-Content "$PSScriptRoot\..\Utility.ps1" -Raw
+            $utilityContent -match '# Function to import SqlPS module' | Should Be $true
+        }
+    }
+}
+
+Write-Host "Security function tests completed"

Tasks/SqlAzureDacpacDeploymentV1/Utility.ps1

@@ -283,8 +283,32 @@ function Execute-Command {
     )
 
     $ErrorActionPreference = 'Continue'
-    Invoke-Expression "& '$FileName' --% $Arguments" 2>&1 -ErrorVariable errors | ForEach-Object {
+
+    # Parse arguments using PowerShell AST Parser for safe tokenization
+    # Prepend placeholder command name since parser expects complete command structure
+    $tokens = $null
+    $parseErrors = $null
+    [void][System.Management.Automation.Language.Parser]::ParseInput(
+        "cmd $Arguments", 
+        [ref]$tokens, 
+        [ref]$parseErrors
+    )
+
+    if ($parseErrors -and $parseErrors.Count -gt 0) {
+        $errorMessages = $parseErrors | ForEach-Object { $_.Message }
+        Write-Error "Failed to parse sqlpackage arguments: $($errorMessages -join '; ')"
+        throw "Invalid sqlpackage argument syntax. Arguments must be properly quoted."
+    }
+
+    $argArray = @($tokens | 
+        Where-Object { $_.Kind -ne 'EndOfInput' } | 
+        Select-Object -Skip 1 | 
+        ForEach-Object { $_.Value })
+
+    $errors = @()
+    & $FileName $argArray 2>&1 | ForEach-Object {
         if ($_ -is [System.Management.Automation.ErrorRecord]) {
+            $errors += $_
             Write-Error $_
         }
         else {
@@ -295,9 +319,176 @@ function Execute-Command {
     foreach ($errorMsg in $errors) {
         Write-Error $errorMsg
     }
+
     $ErrorActionPreference = 'Stop'
     if ($LASTEXITCODE -ne 0) {
-        throw  (Get-VstsLocString -Key "SAD_AzureSQLDacpacTaskFailed" -ArgumentList $LASTEXITCODE)
+        throw (Get-VstsLocString -Key "SAD_AzureSQLDacpacTaskFailed" -ArgumentList $LASTEXITCODE)
+    }
+}
+
+function Publish-FeatureFlagCheckTelemetry {
+    param(
+        [Parameter(Mandatory=$true)]
+        [string]$CheckType,
+
+        [Parameter(Mandatory=$false)]
+        [hashtable]$AdditionalData = @{}
+    )
+
+    $telemetryData = @{
+        checkType = $CheckType
+        agentVersion = $env:AGENT_VERSION
+    }
+
+    foreach ($key in $AdditionalData.Keys) {
+        $telemetryData[$key] = $AdditionalData[$key]
+    }
+
+    $telemetryJson = $telemetryData | ConvertTo-Json -Compress
+    Write-Host "##vso[telemetry.publish area=TaskHub;feature=SqlArgumentSanitizationCheck]$telemetryJson"
+}
+
+function Should-UseSanitizedArguments {
+    try {
+        $orgLevelEnabled = Get-SanitizerCallStatus
+    }
+    catch {
+        Write-Warning "Failed to check org-level sanitizer status: $_. Proceeding without sanitization."
+        Publish-FeatureFlagCheckTelemetry -CheckType "OrgLevelFeatureFlag" -AdditionalData @{
+            checkFailed = $true
+            errorMessage = $_.Exception.Message
+        }
+        return $false
+    }
+
+    if (-not $orgLevelEnabled) {
+        Write-Verbose "SQL argument sanitization disabled: Org-level feature flag not enabled"
+        return $false
+    }
+
+    $hasFeatureFlagCmdlet = Get-Command -Name 'Get-VstsPipelineFeature' -ErrorAction SilentlyContinue
+
+    if (-not $hasFeatureFlagCmdlet) {
+        Write-Warning "Get-VstsPipelineFeature cmdlet not found. Attempting to import VstsTaskSdk module..."
+
+        $vstsTaskSdkPath = Join-Path $PSScriptRoot "ps_modules\VstsTaskSdk"
+        try {
+            if (Test-Path $vstsTaskSdkPath) {
+                Import-Module $vstsTaskSdkPath -ErrorAction Stop
+                Write-Verbose "Successfully imported VstsTaskSdk from: $vstsTaskSdkPath"
+            }
+            else {
+                Import-Module VstsTaskSdk -ErrorAction Stop
+                Write-Verbose "Successfully imported VstsTaskSdk from module path"
+            }
+
+            $hasFeatureFlagCmdlet = Get-Command -Name 'Get-VstsPipelineFeature' -ErrorAction SilentlyContinue
+        }
+        catch {
+            Write-Warning "Failed to import VstsTaskSdk module: $_"
+        }
+
+        if (-not $hasFeatureFlagCmdlet) {
+            Write-Warning "Get-VstsPipelineFeature cmdlet unavailable (old agent or missing module). Proceeding without sanitization."
+            Publish-FeatureFlagCheckTelemetry -CheckType "PipelineLevelFeatureFlag" -AdditionalData @{
+                cmdletMissing = $true
+                importAttempted = $true
+                errorMessage = if ($Error[0]) { $Error[0].Exception.Message } else { "Unknown" }
+                attemptedPath = $vstsTaskSdkPath
+                psModulePath = $env:PSModulePath
+            }
+            return $false
+        }
+    }
+
+    try {
+        $pipelineLevelEnabled = Get-VstsPipelineFeature -FeatureName "EnableSqlAdditionalArgumentsSanitization" -ErrorAction Stop
+    }
+    catch {
+        Write-Warning "Pipeline-level feature flag check failed: $_. Proceeding without sanitization."
+        Publish-FeatureFlagCheckTelemetry -CheckType "PipelineLevelFeatureFlag" -AdditionalData @{
+            checkFailed = $true
+            errorMessage = $_.Exception.Message
+        }
+        return $false
+    }
+
+    if (-not $pipelineLevelEnabled) {
+        Write-Verbose "SQL argument sanitization disabled: Pipeline-level feature flag not enabled"
+        return $false
+    }
+
+    Write-Verbose "SQL argument sanitization ENABLED (both feature flags are active)"
+    return $true
+}
+
+function Get-SanitizedSqlArguments {
+    param(
+        [Parameter(Mandatory=$false)]
+        [ValidateNotNull()]
+        [string]$InputArgs = "",
+
+        [Parameter(Mandatory=$true)]
+        [ValidateNotNullOrEmpty()]
+        [string]$TaskName
+    )
+
+    if ([string]::IsNullOrWhiteSpace($InputArgs)) {
+        return ""
+    }
+
+    if (-not (Should-UseSanitizedArguments)) {
+        Write-Verbose "Returning unsanitized arguments (feature flags disabled)"
+        return $InputArgs
+    }
+
+    try {
+        $sanitizedArray = Protect-ScriptArguments -InputArgs $InputArgs -TaskName $TaskName
+
+        if ($null -eq $sanitizedArray) {
+            throw "Protect-ScriptArguments returned null instead of string array"
+        }
+        if ($sanitizedArray -isnot [Array]) {
+            throw "Protect-ScriptArguments returned unexpected type: $($sanitizedArray.GetType().FullName)"
+        }
+        if ($sanitizedArray.Count -eq 0) {
+            throw "Protect-ScriptArguments returned empty array - all input was blocked"
+        }
+
+        $sanitizedString = $sanitizedArray -join " "
+
+        if ($sanitizedString -ne $InputArgs) {
+            Write-Warning "SQL arguments were sanitized. Potentially dangerous characters were removed."
+
+            $telemetryData = @{
+                taskName = $TaskName
+                sanitizationApplied = $true
+                inputLength = $InputArgs.Length
+                outputLength = $sanitizedString.Length
+                charactersRemoved = $InputArgs.Length - $sanitizedString.Length
+            }
+            $telemetryJson = $telemetryData | ConvertTo-Json -Compress
+            Write-Host "##vso[telemetry.publish area=TaskHub;feature=SqlArgumentSanitization]$telemetryJson"
+        }
+        else {
+            Write-Verbose "SQL arguments passed sanitization without modification"
+        }
+
+        return $sanitizedString
+    }
+    catch {
+        $errorMessage = "SECURITY ERROR: Failed to sanitize SQL arguments. Task cannot proceed safely. Error: $_"
+        Write-Error $errorMessage
+
+        $telemetryData = @{
+            taskName = $TaskName
+            sanitizationFailed = $true
+            errorMessage = $_.Exception.Message
+        }
+        $telemetryJson = $telemetryData | ConvertTo-Json -Compress
+        Write-Host "##vso[telemetry.publish area=TaskHub;feature=SqlArgumentSanitization]$telemetryJson"
+
+        throw $errorMessage
     }
 }
 

Tasks/SqlAzureDacpacDeploymentV1/make.json

@@ -11,6 +11,10 @@
         {
             "module": "../Common/TlsHelper_",
             "type": "ps"
+        },
+        {
+            "module": "../Common/Sanitizer",
+            "type": "ps"
         }
     ],
     "externals": {