-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Replaced Invoke-Expression with Parser #29056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request addresses a security vulnerability by replacing the use of Invoke-Expression with a PowerShell AST (Abstract Syntax Tree) parser for safely parsing metadata parameters in Policy cmdlets.
Changes:
- Replaced
Invoke-Expressionwith AST-based parsing to eliminate security risks - Added two new helper functions:
Convert-AstLiteralandConvertTo-HashtableSafelyto safely parse PowerShell hashtable literals - Added comprehensive test suite for the new parsing functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/Resources/Policy.Autorest/custom/Helpers.ps1 | Added AST parser implementation with recursive literal conversion and safe hashtable parsing; replaced Invoke-Expression call with ConvertTo-HashtableSafely function |
| src/Resources/Policy.Autorest/test/ResolvePolicyMetadataParameter.Tests.ps1 | Added new test file with test cases for simple and complex hashtable parsing, JSON parsing, file content parsing, and error handling |
Comments suppressed due to low confidence (1)
src/Resources/Policy.Autorest/custom/Helpers.ps1:577
- The parameter is declared as
$MetadataValue(line 550) with Pascal case, but it is referenced as$metadataValuewith camel case throughout the function body (lines 554, 558, 562, 577). While PowerShell is case-insensitive for variable names, using inconsistent casing reduces code readability and maintainability. Use consistent casing throughout the function.
$MetadataValue,
[bool]$Debug = $false
)
if ($metadataValue -is [hashtable]) {
return $metadataValue
}
if ([System.String]::IsNullOrEmpty($metadataValue)) {
return $metadataValue
}
$metadata = (GetFileUriOrStringParameterValue $metadataValue).Trim()
if ($debug) {
Write-Host -ForegroundColor Cyan Metadata: $metadata
}
if ($metadata -like '@{*') {
# probably a PSCustomObject, try converting to hashtable
return ConvertTo-HashtableSafely $metadata
}
# otherwise it should be a JSON string
if ($metadata -like '{*}') {
return $metadata | ConvertFrom-JsonSafe -AsHashtable
}
throw "Unrecognized metadata format - value: [$($metadataValue)], type: [$($metadataValue.GetType())]"
src/Resources/Policy.Autorest/test/ResolvePolicyMetadataParameter.Tests.ps1
Show resolved
Hide resolved
src/Resources/Policy.Autorest/test/ResolvePolicyMetadataParameter.Tests.ps1
Show resolved
Hide resolved
src/Resources/Policy.Autorest/test/ResolvePolicyMetadataParameter.Tests.ps1
Outdated
Show resolved
Hide resolved
src/Resources/Policy.Autorest/test/ResolvePolicyMetadataParameter.Tests.ps1
Outdated
Show resolved
Hide resolved
src/Resources/Policy.Autorest/test/ResolvePolicyMetadataParameter.Tests.ps1
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
vidai-msft
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the ChangeLog.md and fix the test errors.
|
This PR was labeled "needs-revision" because it has unresolved review comments or CI failures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
src/Resources/Policy.Autorest/test/ResolvePolicyMetadataParameter.Tests.ps1
Show resolved
Hide resolved
src/Resources/Policy.Autorest/test/ResolvePolicyMetadataParameter.Tests.ps1
Outdated
Show resolved
Hide resolved
src/Resources/Policy.Autorest/test/ResolvePolicyMetadataParameter.Tests.ps1
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Description
We were instructed to remove our usage of 'Invoke-Expression' because it is a security vulnerability. It has been replaced by a PowerShell AST parser that can robustly handle any valid Metadata input. I also added some tests to validate this helper function.
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.