Sanitize ScriptArguments in AzurePowerShellV2-V5 and ServiceFabricPowerShellV1 (MSRC 115118) - #22163
Closed
wawanawna wants to merge 14 commits into
Closed
Conversation
wawanawna
pushed a commit
to wawanawna/azure-pipelines-tasks
that referenced
this pull request
May 12, 2026
…dispatcher Covers the new dispatcher introduced by PR microsoft#22163 (MSRC 115118): - Test-ShouldUseSanitizer gate logic across all combinations of org-level`n toggle (Get-SanitizerCallStatus) and per-task pipeline FF (Get-VstsPipelineFeature), including throw, missing-cmdlet, and audit fallthrough. - Invoke-ScriptArgumentSanitization dispatch: no-op when either gate is off; passes through clean args; re-throws localized ScriptArgsSanitized verbatim; swallows unexpected sanitizer exceptions into telemetry; handles empty input. - Publish-SanitizerErrorTelemetry emits the expected ##vso[telemetry.publish] command shape. Follows the precedent set by PR microsoft#21947's Tasks/SqlAzureDacpacDeploymentV1/Tests/L0SecurityFunctions.ps1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MSRC 115118 / ICM 31000000596029. Mirrors the PowerShellV2 retrofit from PR microsoft#18744 / microsoft#19183. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MSRC 115118 / ICM 31000000596029. Mirrors the PowerShellV2 retrofit from PR microsoft#18744 / microsoft#19183. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MSRC 115118 / ICM 31000000596029. Mirrors the PowerShellV2 retrofit from PR microsoft#18744 / microsoft#19183. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MSRC 115118 / ICM 31000000596029. Mirrors the PowerShellV2 retrofit from PR microsoft#18744 / microsoft#19183. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MSRC 115118 / ICM 31000000596029. Mirrors the PowerShellV2 retrofit from PR microsoft#18744 / microsoft#19183. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR microsoft#21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR microsoft#21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR microsoft#21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR microsoft#21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR microsoft#21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…on + per-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR microsoft#21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dispatcher Covers the new dispatcher introduced by PR microsoft#22163 (MSRC 115118): - Test-ShouldUseSanitizer gate logic across all combinations of org-level`n toggle (Get-SanitizerCallStatus) and per-task pipeline FF (Get-VstsPipelineFeature), including throw, missing-cmdlet, and audit fallthrough. - Invoke-ScriptArgumentSanitization dispatch: no-op when either gate is off; passes through clean args; re-throws localized ScriptArgsSanitized verbatim; swallows unexpected sanitizer exceptions into telemetry; handles empty input. - Publish-SanitizerErrorTelemetry emits the expected ##vso[telemetry.publish] command shape. Follows the precedent set by PR microsoft#21947's Tasks/SqlAzureDacpacDeploymentV1/Tests/L0SecurityFunctions.ps1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…g for re-throw The previous implementation caught any exception from Protect-ScriptArguments and compared its message to Get-VstsLocString -Key 'ScriptArgsSanitized' to decide whether to re-throw (rejection) or swallow into telemetry (unexpected error). Two problems: 1. Protect-ScriptArguments throws Get-VstsLocString -Key 'PS_ScriptArgsSanitized' resolved from the Sanitizer module's resjson, while the dispatcher compared against 'ScriptArgsSanitized' resolved from the task's resjson. The two strings are translated independently. fr-FR uses 'coche' in one and 'backtick' in the other, so on a French agent the -eq comparison returned false and a real sanitizer rejection was silently swallowed into telemetry - the task continued executing the injected arguments. The en-US strings happen to be identical, which is why this latent bug went unnoticed. 2. Even when Protect-ScriptArguments crashes with an unrelated error, the safer default for a security gate is to fail the task rather than execute un-vetted arguments. The dispatcher now fails closed: any exception out of Protect-ScriptArguments produces a telemetry record with the underlying message + stack and re-throws the task-localized 'ScriptArgsSanitized' message to the caller. No locale- dependent string comparison remains. Tests cover all three throw paths (matching message, locale-drifted message, unexpected crash) - all must fail closed with 'ScriptArgsSanitized'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wawanawna
force-pushed
the
users/wawanawna/msrc-115118-azurepowershell-sanitizer
branch
from
May 12, 2026 15:09
fea278d to
7b6762c
Compare
Rebase auto-resolve introduced a 'comma on own line' formatting glitch in all AzurePowerShellV5 locale resources.resjson files (and the V5 generated mirrors). Moves the comma to the end of the preceding line, matching the rest of the resjson format and addressing review feedback from @nemanjarogic. No content / behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nemanjarogic
approved these changes
May 13, 2026
Contributor
Author
|
@nemanjarogic could you trigger CI with /azp run when you have a moment? Since this PR is from a fork branch ( |
Merged
9 tasks
Contributor
Author
|
Superseded by #22171, which is opened from the same branch pushed directly to |
wawanawna
added a commit
that referenced
this pull request
May 18, 2026
Addresses review feedback on PR #22171: 1. ServiceFabricPowerShellV1 was pinning VstsTaskSdk 0.11.0, but Get-VstsPipelineFeature (used by the new Invoke-ScriptArgumentSanitization dispatcher) only landed in 0.20.0+. The dispatcher's Test-ShouldUseSanitizer falls open to no-op when the cmdlet is missing, which silently made the SF V1 sanitization inert in production - exactly the regression the new helper was designed to surface only on real failures. Bumped to 0.21.0 to match AzurePowerShellV5 / PowerShellV2 (sister sanitizer-adopting PowerShell tasks). Verified safe: - SF V1 only uses Get-VstsInput / Trace-VstsEnteringInvocation, which are stable across SDK versions. - All SF V1 L0 tests Register-Mock Invoke-ScriptArgumentSanitization at top, so the actual SDK call is never exercised in tests. - Tests/lib/Initialize-Test.ps1 already provides a global stub for Get-VstsPipelineFeature, so the dispatcher's cmdlet-presence check is neutralized in test runs regardless of SDK version. 2. L0Invoke-ScriptArgumentSanitization.ps1 header comment referenced the predecessor fork PR #22163 (which this PR explicitly replaces); updated to #22171. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wawanawna
added a commit
that referenced
this pull request
May 19, 2026
…erShellV1 (MSRC 115118) (#22171) * AzurePowerShellV2: sanitize ScriptArguments via Common/Sanitizer MSRC 115118 / ICM 31000000596029. Mirrors the PowerShellV2 retrofit from PR #18744 / #19183. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * AzurePowerShellV3: sanitize ScriptArguments via Common/Sanitizer MSRC 115118 / ICM 31000000596029. Mirrors the PowerShellV2 retrofit from PR #18744 / #19183. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * AzurePowerShellV4: sanitize ScriptArguments via Common/Sanitizer MSRC 115118 / ICM 31000000596029. Mirrors the PowerShellV2 retrofit from PR #18744 / #19183. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * AzurePowerShellV5: sanitize ScriptArguments via Common/Sanitizer MSRC 115118 / ICM 31000000596029. Mirrors the PowerShellV2 retrofit from PR #18744 / #19183. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ServiceFabricPowerShellV1: sanitize ScriptArguments via Common/Sanitizer MSRC 115118 / ICM 31000000596029. Mirrors the PowerShellV2 retrofit from PR #18744 / #19183. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Common/Sanitizer: add Invoke-ScriptArgumentSanitization helper MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * AzurePowerShellV2: use shared Invoke-ScriptArgumentSanitization + per-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * AzurePowerShellV3: use shared Invoke-ScriptArgumentSanitization + per-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * AzurePowerShellV4: use shared Invoke-ScriptArgumentSanitization + per-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * AzurePowerShellV5: use shared Invoke-ScriptArgumentSanitization + per-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ServiceFabricPowerShellV1: use shared Invoke-ScriptArgumentSanitization + per-task pipeline FF MSRC 115118 / ICM 31000000596029. Mirrors the Should-UseSanitizedArguments / per-task pipeline FF pattern from PR #21947. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Common/Sanitizer: add L0 tests for Invoke-ScriptArgumentSanitization dispatcher Covers the new dispatcher introduced by PR #22163 (MSRC 115118): - Test-ShouldUseSanitizer gate logic across all combinations of org-level`n toggle (Get-SanitizerCallStatus) and per-task pipeline FF (Get-VstsPipelineFeature), including throw, missing-cmdlet, and audit fallthrough. - Invoke-ScriptArgumentSanitization dispatch: no-op when either gate is off; passes through clean args; re-throws localized ScriptArgsSanitized verbatim; swallows unexpected sanitizer exceptions into telemetry; handles empty input. - Publish-SanitizerErrorTelemetry emits the expected ##vso[telemetry.publish] command shape. Follows the precedent set by PR #21947's Tasks/SqlAzureDacpacDeploymentV1/Tests/L0SecurityFunctions.ps1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Common/Sanitizer: dispatcher fails closed instead of message-comparing for re-throw The previous implementation caught any exception from Protect-ScriptArguments and compared its message to Get-VstsLocString -Key 'ScriptArgsSanitized' to decide whether to re-throw (rejection) or swallow into telemetry (unexpected error). Two problems: 1. Protect-ScriptArguments throws Get-VstsLocString -Key 'PS_ScriptArgsSanitized' resolved from the Sanitizer module's resjson, while the dispatcher compared against 'ScriptArgsSanitized' resolved from the task's resjson. The two strings are translated independently. fr-FR uses 'coche' in one and 'backtick' in the other, so on a French agent the -eq comparison returned false and a real sanitizer rejection was silently swallowed into telemetry - the task continued executing the injected arguments. The en-US strings happen to be identical, which is why this latent bug went unnoticed. 2. Even when Protect-ScriptArguments crashes with an unrelated error, the safer default for a security gate is to fail the task rather than execute un-vetted arguments. The dispatcher now fails closed: any exception out of Protect-ScriptArguments produces a telemetry record with the underlying message + stack and re-throws the task-localized 'ScriptArgsSanitized' message to the caller. No locale- dependent string comparison remains. Tests cover all three throw paths (matching message, locale-drifted message, unexpected crash) - all must fail closed with 'ScriptArgsSanitized'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix stray comma on its own line in V5 resjson files (PR review feedback) Rebase auto-resolve introduced a 'comma on own line' formatting glitch in all AzurePowerShellV5 locale resources.resjson files (and the V5 generated mirrors). Moves the comma to the end of the preceding line, matching the rest of the resjson format and addressing review feedback from @nemanjarogic. No content / behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Bump versions of 12 Common/Sanitizer-dependent tasks (CI policy) MSRC 115118 added a new file (Invoke-ScriptArgumentSanitization.ps1) to Tasks/Common/Sanitizer/. ci/filter-tasks.js requires every task whose Common dependencies change to have its own version bumped, otherwise the 'Filter out unchanged tasks' build step fails with 'The following tasks should have their versions bumped due to changes in common: ...'. This matches the precedent established by PR #19834 (Add audit event for Shell Tasks Validation warnings), which made a similar Common/Sanitizer change and bumped the same set of consumer tasks. Bumps (patch +1, except where the existing _generated mirror's maxVersion already exceeded source — in those cases bumped to maxVersion+1 to satisfy BuildConfigGen's monotonicity invariant): AzureFileCopyV1 1.274.0 -> 1.274.1 AzureFileCopyV2 2.274.0 -> 2.274.1 AzureFileCopyV3 3.274.0 -> 3.274.1 AzureFileCopyV4 4.274.0 -> 4.274.2 (Node24_1 was already 4.274.1) AzureFileCopyV5 5.274.0 -> 5.274.2 (Node24_1was already 5.274.1) AzureFileCopyV6 6.274.2 -> 6.274.4 (Node24_1 was already 6.274.3) PowerShellV2 2.274.0 -> 2.274.1 PowerShellOnTargetMachinesV3 3.273.0 -> 3.273.1 SqlAzureDacpacDeploymentV1 1.274.0 -> 1.274.1 SqlDacpacDeploymentOnMachineGroupV0 0.274.0 -> 0.274.1 WindowsMachineFileCopyV1 1.243.0 -> 1.243.1 WindowsMachineFileCopyV2 2.251.0 -> 2.251.1 Also updates _generated/ mirrors to match (re-run via BuildConfigGen with --write-updates --current-sprint 274). Most of the new files in _generated/ AzureFileCopyV4 and _generated/AzureFileCopyV5 are byte-identical copies of source (54/56 unchanged); they exist because BCG materializes the Default mirror whenever any non-Default config (here Node24_1) is registered. Precedent: _generated/AzureFileCopyV6/ has been checked in this way for the same reason. No source / behavior changes in any of the 12 bumped tasks. The Sanitizer dispatcher (Invoke-ScriptArgumentSanitization.ps1) is consumed only by AzurePowerShellV2-V5 and ServiceFabricPowerShellV1; the 12 tasks bumped here all already have org-level + per-task FF dispatch and gain nothing from the new helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Roll forward minor to sprint 274 for 4 stale-minor tasks (CI downgrade check) ci/check-downgrading.js failed on the previous push because four tasks had Minor versions older than the current sprint (274). The check enforces minor >= currentSprintVersion. Patch-only bumps were not enough. ServiceFabricPowerShellV1 1.228.2 -> 1.274.0 PowerShellOnTargetMachinesV3 3.273.1 -> 3.274.0 WindowsMachineFileCopyV1 1.243.1 -> 1.274.0 WindowsMachineFileCopyV2 2.251.1 -> 2.274.0 None of these have Node24 generated mirrors so no _generated/ updates are needed (BuildConfigGen verified clean for all four). ServiceFabricPowerShellV1 is one of the 5 sanitizer-adopting tasks, so the Risk Assessment / version table in the PR description has been updated to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add ScriptArgsSanitized to task.json messages for all 5 sanitizer tasks The original retrofit added the localized 'ScriptArgsSanitized' message to task.loc.json and Strings/resources.resjson/<locale>/resources.resjson, but forgot to add the canonical source-of-truth entry to task.json's 'messages' block for AzurePowerShellV2-V5 and ServiceFabricPowerShellV1. The CI 'Verify task source changes' step runs 'node make.js build', which regenerates task.loc.json and en-US/resources.resjson from task.json.messages. Without the source entry, that step stripped the ScriptArgsSanitized line on every rebuild — leaving the dispatcher's localized throw with no message to resolve at runtime in en-US. Fix: add the message string to task.json's messages block for each of the 5 tasks. Then runs 'node make.js build' against each so task.loc.json and en-US resources.resjson are emitted by the canonical build path; BCG re-syncs the V4/V5 generated mirrors. No functional change to non-en-US localizations — they continue to use the existing translations from the original retrofit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Sanitizer dispatcher: throw module's own PS_ScriptArgsSanitized key The CI's static loc-resource check ('Find missing string in .ps1/.psm1') scans each PS file under Tasks/Common/<Module>/ and requires every Get-VstsLocString -Key 'X' call to have 'X' declared in that module's module.json. My dispatcher lives in Tasks/Common/Sanitizer/ and threw with the task-level key 'ScriptArgsSanitized', which is declared in each task's task.json but NOT in the Sanitizer module.json — so the test failed. Fix: switch the throw to the module's own 'PS_ScriptArgsSanitized' key, which is already declared in Tasks/Common/Sanitizer/module.json and its 10 locale resjson files. This makes the dispatcher self-contained at the module level (the principle the static check enforces). Behavior is unchanged: the en-US text of 'PS_ScriptArgsSanitized' (module) and 'ScriptArgsSanitized' (task) are byte-identical, and the fail-closed logic is preserved. Non-en-US locales will see the module's translation instead of the task's; this is actually a small improvement because the dispatcher and the underlying Protect-ScriptArguments now speak the same key — no locale-drift surface remains at all. Updates the dispatcher's L0 tests to assert on 'PS_ScriptArgsSanitized' accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove dead ScriptArgsSanitized loc entries from the 5 sanitizer-adopter tasks Earlier commits in this PR added 'ScriptArgsSanitized' to task.json / task.loc.json / 10 locale resjsons for AzurePowerShellV2-V5 and ServiceFabricPowerShellV1, mirroring the convention used by PowerShellV2 and the other inline-sanitizer tasks. After consolidating the dispatch logic into Tasks/Common/Sanitizer/Invoke-ScriptArgumentSanitization.ps1 and switching its throw to the module's own 'PS_ScriptArgsSanitized' key, those task-level entries became unreachable - no .ps1 in these 5 tasks calls Get-VstsLocString -Key 'ScriptArgsSanitized' anymore (verified by grep). This commit removes the dead entries from each of the 5 tasks: - task.json messages -> rebuild via 'node make.js build --task X' to regenerate task.loc.json and en-US/resources.resjson canonically. - 9 non-en-US locale resjsons -> manually stripped. BuildConfigGen re-syncs the AzurePowerShellV4/V5 generated mirrors. Net effect: only the live 'PS_ScriptArgsSanitized' key (declared in Tasks/Common/Sanitizer/module.json + its 10 locales) is referenced at runtime by any code path in this PR. The diff vs master shrinks because we're undoing additions made earlier in the same PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Sanitizer dispatcher: don't say 'Get-VstsLocString' in a comment (L0 lint) The Tests/L0 'Find missing string in .ps1/.psm1' test does a case-insensitive substring search for 'Get-VstsLocString' on every line in every .ps1/.psm1 under Tasks/ (excluding Tests/). When it finds the substring, it tries to parse the line as a 'Get-VstsLocString -Key ...' call. If the parse fails, it asserts 'Bad format string'. The test doesn't distinguish code from comments. My explanatory comment mentioned the cmdlet name verbatim, so the test flagged it. Reworded the comment to describe the behavior ('loc resource references') without using the literal cmdlet name. No code change. Also adds C:/ado/temp/check-loc-strings.js (local equivalent of the CI step) so future iterations of this PR can be validated before pushing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Regenerate _generated/AzurePowerShellV{4,5}{,_Node24} loc files after cleanup After the previous commit removed the dead ScriptArgsSanitized entries from the source task.json files, _generated/AzurePowerShellV4_Node24/task.json and V5_Node24/task.json were updated by BuildConfigGen but their dependent task.loc.json + en-US/resources.resjson were not re-emitted from the build pass. CI 'Verify task source changes' flagged them. Re-ran 'node make.js build --task AzurePowerShellV{4,5}' which regenerates all config mirrors (Default + Node24_1) from the updated task.json sources. Four files updated per task (Default and Node24 both have task.loc.json + en-US resjson). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Clean stale ScriptArgsSanitized from V4/V5 source task.loc.json + en-US resjson Tasks/AzurePowerShellV4 and V5 use the _buildConfigMapping mechanism, so 'node make.js build --task X' regenerates only the _generated/ mirrors — the source Tasks/<task>/task.loc.json and Strings/.../en-US/resources.resjson are hand-maintained / BCG-synced, not build-regenerated. When I removed ScriptArgsSanitized from task.json earlier, the build's regen pass updated V2/V3/SFV1 source loc files (those tasks don't have build configs) but left V4/V5 source loc files with the now-stale entry. BCG verify caught the divergence: V4 task.loc.json had 9 messages (including ScriptArgsSanitized) while V4 task.json had 8. Manually stripped ScriptArgsSanitized from Tasks/AzurePowerShellV{4,5}/ task.loc.json + Strings/.../en-US/resources.resjson (non-en-US locales were already cleaned in a prior commit). BCG verify now reports clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Mock Invoke-ScriptArgumentSanitization in per-task L0 tests The per-task L0 tests for AzurePowerShellV2-V5 and ServiceFabricPowerShellV1 dot-source Tests/lib/Initialize-Test.ps1 (which mocks Import-Module so the Sanitizer common module isn't actually loaded), then directly invoke the task's main script. After this PR's retrofit, those main scripts call Invoke-ScriptArgumentSanitization — which is undefined under the test mock, so PowerShell aborts with 'term not recognized'. MacOS/Linux CI passed because per-task PowerShell-based L0 suites only run on Windows. My local validation also missed it; adding the per-task L0 step to validate-pr-22171.ps1 so future iterations catch this class of failure before push. Fix: add Register-Mock Invoke-ScriptArgumentSanitization (stub no-op) right after Initialize-Test.ps1 in each of the 40 affected source test files across the 5 tasks. BuildConfigGen propagated the mock to the V4/V5 generated mirror Tests/ trees automatically. Local: all 5 tasks now pass L0 (8-13 passing each). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Roll forward all PR-touched task versions to sprint 275 The sprint version on master rolled from 274 to 275 (via #22175), so ci/check-downgrading.js now rejects every task left at *.274.*. Bump each task's Default minor to 275, with the smallest patch >= master's current patch. Tasks with a Node24_1 BuildConfig get Node24_1 = Default + 1 (per the same convention used earlier in this PR). New versions: AzureFileCopyV1 1.275.0 AzureFileCopyV2 2.275.0 AzureFileCopyV3 3.275.0 AzureFileCopyV4 4.275.0 / 4.275.1 (Node24_1) AzureFileCopyV5 5.275.0 / 5.275.1 (Node24_1) AzureFileCopyV6 6.275.2 / 6.275.3 (Node24_1) AzurePowerShellV2 2.275.0 AzurePowerShellV3 3.275.0 AzurePowerShellV4 4.275.2 / 4.275.3 (Node24_1) AzurePowerShellV5 5.275.4 / 5.275.5 (Node24_1) PowerShellOnTargetMachinesV3 3.275.0 PowerShellV2 2.275.0 ServiceFabricPowerShellV1 1.275.0 SqlAzureDacpacDeploymentV1 1.275.0 SqlDacpacDeploymentOnMachineGroupV0 0.275.0 WindowsMachineFileCopyV1 1.275.0 WindowsMachineFileCopyV2 2.275.0 Source task.json/task.loc.json, _generated mirrors, _buildConfigMapping entries, and *.versionmap.txt files are all kept consistent. No code changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Bump ServiceFabricPowerShellV1 SDK to 0.21.0 + fix L0 test PR ref Addresses review feedback on PR #22171: 1. ServiceFabricPowerShellV1 was pinning VstsTaskSdk 0.11.0, but Get-VstsPipelineFeature (used by the new Invoke-ScriptArgumentSanitization dispatcher) only landed in 0.20.0+. The dispatcher's Test-ShouldUseSanitizer falls open to no-op when the cmdlet is missing, which silently made the SF V1 sanitization inert in production - exactly the regression the new helper was designed to surface only on real failures. Bumped to 0.21.0 to match AzurePowerShellV5 / PowerShellV2 (sister sanitizer-adopting PowerShell tasks). Verified safe: - SF V1 only uses Get-VstsInput / Trace-VstsEnteringInvocation, which are stable across SDK versions. - All SF V1 L0 tests Register-Mock Invoke-ScriptArgumentSanitization at top, so the actual SDK call is never exercised in tests. - Tests/lib/Initialize-Test.ps1 already provides a global stub for Get-VstsPipelineFeature, so the dispatcher's cmdlet-presence check is neutralized in test runs regardless of SDK version. 2. L0Invoke-ScriptArgumentSanitization.ps1 header comment referenced the predecessor fork PR #22163 (which this PR explicitly replaces); updated to #22171. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Uladzimir Tratsiakou (Vladimir/Vova) <utratsiakou@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Vladimir Tretyakov <wawanawna@users.noreply.github.com>
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
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.
Context
The PowerShell-runner task family appended the YAML-supplied
ScriptArgumentsinput directly into a generated PowerShell command (& '<scriptPath>' $scriptArguments/. '<scriptPath>' $scriptArguments) without sanitization. A YAML template-parameter splice containing shell metacharacters (;,|,&, ``, etc.) would be parsed by PowerShell as additional statements and executed under the service connection's identity. This is the same class of injection that motivated the original work item #75787 and that has been progressively retrofitted across the repo:The PowerShell-flavored sister tasks that had not been retrofitted were
AzurePowerShellV2-V5andServiceFabricPowerShellV1. This PR closes that gap.Related: ICM
31000000596029, MSRC case115118(resolved as Duplicate; this PR is the proper code-level remediation in the line of #18646 / #18744 / #22066).Task Names
AzurePowerShellV2,AzurePowerShellV3,AzurePowerShellV4,AzurePowerShellV5,ServiceFabricPowerShellV1, plus a new helper file inTasks/Common/Sanitizer/.AzureCloudPowerShellDeploymentV1-deprecated: trueintask.json.AzureCloudPowerShellDeploymentV2-deprecated: trueand preview; noScriptArgumentsconcat sink.PowerShellOnTargetMachinesV2-deprecated: true(PowerShellOnTargetMachinesV3fixed in Fix security issue for PowerShellOnTargetMachines@3 #21968).ChefKnifeV1-deprecated: true, removal date 2024-01-31 intask.json.AzureCLIV1-deprecated: true, explicitly skipped in feat(AzureCLI): Implement security enhancements for scriptArguments validation #22066.ServiceFabricDeployV1- uses its ownSfSafeParserfeature-flagged safe parser instead ofInvoke-Expression.AzureVmssDeploymentV{0,1}/Resources/customScriptInvoker.ps1- by design runs customer-supplied script on the target VM (the customer is the script author, not a third-party injecting into a YAML splice).AzurePowerShellV1andServiceFabricPowerShellV2do not exist in any branch of the repo.Description
Two-flag rollout, mirroring PR #21947's
Should-UseSanitizedArgumentspattern (and #22066'sEnableAzureCliArgsValidationpattern). Sanitization fires only when BOTH:AZP_75787_*env triplet on the agent. Surfaced viaGet-SanitizerCallStatusin the existingTasks/Common/Sanitizermodule.Get-VstsPipelineFeature:EnableAzurePowerShellArgumentsSanitization- shared acrossAzurePowerShellV2-V5.EnableServiceFabricPowerShellArgumentsSanitization-ServiceFabricPowerShellV1.This means a customer who has the org toggle on today (e.g. because they enabled it for the PowerShellV2 fix) will not see any behavior change on these newly-onboarded tasks until Microsoft turns on the per-task pipeline flag, preventing accidental regression. The same rollout model was used by #21947 and #22066.
New shared helper -
Tasks/Common/Sanitizer/Invoke-ScriptArgumentSanitization.ps1(sourced fromSanitizer.psm1and exported). Three functions:Test-ShouldUseSanitizer -TaskName <name> -PipelineFeatureFlagName <name>- combined check, emits FF-check failure telemetry like PR Prevent security exploit in SqlAzureDacpacDeploymentV1 and SqlDacpacDeploymentOnMachineGroupV0 where Invoke-Expression is used #21947.Invoke-ScriptArgumentSanitization -InputArgs <args> -TaskName <name> -PipelineFeatureFlagName <name>- single-call wrapper aroundProtect-ScriptArguments+ try/catch.Publish-SanitizerErrorTelemetry- small##vso[telemetry.publish]emitter shared by both.Fail-closed dispatch (deliberate divergence from PowerShellV2's PS port):
Invoke-ScriptArgumentSanitizationtreats any exception out ofProtect-ScriptArgumentsas a sanitization failure and aborts the task with the task-localizedScriptArgsSanitizedmessage. We deliberately do not compare.Exception.MessageagainstGet-VstsLocString -Key 'ScriptArgsSanitized'to distinguish "rejection" from "unexpected error":Get-VstsLocString -Key 'PS_ScriptArgsSanitized'(its own resjson key) while the task resjson uses keyScriptArgsSanitized. The two strings are translated independently.fr-FRalready diverges today:cochevsbacktick. An-eqcomparison on a French agent would let a real sanitizer rejection fall into the "swallow into telemetry" branch and bypass the gate.This is a divergence from the inline
catchblocks in PowerShellV2 / SqlAzureDacpacDeploymentV1 / AzureCLIV2 (which compare toGet-VstsLocString); those task-specific implementations carry the same latent locale-drift bug but ship in TypeScript / inline PS where it has been masked by en-US-identical strings. The new shared helper fixes it once.Per-task collapse - each task entry script went from a ~25-line inline try/catch + local
Publish-Telemetryhelper to:powershell if ($scriptType -ne "InlineScript") { Invoke-ScriptArgumentSanitization-InputArgs $scriptArguments
-TaskName 'AzurePowerShellV5'-PipelineFeatureFlagName 'EnableAzurePowerShellArgumentsSanitization'
}
`
Per task, the change set is still the proven
PowerShellV2-style retrofit:make.jsonadds../Common/Sanitizerto thecommonarray.The entry script imports the module and calls the shared helper right after the existing
[\r\n]newline check, only when-ne "InlineScript".task.json/task.loc.jsonpatch-bumped (post-rebase against current master):Patch-number gaps (V4: +4, V5: +2 from current master Default) are intentional - they reserve headroom for currently open in-flight PRs (Update package dependencies and fix security vulnerabilities #22058
Update package dependencies and fix security vulnerabilitiesis still based on older V4/V5 patches and is expected to land first).task.loc.json.messagesadds"ScriptArgsSanitized": "ms-resource:loc.messages.ScriptArgsSanitized".All ten locale
Strings/resources.resjson/<locale>/resources.resjsonfiles add the newloc.messages.ScriptArgsSanitizedentry, copying the already-translated text fromPowerShellV2._generated/AzurePowerShellV{4,5}{,_Node24}mirrors the source changes and_buildConfigMapping/*.versionmap.txtare kept consistent (Default = master patch, Node24_1 = Default+1).InlineScriptandScriptPathare intentionally not sanitized: inline scripts are author-written; the path is not shell-spliced (it goes through''quoting).Risk Assessment - Low
Test-ShouldUseSanitizerfalls open (returnsFalse) on cmdlet-missing or FF-check error and emits diagnostic telemetry - never a hard failure on the FF query path itself.Tasks/Common/Sanitizermodule (no new sanitization code).Sanitizer.psm1and running five scenarios:ScriptArgsSanitizedmessage.Get-VstsPipelineFeaturemissing -> no-op, emitscmdletMissingtelemetry.Documentation changes required: Y
The Learn topic at https://learn.microsoft.com/azure/devops/pipelines/security/inputs#enable-shell-tasks-arguments-validation has not been updated since the 2023 retrofit and is missing the four subsequent retrofits (#21947, #21968, #22066, this PR). Companion docs PR: MicrosoftDocs/azure-devops-docs#14371.
Added unit tests: Y
Tasks/Common/Sanitizer/Tests/L0Invoke-ScriptArgumentSanitization.ps1covers the new dispatcher across six gating branches, fail-closed behaviour, and locale-drift regression. Registered inTasks/Common/Sanitizer/Tests/L0.ts. The existingTasks/Common/Sanitizer/Tests/L0*suite continues to cover the underlying regex/throw/audit/telemetry FF behaviour and bash/cmd/PowerShell format-compatibility.Attached related issue: ICM 31000000596029, MSRC 115118, AB#75787
Checklist
Register-Mock Import-ModuleinTests/lib/Initialize-Test.ps1neutralizes the new import; the helper falls open through the FF gate whenGet-VstsPipelineFeatureis unavailable)._generated/regenerated for the two tasks that have generated mirrors (V4, V5)._generated/and versionmaps re-validated.EnableAzurePowerShellArgumentsSanitizationandEnableServiceFabricPowerShellArgumentsSanitizationinTools.PipelineFeatures(mseng/AzureDevOps) with defaultoff. Without this, the PR is a permanent no-op.Tasks/Common/Sanitizerowners requested before merge.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com