Powershell: New-ModuleManifest accepts invalid entry values / Update-ModuleManifest complains when adding an invalid entry value, yet still updates the manifest.

[mklement0]

_Update_:

This issue was originally about Update-ModuleManifest, which is maintained in the PowerShellGet repository. However, Update-ModuleManifest calls this repository's New-ModuleManifest cmdlet, which itself exhibits problematic behavior:

New-ModuleManifest tmp.psd1 -RootModule '\/'  # invalid value is quietly accepted.

---

This is a regression from Windows PowerShell.

I've only tried with one entry, RootModule, where the bug only surfaces if the input manifest file does _not_ have the entry being updated yet.

Steps to reproduce

# Important: create the manifest *without* -RootModule
New-ModuleManifest tmp.psd1
# Try to update (add) the RootModule entry with an invalid value.
Update-ModuleManifest tmp.psd1 -RootModule '\/'
'---'
# Extract the now-current RootModule entry.
Select-String ^RootModule tmp.psd1

Expected behavior

Microsoft.PowerShell.Core\Test-ModuleManifest : The module manifest '\/' could not be processed because it is not a valid PowerShell module manifest file.
...
---

That is, an error should be reported, and the manifest file should remain unaltered, i.e., still have no RootModule entry.

Actual behavior

Microsoft.PowerShell.Core\Test-ModuleManifest : The module manifest '\/' could not be processed because it is not a valid PowerShell module manifest file.
...
---

tmp.psd1:12:RootModule = '\/'

That is, the manifest file was unexpectedly still updated with the invalid entry.

Environment data

PowerShell Core 6.2.0-preview.1 on macOS 10.14.1
PowerShell Core 6.2.0-preview.1 on Ubuntu 16.04.5 LTS
PowerShell Core 6.2.0-preview.1 on Microsoft Windows 10 Pro (64-bit; Version 1803, OS Build: 17134.345)

[pougetat]

Hey @mklement0 ,
Do you mind if I work on this ? :)

[mklement0]

Thanks for being willing to take this on, @pougetat.

It's certainly OK with me personally, but let's ask @SteveL-MSFT: Is this up for grabs?

[pougetat]

@mklement0 If I understand correctly, this cmdlet is implemented as a part of PowerShellGet. I'll at least take the liberty of recreating this issue in that repo ? :)

[mklement0]

@pougetat. Oops! Didn't even realize that - please do.

[pougetat]

@mklement0

Hey I started looking into this and the issue is not actually within PowerShellGet which works correctly but rather in here :

https://github.com/PowerShell/PowerShell/blob/4a63201139ce1625b43f1d5b7fb0d5c4b6d80cd5/src/System.Management.Automation/engine/Modules/NewModuleManifestCommand.cs#L1046

I'm not really sure why NewModuleManifest gets called when running Update-ModuleManifest instead of the script from PowershellGet that's included as a nuget package.

I think it would be a good idea to reopen this issue in this repo and can I close the one I opened in PowerShellGet ?

[mklement0]

Thanks for the sleuthing, @pougetat.

It sounds like you think that the proper fix is for Update-ModuleManifest not to call New-ModuleManifest. If so, then the issue belongs in the PowerShellGet repository after all, right?

[pougetat]

Hey @mklement0 ,

From my debugging I notice :

• First NewModuleManifest is called with the RootModule parameter (NewModuleManifest isn't a part of PowerShellGet)
• Later the actual PowerShellGet Update-ModuleManifest commandlet gets called.

NewModuleManifest does not check whether or not RootModule is valid so the issue is indeed within this method.

We could :

• Not call NewModuleManifest when running Update-ModuleManifest in which case an invalid RootModule would not be added to the manifest. Unfortunately this means that there is still an issue when creating a new manifest with an invalid RootModule (I'm guessing this is indeed an issue)

OR

• Add a verification within NewModuleManifest that checks for a valid RootModule.

[mklement0]

Thanks, @pougetat.

It sounds like New-ModuleManifest should be fixed in its own right, so I've reopened this issue with an updated description.

Knowing little about either cmdlet, I'm a bit confused: New-ModuleManifest itself seems capable of _updating_ existing manifests as well, so why was Update-ModuleManifest created (with different / additional functionality?)

[pougetat]

@mklement0
Very good question to which I don't know the answer. I also know little about these commandlets.

[vexx32]

/cc @SteveL-MSFT @vors as it seems they've worked with this file in the past and may be aware of some of the reasoning behind it. 🙂

[SteveL-MSFT]

Regarding New-ModuleManifest, this seems vaguely familiar and I think the reasoning is that you may use New-ModuleManifest before authoring rootmodule.psm1 so it shouldn't fail if the file doesn't exist. Update-ModuleManifest, however implies everything is working (and thus existing), so it seems that a fix would be to have Update-ModuleManifest do the verification rather than making any change in New-ModuleManifest.

[pougetat]

Hey @SteveL-MSFT

The issue is that simply executing Update-ModuleManifest in PowerShell calls first NewModuleManifest which modifies the current manifest without making verifications and then calls the actual Update-ModuleManifest cmdlet script.

The Update-ModuleManifest cmdlet does the required verifications and indicates that '\/' is an invalid root module but once we enter this script it is too late since NewModuleManifest has already been run as part of the same execution flow and has added an inexisting RootModule.

I was wondering why NewModuleManifest gets called as a part of the Update-ModuleManifest flow and more specifically why the line mentionned above gets executed. :)

I'll continue to investigate this tomorrow. :)

[SteveL-MSFT]

Update-ModuleManifest using New-ModuleManifest is an implementation detail. I can't say whether that is the right thing to do since I'm not familiar with the Update-ModuleManifest code. My suggestion is to have this conversation in the PSGet repo since that cmdlet belongs to PSGet. If the expectation is that New-ModuleManifest needs a -Verify type switch, we could add that, but if it's only used by Update-ModuleManifest, it may be better to do something specific in that code.

[pougetat]

@SteveL-MSFT Yes you're right. I got mixed up in the method calls and it seems that all important code pertaining to this issue is actually self-contained within Update-ModuleManifest within the PowerShellGet repo including a call to New-ModuleManifest. Sorry for wasting people's time :(.

[mklement0]

@SteveL-MSFT:

you may use New-ModuleManifest before authoring rootmodule.psm1 so it shouldn't fail if the file doesn't exist.

Deferring the _existence check_ is perfectly fine, but accepting _syntactically invalid_ values is a problem that should be fixed.

[SteveL-MSFT]

@mklement0 that is valid, doing a check if a FileInfo can be created with that name would be ok, however, "/" is valid on Unix as it resolves to "/". Of course it won't actually work, but the name itself is valid.

[mklement0]

@SteveL-MSFT: You're right, I didn't realize that \/ resolves to /, but the check that should be applied is whether the value is _formally_ a valid _file_ name or path.

So, yes, / wouldn't work and neither would anything _ending_ in /, or, on Windows, anything with embedded \ / : * ? " < > | or embedded newlines, with multiple :, ... - and these cases should be caught.

[SteveL-MSFT]

@mklement0 actually, I take it back "/" doesn't resolve to "/", but rather FileInfo allows this name:

PS /Users/steve/repos/PowerShell> [System.IO.FileInfo]::new("\/") | fl *

Mode              : darhs-
VersionInfo       : 
BaseName          : 
Target            : 
LinkType          : 
Length            : 
DirectoryName     : /Users/steve/\
Directory         : /Users/steve/\
IsReadOnly        : True
FullName          : /Users/steve/\/
Extension         : 
Name              : 
Exists            : False
CreationTime      : 1/1/01 9:18:00 AM
CreationTimeUtc   : 1/1/01 12:00:00 AM
LastAccessTime    : 1/1/01 9:18:00 AM
LastAccessTimeUtc : 1/1/01 12:00:00 AM
LastWriteTime     : 1/1/01 9:18:00 AM
LastWriteTimeUtc  : 1/1/01 12:00:00 AM
Attributes        : -1

In fact, it seems [FileInfo] doesn't do any validation which I thought it would so we can't rely on that. If there is some API in .Net to validate the filename is valid, I can see this being implemented.

[vexx32]

@SteveL-MSFT at least from a cursory glance, would the APIs leveraged in Test-Path -IsValid be a good start? Really the only thing missing _there_ is that a "filename" that ends in a trailing slash isn't really valid because it would be a folder path instead.

[pougetat]

@SteveL-MSFT, @mklement0
After some more investigations I have the following conclusion :

Update-ModuleManifest is still updating RootModule because of a call to Test-ModuleManifest wich is writing an error due to the new RootModule being invalid but is not throwing a terminating error. Therefore Update-ModuleManifest can't actually catch the error and not write the new module manifest to disk.

A solution (which I have already tested locally) is to make Test-ModuleManifest (which is a part of the PowerShell repo and not PowerShellGet) throw a terminating error upon finding an invalid root module instead of simply writing an error. I can open a PR today for it.

[SteveL-MSFT]

@pougetat that would be considered a breaking change, alternatively, when Update-ModuleManifest calls Test-ModuleManifest, it can use -ErrorAction Stop to make it terminating

[pougetat]

@SteveL-MSFT , @mklement0

I have opened a PR in the PowerShellGet repo to address this issue. :)