Use type conversion in SSHConnection hashtables when value doesn't match expected type
#10720
Conversation
Use LanguagePrimitives.ConvertTo in GetSSHConnectionStringParameter and GetSSHConnectionIntParameter.
| private static string GetSSHConnectionStringParameter(object param) | ||
| { | ||
| if (param is string paramValue && !string.IsNullOrEmpty(paramValue)) | ||
| string paramValue = LanguagePrimitives.ConvertTo<string>(param); |
There was a problem hiding this comment.
ConvertTo() can throw PSInvalidCastException but we should keep PSArgumentException.
There was a problem hiding this comment.
Is there a circumstance where converting to string will throw?
There was a problem hiding this comment.
Yes, it is not common but possible (if someone creates ToString() method which throw).
| } | ||
|
|
||
| throw new PSArgumentException(RemotingErrorIdStrings.InvalidSSHConnectionParameter); | ||
| return LanguagePrimitives.ConvertTo<int>(param); |
Any thoughts on that? If I try to test that the conversion works then we'll hit the interactive SSH prompt. I could give it a bad port and/or hostname, but that'll add ~5 seconds per test. If that's acceptable I'll add a few of those. |
|
I'd expect 2 tests for these two code paths modified. |
|
@iSazonov I've added tests and normalized the exception type. In the tests I used port |
|
|
||
| It "<testName>" -TestCases $TestCasesSSHConnection { | ||
| param($scriptBlock) | ||
| { & $scriptBlock } | Should -Throw -ErrorId '2100,PSSessionOpenFailed' |
There was a problem hiding this comment.
It is out of the PR but I'd prefer to see more useful message.
|
@SeeminglyScience Thanks for your contribution! |
|
🎉 Handy links: |
PR Summary
Use
LanguagePrimitives.ConvertTowhen reading a hashtable provided to theSSHConnectionparameter.PR Context
Currently if the hashtable value does not match the expected type exactly, an exception is thrown stating that the value was
null.Fixes #10687
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.