Fix env:PAGER not tokenized properly by help command

[pougetat]

PR Summary

This PR fixes #8912 which is due to a tokenization issue in the help command.

PR Context

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
    • Add [feature] to your commit messages if the change is significant or affects feature tests

[vexx32]

That won't interpolate variables. Try instead:

Suggested change

	$env:PAGER = "`"$fakePagerPath`" `"$fakePagerCustomArgs`""
	$env:PAGER = '"{0}" "{1}"' -f $fakePagerPath, $fakePagerCustomArgs

[rjmholt]

My only question is whether is makes sense to handle all the tokens, rather than just the first two. I don't know much about this functionality though, so perhaps not

[pougetat]

Hey @rjmholt,
I briefly looked into it and I'm guessing we'd rather use a powershell function specifically designed to split the command line into it's command and arguments. Having said that I haven't found a powershell function that does that and is a part of PSCore.

I am of the opinion that using Tokenize and only keeping the first two tokens is the best option.

Thoughts ? :)

[vexx32]

That won't interpolate variables. Try instead:

Suggested change

	$env:PAGER = "`"$fakePagerPath`" `"$fakePagerCustomArgs`""
	$env:PAGER = '"{0}" "{1}"' -f $fakePagerPath, $fakePagerCustomArgs

[iSazonov]

@rkeithhill Please review the PR.

[iSazonov]

My fear is that this function becomes ponderous.

[rkeithhill]

I'll look at it this weekend. I still have some concerns. I'd like to avoid any sort of script injection e.g. less "format string $(remove-item $home\* -r -for)". Which I "think" we could avoid with: & $pagerCmd --% $pagerArgs.

Also the tokenizer approach doesn't account for single value, path to pager with spaces. That is, it parses this C:\Program Files\Pscx\bin\less.exe as command = C:\Program followed by an arg/param of Files\Pscx\bin\less.exe. So before trying the tokenize approach we should check if the entire pager value corresponds to an application path. If so, there are no args and no need to tokenize. If not, then we can tokenize cmd/args.

[pougetat]

The tokenizer approach can still work if you put quotes around the pager path. Without quotes and extra arguments it indeed becomes harder to parse the page path with a space in it. Without an extra argument I agree we shouldn't need quotes and checking if the entire command-line is a valid path is a good idea and this is exactly how to code works currently.

[iSazonov]

Should we follow only env:PAGER? Could we put in env:PAGER only application name and params in env:PAGERPARAMS?

[pougetat]

Should we follow only env:PAGER? Could we put in env:PAGER only application name and params in env:PAGERPARAMS?

@rjmholt :)

[rkeithhill]

OK, here is my take on how to do this without incurring script injection - at least unintental script injection. Note there is probably a better way but this does seem to work. Here it is in pseudo-code - ish form:

$customPagerCommand = $customPagerArgs = $null

# Disallow a pure whitespace value as that would cause the tokenizer to return 0 tokens.
if (![string]::IsNullOrWhitespace($env:PAGER)) {
    # Check if the PAGER value corresponds to a command.
    # This allows the user to specify a path to an exe with spaces without having to quote the path.
    # However, if the user wants to also specify args then they will have to quote the exe path.
    if (Microsoft.PowerShell.Core\Get-Command $env:PAGER -ErrorAction Ignore) {
        $customPagerCommand = $env:PAGER
    }
    else {
        # Custom pager has been specified with arguments OR single arg is not a command.
        # Tokenize the specified $env:PAGER command / command line.  Ignore tokenizing
        # errors because what is an error for PowerShell may be allowed for the paging
        # utility to be invoked.
        $errs = $null
        $tokens = [System.Management.Automation.PSParser]::Tokenize($env:PAGER, [ref]$errs)

        $customPagerCommand = $tokens[0].Content
        if (!(Microsoft.PowerShell.Core\Get-Command $customPagerCommand -ErrorAction Ignore)) {
            # Custom pager command is invalid, issue a warning.
            Write-Warning ""Ignoring invalid custom-paging utility command line specified in `$env:PAGER: $env:PAGER""
            $customPagerCommand = $null
        }
        elseif ($tokens.Count -gt 1) {
            # This approach will preserve all the pagers args.
            $customPagerArgs = $env:PAGER.Substring($tokens[1].Start)
        }
    }
}
...
if ($customPagerCommand) {
    if ([string]::IsNullOrWhitespace($customPagerArgs)) {
        $help | & $customPagerCommand
    }
    else {
        # Using the stop parsing operator prevents PowerShell from executing/interpolating 
        # anything in the PAGER args and choking on what it thinks is a parse error.
        $env:__PAGER_ARGS = $customPagerArgs
        $help | & $customPagerCommand --% %__PAGER_ARGS%
        Remove-Item Env:\__PAGER_ARGS -ErrorAction Ignore # Or maybe keep for debugging?
    }
}
...

[rkeithhill]

BTW while we have "the hood up", why is it that on Windows - when I use less, I have to first pipe to Out-String -Width 120 before sending on to less? Otherwise, help topics wrap and do not look right:

man Get-Content
...
DESCRIPTION
    The `Get-Content` cmdlet gets the content of the item at the location specified by the path, such as the text in a f
ile or the content of a function. For files, the content is read one line at a time and returns a collection of objects
each of which represents a line of content.

    Beginning in Windows PowerShell 3.0, `Get-Content` can also get a specified number of lines from the beginning or ed
 of an item.

versus:

Get-Help Get-Content | Out-String -Width 120 | less
...
DESCRIPTION
    The `Get-Content` cmdlet gets the content of the item at the location specified by the path, such as the text in a
    file or the content of a function. For files, the content is read one line at a time and returns a collection of
    objects, each of which represents a line of content.

    Beginning in Windows PowerShell 3.0, `Get-Content` can also get a specified number of lines from the beginning or
    end of an item.

Note that on Linux, piping directly to less works just fine. BTW I've tried setting $COLUMNS/$LINES on Windows and that doesn't help.

[iSazonov]

@rkeithhill Please open new Issue for your question.

I am not sure that Tokenizer is right tool to resolve paths.

[rkeithhill]

@iSazonov see #7175

[iSazonov]

What if there is some arguments?

Also now we should change
$cmds = Get-Command $customPagerCommand, $customPagerCommandLine -ErrorAction Ignore
with
$cmds = Get-Command $customPagerCommand-ErrorAction Ignore