Invoke-Command: improve handling of variables with $using: expression by dwtaber · Pull Request #16113 · PowerShell/PowerShell

dwtaber · 2021-09-16T15:35:06Z

PR Summary

This PR improves Invoke-Command's handling of cases that combine $using: expressions with variable namespace notation when the -ComputerName parameter is used.
Fixes #9204
Fixes #16019

PR Context

When the -ComputerName parameter is used, Invoke-Command is unable to check which version of PowerShell is available on the remote computer and assumes v2.0. Because PowerShell 2.0 can't parse $using: expressions, the ScriptBlock is modified before sending, with $using: variables renamed to replace the $using: expression with $__using_. Currently, the code that renames these variables doesn't check whether the variable path is qualified. In cases where variable namespace notation is used, this code produces variable names like $__using_env:foo, preventing the remote computer from parsing the variables correctly. The changes in this PR add handling for qualified variable paths, producing variable names like $__using_env_foo

This PR does not fix all issues with the handling of $using: expressions. Variable names that include special characters still aren't parsed correctly, for instance. Since a user technically could name a variable $__using_env_foo, this might be considered an "unlikely grey area" breaking change. Long-term, it might be worth reconsidering how Invoke-Command handles the -ComputerName parameter; always falling back to v2.0 seems unnecessarily restrictive.

PR Checklist

ghost · 2021-09-16T15:35:18Z

All CLA requirements met.

iSazonov · 2021-09-16T16:52:15Z

When the -ComputerName parameter is used, Invoke-Command is unable to check which version of PowerShell is available on the remote computer and assumes v2.0.

I wonder why we need the workaround if we can prepare full fix.

If I understand correctly, with ComputerName we create new runspace but do not open it in CreateHelpersForSpecifiedComputerNames(), as result we can not detect remote PowerShell version. So I guess we need to open the new runspace in CreateHelpersForSpecifiedComputerNames().

dwtaber · 2021-09-16T17:41:11Z

When the -ComputerName parameter is used, Invoke-Command is unable to check which version of PowerShell is available on the remote computer and assumes v2.0.

I wonder why we need the workaround if we can prepare full fix.

If I understand correctly, with ComputerName we create new runspace but do not open it in CreateHelpersForSpecifiedComputerNames(), as result we can not detect remote PowerShell version. So I guess we need to open the new runspace in CreateHelpersForSpecifiedComputerNames().

Implementing proper version detection for the -ComputerName parameter seems preferable to me as well. Since the current behavior (falling back to v2) is documented at various points in the code comments, it seemed to be an intentional design choice, maybe based on factors I wasn't aware of. Honestly, this is the first pull request I've submitted, and I didn't want to overstep with a potentially significant change to how Invoke-Command connections work, so I went with a more conservative fix, but I'm not married to it.

dwtaber · 2021-09-22T12:56:53Z

Can I get some guidance on the two tests that have failed? It looks like Invoke-WebRequest failed to reach a URI, but I can't see how my changes would have affected that cmdlet. Is it possible the resource was just unavailable when the test was run?

I'd also appreciate any suggestions on writing a Pester test for this issue. I don't think any of the current remoting tests target a remote computer, so they don't offer much guidance. So far I've been testing against computers available to me, but obviously that's too context-dependent to be of use to anyone else. Are there any designated endpoints for this sort of testing?

iSazonov · 2021-09-22T16:20:33Z

I restarted CI-static.

ghost · 2021-09-30T02:00:41Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

rjmholt · 2021-10-05T18:54:48Z

                if (varAst != null)
                {
-                    string varName = varAst.VariablePath.UserPath;
+                    var varPath = varAst.VariablePath;


Please use the type name instead of var here

rjmholt · 2021-10-05T18:54:56Z

                if (astStartOffset >= endOffset) { break; }

-                string varName = varAst.VariablePath.UserPath;
+                var varPath = varAst.VariablePath;


rjmholt · 2021-10-05T18:55:09Z

                foreach (var varAst in usingVariables)
                {
-                    string varName = varAst.VariablePath.UserPath;
+                    var varPath = varAst.VariablePath;


rjmholt · 2021-10-05T22:19:22Z

Not sure what the state of testing is for $using: vars, but would be good if we had some to cover the code path this change affects

rjmholt · 2021-10-05T22:19:42Z

Tagging @PaulHigin for review as well

…sWithQualifiedVarPath

dwtaber · 2021-10-06T00:32:41Z

Git/GitHub novice question: when updating from the master branch, I should have used a fast-forward merge to avoid creating a separate merge commit, right? How much of a problem is it that I, uh, didn't do that?

iSazonov · 2021-10-06T04:26:29Z

@dwtaber Never mind - we use "squash and merge" to get one commit in main branch.

PaulHigin

Changes look good to me.

dwtaber · 2021-10-28T16:24:46Z

@rjmholt, judging by its file history, it looks like System.Management.Automation/engine/parser/ast.cs wasn't updated during the merge, so the bug addressed by this PR still exists.

daxian-dbw · 2021-10-28T16:52:33Z

@dwtaber Changes from this PR exists in the master branch. The PR will not be included in 7.2.0-rc.1 though. It will be included in 7.3.0-preview.1.

dwtaber · 2021-10-28T17:20:45Z

@dwtaber Changes from this PR exists in the master branch. The PR will not be included in 7.2.0-rc.1 though. It will be included in 7.3.0-preview.1.

Ah, okay. Thanks for clarifying!

ghost · 2021-12-16T19:58:42Z

🎉v7.3.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

God-damnit-all · 2021-12-23T00:41:36Z

I really wish this had made it into the stable release of 7.2, I try to avoid using preview-exclusive stuff in my code, so having to wait for 7.3's stable release really sucks.

Is there any chance this could be slipped into a 7.2 build, i.e. 7.2.2?

…PowerShell#16113)

TravisEz13 · 2022-03-02T19:51:13Z

@PowerShell/powershell-maintainers
We only take security fixes and regression to LTS. This is appropriate for 7.3. Rejecting back-port.

Fixes PowerShell#9204. Fixes PowerShell#16019.

c5c6882

dwtaber requested a review from daxian-dbw as a code owner September 16, 2021 15:35

ghost assigned rjmholt Sep 16, 2021

ghost added the Review - Needed The PR is being reviewed label Sep 30, 2021

rjmholt reviewed Oct 5, 2021

View reviewed changes

ghost removed the Review - Needed The PR is being reviewed label Oct 5, 2021

rjmholt requested a review from PaulHigin October 5, 2021 22:19

dwtaber and others added 2 commits October 5, 2021 19:50

Merge branch 'PowerShell:master' into InvokeCommandFixUsingExpression…

a38343a

…sWithQualifiedVarPath

Replace var with explicit typename for varPath variables

4acd059

PaulHigin approved these changes Oct 6, 2021

View reviewed changes

rjmholt enabled auto-merge (squash) October 6, 2021 17:43

rjmholt approved these changes Oct 6, 2021

View reviewed changes

rjmholt merged commit 53ac646 into PowerShell:master Oct 6, 2021

dwtaber deleted the InvokeCommandFixUsingExpressionsWithQualifiedVarPath branch October 6, 2021 20:56

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Oct 7, 2021

iSazonov modified the milestones: 7.2.0-preview.10, 7.2.0-rc.1 Oct 7, 2021

daxian-dbw removed this from the 7.2.0-rc.1 milestone Oct 28, 2021

This was referenced Dec 16, 2021

In remoting calls, combining the $using: scope with namespace variable notation is quietly ignored #16019

Closed

The $using: scope for remoting quietly accepts scope modifiers, but resolves to $null #9204

Closed

iSazonov added the Backport-7.2.x-Consider label Dec 23, 2021

TrapGodBrim pushed a commit to TrapGodBrim/PowerShell that referenced this pull request Jan 19, 2022

Invoke-Command: improve handling of variables with $using: expression (…

313281d

…PowerShell#16113)

TravisEz13 removed the Backport-7.2.x-Consider label Mar 2, 2022

Conversation

dwtaber commented Sep 16, 2021

PR Summary

PR Context

PR Checklist

Uh oh!

ghost commented Sep 16, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Sep 16, 2021

Uh oh!

dwtaber commented Sep 16, 2021

Uh oh!

dwtaber commented Sep 22, 2021

Uh oh!

iSazonov commented Sep 22, 2021

Uh oh!

ghost commented Sep 30, 2021

Uh oh!

rjmholt Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

rjmholt Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

rjmholt Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

rjmholt commented Oct 5, 2021

Uh oh!

rjmholt commented Oct 5, 2021

Uh oh!

dwtaber commented Oct 6, 2021

Uh oh!

iSazonov commented Oct 6, 2021

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

dwtaber commented Oct 28, 2021

Uh oh!

daxian-dbw commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwtaber commented Oct 28, 2021

Uh oh!

ghost commented Dec 16, 2021

Uh oh!

God-damnit-all commented Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisEz13 commented Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ghost commented Sep 16, 2021 •

edited by ghost

Loading

daxian-dbw commented Oct 28, 2021 •

edited

Loading

God-damnit-all commented Dec 23, 2021 •

edited

Loading