Skip to content

Common/Sanitizer: terminate $env: env-name on whitespace (LF/CR/TAB) — PS twin of #22181 - #22206

Closed
wawanawna wants to merge 2 commits into
microsoft:masterfrom
wawanawna:users/wawanawna/fix-expand-env-newline-terminator
Closed

Common/Sanitizer: terminate $env: env-name on whitespace (LF/CR/TAB) — PS twin of #22181#22206
wawanawna wants to merge 2 commits into
microsoft:masterfrom
wawanawna:users/wawanawna/fix-expand-env-newline-terminator

Conversation

@wawanawna

@wawanawna wawanawna commented May 26, 2026

Copy link
Copy Markdown
Contributor

Context

PR #22181 fixed a YAML folded-scalar / $env: expansion regression (#22173) in the TypeScript PowerShell expander (expandPowerShellEnvVariables, ported by that PR from Tasks/PowerShellV2/helpers.ts into Tasks/AzureCLI{V2,V3}/src/argsSanitizer.ts). The root cause was an env-name terminator regex (split(/[ |"|'|;|$]/)) that does not include \n / \r / \t, so a YAML folded scalar (arguments: >) produces newlines that make the env-name greedily span across lines, process.env[name] returns undefined, expansion silently fails, and the literal $env:VAR\n... falls through to the sanitizer allowlist (which rejects $).

The PowerShell twin of that expander, Tasks/Common/Sanitizer/Expand-EnvVariables.ps1, has had the same bug since the module was created (PR #19183, Nov 2023):

$envName = $result.Substring($envStartIndex).Split(' ', '"', "'", ';', '$')[0]

It has been latent because the only consumer until now (PowerShellV2) rarely combines folded-scalar arguments with $env: references. PR #22171 (already merged) wired AzurePowerShellV2-V5 and ServiceFabricPowerShellV1 through the same Protect-ScriptArguments -> Expand-EnvVariables chain — and $env:servicePrincipalKey / $env:tenantId inside arguments: > is the documented pattern for those tasks, so the latent regression now has a much larger blast radius the moment EnableAzurePowerShellArgumentsSanitization is turned on. This PR removes that foot-gun before rollout.

Related: #22181 (TypeScript fix), #22173 (regression report), #22171 (the AzurePowerShell sanitizer wiring), AB#75787, AB#2382217.

Change

  • Tasks/Common/Sanitizer/Expand-EnvVariables.ps1: change the env-name terminator to include LF / CR / tab, exactly mirroring Ivan's split(/[\s|"|'|;|$]/) fix on the TypeScript side. PowerShell's String.Split(char[]) doesn't accept a regex, so terminators are enumerated explicitly: ' ', '"', "'", ';', '$', "\n", "`r", "`t"`.
  • Tasks/Common/Sanitizer/Tests/L0Expand-EnvVariables.ps1: add five regression cases (LF, CRLF, tab, multiple env vars separated by LF, and the full folded-scalar arguments: > reproducer from the AzurePowerShellV* scenario). The new tests fail on the unfixed expander and pass with the fix.
  • Version bumps on all 17 tasks that have ../Common/Sanitizer in their make.json, as required by ci/filter-tasks.js (same bookkeeping Sanitize ScriptArguments in AzurePowerShellV2-V5 and ServiceFabricPowerShellV1 (MSRC 115118) #22171 performed for its own change). Patch +1 for tasks without a Node24 buildconfig, patch +2 for tasks with a Node24_1 config so that Node24_1 remains Default+1. _generated/<task>{,_Node24} mirrors and <task>.versionmap.txt updated for the five tasks with Node24_1 configs (AzureFileCopy V4/V5/V6, AzurePowerShell V4/V5).

Risk

Low. The change is to a helper that runs only when the existing AZP_75787_* feature-flag triplet is on. Behavior change is strictly: inputs that previously erroneously failed sanitization (because expansion silently no-op'd on a folded-scalar \n) now expand correctly and pass — which is the intent. No allowlist relaxation. No new dependencies. No public API change.

Tests

  • Tasks/Common/Sanitizer/Tests/L0Expand-EnvVariables.ps1 — five new cases added, runs as part of the existing Tasks/Common/Sanitizer/Tests/L0.ts Security Suite (no new test registration needed).
  • Confirmed locally: all new cases fail without the one-line fix, pass with it. Remaining Common/Sanitizer L0s (Protect-ScriptArguments.Passes/Throws, Get-SanitizedArgumentsArray.{ReplacesForbiddenCharacters,DoesNotBreakExisting*}) still green.

Feature flag

No new flag. Gated behind the existing AZP_75787_ENABLE_NEW_LOGIC / _LOG / _COLLECT triplet (already controlling the entire Sanitizer module).

Checklist

  • Task versions bumped for all 17 Common/Sanitizer dependents (per versioning guide).
  • _generated/ mirrors regenerated for tasks with Node24_1 buildconfigs.
  • Behavior gated behind the existing FF triplet — no-op when flags are off.
  • Targeted L0 added; existing L0s unaffected.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

wawanawna1984 and others added 2 commits May 26, 2026 14:06
Fix Expand-EnvVariables.ps1 so that YAML folded scalars ('arguments: >'), which emit literal newlines between joined lines, don't make the env-name greedily span across lines, miss the lookup, and fall through to the sanitizer allowlist.

Companion to PR microsoft#22181 / issue microsoft#22173, which fixed the same bug in the TypeScript twin (Tasks/PowerShellV2/helpers.ts -> expandPowerShellEnvVariables). The PowerShell expander shares the design (and the bug) but had not been ported. This PR ('AzurePowerShellV2-V5 + ServiceFabricPowerShellV1 sanitization', microsoft#22171) is the first time Expand-EnvVariables is wired into a task family where folded-scalar arguments containing $env:* references is a documented pattern, so fixing it here removes a known false-positive before rollout.

Adds five regression cases to L0Expand-EnvVariables (LF, CRLF, tab, multiple env vars separated by LF, and the full folded-scalar reproducer from the AzurePowerShell scenario). Verified the new tests fail on the unfixed expander and pass with the fix; remaining Common/Sanitizer L0s (Protect-ScriptArguments.Passes/Throws, Get-SanitizedArgumentsArray.*) still green.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ci/filter-tasks.js requires that every task with Tasks/Common/Sanitizer in its make.json have its source version bumped in the same PR that modifies the Sanitizer module (same policy that drove the bump batch in microsoft#22171). Patch is bumped by +1 on tasks without a Node24 build config and by +2 on tasks with a Node24_1 config so that Node24_1 stays Default+1.

_generated/<task>{,_Node24}/task.json + task.loc.json + <task>.versionmap.txt mirrors are updated to match for the five tasks with Node24_1 buildconfigs (AzureFileCopy V4/V5/V6, AzurePowerShell V4/V5).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@wawanawna

Copy link
Copy Markdown
Contributor Author

Closing in favor of a same-name branch pushed directly to microsoft/azure-pipelines-tasks so the internal Azure Pipelines CI fires (it does not auto-run on fork PRs).

@wawanawna wawanawna closed this May 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants