Skip to content
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

Invoke-Command -SSHConnection breaks if the HostName string is [psobject]-wrapped #10687

Closed
mklement0 opened this issue Oct 2, 2019 · 6 comments · Fixed by #10720

Comments

@mklement0
Copy link
Contributor

commented Oct 2, 2019

The root cause is that a string returned from a cmdlet call or external program has an invisible extra [psobject] wrapper, and the Invoke-Command cmdlet fails to take that possibility into account.

In short: The command below breaks, because (hostname) has such an invisible wrapper, as evidenced by (hostname) -is [psobject] being $true.

This is yet another manifestation of our old friend #5579.

The specific problem in Invoke-Command is here.

Steps to reproduce

{ icm -SSHConnection @{ Hostname = (hostname) } -ScriptBlock { 'hi' }  } | 
  Should -not -Throw

Expected behavior

The test should succeed, if the local computer is set up for being a SSH remoting target.

Actual behavior

The test fails:

Expected no exception to be thrown, but an exception 
"The provided SSHConnection hashtable parameter name or element is null or empty." was thrown from line:1 char:3

Note that using (hostname).psobject.BaseObject or "$(hostname)" or (hostname).ToString() makes the problem go away.

As an aside: if the host name were truly invalid due not being a string, the error message would be misleading, because the problem is that the entry is of the wrong type, not that it is null or empty.

A further aside: The comment above the SSHConnection parameter definition is incomplete: keys Port and Subsystem are missing.

Environment data

PowerShell Core v7.0.0-preview.4
@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2019

We have LanguagePrimitives.TryConvertTo. Question is should we accept only PSObject with base string type or we need to try convert any PSObject to string?

private static string GetSSHConnectionStringParameter(object param)
{
if (param is string paramValue && !string.IsNullOrEmpty(paramValue))
{
return paramValue;
}
throw new PSArgumentException(RemotingErrorIdStrings.InvalidSSHConnectionParameter);
}

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

@iSazonov:

The current code accepts string only, and I think that's reasonable.

I don't think that users will gain anything if we support other types here - at least I don't see what other types might be useful in this scenario.

If we stick with the current approach, I recommend implementing amending the existing error message to say something like "element is null, empty, or of the wrong type".

@SeeminglyScience

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

I think it should follow the same path that a normal parameter binding would, so LanguagePrimitives.ConvertTo makes the most sense imo (probably don't need TryConvertTo, everything can be converted to a string in some way).

@mklement0

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

That makes sense, @SeeminglyScience - I expect the only real problem for end users to be the scenario detailed in this report.

However, the point about amending the error message still applies, because there's also GetSSHConnectionIntParameter for int values - where it's definitely possible to pass the wrong type.

Speaking of: The latter currently accepts an int value only; passing a long breaks, for instance.
Thus, I suggest modifying GetSSHConnectionIntParameter to use LanguagePrimitives.ConvertTo as well - see

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2019

@SeeminglyScience Can you grab this? I already have over 15 opened PRs but I'd prefer to get the null reference exception fixed before GA.

@SeeminglyScience

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

@iSazonov Yeah I can grab it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.