From f35810ee5779343a0fc5ccdee60de579d4d85eb6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Feb 2026 16:42:15 +0000 Subject: [PATCH 1/5] Initial plan From 97e73f13b1f0a2ec819f2dcc08e173fb73555b72 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Feb 2026 16:49:14 +0000 Subject: [PATCH 2/5] Fix critical KQL injection vulnerabilities and security issues Co-authored-by: arthurclares <53261392+arthurclares@users.noreply.github.com> --- .../CostRecommendations-Prerequisites.ps1 | 106 +++++++++++++++++- src/wacoa/tools/CostRecommendations.ps1 | 74 ++++++++++-- 2 files changed, 168 insertions(+), 12 deletions(-) diff --git a/src/wacoa/tools/CostRecommendations-Prerequisites.ps1 b/src/wacoa/tools/CostRecommendations-Prerequisites.ps1 index db2032379..cdad6f34e 100644 --- a/src/wacoa/tools/CostRecommendations-Prerequisites.ps1 +++ b/src/wacoa/tools/CostRecommendations-Prerequisites.ps1 @@ -11,6 +11,45 @@ $IsRunningOnWindows = $PSVersionTable.Platform -eq 'Win32NT' Author: arclares #> +function Test-SubscriptionId { + param ( + [Parameter(Mandatory = $true)] + [string]$SubscriptionId + ) + + # Validate subscription ID is a valid GUID format + $guidRegex = '^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$' + return $SubscriptionId -match $guidRegex +} + +function Test-ResourceGroupName { + param ( + [Parameter(Mandatory = $true)] + [string]$ResourceGroupName + ) + + # Validate resource group name follows Azure naming conventions + # Alphanumeric, hyphens, underscores, periods, and parentheses only + # Between 1 and 90 characters + # Cannot end with a period + $rgNameRegex = '^[a-zA-Z0-9_\-\.\(\)]{1,90}$' + $endsWithPeriod = $ResourceGroupName -match '\.$' + + return ($ResourceGroupName -match $rgNameRegex) -and (-not $endsWithPeriod) +} + +function Test-ResourceType { + param ( + [Parameter(Mandatory = $true)] + [string]$ResourceType + ) + + # Validate resource type follows Azure format: Provider.Service/resourceType + # Can have multiple levels like Provider.Service/parentType/childType + $resourceTypeRegex = '^[A-Za-z0-9]+(\.[A-Za-z0-9]+)+/[A-Za-z0-9/]+$' + return $ResourceType -match $resourceTypeRegex +} + function Write-Log { param ( [Parameter(Mandatory = $true)] @@ -184,7 +223,53 @@ function Download-GitHubFolder { Write-Log -Message "Extracting zip file to: $Destination" -Level "INFO" try { - Expand-Archive -Path $zipFilePath -DestinationPath $Destination -Force -ErrorAction Stop + # First, extract to a temporary staging directory for validation + $stagingPath = Join-Path $tempPath "azure-resources-staging" + if (Test-Path -Path $stagingPath) { + Remove-Item -Path $stagingPath -Recurse -Force -ErrorAction Stop + } + New-Item -Path $stagingPath -ItemType Directory -ErrorAction Stop | Out-Null + + # Extract without -Force to staging directory first + Expand-Archive -Path $zipFilePath -DestinationPath $stagingPath -ErrorAction Stop + + # Validate extracted contents for path traversal attempts + $extractedFiles = Get-ChildItem -Path $stagingPath -Recurse -File + $hasPathTraversal = $false + foreach ($file in $extractedFiles) { + $relativePath = $file.FullName.Substring($stagingPath.Length).TrimStart([IO.Path]::DirectorySeparatorChar) + # Check for path traversal attempts (../ or ..\) + if ($relativePath -match '\.\.[/\\]' -or $relativePath -match '^\.\.') { + Write-Log -Message "Potential path traversal detected in zip file: $relativePath" -Level "ERROR" + $hasPathTraversal = $true + break + } + } + + if ($hasPathTraversal) { + Remove-Item -Path $stagingPath -Recurse -Force -ErrorAction SilentlyContinue + throw "Zip file contains potentially malicious path traversal entries." + } + + # If validation passed, copy to final destination + Write-Log -Message "Validation passed. Moving files to final destination." -Level "INFO" + Get-ChildItem -Path $stagingPath -Recurse | ForEach-Object { + $targetPath = Join-Path $Destination ($_.FullName.Substring($stagingPath.Length).TrimStart([IO.Path]::DirectorySeparatorChar)) + if ($_.PSIsContainer) { + if (-not (Test-Path -Path $targetPath)) { + New-Item -Path $targetPath -ItemType Directory -ErrorAction Stop | Out-Null + } + } else { + $targetDir = Split-Path -Parent $targetPath + if (-not (Test-Path -Path $targetDir)) { + New-Item -Path $targetDir -ItemType Directory -ErrorAction Stop | Out-Null + } + Copy-Item -Path $_.FullName -Destination $targetPath -Force -ErrorAction Stop + } + } + + # Clean up staging directory + Remove-Item -Path $stagingPath -Recurse -Force -ErrorAction SilentlyContinue } catch { Write-Log -Message "Failed to extract zip file: $_" -Level "ERROR" @@ -342,6 +427,13 @@ function Get-Scope { if ($rawScope -match "^/subscriptions/([^/]+)$") { $subId = $Matches[1] + + # Validate subscription ID format + if (-not (Test-SubscriptionId -SubscriptionId $subId)) { + Write-Log -Message "Invalid subscription ID format: $subId (must be a valid GUID)" -Level "ERROR" + continue + } + $allSubscriptionIds += $subId $individualScopes += @{ @@ -355,6 +447,18 @@ function Get-Scope { elseif ($rawScope -match "^/subscriptions/([^/]+)/resourceGroups/([^/]+)$") { $subId = $Matches[1] $rgName = $Matches[2] + + # Validate subscription ID and resource group name formats + if (-not (Test-SubscriptionId -SubscriptionId $subId)) { + Write-Log -Message "Invalid subscription ID format: $subId (must be a valid GUID)" -Level "ERROR" + continue + } + + if (-not (Test-ResourceGroupName -ResourceGroupName $rgName)) { + Write-Log -Message "Invalid resource group name format: $rgName" -Level "ERROR" + continue + } + $allSubscriptionIds += $subId $individualScopes += @{ diff --git a/src/wacoa/tools/CostRecommendations.ps1 b/src/wacoa/tools/CostRecommendations.ps1 index bd13f5929..742e9faf0 100644 --- a/src/wacoa/tools/CostRecommendations.ps1 +++ b/src/wacoa/tools/CostRecommendations.ps1 @@ -51,7 +51,7 @@ function Update-Scripts { Write-Host "Scripts updated successfully!" -ForegroundColor Green - $restart = Read-Host "Do you want to restart the script with the new version? (Yes/No or Y/N)" + $restart = (Read-Host "Do you want to restart the script with the new version? (Yes/No or Y/N)").Trim().ToLower() if ($restart -eq "yes" -or $restart -eq "y") { Write-Host "Restarting script..." -ForegroundColor Cyan & $mainScriptPath @@ -164,10 +164,15 @@ function Process-KQLFiles { foreach ($scope in $ScopeObject.IndividualScopes) { if ($scope.Type -eq "Subscription") { - $scopeConditions += "(SubAccountId == '$($scope.SubscriptionId)')" + # Escape single quotes in subscription ID to prevent KQL injection + $escapedSubId = $scope.SubscriptionId -replace "'", "''" + $scopeConditions += "(SubAccountId == '$escapedSubId')" } elseif ($scope.Type -eq "ResourceGroup") { - $scopeConditions += "(SubAccountId == '$($scope.SubscriptionId)' and x_ResourceGroupName == '$($scope.ResourceGroupName)')" + # Escape single quotes to prevent KQL injection + $escapedSubId = $scope.SubscriptionId -replace "'", "''" + $escapedRgName = $scope.ResourceGroupName -replace "'", "''" + $scopeConditions += "(SubAccountId == '$escapedSubId' and x_ResourceGroupName == '$escapedRgName')" } } @@ -366,7 +371,25 @@ function Manual-Validations { Write-Host "Unique resource types in YAML files: $($uniqueResourceTypes -join ', ')" -ForegroundColor Cyan Write-Log -Message "Unique resource types in YAML files: $($uniqueResourceTypes -join ', ')" -Level "INFO" - $resourceTypeConditions = $uniqueResourceTypes | ForEach-Object { "type == '$_'" } + # Validate and escape resource types to prevent KQL injection + $validResourceTypes = @() + foreach ($resourceType in $uniqueResourceTypes) { + if (Test-ResourceType -ResourceType $resourceType) { + $validResourceTypes += $resourceType + } else { + Write-Log -Message "Invalid resource type format detected and skipped: $resourceType" -Level "WARNING" + } + } + + if ($validResourceTypes.Count -eq 0) { + Write-Log -Message "No valid resource types found in YAML files." -Level "ERROR" + return @() + } + + $resourceTypeConditions = $validResourceTypes | ForEach-Object { + $escapedType = $_ -replace "'", "''" + "type == '$escapedType'" + } $resourceTypeFilter = $resourceTypeConditions -join ' or ' $query = "resources | where $resourceTypeFilter" @@ -376,10 +399,15 @@ function Manual-Validations { foreach ($scope in $ScopeObject.IndividualScopes) { if ($scope.Type -eq "Subscription") { - $scopeConditions += "(subscriptionId == '$($scope.SubscriptionId)')" + # Escape single quotes in subscription ID to prevent KQL injection + $escapedSubId = $scope.SubscriptionId -replace "'", "''" + $scopeConditions += "(subscriptionId == '$escapedSubId')" } elseif ($scope.Type -eq "ResourceGroup") { - $scopeConditions += "(subscriptionId == '$($scope.SubscriptionId)' and resourceGroup == '$($scope.ResourceGroupName)')" + # Escape single quotes to prevent KQL injection + $escapedSubId = $scope.SubscriptionId -replace "'", "''" + $escapedRgName = $scope.ResourceGroupName -replace "'", "''" + $scopeConditions += "(subscriptionId == '$escapedSubId' and resourceGroup == '$escapedRgName')" } } @@ -521,9 +549,33 @@ function Export-ResultsToExcel { if ($AssessmentFilePath) { try { - $assessmentData = Get-Content -Path $AssessmentFilePath | Select-Object -Skip 11 | ConvertFrom-Csv -ErrorAction Stop - $assessmentData | Export-Excel -Path $ExcelFilePath -WorksheetName 'Well-Architected Assessment' -AutoSize -TableName 'WAF Assessment' -TableStyle $script:settings.defaultSettings.excelTableStyle - Write-Log -Message "Added Well-Architected Cost Optimization assessment as a new tab in the Excel file." -Level "INFO" + # Read the CSV file content + $csvContent = Get-Content -Path $AssessmentFilePath -ErrorAction Stop + + # Find the line that contains CSV headers (usually contains comma-separated values) + $headerLineIndex = -1 + for ($i = 0; $i -lt $csvContent.Count; $i++) { + # Look for a line that looks like CSV headers (has commas and looks like header text) + if ($csvContent[$i] -match '^\w+,\w+' -or $csvContent[$i] -match '^"[^"]+","[^"]+"') { + $headerLineIndex = $i + break + } + } + + if ($headerLineIndex -eq -1) { + Write-Log -Message "Could not find CSV header line in assessment file. Trying default skip of 11 lines." -Level "WARNING" + $assessmentData = $csvContent | Select-Object -Skip 11 | ConvertFrom-Csv -ErrorAction Stop + } else { + Write-Log -Message "Found CSV header at line $($headerLineIndex + 1)." -Level "INFO" + $assessmentData = $csvContent | Select-Object -Skip $headerLineIndex | ConvertFrom-Csv -ErrorAction Stop + } + + if (-not $assessmentData -or $assessmentData.Count -eq 0) { + Write-Log -Message "No data found in assessment CSV file after parsing." -Level "WARNING" + } else { + $assessmentData | Export-Excel -Path $ExcelFilePath -WorksheetName 'Well-Architected Assessment' -AutoSize -TableName 'WAF Assessment' -TableStyle $script:settings.defaultSettings.excelTableStyle + Write-Log -Message "Added Well-Architected Cost Optimization assessment ($($assessmentData.Count) rows) as a new tab in the Excel file." -Level "INFO" + } } catch { Write-Log -Message "Failed to import or add the Well-Architected Cost Optimization assessment: $_" -Level "ERROR" @@ -620,7 +672,7 @@ function Start-CostRecommendations { Write-Log -Message "Folder '$tempDir' already exists. Skipping download." -Level "INFO" } - $includeAssessment = Read-Host "Would you like to include the results of a Well-Architected Cost Optimization assessment? (Yes/No or Y/N)" + $includeAssessment = (Read-Host "Would you like to include the results of a Well-Architected Cost Optimization assessment? (Yes/No or Y/N)").Trim().ToLower() $assessmentFilePath = $null if ($includeAssessment -eq "yes" -or $includeAssessment -eq "y") { $assessmentFilePath = Get-FilePath @@ -633,7 +685,7 @@ function Start-CostRecommendations { $ExcelFilePath = Join-Path $PSScriptRoot ('ACORL-File-' + (Get-Date -Format 'yyyy-MM-dd-HH-mm') + '.xlsx') - $runManualChecks = Read-Host "Would you like to run manual checks? (Yes/No or Y/N)" + $runManualChecks = (Read-Host "Would you like to run manual checks? (Yes/No or Y/N)").Trim().ToLower() if ($runManualChecks -eq "yes" -or $runManualChecks -eq "y") { Write-Log -Message "Running manual checks." -Level "INFO" Manual-Validations -BasePath $tempDir -ExcelFilePath $ExcelFilePath -ScopeObject $scope From 6cb5d04135183d16995c1ae7ef70d9c88b579e11 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Feb 2026 16:50:06 +0000 Subject: [PATCH 3/5] Fix empty CurrentVersion parameter error in settings loading Co-authored-by: arthurclares <53261392+arthurclares@users.noreply.github.com> --- .../CostRecommendations-Prerequisites.ps1 | 7 +++ src/wacoa/tools/CostRecommendations.ps1 | 46 +++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/wacoa/tools/CostRecommendations-Prerequisites.ps1 b/src/wacoa/tools/CostRecommendations-Prerequisites.ps1 index cdad6f34e..bb550528e 100644 --- a/src/wacoa/tools/CostRecommendations-Prerequisites.ps1 +++ b/src/wacoa/tools/CostRecommendations-Prerequisites.ps1 @@ -72,11 +72,18 @@ function Write-Log { function Check-ScriptVersion { param ( [Parameter(Mandatory = $true)] + [AllowEmptyString()] [string]$CurrentVersion, [Parameter(Mandatory = $true)] [string]$RemoteVersionUrl ) + + # Handle empty or null CurrentVersion + if ([string]::IsNullOrWhiteSpace($CurrentVersion)) { + Write-Log -Message "Current version is not set. Skipping version check." -Level "WARNING" + return + } Write-Log -Message "Current script version: $CurrentVersion" -Level "INFO" Write-Log -Message "Checking for latest version at: $RemoteVersionUrl" -Level "INFO" diff --git a/src/wacoa/tools/CostRecommendations.ps1 b/src/wacoa/tools/CostRecommendations.ps1 index 742e9faf0..45e81473e 100644 --- a/src/wacoa/tools/CostRecommendations.ps1 +++ b/src/wacoa/tools/CostRecommendations.ps1 @@ -96,6 +96,52 @@ function Load-Settings { try { $settings = Get-Content -Path $settingsPath -Raw | ConvertFrom-Json + + # Ensure scriptVersion exists and is not empty + if (-not $settings.scriptVersion -or [string]::IsNullOrWhiteSpace($settings.scriptVersion)) { + Write-Host "Warning: settings.json is missing scriptVersion. Using default version 2.0" -ForegroundColor Yellow + $settings | Add-Member -NotePropertyName scriptVersion -NotePropertyValue "2.0" -Force + } + + # Ensure repositoryUrls exists + if (-not $settings.repositoryUrls) { + Write-Host "Warning: settings.json is missing repositoryUrls. Adding defaults." -ForegroundColor Yellow + $settings | Add-Member -NotePropertyName repositoryUrls -NotePropertyValue @{ + mainScript = "https://raw.githubusercontent.com/microsoft/finops-toolkit/refs/heads/features/wacoascripts/src/wacoa/tools/CostRecommendations.ps1" + prerequisitesScript = "https://raw.githubusercontent.com/microsoft/finops-toolkit/refs/heads/features/wacoascripts/src/wacoa/tools/CostRecommendations-Prerequisites.ps1" + versionFile = "https://raw.githubusercontent.com/microsoft/finops-toolkit/refs/heads/features/wacoascripts/src/wacoa/tools/version.txt" + resourcesZip = "https://github.com/microsoft/finops-toolkit/raw/refs/heads/features/wacoascripts/src/wacoa/content/azure-resources.zip" + } -Force + } + + # Ensure paths exists + if (-not $settings.paths) { + Write-Host "Warning: settings.json is missing paths. Adding defaults." -ForegroundColor Yellow + $settings | Add-Member -NotePropertyName paths -NotePropertyValue @{ + tempDir = "Temp" + resourcesDir = "Temp/azure-resources" + cacheFile = "ScopeCache.txt" + } -Force + } + + # Ensure defaultSettings exists + if (-not $settings.defaultSettings) { + Write-Host "Warning: settings.json is missing defaultSettings. Adding defaults." -ForegroundColor Yellow + $settings | Add-Member -NotePropertyName defaultSettings -NotePropertyValue @{ + parallelThrottleLimit = 5 + excelTableStyle = "Light19" + logLevel = "INFO" + } -Force + } + + # Save the updated settings back to file if any changes were made + try { + $settings | ConvertTo-Json -Depth 10 | Set-Content -Path $settingsPath + } + catch { + Write-Host "Warning: Could not update settings.json: $_" -ForegroundColor Yellow + } + return $settings } catch { From 825cd490d0440a634920d1a266245db0801f9ae3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Feb 2026 16:51:14 +0000 Subject: [PATCH 4/5] Add security warnings for script downloads and improve logging Co-authored-by: arthurclares <53261392+arthurclares@users.noreply.github.com> --- src/wacoa/tools/CostRecommendations.ps1 | 28 ++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/src/wacoa/tools/CostRecommendations.ps1 b/src/wacoa/tools/CostRecommendations.ps1 index 45e81473e..a0ed9df4b 100644 --- a/src/wacoa/tools/CostRecommendations.ps1 +++ b/src/wacoa/tools/CostRecommendations.ps1 @@ -35,6 +35,11 @@ function Update-Scripts { $mainScriptDir = Split-Path -Parent $mainScriptPath $prerequisitesScriptPath = Join-Path $mainScriptDir "CostRecommendations-Prerequisites.ps1" + Write-Host "WARNING: Downloading and executing scripts from remote URLs." -ForegroundColor Yellow + Write-Host "WARNING: This operation trusts content from: $($MainScriptUrl -replace '/[^/]+$', '')" -ForegroundColor Yellow + Write-Host "WARNING: Ensure you trust this source before proceeding." -ForegroundColor Yellow + Write-Host "" + Write-Host "Downloading latest script versions..." -ForegroundColor Cyan $tempMainScriptPath = Join-Path $env:TEMP "CostRecommendations.ps1.new" @@ -43,6 +48,20 @@ function Update-Scripts { $tempPrerequisitesScriptPath = Join-Path $env:TEMP "CostRecommendations-Prerequisites.ps1.new" Invoke-WebRequest -Uri $PrerequisitesScriptUrl -OutFile $tempPrerequisitesScriptPath -ErrorAction Stop + # Verify downloaded files are not empty + $mainScriptSize = (Get-Item $tempMainScriptPath).Length + $prereqScriptSize = (Get-Item $tempPrerequisitesScriptPath).Length + + if ($mainScriptSize -lt 100 -or $prereqScriptSize -lt 100) { + Write-Host "ERROR: Downloaded scripts appear to be too small or empty. Update aborted." -ForegroundColor Red + Remove-Item -Path $tempMainScriptPath -Force -ErrorAction SilentlyContinue + Remove-Item -Path $tempPrerequisitesScriptPath -Force -ErrorAction SilentlyContinue + return $false + } + + Write-Host "Downloaded main script: $mainScriptSize bytes" -ForegroundColor Cyan + Write-Host "Downloaded prerequisites script: $prereqScriptSize bytes" -ForegroundColor Cyan + Copy-Item -Path $tempMainScriptPath -Destination $mainScriptPath -Force Copy-Item -Path $tempPrerequisitesScriptPath -Destination $prerequisitesScriptPath -Force @@ -244,7 +263,8 @@ function Process-KQLFiles { ) $timestamp = Get-Date -Format "yyyy-MM-dd HH:mm:ss" $logMessage = "$timestamp [$Level] [Thread $([System.Threading.Thread]::CurrentThread.ManagedThreadId)] $Message" - Add-Content -Path $PathToLogFile -Value $logMessage -ErrorAction SilentlyContinue + # Change to Continue so we see if there are write failures + Add-Content -Path $PathToLogFile -Value $logMessage -ErrorAction Continue } try { @@ -389,7 +409,8 @@ function Manual-Validations { ) $timestamp = Get-Date -Format "yyyy-MM-dd HH:mm:ss" $logMessage = "$timestamp [$Level] [Thread $([System.Threading.Thread]::CurrentThread.ManagedThreadId)] $Message" - Add-Content -Path $logFile -Value $logMessage -ErrorAction SilentlyContinue + # Change to Continue so we see if there are write failures + Add-Content -Path $logFile -Value $logMessage -ErrorAction Continue } try { @@ -496,7 +517,8 @@ function Manual-Validations { ) $timestamp = Get-Date -Format "yyyy-MM-dd HH:mm:ss" $logMessage = "$timestamp [$Level] [Thread $([System.Threading.Thread]::CurrentThread.ManagedThreadId)] $Message" - Add-Content -Path $logFile -Value $logMessage -ErrorAction SilentlyContinue + # Change to Continue so we see if there are write failures + Add-Content -Path $logFile -Value $logMessage -ErrorAction Continue } try { From d80bf133b1c2eafb686539faca82093167f98b0f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Feb 2026 16:54:17 +0000 Subject: [PATCH 5/5] Fix critical KQL injection vulnerabilities and runtime errors in WACOA collector Co-authored-by: arthurclares <53261392+arthurclares@users.noreply.github.com> --- src/wacoa/tools/CostRecommendations.ps1 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/wacoa/tools/CostRecommendations.ps1 b/src/wacoa/tools/CostRecommendations.ps1 index a0ed9df4b..84a0e0801 100644 --- a/src/wacoa/tools/CostRecommendations.ps1 +++ b/src/wacoa/tools/CostRecommendations.ps1 @@ -623,8 +623,9 @@ function Export-ResultsToExcel { # Find the line that contains CSV headers (usually contains comma-separated values) $headerLineIndex = -1 for ($i = 0; $i -lt $csvContent.Count; $i++) { - # Look for a line that looks like CSV headers (has commas and looks like header text) - if ($csvContent[$i] -match '^\w+,\w+' -or $csvContent[$i] -match '^"[^"]+","[^"]+"') { + # Look for a line that has commas and reasonable content (not just empty or whitespace) + $line = $csvContent[$i].Trim() + if ($line.Length -gt 0 -and $line.Contains(',') -and -not $line.StartsWith('#')) { $headerLineIndex = $i break }