Fix incorrect name check when autoloading required modules by rjmholt · Pull Request #8218 · PowerShell/PowerShell

rjmholt · 2018-11-09T06:39:20Z

PR Summary

The change I made in #7125 made the check too strict on the module we try to autoload it from requires and rejected it based on name.

This PR removes that name check but also sets up a fix for the more general problem that a ModuleSpecification can always specify a path rather than a name. I added some reusable logic to make this easier to service.

The main contribution is regression testing for RequiredModules, #requires -Modules and other cmdlets that can use ModuleSpecifications.

However, the general pervasive problem is that ModuleSpecifications contain raw user input and we don't do very much about handling paths that might be given as module names, namely:

Normalising directory separators
Resolving relative paths (which should be resolved differently depending on context)
Dealing with case-sensitivity
Handling trailing slashes
Matching on module directories accounting for the versioned directory structure
Working with special directory names like . and ..

The methods I've added deal with these and I've applied them in one place to help correct the immediate problem and make it more efficient.

But the fact remains that we have no way in the type system to identify when this resolution has already occurred; there are over 120 uses of the ModuleSpecification type in the code base, and we don't have a way to differentiate when they contain raw user input and when they can be trusted to have a canonical path. The way the module cmdlets are written currently, we redo a fair amount of this validation and normalisation logic, and it's probably somewhere we could find serious perf improvements.

PR Checklist

bergmeister · 2018-11-09T07:15:44Z

Thanks for handling the issue. I'd consider a regression test for this important feature to be essential.

rjmholt · 2018-11-09T07:19:07Z

Agreed that it's essential -- I'm pretty surprised we didn't have one already.

I'm halfway through writing some already.

bergmeister · 2018-11-09T09:46:11Z

Ok, feel free to add the test in a 2nd PR as I can understand that this is a time-pressing issue that can initially be verified with some manual testing (please include Windows as test platform though)

rjmholt · 2018-11-09T10:56:18Z

We also seem to do required module autoloading in #requires pragmas:

PowerShell/src/System.Management.Automation/engine/CommandDiscovery.cs

Lines 307 to 313 in 4118fd2

 ModuleCmdletBase.LoadRequiredModule( 

 context: context, 

 currentModule: null, 

 requiredModuleSpecification: requiredModule, 

 moduleManifestPath: null, 

 manifestProcessingFlags: ModuleCmdletBase.ManifestProcessingFlags.LoadElements | ModuleCmdletBase.ManifestProcessingFlags.WriteErrors, 

 error: out error);

If those are supposed to allow paths, I'll need to do the same path normalisation logic there.

adityapatwardhan · 2018-11-09T19:17:33Z

@rjmholt 1 new test is failing on Linux and macOS. Please have a look.

… needed

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

rjmholt · 2018-11-12T23:15:39Z

The failing AppVeyor test is passing on my machine. Looking into it.

src/System.Management.Automation/engine/Modules/GetModuleCommand.cs

src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs

src/System.Management.Automation/engine/Modules/ModuleSpecification.cs

src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs

daxian-dbw · 2018-11-13T01:10:38Z

src/System.Management.Automation/engine/Modules/RemoveModuleCommand.cs

+                // Paths in the module name may fail here because
+                // they the wrong directory separator or are relative.
+                // Fix with the code below:
+                // FullyQualifiedName = FullyQualifiedName.Select(ms => ms.WithNormalizedName(Context, SessionState.Path.CurrentLocation.Path)).ToArray();


What's the concern about putting this fix in place? Regression? Perf degradation?

Just keeping the fix constrained in this PR. It shouldn't cause regressions or perf degradations, but to ensure there are no regressions (since this PR is a regression fix itself) I just left it as a comment.

Will you open issue to track all those todo comments? They can be easily forgotten :)

Did you open an issue to track those todo's?

Next on my list

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs

daxian-dbw · 2018-11-13T21:30:07Z

src/System.Management.Automation/engine/Modules/RemoveModuleCommand.cs

+                // Paths in the module name may fail here because
+                // they the wrong directory separator or are relative.
+                // Fix with the code below:
+                // FullyQualifiedName = FullyQualifiedName.Select(ms => ms.WithNormalizedName(Context, SessionState.Path.CurrentLocation.Path)).ToArray();


Did you open an issue to track those todo's?

daxian-dbw

Only pending action item left is to open an issue tracking all "TODO" comments.

adityapatwardhan · 2018-11-13T22:52:35Z

Since the CodeFactor issues are for existing methods, re-ordering will cause larger diffs. We can ignore CodeFactor issues for this PR.

Skip name check when loading required modules

c6dcb43

rjmholt requested review from BrucePay and daxian-dbw as code owners November 9, 2018 06:39

iSazonov requested review from TravisEz13 and adityapatwardhan November 9, 2018 06:43

iSazonov assigned adityapatwardhan Nov 9, 2018

rjmholt added 2 commits November 8, 2018 23:47

[Feature] Fix null ref with max version

4e693d2

[Feature] Fix path comparisons with required modules

ae5c56b

rjmholt force-pushed the fix-required-module-load-failure branch from 3205e0d to ae5c56b Compare November 9, 2018 09:05

rjmholt added 4 commits November 9, 2018 01:55

[Feature] Match module when required path is base dir

b3c9a87

[Feature] Trim end of required module path

5c04829

[Feature] Add testing for required module autoloading failure

28981df

[Feature] Address CodeFactor issues

cea4dd5

[Feature] Make relative paths work in module specification checks

b8e513a

rjmholt requested a review from Francisco-Gamino as a code owner November 9, 2018 20:16

rjmholt added 10 commits November 9, 2018 12:44

[Feature] Add copyright header

8a843ce

[Feature] Remove unused vars from tests

fa950a1

[Feature] Add more testing for relative paths in fullyqualifiednames

2d9cf9c

[Feature] Ensure required module path is absolute at autoloading time

55f8a7d

[Feature] Update Get-Module test to account for version ignoring quirk

d3b9651

[Feature] Reduce complexity of fix - normalize module spec path where…

980b8f0

… needed

[Feature] Revert further changes

0a414ff

[Feature] Add path normalization method to ModuleSpecification

9163bc0

[Feature] Fix module specification method call

59cf3d1

[Feature] Revert unneeded changes - change to TODOs.

5b4bd92

rjmholt commented Nov 12, 2018

View reviewed changes

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs Show resolved Hide resolved

daxian-dbw reviewed Nov 13, 2018

View reviewed changes

rjmholt added 5 commits November 12, 2018 18:48

Make required name null check clearer

5950e75

Protect against path resolution failure

cbc650e

Fix Path.AltDirectorySeparatorChar misuse

87a9704

[Feature] Fix non-const const assignment

2378bb8

[Feature] Undo change to ModuleCmdletBase.IsRooted

9faf21e

daxian-dbw reviewed Nov 13, 2018

View reviewed changes

Robert Holt added 6 commits November 13, 2018 11:27

[Feature] Improve TODOs in GetModuleCommand.cs

60ece17

[Feature] Add comments to module path matching logic

c21eb25

[Feature] Fix up path normalization code

336ba07

[Feature] Add extra fallback normalization

30c07b8

[Feature] Fix IsModulePath check method

e381a50

[Feature] Add full stop to satisfy code factor

9344345

daxian-dbw reviewed Nov 13, 2018

View reviewed changes

[Feature] Fix comment typo

389b245

daxian-dbw approved these changes Nov 13, 2018

View reviewed changes

adityapatwardhan merged commit aa76c68 into PowerShell:master Nov 13, 2018

This was referenced Nov 13, 2018

ModuleSpecification/FullyQualifiedName usage does not handle paths well #8261

Closed

Get-Module [-ListAvailable] -FullyQualifiedName with path ignores other constraints #8262

Closed

bergmeister mentioned this pull request Nov 14, 2018

RequiredModules manifest property stopped working recently (noticed in daily build) #8204

Closed

iSazonov added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Jan 17, 2019

rjmholt mentioned this pull request Feb 21, 2019

Refactor ImportModule commandlet #8936

Closed

Conversation

rjmholt commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

bergmeister commented Nov 9, 2018

Uh oh!

rjmholt commented Nov 9, 2018

Uh oh!

bergmeister commented Nov 9, 2018

Uh oh!

rjmholt commented Nov 9, 2018

Uh oh!

adityapatwardhan commented Nov 9, 2018

Uh oh!

Uh oh!

rjmholt commented Nov 12, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daxian-dbw Nov 13, 2018

Choose a reason for hiding this comment

Uh oh!

rjmholt Nov 13, 2018

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Nov 13, 2018

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Nov 13, 2018

Choose a reason for hiding this comment

Uh oh!

rjmholt Nov 13, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daxian-dbw Nov 13, 2018

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Nov 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

rjmholt commented Nov 9, 2018 •

edited

Loading

daxian-dbw left a comment •

edited

Loading