fix(powershell-format): refuse computed -FilePath on Invoke-Command and Start-Job

[kyle-sexton]

Closes #1490

Related

N/A

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @kyle-sexton's task in 2m 30s —— View job

---

Security Review

• Gather context (diff, changed files)
• Review plugins/powershell-format/hooks/powershell-format.sh for security issues
• Review plugins/powershell-format/CHANGELOG.md
• Post findings

This PR extends the PSScriptAnalyzer CustomRulePath trust gate's $loaders list (used to decide whether a code-loading target can be "pinned" into the approval signature) to also cover Invoke-Command and Start-Job, closing the gap where a computed -FilePath on those cmdlets could execute an un-hashed file without tripping PSSA_TRUST UNPINNABLE.

IMPORTANT — New loaders can be bypassed via their built-in aliases, reopening the exact hole this PR closes

Confidence: High

hooks/powershell-format.sh#L368-L370

The $loaders array matches on the literal command name returned by $cmdAst.GetCommandName() (L375), with no alias-table resolution anywhere in the file. The existing entries account for this — Import-Module/ipmo and Invoke-Expression/iex are both listed. But the two cmdlets added in this PR are listed only by their full names:

$loaders = @("Import-Module", "ipmo", "Add-Type", "New-Module",
    "Invoke-Expression", "iex", "Import-PowerShellDataFile",
    "Invoke-Command", "Start-Job")

Invoke-Command's built-in alias icm and Start-Job's built-in alias sajb are not included. A custom rule module written as:

icm -FilePath $computedPath

parses to a CommandAst whose GetCommandName() returns "icm", which is not -contains'd by $loaders, so $isLoad stays false and the composed -FilePath argument is never inspected. The file never enters $pending, is never hashed into the reachable-file set, and no PSSA_TRUST UNPINNABLE is raised — the exact "approval would then survive arbitrary edits to it" scenario described in the surrounding comments (L337-346) that this PR is meant to prevent, just reached through the alias instead of the full name.

Suggested fix: add "icm" and "sajb" to the $loaders array alongside the two new full names, consistent with how ipmo/iex are already handled.

Fix this →

---

No other security issues found in this diff. The CHANGELOG.md change is documentation-only. Supply-chain/pin-related and workflow-permission concerns are out of scope here per the review brief (covered by zizmor) and don't apply to this diff regardless (no workflow or dependency-pin changes).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 06121018a9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 1m 37s —— View job

---

Security Review

• Gather context (diff vs origin/main, changed files)
• Review plugins/powershell-format/hooks/powershell-format.sh $loaders change
• Confirm whether the previously-flagged alias gap was addressed
• Review plugin.json / CHANGELOG.md changes
• Post findings

This PR extends the PSSA trust gate's $loaders list (which decides whether a code-loading command's target must be pinnable to a hashed file) to cover Invoke-Command and Start-Job, closing the gap where a computed -FilePath on those cmdlets could execute an un-hashed file without tripping PSSA_TRUST UNPINNABLE.

IMPORTANT — Invoke-Command/Start-Job still bypassable via their built-in aliases (icm, sajb)

Confidence: High

hooks/powershell-format.sh#L368-L370

$loaders = @("Import-Module", "ipmo", "Add-Type", "New-Module",
    "Invoke-Expression", "iex", "Import-PowerShellDataFile",
    "Invoke-Command", "Start-Job")

$loaders is matched against the literal string from $cmdAst.GetCommandName() (L375) with no alias-table resolution. The file already accounts for this for the other two aliasable loaders — Import-Module/ipmo and Invoke-Expression/iex are both listed — but the two cmdlets this PR adds are listed only by their full names. Invoke-Command's built-in alias is icm and Start-Job's is sajb; neither is present.

A custom rule module written as:

icm -FilePath $computedPath

parses to a CommandAst whose GetCommandName() is "icm", which $loaders -contains does not match, so $isLoad stays false, the loop at L398–417 never inspects the composed -FilePath value, the file is never queued into $pending/hashed into the reachable-file set, and PSSA_TRUST UNPINNABLE never fires. This is exactly the "approval would then survive arbitrary edits to it" scenario the surrounding comments (L337–346) describe — reached through the alias instead of the full name — and it re-opens the specific hole this PR's title says it closes, just for the two newly-added loaders.

Suggested fix: add "icm" and "sajb" to the $loaders array, consistent with how ipmo/iex are already handled.

Fix this →

---

Note: Codex's earlier comment on this PR (that adding these two names makes the L398–415 loop treat every non-parameter argument — e.g. -ComputerName, -ArgumentList values on ordinary -ScriptBlock calls — as an unpinnable load target) is a real functional over-refusal risk, but it fails closed (blocks formatting) rather than open (lets code through unchecked), so it's a correctness/availability bug rather than a security vulnerability and is left out of this review's scope.

No other security issues found in this diff. plugin.json's version bump and CHANGELOG.md are documentation-only. Supply-chain/pin-related and workflow-permission concerns are out of scope here (covered by zizmor) and don't apply regardless — no workflow or dependency-pin changes in this diff.

[claude]

Claude finished @kyle-sexton's task in 48s —— View job

---

Security Review

• Gather context (diff vs origin/main, changed files)
• Review plugins/powershell-format/hooks/powershell-format.sh $loaders change
• Check whether previously-flagged alias gap (icm/sajb) was addressed
• Review plugin.json / CHANGELOG.md changes
• Post findings

This PR extends the PSScriptAnalyzer CustomRulePath trust gate's $loaders list to also cover Invoke-Command and Start-Job, closing the gap where a computed -FilePath on those cmdlets could execute an un-hashed file without tripping PSSA_TRUST UNPINNABLE. At HEAD (cf37cc7), the diff is unchanged from the version reviewed in the earlier automated passes on this PR — the previously-reported finding below is still present.

IMPORTANT — Invoke-Command/Start-Job still bypassable via their built-in aliases (icm, sajb)

Confidence: High

hooks/powershell-format.sh#L368-L370

$loaders = @("Import-Module", "ipmo", "Add-Type", "New-Module",
    "Invoke-Expression", "iex", "Import-PowerShellDataFile",
    "Invoke-Command", "Start-Job")

$loaders is matched against the literal string from $cmdAst.GetCommandName() (L375), with no alias-table resolution anywhere in this file. The two other aliasable loaders already account for this — Import-Module/ipmo and Invoke-Expression/iex are both listed — but the two cmdlets this PR adds are listed only by their full names. Invoke-Command's built-in alias is icm and Start-Job's is sajb; neither is present.

A custom rule module written as:

icm -FilePath $computedPath

parses to a CommandAst whose GetCommandName() is "icm", which $loaders -contains does not match, so $isLoad stays false, the loop at L398–417 never inspects the composed -FilePath value, the file is never queued into $pending/hashed into the reachable-file set, and PSSA_TRUST UNPINNABLE never fires. This is exactly the "approval would then survive arbitrary edits to it" scenario the surrounding comments (L337–346) describe — reached through the alias instead of the full name — and it re-opens the specific hole this PR's title says it closes, just for the two newly-added loaders.

Suggested fix: add "icm" and "sajb" to the $loaders array, consistent with how ipmo/iex are already handled.

Fix this →

---

Out of scope note: Codex's review comment on this PR (that adding these two names makes the L398–417 loop treat every non-parameter argument — e.g. -ComputerName, -ArgumentList values on ordinary -ScriptBlock calls — as an unpinnable load target) is a real functional over-refusal risk, but it fails closed (blocks formatting) rather than open (lets untrusted code through unchecked), so it's a correctness/availability bug rather than a security vulnerability and is left out of this review's scope.

No other security issues found in this diff. plugin.json's version bump (0.7.7 → 0.7.8, matching the CHANGELOG entry) and the CHANGELOG.md addition are documentation-only. Supply-chain/pin-related and workflow-permission concerns are out of scope here (covered by zizmor) and don't apply regardless — no workflow or dependency-pin changes in this diff.

[claude]

Claude finished @kyle-sexton's task in 2m 42s —— View job

---

Code Review

• Gather context (diff vs origin/main, changed files, REVIEW.md scope)
• Review plugins/powershell-format/hooks/powershell-format.sh for correctness
• Review plugin.json / CHANGELOG.md
• Post findings

Per this repo's REVIEW.md, this lane owns correctness/design/tests and leaves security findings to the dedicated claude-security-review lane (which already flagged the icm/sajb alias bypass on this PR — not repeated here).

One correctness finding posted inline on hooks/powershell-format.sh line 370: adding Invoke-Command/Start-Job to $loaders extends the existing pin-or-refuse loop (L398-415) to these two cmdlets, but that loop treats every non-parameter argument as a load target requiring pinning, not just the one bound to -FilePath. Unlike the previously-listed loaders (which effectively take one meaningful path/name argument), Invoke-Command/Start-Job commonly take several unrelated variable arguments (-ComputerName, -ArgumentList, -Session, ...). Ordinary remoting/job-control calls using those parameters with variables will now hit PSSA_TRUST UNPINNABLE and permanently block trust approval for a rule module that loads no file at all — with no approval route. Codex's review comment on this same line raised the identical concern; both security-lane passes on this PR explicitly deferred it as a correctness issue outside their scope, so it's captured here.

plugin.json's version bump (0.7.7 → 0.7.8) and the CHANGELOG.md entry are consistent with a Fixed patch release and accurately describe the intended change. No test suite exists for this plugin's hook script (none did before this PR either), so I'm not flagging test coverage as a regression introduced here.

[claude]

Correctness: adding Invoke-Command/Start-Job to $loaders will false-positive on ordinary remoting calls that load no file

The loop that consumes $loaders (L398-415) doesn't check which parameter a value belongs to — it skips only the CommandParameterAst tokens themselves (-ComputerName, -ArgumentList, -ScriptBlock, ...) and then treats every remaining non-constant command element as a load target requiring pinning, else it refuses with PSSA_TRUST UNPINNABLE (exit 6) at L415.

For the loaders added before this PR (Import-Module, Add-Type, New-Module, Invoke-Expression, Import-PowerShellDataFile), that's mostly harmless because those cmdlets effectively take one meaningful path/name argument. Invoke-Command and Start-Job break that assumption — they commonly take several variable arguments that have nothing to do with loading a file, e.g.:

Invoke-Command -ComputerName $server -ScriptBlock { ... } -ArgumentList $value
Start-Job -ScriptBlock { ... } -ArgumentList $value

Here $server/$value are VariableExpressionAst nodes, not StringConstantExpressionAst/ExpandableStringExpressionAst with fully-expandable content, so they fall through to the final else at L415 and the scan exits UNPINNABLE — permanently blocking the trust approval for a rule module that never loads a file this way, with "no approval route" (as Codex's review on this same line noted).

Since this hook's opt-in trust gate exists specifically so legitimate CustomRulePath modules can be approved (see README's Trust model section), a rule module using either cmdlet for ordinary remoting/job control — not file loading — would be unable to ever pass the gate. Given Invoke-Command/Start-Job are far more likely to appear in non-loading remoting/job-control usage than the previous loaders, suggest scoping the pinning check to the value bound to -FilePath specifically (as Codex suggested), rather than every non-parameter element, for these two entries.

Fix this →

[github-actions]

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.