Skip to content

CommandSearcher: treat invalid wildcard path as literal #7399

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.

Sign up for GitHub

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

Closed
kwkam wants to merge 2 commits into from
Closed

CommandSearcher: treat invalid wildcard path as literal #7399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41e20d7
[Feature] CommandSearcher: treat invalid wildcard path as literal
kwkam Apr 7, 2018
f915ade
Merge branch 'master' into wc-cmdsearch
TravisEz13 May 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/CommandSearcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1129,7 +1129,7 @@ private static bool ShouldSkipCommandResolutionForConstrainedLanguage(CommandInf
resolvedPath = GetNextLiteralPathThatExists(path, out provider);
}

if (WildcardPattern.ContainsWildcardCharacters(path) &&
if (WildcardPattern.ContainsValidWildcardPattern(path) &&
((resolvedPath == null) || (provider == null)))
{
// Let PowerShell resolve relative path with wildcards.
Expand Down
4 changes: 2 additions & 2 deletions src/System.Management.Automation/engine/GetCommandCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public string[] Name
{
foreach (string commandName in value)
{
if (WildcardPattern.ContainsWildcardCharacters(commandName))
if (WildcardPattern.ContainsValidWildcardPattern(commandName))
{
_nameContainsWildcard = true;
break;
Expand Down Expand Up @@ -812,7 +812,7 @@ private void AccumulateMatchingCommands(IEnumerable<string> commandNames)
moduleName = this.Module[0];
}

bool isPattern = WildcardPattern.ContainsWildcardCharacters(plainCommandName) || UseAbbreviationExpansion || UseFuzzyMatching;
bool isPattern = WildcardPattern.ContainsValidWildcardPattern(plainCommandName) || UseAbbreviationExpansion || UseFuzzyMatching;
if (isPattern)
{
options |= SearchResolutionOptions.CommandNameIsPattern;
Expand Down
64 changes: 64 additions & 0 deletions src/System.Management.Automation/engine/regex.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,70 @@ public static bool ContainsWildcardCharacters(string pattern)
return result;
}

/// FIXME
/// For invalid wildcard pattern, it currently only check if there is
/// unclosed bracket. Need to find a better solution and rewrite this.
///
/// <summary>
/// Checks to see if the given string might contains wildcard pattern.
/// </summary>
/// <param name="pattern">
/// String which needs to be checked
/// </param>
/// <returns>
/// true if the string does not contain invalid wildcard pattern,
/// false otherwise.
/// </returns>
/// <remarks>
/// Currently string contains { '*', '?' } or both { '[', ']' } is
/// considered to be properly a wildcard pattern and
/// '`' is the escape character.
/// </remarks>
internal static bool ContainsValidWildcardPattern(string pattern)
Copy link
Copy Markdown
Collaborator

@rjmholt rjmholt Apr 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of extra cases this method should reject:

  • "[a-mt]ook"
  • "[ab-z]ook"
  • "[a?b]"
  • "[a*]x"
  • "goose]"

Copy link
Copy Markdown
Collaborator

@rjmholt rjmholt Apr 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth putting these into test cases

Copy link
Copy Markdown
Contributor Author

@kwkam kwkam Apr 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused now. All patterns suggested in the list are valid, would you please explain why should ContainsValidWildcardPattern reject them?

Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw Apr 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"goose]" is a not a pattern, even though it's valid (not throw when calling WildCardPatter.IsMatch)

{
if (string.IsNullOrEmpty(pattern))
{
return false;
}

bool hasWildcard = false;
bool openBracket = false;

for (int index = 0; index < pattern.Length; ++index)
{
switch (pattern[index])
Copy link
Copy Markdown
Member

@TravisEz13 TravisEz13 Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to reuse as much code as possible. Perhaps add an overload of IsWildcardChar which takes the state of openBracket and gives you the new state?

Copy link
Copy Markdown
Contributor Author

@kwkam kwkam Mar 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is little benefit as IsWildcardChar only tells whether the character is one of the wildcard characters, while in this function, we need to know exactly which character it is to determine the result.

{
case '*':
case '?':
hasWildcard = true;
break;

case '[':
hasWildcard = true;
Copy link
Copy Markdown
Collaborator

@rjmholt rjmholt Apr 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContainsValidWildcardPattern("du[[ck]") will return true.

We probably need something like:

case '[':
    if (openBracket)
    {
        return false;
    }

Copy link
Copy Markdown
Contributor Author

@kwkam kwkam Apr 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

du[[ck] should be a valid wildcard pattern.

openBracket = true;
break;

case ']':
// ']' is wildcard only if '[' exists
// so we do not set hasWildcard here
openBracket = false;
break;
Comment thread
kwkam marked this conversation as resolved.
Outdated

// If it is an escape character then advance past
// the next character
case escapeChar:
++index;
break;
}
}

if (openBracket) {
return false;
}

return hasWildcard;
}

/// <summary>
/// Unescapes any escaped characters in the input string.
/// </summary>
Expand Down
7 changes: 7 additions & 0 deletions ...powershell/Modules/Microsoft.PowerShell.Core/Pester.Commands.Cmdlets.GetCommand.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ Describe "Tests Get-Command with relative paths and wildcards" -Tag "CI" {
# Create temporary EXE command files
$file1 = Setup -f WildCardCommandA.exe -pass
$file2 = Setup -f WildCardCommand[B].exe -pass
$file3 = Setup -f [.exe -pass
Copy link
Copy Markdown
Collaborator

@rjmholt rjmholt Apr 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are probably best as Pester TestCases:

BeforeAll {
    $testFileNames = @(
        'WildCardCommandA'
        'WildCardCommand[B]'
        '['
        '[a-mt]ook'
        '[ab-z]ook'
        '[a?b]'
        '[a*]x'
        'goose]'
        'du[[ck]'
        'du[ck]]'
    ) | ForEach-Object { "$_.exe" }

    foreach ($testFileName in $testFileNames)
    {
        Setup -f $testFileName -pass
        if ($IsLinux -or $IsMacOS)
        {
            /bin/chmod 777 $testFileName
        }
    }

    $fileTestCases = $testFileNames | ForEach-Object { @{ Filename = $_ } }
}

...

It "Should find the file <Filename>" -TestCases $fileTestCases {
    param([string]$Filename)

    $result = Get-Command -Name $Filename -Type Application
    $result | Should -Not -BeNullOrEmpty
    $result | Should -BeExactly $Filename
}

Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Apr 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add "du[[ck]" too. And perhaps "du[ck]]" too.

Copy link
Copy Markdown
Collaborator

@rjmholt rjmholt Apr 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwkam you're absolutely right that all the cases I suggested are valid wildcard patterns (except goose]). I discussed it with some others offline.

I think they're worth adding as positive test cases if possible, since they represent edges.

#$null = New-Item -ItemType File -Path (Join-Path $TestDrive WildCardCommandA.exe) -ErrorAction Ignore
#$null = New-Item -ItemType File -Path (Join-Path $TestDRive WildCardCommand[B].exe) -ErrorAction Ignore
if ( $IsLinux -or $IsMacOS ) {
/bin/chmod a+rw "$file1"
/bin/chmod a+rw "$file2"
/bin/chmod a+rw "$file3"
}
$commandInfo = Get-Command Get-Date -ShowCommandInfo
}
Expand Down Expand Up @@ -54,6 +56,11 @@ Describe "Tests Get-Command with relative paths and wildcards" -Tag "CI" {
$result | Should -Not -BeNullOrEmpty
$result | Should -Be WildCardCommand[B].exe

# This should find the file [.exe
$result = Get-Command -Name .\[.exe -Type Application
$result | Should -Not -BeNullOrEmpty
$result | Should -BeExactly [.exe

Pop-Location
}

Expand Down