microsoft / PSRule

Validate infrastructure as code (IaC) and objects using PowerShell rules.
https://microsoft.github.io/PSRule/v2/
MIT License
396 stars 49 forks source link

v2.5.0 subtle regression due to using AppContext.BaseDirectory #1343

Closed nightroman closed 1 year ago

nightroman commented 1 year ago

v2.4.2 works fine with the following command:

Get-PSRule .\Some.Rule.ps1

Starting from v2.5.0 the above may fail in a particular app hosting .NET Core and PS Core:

GetSource: Exception calling "RuleSource" with "2" argument(s): "Value cannot be null. (Parameter 'path2')"

In v2.6.0 the underlying exception and stack is (from debugger):

"System.ArgumentNullException: Value cannot be null. (Parameter 'path2')\r\n
   at System.ArgumentNullException.Throw(String paramName)\r\n
   at System.IO.Path.Combine(String path1, String path2)\r\n
   at PSRule.Configuration.PSRuleOption.GetRootedPath(String path, Boolean normalize, String basePath) in /_/src/PSRule/Configuration/PSRuleOption.cs:line 546\r\n
   at PSRule.Configuration.PSRuleOption.GetRootedBasePath(String path, Boolean normalize) in /_/src/PSRule/Configuration/PSRuleOption.cs:line 561\r\n
   at PSRule.Engine.GetLocalPath() in /_/src/PSRule/Common/Engine.cs:line 14\r\n
   at PSRule.Pipeline.SourcePipelineBuilder..ctor(IHostContext hostContext, PSRuleOption option) in /_/src/PSRule/Pipeline/SourcePipeline.cs:line 129\r\n
   at CallSite.Target(Closure, CallSite, Type, PSRuleOption, Object)"

The culprit is this method in the newly added Engine.cs:

    internal static string GetLocalPath()
    {
        return PSRuleOption.GetRootedBasePath(Path.GetDirectoryName(AppContext.BaseDirectory));
    }

and this change (the old code apparently worked fine):

src/PSRule/Pipeline/SourcePipeline.cs
-            _LocalPath = PSRuleOption.GetRootedBasePath(Path.GetDirectoryName(typeof(SourcePipelineBuilder).Assembly.Location));
+            _LocalPath = Engine.GetLocalPath();

The problem is that AppContext.BaseDirectory is an empty string in my scenario. The scenario is:

If you need to install the above bits and reproduce the issue, it's possible, I will guide. Or instead we may try find and use something more reliable than AppContext.BaseDirectory.

What is the assumption about AppContext.BaseDirectory (and _LocalPath)? What is it expected to get (and _LocalPath to be)? Per the docs:

The file path of the base directory that the assembly resolver uses to probe for assemblies.
...
In .NET 5 and later versions, for bundled assemblies, the value returned is the containing directory of the host executable.

Is _LocalPath supposed to be based on a directory for probing assemblies or something else by design? Having this answer we may find a more reliable way of getting this value.

If I get the code right:

nightroman commented 1 year ago

Also, the code Path.GetDirectoryName(AppContext.BaseDirectory) looks a little "not safe". Here is for example what I get by [AppContext]::BaseDirectory in my pwsh and powershell:

C:\Bin\pwsh\
C:\windows\System32\WindowsPowerShell\v1.0\

Note the trailing backslashes. Path.GetDirectoryName returns:

C:\Bin\pwsh
C:\windows\System32\WindowsPowerShell\v1.0

Now imaging AppContext.BaseDirectory were returning the same paths but without trailing backslashes. Then we would get:

> [IO.Path]::GetDirectoryName('C:\Bin\pwsh')
C:\Bin

> [IO.Path]::GetDirectoryName('C:\windows\System32\WindowsPowerShell\v1.0')
C:\windows\System32\WindowsPowerShell

I.e. we get different directories. This all looks a little strange. What was the original intent? What directory should be obtained by Engine.GetLocalPath()?

BernieWhite commented 1 year ago

@nightroman Thanks for reporting the issue.

The paths that you've provided look correct.

_LocalPath is used for a specific case around CLI hosting, where the assembly is used directly instead of from within a PowerShell host. It's not required in the case you have outlined, so it probably just need some additional handling and test cases.

nightroman commented 1 year ago

The paths that you've provided look correct.

By chance, as I described...

What is the code supposed to do by Path.GetDirectoryName(AppContext.BaseDirectory)?

It does the latter.

But in any case, regardless of this, in my scenario AppContext.BaseDirectory is an empty string, so Path.GetDirectoryName returns null and this null causes exception later in Path.Combine.

By the way, I think it all might work fine if you remove Path.GetDirectoryName call. My case included because an empty string (not null) might be processed fine later. But I'm just speculating.

nightroman commented 1 year ago

If you want me to test an intermediate beta, please let me know, will do any time.

BernieWhite commented 1 year ago

The paths that you've provided look correct.

By chance, as I described...

What is the code supposed to do by Path.GetDirectoryName(AppContext.BaseDirectory)?

  • get the parent of AppContext.BaseDirectory? (as GetDirectoryName usually does) Then why is it?
  • or trim the trailing backslash? (the choice of the method is strange then)

It does the latter.

But in any case, regardless of this, in my scenario AppContext.BaseDirectory is an empty string, so Path.GetDirectoryName returns null and this null causes exception later in Path.Combine.

By the way, I think it all might work fine if you remove Path.GetDirectoryName call. My case included because an empty string (not null) might be processed fine later. But I'm just speculating.

Sure I get the point Path.GetDirectoryName is probably not required, it was required for the previous implementation because it was a file path but not now just AppContext.BaseDirectory. Thanks for the call out.

Either way some additional test cases are a good idea and Engine.GetLocalPath() can be optimized out for some cases.

nightroman commented 1 year ago

@BernieWhite 2.7.0-B0001 works fine in my unusual scenario, thank you.