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-Expression validates nonexistent parameter default values. #8778

Closed
mklement0 opened this issue Jan 29, 2019 · 15 comments
Closed

Invoke-Expression validates nonexistent parameter default values. #8778

mklement0 opened this issue Jan 29, 2019 · 15 comments
Labels
Issue-Bug Issue has been identified as a bug in the product Resolution-No Activity Issue has had no activity for 6 months or more WG-Engine core PowerShell engine, interpreter, and runtime

Comments

@mklement0
Copy link
Contributor

mklement0 commented Jan 29, 2019

Note: The repro below uses the ValidateSet attribute to demonstrate the problem, but at least one other attribute is affected too, namely ValidatePattern - possibly more. Presumably, fixing the problem for one attribute also fixes it for the others.

Update:

  • The core issue is that when script block text with parameter declarations is invoked via Invoke-Expression, default-parameter-value validation for parameters that have Validate* attributes takes place in an overzealous manner: even if there is no default value, the implied value of $null is evaluated against the validation attributes - however, it should be possible to simply omit a default value for validation-constrained parameters (and that is how it works with direct invocations of functions / scripts / script-block literals).

  • As an aside: The helpful aspect of this validation - namely if there is a default value - is currently missing from direct function / script / script-block literal invocations; remedying that is the subject of For validation-constrained function parameters, prevent invalid default values. #8795.

Steps to reproduce

. { param([ValidateSet("one", "two")] $foo) "hi" }
# This command should be equivalent to the above.
Invoke-Expression 'param([ValidateSet("one", "two")] $foo) "hi"'

Expected behavior

hi
hi

Actual behavior

hi
Invoke-Expression : The attribute cannot be added because variable foo with value  would no longer be valid.
PowerShell Core v6.2.0-preview.3 on macOS 10.14.2
PowerShell Core v6.2.0-preview.3 on Ubuntu 18.04.1 LTS
PowerShell Core v6.2.0-preview.3 on Microsoft Windows 10 Pro (64-bit; Version 1803, OS Build: 17134.471)
Windows PowerShell v5.1.17134.407 on Microsoft Windows 10 Pro (64-bit; Version 1803, OS Build: 17134.471)
@mklement0 mklement0 added the Issue-Bug Issue has been identified as a bug in the product label Jan 29, 2019
@BrucePay
Copy link
Collaborator

@mklement0 If you add a valid initializer then it's fine

PSCore (2:59) >  Invoke-Expression 'param([ValidateSet("one", "two")] $foo = "one") "hi"'
hi

Note that the behaviour is the same with the InvokeScript method:

$ExecutionContext.InvokeCommand.InvokeScript('param([ValidateSet("one", "two")] $foo) "hi"')

as well as

{param([ValidateSet("one", "two")] $foo) "hi}.Invoke()

Why the error isn't raised when running a script does warrant further investigation.

@vexx32
Copy link
Collaborator

vexx32 commented Jan 29, 2019

@BrucePay that discrepancy is weird. This pattern of declaring a parameter with a validation attribute and then not giving it a default value is pretty common and has been the way it is understood to work for a long time.

I think the better question is why is this error appearing in this context when this pattern has been used for a long time now in declaring parameters. 😕

@SeeminglyScience
Copy link
Collaborator

SeeminglyScience commented Jan 29, 2019

The scenarios that fail use ScriptBlock.InvokeWithPipeImpl (or any of the other *Invoke* methods that eventually hit it)

The scenarios that don't fail are from a compiled command AST, which creates a command processor instead.

More specifically, here's the line from the InvokeWithPipe code path where it fails. I believe it should use PSVariable.AddParameterAttributesNoChecks instead of that constructor.

Note Same thing happens with the Where/ForEach magic methods as well as ForEach-Object

@mklement0
Copy link
Contributor Author

mklement0 commented Jan 29, 2019

Thanks, @vexx32 and @SeeminglyScience.

@BrucePay:

@vexx32 is correct that not specifying a value has always worked and should work in all situations.
In fact, even specifying an invalid default value has always worked:

PS> . { param([ValidateSet("one", "two")] $foo = "invalid") $foo }
invalid

Also, the fact that & ([scriptblock]::Create('param([ValidateSet("one", "two")] $foo = "invalid") $foo'))') works fine too supports @SeeminglyScience's findings.

I haven't delved into the code, but, functionally, it seems that the default value / lack thereof is validated when it shouldn't be.

@mklement0
Copy link
Contributor Author

Thinking about this some more, based on @BrucePay's response and @SeeminglyScience's findings:

  • It sounds like the - sensible - original design intent was indeed to check whether a validation-constrained parameter's default value passes the validation and to complain at parse time, if not.

  • Currently only the Invoke-Expression code path enforces that check, but it does so overzealously, because not using a default value at all should definitely be considered acceptable.

  • Conversely, the fact that regular invocation - perhaps accidentally - bypass the check opened the door for behavior that is arguably too permissive:

    • As demonstrated, default values that are invalid in terms of the parameter's validation attributes are happily accepted.

I see two possible resolutions:

  • Make Invoke-Expression invocations as permissive as the AST-based invocations.

  • Fix the Invoke-Expression invocations to accept the case where a validation-constrained parameter has no default value - while still enforcing the validation of default values - and make AST-based invocations do the same.

    • Caveat: Technically, that would be a breaking change, as it is conceivable that there are functions out there that use an invalid default value as an explicit signal that no argument was passed, along the lines of: . { param([ValidateSet("one", "two")] $foo="<none>") $foo }

@SeeminglyScience
Copy link
Collaborator

@mklement0 I believe it's just a bug. I think the ScriptBlock.InvokeWithPipeImpl code path should just use the PSVariable.AddParameterAttributesNoChecks method, like the AST compiler code path already does.

@mklement0
Copy link
Contributor Author

@SeeminglyScience: That's definitely the most straightforward way to resolve this issue.

However, the question is whether we see value in preventing invalid default values, which currently doesn't happen (and the bug fix wouldn't give us).

I personally see value in that (e.g., during refactoring of a validation set you may forget to update the default value accordingly), but there are two considerations:

  • backward compatibility, as stated above

  • performance (introducing additional checks; seemingly even script blocks for ValidateScript attributes are executed during the validation; e.g.: iex 'param([ValidateScript({ $_ -eq "A" })] $foo="B") $foo')

@vexx32
Copy link
Collaborator

vexx32 commented Jan 30, 2019

@mklement0 while I do agree that preventing invalid default values is probably a good idea — after all, one can generally check $PSBoundParameters.ContainsKey() if there is a need to verify if a particular parameter is supplied — I think it would be best if that were questioned in a separate issue, rather than attempting to answer both questions in this one issue. 🙂

@mklement0
Copy link
Contributor Author

Good point, @vexx32 - please see #8795

@mklement0 mklement0 changed the title [BUG] Invoke-Expression fails on encountering certain parameter validation attributes [BUG] Invoke-Expression validates nonexistent parameter default values. Jan 30, 2019
@iSazonov iSazonov added the WG-Engine core PowerShell engine, interpreter, and runtime label Feb 2, 2019
@iSazonov iSazonov changed the title [BUG] Invoke-Expression validates nonexistent parameter default values. Invoke-Expression validates nonexistent parameter default values. Feb 2, 2019
@johngeorgef1
Copy link

Also interestingly it seems that if you declare a parameter with validation, then change the value of that variable inside the script to something that is not a valid value; it will throw this exception.

e.g.

Save this as a file and execute, supplying one of the three valid options for this parameter [ biff, bash, bosh ];

[Cmdletbinding()]
Param(
    [Parameter(Mandatory=$True)]
    [ValidateSet("biff","bash","bosh")]
    [String]$Word
)

$ErrorActionPreference = "Stop";
Write-Host "Let's give this thing something to do";

$Word = "boosh"; # This will throw an exception

aaand..

PS C:\DevOps\Snips> .\param-validation.ps1 biff
Let's give this thing something to do
C:\DevOps\Snips\param-validation.ps1 : The variable cannot be validated because the value boosh is not a valid value for the Word variable.
At line:1 char:1
+ .\param-validation.ps1 biff
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : MetadataError: (:) [param-validation.ps1], ValidationMetadataException
    + FullyQualifiedErrorId : ValidateSetFailure,param-validation.ps1
 

PS C:\DevOps\Snips> 

It's quite interesting to learn that the params are continually evaluated to see whether they confirm to the validation attributes.

-J

@mklement0
Copy link
Contributor Author

@johngeorgef1, this is actually standard behavior, and not specific to Invoke-Expression use:

& { param([ValidateRange(1,2)] $foo) $foo = 3 }
MetadataError: The variable cannot be validated because the value 3 is not a valid value for the foo variable.

It is not well known, but you can apply constraints to any variable, not just a parameter variable:

[ValidateRange(1,2)] $foo = 1;  # regular variable with validation attribute
$foo = 3 # same error as above

While that is a powerful feature, constraints that apply to parameter variables only are inappropriately applied in at least one case: #10426

Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

2 similar comments
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

@microsoft-github-policy-service microsoft-github-policy-service bot added Resolution-No Activity Issue has had no activity for 6 months or more labels Nov 16, 2023
Copy link
Contributor

This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug Issue has been identified as a bug in the product Resolution-No Activity Issue has had no activity for 6 months or more WG-Engine core PowerShell engine, interpreter, and runtime
Projects
None yet
Development

No branches or pull requests

6 participants