Open
Conversation
What changed: 1. Added a $varsToPass hashtable capturing all NinjaRMM variables 2. Builds a variable declaration block that gets prepended to the PS7 script 3. Uses a marker (# === MAIN SCRIPT START ===) to identify where the real script begins 4. Writes a temp script file with variables injected, runs it in PS7, then cleans up 5. Properly passes through the exit code back to NinjaRMM
Contributor
📝 WalkthroughWalkthroughReworked startup flow to bootstrap PowerShell 7 x64 with automatic detection, download, and installation if needed. Added script relaunching mechanism that preserves variables. Converted hard-coded folder name references to lowercase variables, expanded logging with transcript and secret key redaction, and refined repository creation logic for S3 and local types. Changes
Sequence DiagramsequenceDiagram
participant User as User/Script Start
participant PS5 as PowerShell 5 (Initial)
participant Check as PS7 Check
participant DL as Downloader
participant Install as PS7 Installer
participant PS7 as PowerShell 7 x64
participant Veeam as Veeam Module
User->>PS5: Execute Script
PS5->>Check: Check if PS7 x64 exists?
alt PS7 x64 Missing
Check-->>DL: PS7 not found
DL->>DL: Download PS7 x64
DL->>Install: Silent Install
Install->>Install: Install PS7 x64
Install-->>PS5: Installation Complete
PS5->>PS5: Capture Variables & Script Block
PS5->>PS7: Relaunch with Temp Script
else PS7 x64 Exists
Check-->>PS7: PS7 Found
end
PS7->>PS7: Start Transcript Logging
PS7->>Veeam: Load Veeam Modules
Veeam-->>PS7: Modules Loaded
PS7->>PS7: Create Repository (S3/Local)
PS7->>PS7: Log Config & Exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bdr-veeam/veeam-add-backup-repo.ps1 (1)
142-162:$repositoryis never assigned in S3 creation block.The
Add-VBRAmazonS3CompatibleRepositoryresult is not captured, so line 162 outputs nothing.if ($veeamVersion -ge $requiredVersion) { - Add-VBRAmazonS3CompatibleRepository ` + $repository = Add-VBRAmazonS3CompatibleRepository ` -AmazonS3Folder $folder ` -Connection $connect ` -Name "S3 $folderName" ` ... } else { - Add-VBRAmazonS3CompatibleRepository ` + $repository = Add-VBRAmazonS3CompatibleRepository ` -AmazonS3Folder $folder ` ... }
🤖 Fix all issues with AI agents
In `@bdr-veeam/veeam-add-backup-repo.ps1`:
- Around line 49-63: The here-string construction in $scriptBlock naively
injects variables (e.g., $description, $bucketName, $folderName, $moveBackups)
using single quotes which breaks when values contain apostrophes and also writes
$secretKey in plain text to a temp file; fix by sanitizing/escaping single
quotes for each interpolated value before building $scriptBlock (e.g., replace '
with '\'' or use a serialization method that preserves content), and stop
writing the raw $secretKey to disk—pass secrets as a SecureString or via an
environment variable/secure credential object or restrict temp file permissions
and delete immediately if disk use is unavoidable; update the code that
constructs $scriptBlock and the logic that writes/reads the temp file containing
$secretKey accordingly.
- Around line 1-3: Add a top-of-file comment block documenting all required RMM
variables: list each variable referenced later in the script (the variables used
around lines 50-61 such as those controlling repository type, bucket/name,
endpoint, access key, secret, region and any $RMM_* or $env:* variables the
script reads) with a short description and whether it’s required; insert this
block above the existing header comments so the script follows the coding
guideline requiring RMM variable declarations.
🧹 Nitpick comments (3)
bdr-veeam/veeam-add-backup-repo.ps1 (3)
168-168: UseGet-CimInstanceinstead of deprecatedGet-WmiObject.Since this script explicitly targets PowerShell 7, use the modern CIM cmdlet:
- $drives = Get-WmiObject -Class Win32_LogicalDisk | Where-Object { $_.DriveType -eq 3 } + $drives = Get-CimInstance -ClassName Win32_LogicalDisk | Where-Object { $_.DriveType -eq 3 }
Get-WmiObjectis deprecated and not included by default in PS7 (requires Windows compatibility module).
186-186: Inconsistent$localRepositoryhandling.Line 186 assigns
$localRepositoryinside the ForEach-Object loop, but line 194 re-queries it from Veeam. This creates two issues:
- If multiple drives exist, line 186 only retains the last repository created
- Line 194's query (
Where -Property Name -like "Local*") may match unrelated repositoriesConsider using the repository created in this script directly:
if ($moveBackups -eq 1){ $backups = Get-VBRBackup | Where-Object -Property TypeToString -ne "Backup Copy" - $localRepository = Get-VBRBackupRepository | Where -Property Name -like "Local*" | Select -First 1 | Select -Expand Name + # Use repository created earlier, or query if not available + if (-not $localRepository) { + $localRepository = Get-VBRBackupRepository | Where-Object -Property Name -like "Local $folderName" | Select-Object -First 1 + } + $localRepositoryName = $localRepository.Name - Write-Host "moving all backups to $localRepository" + Write-Host "moving all backups to $localRepositoryName" $backups | ForEach-Object { - Move-VBRBackup -Repository $localRepository -Backup $_ -RunAsync -Force + Move-VBRBackup -Repository $localRepositoryName -Backup $_ -RunAsync -Force } }Also applies to: 192-200
203-209: Type mismatch risk for$localRepositoryin move-listed-backups block.
$localRepositorymay be either a string (from line 194) or a repository object (from line 186), depending on which code path executed. The-Repositoryparameter behavior may differ.Ensure consistent typing by always using the repository name:
if ($moveListedBackups){ - Write-Host "Moving $moveListedBackups to $localRepository." + $repoName = if ($localRepository -is [string]) { $localRepository } else { $localRepository.Name } + Write-Host "Moving $moveListedBackups to $repoName." $moveListedBackups | ForEach-Object { - Move-VBRBackup -Repository $localRepository -Backup $_ -RunAsync + Move-VBRBackup -Repository $repoName -Backup $_ -RunAsync } }
Comment on lines
1
to
3
| ## Please note this script can only support the following backup repository types ## | ||
| # S3 Compabitble & Local. Both are forced. | ||
| # S3 Compatible & Local. Both are forced. | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing RMM Variable Declaration Section per coding guidelines.
The script should begin with a comment block listing all required RMM variables. Based on the variables used (lines 50-61), add the documentation block:
## Please note this script can only support the following backup repository types ##
# S3 Compatible & Local. Both are forced.
+## PLEASE COMMENT YOUR VARIABLES DIRECTLY BELOW HERE IF YOU'RE RUNNING FROM A RMM
+## $repositoryType - 1 for S3, 2 for Local, 3 for Both
+## $moveBackups - 1 to move all backups to local repository
+## $description - Description for audit trail
+## $immutabilityPeriod - Immutability period for S3 repository
+## $accessKey - S3 access key
+## $secretKey - S3 secret key (sensitive)
+## $endpoint - S3 endpoint URL
+## $regionId - S3 region ID
+## $bucketName - S3 bucket name
+## $driveLetters - Drive letters for WinLocal repos
+## $folderName - Folder name for repository
+## $moveListedBackups - Specific backups to move
+As per coding guidelines, all PowerShell scripts must document required RMM variables in the top comment block.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## Please note this script can only support the following backup repository types ## | |
| # S3 Compabitble & Local. Both are forced. | |
| # S3 Compatible & Local. Both are forced. | |
| ## Please note this script can only support the following backup repository types ## | |
| # S3 Compatible & Local. Both are forced. | |
| ## PLEASE COMMENT YOUR VARIABLES DIRECTLY BELOW HERE IF YOU'RE RUNNING FROM A RMM | |
| ## $repositoryType - 1 for S3, 2 for Local, 3 for Both | |
| ## $moveBackups - 1 to move all backups to local repository | |
| ## $description - Description for audit trail | |
| ## $immutabilityPeriod - Immutability period for S3 repository | |
| ## $accessKey - S3 access key | |
| ## $secretKey - S3 secret key (sensitive) | |
| ## $endpoint - S3 endpoint URL | |
| ## $regionId - S3 region ID | |
| ## $bucketName - S3 bucket name | |
| ## $driveLetters - Drive letters for WinLocal repos | |
| ## $folderName - Folder name for repository | |
| ## $moveListedBackups - Specific backups to move | |
🤖 Prompt for AI Agents
In `@bdr-veeam/veeam-add-backup-repo.ps1` around lines 1 - 3, Add a top-of-file
comment block documenting all required RMM variables: list each variable
referenced later in the script (the variables used around lines 50-61 such as
those controlling repository type, bucket/name, endpoint, access key, secret,
region and any $RMM_* or $env:* variables the script reads) with a short
description and whether it’s required; insert this block above the existing
header comments so the script follows the coding guideline requiring RMM
variable declarations.
Comment on lines
+49
to
+63
| $scriptBlock = @" | ||
| `$repositoryType = '$repositoryType' | ||
| `$moveBackups = '$moveBackups' | ||
| `$description = '$description' | ||
| `$immutabilityPeriod = '$immutabilityPeriod' | ||
| `$accessKey = '$accessKey' | ||
| `$secretKey = '$secretKey' | ||
| `$endpoint = '$endpoint' | ||
| `$regionId = '$regionId' | ||
| `$bucketName = '$bucketName' | ||
| `$driveLetters = '$driveLetters' | ||
| `$folderName = '$folderName' | ||
| `$moveListedBackups = '$moveListedBackups' | ||
|
|
||
| "@ |
Contributor
There was a problem hiding this comment.
String interpolation vulnerability and secret exposure risk.
Two issues with the variable capture block:
-
Injection/syntax error: If any variable contains a single quote (e.g.,
$description = "John's Backup"), the generated script will have invalid syntax:$description = 'John's Backup' -
Security:
$secretKeyis written in plain text to a temp file on disk (line 75-76), which could be read by other processes.
Proposed fix using escaped single quotes and SecureString
$scriptBlock = @"
-`$repositoryType = '$repositoryType'
-`$moveBackups = '$moveBackups'
-`$description = '$description'
-`$immutabilityPeriod = '$immutabilityPeriod'
-`$accessKey = '$accessKey'
-`$secretKey = '$secretKey'
-`$endpoint = '$endpoint'
-`$regionId = '$regionId'
-`$bucketName = '$bucketName'
-`$driveLetters = '$driveLetters'
-`$folderName = '$folderName'
-`$moveListedBackups = '$moveListedBackups'
+`$repositoryType = '$($repositoryType -replace "'", "''")'
+`$moveBackups = '$($moveBackups -replace "'", "''")'
+`$description = '$($description -replace "'", "''")'
+`$immutabilityPeriod = '$($immutabilityPeriod -replace "'", "''")'
+`$accessKey = '$($accessKey -replace "'", "''")'
+`$secretKey = '$($secretKey -replace "'", "''")'
+`$endpoint = '$($endpoint -replace "'", "''")'
+`$regionId = '$($regionId -replace "'", "''")'
+`$bucketName = '$($bucketName -replace "'", "''")'
+`$driveLetters = '$($driveLetters -replace "'", "''")'
+`$folderName = '$($folderName -replace "'", "''")'
+`$moveListedBackups = '$($moveListedBackups -replace "'", "''")'
"@For the secret key exposure, consider restricting temp file permissions or using environment variables to pass sensitive data.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $scriptBlock = @" | |
| `$repositoryType = '$repositoryType' | |
| `$moveBackups = '$moveBackups' | |
| `$description = '$description' | |
| `$immutabilityPeriod = '$immutabilityPeriod' | |
| `$accessKey = '$accessKey' | |
| `$secretKey = '$secretKey' | |
| `$endpoint = '$endpoint' | |
| `$regionId = '$regionId' | |
| `$bucketName = '$bucketName' | |
| `$driveLetters = '$driveLetters' | |
| `$folderName = '$folderName' | |
| `$moveListedBackups = '$moveListedBackups' | |
| "@ | |
| $scriptBlock = @" | |
| `$repositoryType = '$($repositoryType -replace "'", "''")' | |
| `$moveBackups = '$($moveBackups -replace "'", "''")' | |
| `$description = '$($description -replace "'", "''")' | |
| `$immutabilityPeriod = '$($immutabilityPeriod -replace "'", "''")' | |
| `$accessKey = '$($accessKey -replace "'", "''")' | |
| `$secretKey = '$($secretKey -replace "'", "''")' | |
| `$endpoint = '$($endpoint -replace "'", "''")' | |
| `$regionId = '$($regionId -replace "'", "''")' | |
| `$bucketName = '$($bucketName -replace "'", "''")' | |
| `$driveLetters = '$($driveLetters -replace "'", "''")' | |
| `$folderName = '$($folderName -replace "'", "''")' | |
| `$moveListedBackups = '$($moveListedBackups -replace "'", "''")' | |
| "@ |
🤖 Prompt for AI Agents
In `@bdr-veeam/veeam-add-backup-repo.ps1` around lines 49 - 63, The here-string
construction in $scriptBlock naively injects variables (e.g., $description,
$bucketName, $folderName, $moveBackups) using single quotes which breaks when
values contain apostrophes and also writes $secretKey in plain text to a temp
file; fix by sanitizing/escaping single quotes for each interpolated value
before building $scriptBlock (e.g., replace ' with '\'' or use a serialization
method that preserves content), and stop writing the raw $secretKey to disk—pass
secrets as a SecureString or via an environment variable/secure credential
object or restrict temp file permissions and delete immediately if disk use is
unavoidable; update the code that constructs $scriptBlock and the logic that
writes/reads the temp file containing $secretKey accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed:
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.